-
Notifications
You must be signed in to change notification settings - Fork 140
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
External PSK mode #321
External PSK mode #321
Conversation
Design of picotls is based on the assumption that PSK-based handshake will not be used together with HRR.
…cept (ec)dhe + external_psk + resumption
I'm not familiar with hello retries, but do I understand correctly that this PR also enables support for external PSK with initial authentication (so one could use PSK instead of certs)? |
@sshock That's correct. IIRC, HelloRetryRequest is a used for renegotiating the ECDHE algorithm being used in case the client's initial offer did not match what the server can handle. But when PSK is used, it would be unlikely that the client and server to initially disagree on which ECDHE algorithm to be used considering that they've already agreed on the PSK to be used. That's what the original comment says. I'm not sure if we are going to merge this PR soon or ever, but I believe it works. |
Ok. I may do some testing with it and let you know how it works for me. |
I created a new PR with these changes rebased against the latest master and all conflicts resolved. I tried to be careful, but there were a lot of conflicts, including 2 lines of code that I was unable to find a corresponding location for in the latest code, so I don't know if they are needed or not. I am able to confirm that the new pre-shared-key test is passing, both with |
I modified the It's calling /* build the raw nsk */
ret = encode_session_identifier(tls->ctx, &session_id, ticket_age_add, ptls_iovec_init(NULL, 0), tls->key_schedule,
tls->server_name, tls->key_share->id, tls->cipher_suite->id, tls->negotiated_protocol); I'm not a TLS expert (yet 😄), so I'm not even sure if it should be sending a new session ticket, but I believe it is doing that because can_send_session_ticket gets set to true in And the reason Any advice on how this should be corrected? |
Sounds like we have a bug in By setting |
Thanks @kazuho. This workaround worked so that I am no longer encountering a seg fault on the server side. But now I am running into a PTLS_ALERT_ILLEGAL_PARAMETER on the client side, because the server is sending an early data extension (although the length is 0), but the client isn't expecting that:
The server is sending a 0-length early data here in the /* send EncryptedExtensions */ block:
So one of these implications doesn't make sense (or the tls->client.using_early_data should have gotten set):
I'll have to go study the RFC, because I don't understand how handshake secrets and traffic protection relate to early data. Update: I understand now from RFC sections 4.2.10 and 4.2.11 that the early_data extension is just an indicator and is supposed to be empty (the only exception is in a NewSessionTicket where it only contains a uint32 max_early_data_size). The early_data extension from the server indicates that it accepts the early data from the client, so it is indeed incorrect for the server to send that when the client has not. Update: I found the bug. The new code in try_psk_handshake for external psk is setting *accept_early_data = 1 unconditionally; it needs to only do that if the client sent early data (if ch->psk.early_data_indication). I'll be adding this one-line fix to my PR sometime today. |
Here's what I noticed when I tried to connect between sample client and server with picotls, wolfssl, and openssl: One key difference I noticed when viewing everything in wireshark was that wolfssl and openssl servers respond with a key_share whereas picotls doesn't. But I think that may be because picotls is choosing to do PSK-only mode, whereas the others are doing PSK with (EC)DHE, so maybe it's fine. |
This is the block of code on the openssl server that is failing (with picotls client) is here in tls_parse_ctos_psk(): if (PACKET_remaining(&binder) != hashsize) {
SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PARSE_CTOS_PSK,
SSL_R_BAD_EXTENSION);
goto err;
} I think it's because the PSK Binder hmac is 48 bytes (whereas with wolfssl and openssl it is 32 bytes). It appears the picotls client is using SHA-384 here, but I think it needs to default to SHA-256. From RFC 8446 section 4.2.11:
Update: What I've learned about PSK and the cipher suite: From section 4.2.11 mentioned above, I've learned that the hash algorithm MUST be established beforehand or default to SHA-256. From section 4.2.10, I've learned that in order to accept early data, the server must choose the cipher suite associated with the selected PSK, which for an external PSK was "provisioned along with the key". But the RFC doesn't indicate exactly how or where clients and servers should store this associated cipher suite. Which means, unless the client and server have predetermined beforehand which cipher suite to use with a specific external PSK, early data can't work. However, external PSK can still work fine (without early data), but the client must default to SHA-256 for the hash algorithm, and the server must select a compatible cipher suite (e.g., TLS_AES_128_GCM_SHA256). |
I think I found another issue: picotls needs to use "ext binder" not "res binder" for the binder key label in this case where we are doing external PSK not resumption. Update: This is confirmed in Section 7.1 of the RFC:
I'll work on fixing this and the hash algorithm in my PR. It should be pretty easy. |
I made two fixes to the client side, and now the picotls cli client is able to connect and communicate with both wolfssl and openssl servers: |
I made the following fixes to the server side today:
As a result, things have improved in my interop testing: |
Good news. I was able to do more testing with wolfssl and openssl clients and get them working against picotls server with only one real failure remaining in one scenario (when wolfssl presents both ke modes). But at this point I'm ready to call it good, and that could even be a bug in wolfssl. @kazuho could we consider merging my PR? It would be great if I didn't have to maintain it in a fork. |
@sshock Thank you for all the work and coming up with a fix. I'll take a look at your code next work. |
should we close this out since we have #481 ? |
At the moment, external PSK cannot be used together with retry.
Current design of picotls is based on the assumption that (ec)dhe will be used when a HelloRetryRequest is involved. Ideally we should support handshake that uses both retry and external PSK, but that does not need to happen now.