When using PSK for TLS authentication, look up and use the PSK ID requested by the client#379
Conversation
|
Thank you for this contribution, @tkEmLogic. Before we can accept it, you need to sign the Eclipse Contributor Agreement (ECA). The purpose of the ECA is to provide a written record that you have agreed to provide your code and documentation contributions under the licenses used by the Eclipse ThreadX project. It also makes it clear that you are promising that what you are contributing to Eclipse is code you wrote, and you have the necessary rights to contribute it to our projects. And finally, it documents a commitment from you that your open source contributions will be permanently on the public record. Signing the ECA requires an Eclipse Foundation account if you do not already have one. You can create one for free at https://accounts.eclipse.org. Be sure to use the same email address when you register for the account that you intend to use on Git commit records. Also, please add your GitHub ID to your Eclipse account. This enables synchronisation between Eclipse-owned infrastructure and GitHub. Here is the link to sign the ECA: |
…uested by the client in ClientKeyExchange as defined in RFC 4729
3f7debe to
9b6608d
Compare
Thanks for the prompt response and clarification, Frédéric. I have fixed the git log to use the proper user and email address, and signed the commit. I have also created an Eclipse account with the same email address, signed the ECA and linked the Github Account to the Eclipse account. I hope everything is in order now. |
|
Hi @tkEmLogic. The ECA check now passes. One of us will review and provide feedback. |
…xceed NX_SECURE_TLS_MAX_PSK_ID_SIZE
| { | ||
| return NX_INVALID_PACKET; | ||
| } | ||
| tls_credentials -> nx_secure_tls_remote_psk_id_size = ((packet_buffer[0] << 8) | packet_buffer[1]); |
There was a problem hiding this comment.
It looks like you read past the handshake buffer for truncated PSK identities. The code checks message_length >= 2 and psk_id_size <= NX_SECURE_TLS_MAX_PSK_ID_SIZE, but never checks 2 + psk_id_size <= message_length before copying from &packet_buffer[2]. A malformed ClientKeyExchange can advertise a valid-sized identity but provide fewer bytes. Return NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH before the copy.
There was a problem hiding this comment.
Fixed, now returns NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH if message_length < 2 + tls_credentials -> nx_secure_tls_remote_psk_id_size
| } | ||
| if (psk_length == 0) | ||
| { | ||
| return(NX_OPTION_ERROR); |
There was a problem hiding this comment.
You return NX_OPTION_ERROR when the requested PSK identity is not found. Existing PSK lookup helpers return NX_SECURE_TLS_NO_MATCHING_PSK, which maps to unknown_psk_identity. NX_OPTION_ERROR falls through to an internal-error alert. Use the TLS-specific error, and preferably scan only nx_secure_tls_psk_count or share
the existing lookup behavior.
There was a problem hiding this comment.
Fixed, return code changed to NX_SECURE_TLS_NO_MATCHING_PSK.
|
Thanks again for the fix, @tkEmLogic. I validated the direction and found a couple of adjacent PSK cases that should be handled in the same change set. Your patch fixes the plain server-side PSK lookup, but NetX Duo also advertises psk_identity || ClientECDiffieHellmanPublicSo, the server needs to parse and store the client PSK identity first, then process the ECDHE public key from the remaining bytes, and finally build the RFC 5489 premaster secret using the PSK selected by that client identity. I pushed a complementary branch here: Relevant commits:
It also adds regression coverage for:
The tests pass on Linux. Feel free to incorporate/cherry-pick these changes into your PR, or I can help get them integrated. |
…_client_key_exchange if the message is too short to contain the indicated psk_id_size. Return NX_SECURE_TLS_NO_MATCHING_PSK instead of NX_OPTION_ERROR if the PSK ID requested by the client is not found in the PSK key store in _nx_secure_generate_premaster_secret.
Thanks for that @fdesbiens . I have updated my PR and fixed your remarks above. I am not overly familiar with the code base here, so I think the best/cleanest option is if you incorporate your branch separately instead of me cherry picking and potentially making a mess of things :) If you would rather that I incorporate your branch into mine before merging, let me know and I'll do it 👍 |
|
Hi @tkEmLogic. In the end, I created a broader PR (#386) to address additional PSK issues. To clarify, your original PSK PR was the starting point for my follow-up work. Your patch identified the core server-side issue: the PSK identity sent in ClientKeyExchange needs to be used to select the matching PSK from the store instead of defaulting to the first entry. While validating that path, I found a few related PSK issues that affected broader coverage, especially ECDHE-PSK handling, client-side identity emission, and preserving the selected PSK identity when the client uses the PSK store path. I created a patch that includes your original functional intent, adds regression coverage, and fixes those additional cases. I made sure my PR description credits your PR as the work that surfaced the issue and led to the broader fix. If you see anything from your original patch that I missed or represented differently, I'd appreciate the review. |
|
Hi @tkEmLogic. I would be ready to merge my own PR and close this one. Were you able to check if my code is missing anything? |
|
Closing this PR as it was superseded by #386. |
Sorry for the late response Frédéric, it looks good to me 👍 |
When acting as TLS server and using PSK based authentication, NetX currently just blindly selects the first available key from the PSK key store. It is up to the client to request a PSK ID to use to establish the connection in the ClientKeyExchange message. This patch stores the remote PSK ID requested by the client in the TLS credentials, then looks up the correct key in the PSK keystore to proceed with the authentication.
See chapter 2 of RFC 4279 Pre-Shared Key Ciphersuites for Transport Layer Security (TLS)