Repository navigation
extmod/modtls_mbedtls: Add support for TLS PSK - #17074
Conversation
|
Code size report: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17074 +/- ##
==========================================
+ Coverage 98.47% 98.48% +0.01%
==========================================
Files 176 176
Lines 22845 22910 +65
==========================================
+ Hits 22497 22564 +67
+ Misses 348 346 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hey @projectgus and @dpgeorge . I've created this draft of an approach to add support for TLS PSK based on the work in #17074. I've bumped the other thread a few times, but figured I would move the discussion into my actual implementation here. There was some discussion on the different possible approaches and I believe what I have implemented here follows @dpgeorge 's suggestion in 17074. How does it look and does this direction seem good? If so, I can finalize the paperwork to close this out. |
There was a problem hiding this comment.
The commits in this PR need to be squashed and reworded to match MicroPython's Code Conventions. Aside from that:
|
For overall structure and direction, though:
|
|
This should probably be feature-gated? e.g. with a new Re: #14396 and the client-callback and server-callback API, in my opinion it could be appropriate to add those too, at least if behind another feature gate like |
|
Squashed and working through review feedback now. |
|
Hey @AJMansfield , I appreciate the detailed feedback, but I'm still working through it all, so I'm not sure that's very useful you providing feedback on my intermediate work. I'll ping you directly and set this PR out of draft to ready once I'm ready for another round of feedback. I just like to push to the branch often to get the CI to run and cross check my local setup. |
Understood! Apologies for the pressure: you're right it does neither of us any good to have me constantly looking over your shoulder, we've all got our own workflows. (I certainly made extensive use of the CI for testing changes myself at one point, lol.) Just be careful with the LLMs --- this is a cryptographic module, after all. |
|
@keenanjohnson Feel free to ping me as well once you're done updating. Thanks @AJMansfield for getting the review process underway. |
|
Hi @keenanjohnson, No rush on this as I assume you've been very busy with other things. I just now left a long comment on the related PR: #14396 (comment) - and the considerations there apply here more or less as well. One high-level concern for this PR is how we'd manage to add a CPython-compatible callback wrapper, as discussed in the other PR.
As I said, no rush or pressure to respond but I wanted to leave this comment while it was on my mind. If anyone wants to pick up either of these TLS-PSK PRs and work on a new one, that'd be much appreciated - the feature is potentially very useful to have. |
|
Thanks @projectgus and @dpgeorge ! I have stepped away from this for a bit, but I would very much like to see it upstreamed as it would be useful for my particular project! Let me re read everything and recontextuatlize myself here and I'll try to address the design decision / point you brought up in the other PR. |
77bd39e to
f3c659b
Compare
|
Ok all. Thanks for the bump @projectgus to put this back on my radar. I believe I have addressed everything and this PR is ready to go. The CI is all happy. There is one line of code coverage missing which I believe to be a bit impractical to consistently hit in test. @AJMansfield did some initial review a long while ago. I believe I have addressed all of your feedback, but check me if I missed anything. @projectgus and @dpgeorge I have taken your feedback and implemented roughly the design you proposed in #14396 (comment) where there is a PSK Dict and should keep things as micropython-y as we can. @projectgus you also mentioned adding some adversarial tests and I have added all the ones I initially thought of and helped me verify some of the security / linking concerns that @AJMansfield originally brought up (i first wrote the tests to prove the security hole and then fixed it until the tests were green). I enabled the feature on the esp32 port as that's the usecase I personally have. Finally i added some basic docs update. Let me know what you all think and I am happy to respond to review feedback in a timely manner as I want to finally push this over the finish line! |
| corresponding key as a `bytes` object, or raise ``KeyError`` to reject an | ||
| unknown identity. Any object implementing ``__getitem__`` may be used, so | ||
| keys can be computed or fetched on demand. As with the client, the context | ||
| is restricted to PSK cipher suites while this is set. |
There was a problem hiding this comment.
Did you test this scenario, when a user object is passed in and a callback is made into Python? (I couldn't see anything in the tests below, would be good to add something.)
(Also, an alternative could be to use dict.get(key) instead of dict[key] which would return None if the key didn't exist. That could be more efficient than raising and catching KeyError.)
There was a problem hiding this comment.
Good catch! I added a new test for that in tests/multi_net/sslcontext_server_client_psk_getitem.py just now.
We definitely could switch to using dict.get(key). Honestly I didn't really consider the tradeoff too much when I implemented. I'm happy to switch to that if we think it's better.
There was a problem hiding this comment.
I did a quick little benchmark and yes the get method was about 3X faster than the currently impelmeted version. I've gone ahead and switched to dict.get(key) and updated everything to match that.
| nlr_buf_t nlr; | ||
| if (nlr_push(&nlr) == 0) { | ||
| mp_obj_t key = mp_obj_subscr(self->server_psk_keys, | ||
| mp_obj_new_str((const char *)identity, identity_len), MP_OBJ_SENTINEL); |
There was a problem hiding this comment.
I'm not sure, but maybe this should be a bytes rather than a str. Need to understand what happens when non-ascii characters are used in the identity.
There was a problem hiding this comment.
Also a good catch. Upon testing any non-ascii characters would raise UnicodeError, so any non UTF-8 ID can never authenticate. Switching to bytes does fix this and lets us eliminate one special case. I have implemented that and just pushed.
|
Thanks @keenanjohnson for jumping back on this, it's good to see. I didn't do a full review yet, but made a few comments above based on initial observations. |
Of course @dpgeorge ! I am glad that this finally feels very close. I responded to your initial feedback above. |
projectgus
left a comment
projectgus
left a comment
There was a problem hiding this comment.
This looks good, thanks @keenanjohnson! Very useful feature to have, and I appreciate all the test coverage.
|
You are very welcome @AJMansfield ! This is quite useful for me, as our project has been running on a fork of micropython for a long time due to this missing feature, and I am glad to finally upstream it for others. It's taken a while (mostly my fault for finding the time to get around to it), but I am glad it seems useful. Hopefully I didn't over do it with the test coverage haha. |
|
Again thank you for the incredible work that you (@projectgus ) and @dpgeorge and the rest of the team and community around micropython do. I'm certain you likely don't get enough thanks or credit, but this project is incredible and enables a lot of learning and projects around the world. |
|
I ran the included tests on PYBD_SF6 vs RPI_PICO_W. Everything passed except one test, The following fixes it for me and gets the test passing on those boards. The test also still passes on the unix port. --- a/tests/multi_net/sslcontext_server_client_psk_wrong_key_error.py
+++ b/tests/multi_net/sslcontext_server_client_psk_wrong_key_error.py
@@ -32,6 +32,7 @@ def instance0():
except OSError:
print("server: handshake failed")
multitest.broadcast("finished")
+ s2.close()
s.close()It may be that this is needed due to our implementation of TCP/IP with lwIP on bare metal (because it passes without it on unix, and also esp32 I assume). I don't have time to investigate why that close is needed, but it seems sensible enough to clean up all the sockets on the server side, and I guess it unblocks the client allowing it to see that the handshake failed. |
|
Huh very curious @dpgeorge . The theory seems reasonable and I can't see any downside to adding the close socket so I have added it to the test that you mentioned, but alos the other four tests that follow this pattern here to try and prevent any flakiness there as well. |
Thanks for adding that close socket. I tested again on PYBD_SF6 and RPI_PICO2_W, and all tests pass there. |
| // Restrict the context to offer/accept only PSK cipher suites, so a PSK | ||
| // connection cannot silently fall back to a non-PSK (e.g. certificate-based) | ||
| // suite. Does nothing if the user already chose ciphers via set_ciphers(). | ||
| static void ssl_context_restrict_to_psk_ciphersuites(mp_obj_ssl_context_t *self) { |
There was a problem hiding this comment.
I don't think this function is tested by the existing tests: I added a simple return statement at the start of this function and all tests continued to pass. I thought that sslcontext_server_client_psk_client_only_error.py was testing the logic here, but it seems not.
Are you able to write a test that exercises this function? So that if this function just returns immediately then the test fails?
There was a problem hiding this comment.
Ah yes that's a good catch that I missed in my test coverage. I confirmed the same with an immediate return. I added a test for both the client and server side version of this for good measure where each direction negotiates the PSK cipher suite, but presents the incorrect key.
tests/multi_net/sslcontext_server_client_psk_no_fallback_error.py
tests/multi_net/sslcontext_server_client_psk_server_no_fallback_error.py
There was a problem hiding this comment.
Thanks for updating and adding the new tests. I can confirm that they both test unique code paths (ie the tests are both necessary and the two calls to ssl_context_restrict_to_psk_ciphersuites() are both necessary).
| @@ -637,6 +737,29 @@ static mp_obj_t ssl_socket_make_new(mp_obj_ssl_context_t *ssl_context, mp_obj_t | |||
|
|
|||
| mbedtls_ssl_init(&o->ssl); | |||
|
|
|||
| #if defined(MBEDTLS_SSL_HANDSHAKE_WITH_PSK_ENABLED) | |||
| // Configure PSK from the context. A server looks up keys by the client's | |||
There was a problem hiding this comment.
This block of logic that sets the PSK values in mbedTLS doesn't seem to depend at all on the actual socket state. It only depends on the SSLContext.
So I guess all this logic could be moved to the ssl_context_attr() function:
- if the user stores into
server_psk_keysthenmbedtls_ssl_conf_psk_cb()is called - if the user stores into
psk_identity/psk_keyand they are both non-None, thenmbedtls_ssl_conf_psk()is called
Benefits of doing it that way are:
- only needs to configure things once (when the attributes are set), instead of each time a socket is created (and the whole point of SSLContext is to be able to configure everything in the context and then creating sockets is less work)
- allows errors to be caught earlier
- probably smaller code size
- makes it clearer in the code that setting PSK attributes affects all sockets created from that SSLContext (whereas the current code seems to indicate that PSK settings are stored per socket, rather than per context)
- matches how the existing
verify_modeis configured
There was a problem hiding this comment.
Yes that is probably better. I created a new helper function ssl_context_set_psk() that is called as you have described in the socket context.
| try: | ||
| import socket | ||
| import tls | ||
| except ImportError: |
There was a problem hiding this comment.
All these multi_net tests need to skip if PSK is not supported (eg using the same skip logic from extmod/tls_psk.py).
Eg esp8266 which uses axTLS will attempt to run this test and needs to skip it.
There was a problem hiding this comment.
Ah yes I see. I did not think about that. Implemented the block below in all tests:
try:
import socket
import tls
# PSK support is optional; psk_identity only exists when it's enabled.
tls.SSLContext(tls.PROTOCOL_TLS_CLIENT).psk_identity
except (ImportError, AttributeError):
print("SKIP")
raise SystemExit
There was a problem hiding this comment.
Thanks, I confirm that skips correctly on esp8266.
Add support for TLS-PSK (pre-shared key) cipher suites to the mbedtls bindings, via new SSLContext attributes: - psk_identity / psk_key: the identity and key a client presents. - server_psk_keys: a mapping (e.g. dict) the server uses to look up the key for a client's identity; any object with a get() method works. Identities are handled as bytes (an opaque octet string on the wire). When PSK is configured the context is restricted to PSK cipher suites, so a connection cannot silently fall back to a non-PSK suite. PSK is enabled for the bundled mbedtls config and for the esp32 port, and the new attributes are documented in the ssl module reference. Tests cover the PSK attributes and key-length validation; successful handshakes via a dict, via a custom mapping object, and with a non-ASCII identity; and adversarial cases: PSK vs non-PSK peers (both directions), a wrong key, an unknown identity, and a misbehaving server_psk_keys. Signed-off-by: Keenan Johnson <[email protected]>
|
I believe I have addressed all the feedback @dpgeorge |
dpgeorge
left a comment
dpgeorge
left a comment
There was a problem hiding this comment.
This PR is looking very good now, thanks for responding to all the feedback.
|
Excellent. I am so glad we got this closed out after so long. Thank you for all the review help! |
|
Any other open items before this is ready for merge? |
No, this is ready for merging now. |
|
Merged! Thank you very much @keenanjohnson for your efforts here. |
|
Excellent and thank you for your help! I hope others find this as useful as I do, |
4 participants
This pull request adds support to the ssl module for TLS-PSK (pre-shared key) authentication.
It enables the feature on the esp32 port as well, which fits my use case.
This PR was originally based on the work in #17074. I've bumped the other thread a few times, but figured I would move the discussion into my actual implementation here and the implementation has since diverged.