Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation of TLSSocket behavior on AUTH_FAILURE #9392

Merged

Conversation

Projects
None yet
7 participants
@michalpasztamobica
Copy link
Contributor

commented Jan 16, 2019

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

@0xc0170 0xc0170 requested a review from SeppoTakalo Jan 16, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

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

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_documentation_update branch Jan 16, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

@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?

features/netsocket/TLSSocket.h Outdated
@@ -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.

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 17, 2019

Contributor

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.

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 17, 2019

Contributor

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.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Jan 21, 2019

Author Contributor

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?

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

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

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.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_documentation_update branch Jan 21, 2019

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

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_documentation_update branch Jan 21, 2019

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

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

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

@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
Copy link
Contributor

left a comment

Looks good

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

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

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:tlssocket_documentation_update branch Jan 22, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

Rebased and conflicts resolved.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 22, 2019

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 michalpasztamobica force-pushed the michalpasztamobica:tlssocket_documentation_update branch Jan 22, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 22, 2019

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 michalpasztamobica force-pushed the michalpasztamobica:tlssocket_documentation_update branch to 2cda5d2 Jan 22, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

CI restarted

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Yes, test team is investigating.

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 22, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

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 0xc0170 added needs: CI and removed needs: work labels Jan 23, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

CI restarted

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

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

23 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10463 cycles (-88 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@adbridge

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

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

@michalpasztamobica michalpasztamobica deleted the michalpasztamobica:tlssocket_documentation_update branch Jan 25, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

5.12 is fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.