Documentation of TLSSocket behavior on AUTH_FAILURE #9392

michalpasztamobica · 2019-01-16T08:23:28Z

Description

It is not possible to store TLS certificate after opening a TLSSocket, due to mbedTLS limitation. We update the documentation to warn users and explain the reason.
(Internal JIRA with the full discussion is ONME-4121)

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[x] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@VeijoPesonen

kjbracey · 2019-01-16T14:08:29Z

Surely this is nothing to do with "AUTH_FAILURE"? It's a general restriction that the socket can only be used for one connection (attempt).

That limitation is not that unusual for connection-type sockets in general, but may be worth noting here.

(POSIX says: "If connect() fails, the state of the socket is unspecified. Conforming applications should close the file descriptor and create a new socket before attempting to reconnect.")

0xc0170 · 2019-01-16T15:43:29Z

Note: Travis failure is fixed on master (#9391) , if you can rebase to resolve the error

michalpasztamobica · 2019-01-16T15:53:25Z

@0xc0170 , thanks, I rebased.
@kjbracey-arm , thank you for your remark. I modified the comment. I guess if we warn users that after close() they need to create the socket from scratch, we can also warn them that they should do so in case connect() fails. Is the current comment content acceptable?

SeppoTakalo · 2019-01-17T10:17:57Z

features/netsocket/TLSSocket.h

@@ -75,6 +75,9 @@ class TLSSocket : public TLSSocketWrapper {
     *  Initiates a connection to a remote server specified by either
     *  a domain name or an IP address and a port.
     *
+     *  @note: In case connect() returns an error, the state of the socket is
+     *  unspecified. A new socket should be created before reconnecting.


unspecified is not quite accurate.

See the documentation for mbedtls_ssl_handshake()
https://tls.mbed.org/api/ssl_8h.html#a4a37e497cd08c896870a42b1b618186e

We should reflect that in our documentation. So if this function returns failures other than what we specify, user should stop using the TLS context and free it. We don't support the mbedtls_ssl_session_reset() but maybe we should.

It might be hard to reflect, but basically we could try the connect() as long as it is the TCP session that fails the connection.

Basically, if SSL handshake fails, we return NSAPI_ERROR_AUTH_FAILURE and after that, the socket cannot be used anymore.

Basically, if SSL handshake fails, we return NSAPI_ERROR_AUTH_FAILURE

I noticed we sometimes return PARAMETER_ERROR instead, but I saw no reason to do that and I modified the return value. Please take a look and if you agree with that change, I will amend the test's expectations (the tests documentation just says "must return an error", without specifying the exact error code, so that's fine).
If we always return AUTH_ERROR on ssl-related errors we can safaely tell the users that if they see this error, then they must get a new socket before retrying (which means they don't have to do that if they just want to retry the TCP connection).
Is that ok?

SeppoTakalo · 2019-01-17T10:29:07Z

Do we actually need to reflect this connect() thing in the main Socket.h?

We seem to allow TCPSocket::connect() to be retried, but that is not necessary always the case. Therefore I propose that we amend the Socket::connect() documentation as well.

features/netsocket/TLSSocket.h

0xc0170 · 2019-01-21T15:43:39Z

@michalpasztamobica All reviews addressed (we can see a new commit here)

michalpasztamobica · 2019-01-21T15:45:39Z

I think they are addressed, although I would prefer to see an explicit OK from @SeppoTakalo regarding the error code returns ;-)
I checked - no test code modification are needed because we don't check for a particular error code (as the documentation suggests).

michalpasztamobica · 2019-01-22T07:24:53Z

@0xc0170 , Travis passed and I think that the builds which were failing so far should now pass (as they don't really compile anything any more). Could you trigger the CI, please?

SeppoTakalo

Looks good

0xc0170 · 2019-01-22T09:55:57Z

Ready for CI, please rebase first to resolve conflicts (documentation update was merged recently)

michalpasztamobica · 2019-01-22T10:06:07Z

Rebased and conflicts resolved.

0xc0170 · 2019-01-22T10:13:22Z

CI started

mbed-ci · 2019-01-22T10:19:51Z

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

michalpasztamobica · 2019-01-22T11:10:41Z

Sorry, I checked Icetea tests and greentea tests, but forgot about the unit tests - latest push fixes the expected error return values.

0xc0170 · 2019-01-22T11:29:09Z

CI restarted

mbed-ci · 2019-01-22T11:35:16Z

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

michalpasztamobica · 2019-01-22T11:42:54Z

Now the CI pointed out that I should also modify the DTLSSocketWrapper tests. I hope nothing more will come up...

0xc0170 · 2019-01-22T12:28:21Z

CI restarted

michalpasztamobica · 2019-01-22T13:54:47Z

I doubt my changes are causing the dynamic-memory-usage failure as I can see the same is happening in @VeijoPesonen 's Pull Request...

0xc0170 · 2019-01-22T13:55:46Z

Yes, test team is investigating.

mbed-ci · 2019-01-22T17:14:44Z

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_dynamic-memory-usage
jenkins-ci/mbed-os-ci_greentea-test

michalpasztamobica · 2019-01-23T08:27:24Z

I can see that 4 out of 5 failing greentea tests are not even related to ipcore.
The error seems to be that the resource NUCLEO_F429ZI wasn't allocated correctly:

[1548175529.18][GLRM]079602210542680C3A06FEA7 - Received response
[1548175529.18][GLRM]079602210542680C3A06FEA7 - 409 Conflict

[1548175532.48][HTST][WRN] stopped to consume events due to __notify_sync_failed event

@0xc0170 , is this something I should worry about?

0xc0170 · 2019-01-23T09:34:23Z

CI restarted

adbridge · 2019-01-25T14:34:54Z

I can't seem to patch this with either git am or git cherry-pick so I am bumping it to 5.12

SeppoTakalo · 2019-01-28T08:24:37Z

5.12 is fine

michalpasztamobica mentioned this pull request Jan 16, 2019

Add TLSSocket greentea tests. #9283

Merged

VeijoPesonen approved these changes Jan 16, 2019

View reviewed changes

0xc0170 requested a review from SeppoTakalo January 16, 2019 08:38

0xc0170 added the needs: review label Jan 16, 2019

michalpasztamobica force-pushed the tlssocket_documentation_update branch from bd07ade to ade5624 Compare January 16, 2019 15:51

SeppoTakalo suggested changes Jan 17, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jan 17, 2019

michalpasztamobica force-pushed the tlssocket_documentation_update branch from ade5624 to 9bb47d6 Compare January 21, 2019 08:49

SeppoTakalo reviewed Jan 21, 2019

View reviewed changes

features/netsocket/TLSSocket.h Outdated Show resolved Hide resolved

michalpasztamobica force-pushed the tlssocket_documentation_update branch from 9bb47d6 to f4caf44 Compare January 21, 2019 11:47

0xc0170 added needs: review and removed needs: work labels Jan 21, 2019

SeppoTakalo approved these changes Jan 22, 2019

View reviewed changes

0xc0170 approved these changes Jan 22, 2019

View reviewed changes

0xc0170 added needs: work release-version: 5.11.3 and removed needs: review labels Jan 22, 2019

michalpasztamobica force-pushed the tlssocket_documentation_update branch from f4caf44 to dbd00f5 Compare January 22, 2019 10:05

michalpasztamobica force-pushed the tlssocket_documentation_update branch from dbd00f5 to a4ba6aa Compare January 22, 2019 11:09

Documentation of TLSSocket behavior on AUTH_FAILURE

2cda5d2

michalpasztamobica force-pushed the tlssocket_documentation_update branch from a4ba6aa to 2cda5d2 Compare January 22, 2019 11:41

0xc0170 added needs: CI and removed needs: work labels Jan 23, 2019

0xc0170 added ready for merge and removed needs: CI labels Jan 23, 2019

0xc0170 merged commit 2cd83b4 into ARMmbed:master Jan 23, 2019

0xc0170 removed the ready for merge label Jan 23, 2019

adbridge added Manually Patch and removed release-version: 5.11.3 labels Jan 25, 2019

adbridge added release-version: 5.12.0-rc1 and removed Manually Patch labels Jan 25, 2019

michalpasztamobica deleted the tlssocket_documentation_update branch January 25, 2019 15:00

Documentation of TLSSocket behavior on AUTH_FAILURE #9392

Documentation of TLSSocket behavior on AUTH_FAILURE #9392

Uh oh!

Conversation

michalpasztamobica commented Jan 16, 2019

Description

Pull request type

Reviewers

Uh oh!

kjbracey commented Jan 16, 2019

Uh oh!

0xc0170 commented Jan 16, 2019

Uh oh!

michalpasztamobica commented Jan 16, 2019

Uh oh!

SeppoTakalo Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

michalpasztamobica Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo commented Jan 17, 2019

Uh oh!

Uh oh!

0xc0170 commented Jan 21, 2019

Uh oh!

michalpasztamobica commented Jan 21, 2019

Uh oh!

michalpasztamobica commented Jan 22, 2019

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jan 22, 2019

Uh oh!

michalpasztamobica commented Jan 22, 2019

Uh oh!

0xc0170 commented Jan 22, 2019

Uh oh!

mbed-ci commented Jan 22, 2019

Test run: FAILED

Uh oh!

michalpasztamobica commented Jan 22, 2019

Uh oh!

0xc0170 commented Jan 22, 2019

Uh oh!

mbed-ci commented Jan 22, 2019

Test run: FAILED

Uh oh!

michalpasztamobica commented Jan 22, 2019

Uh oh!

0xc0170 commented Jan 22, 2019

Uh oh!

michalpasztamobica commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Jan 22, 2019

Uh oh!

mbed-ci commented Jan 22, 2019

Test run: FAILED

Uh oh!

michalpasztamobica commented Jan 23, 2019

Uh oh!

0xc0170 commented Jan 23, 2019

Uh oh!

adbridge commented Jan 25, 2019

Uh oh!

SeppoTakalo commented Jan 28, 2019

Uh oh!

Uh oh!

michalpasztamobica commented Jan 22, 2019 •

edited

Loading