Perform 2-RTT Handshake to upgrade to PQ when possible #4526
Conversation
alexw91
force-pushed
the
2-rtt-pq-handshake
branch
4 times, most recently
from
e923ece
to
e2d4a2c
Compare
alexw91
requested review from
lrstewart and
WillChilds-Klein
and removed request for
lrstewart
alexw91
force-pushed
the
2-rtt-pq-handshake
branch
from
e2d4a2c
to
bb5f254
Compare
alexw91
force-pushed
the
2-rtt-pq-handshake
branch
from
bb5f254
to
2013258
Compare
There was a problem hiding this comment.
code changes look good, just a few questions and nit-picks. perhaps consider adding two additional test cases:
- an integration test asserting that a client sending a server-usupported PQ keyshare negotiates Kyber512
- a unit test with the client sending multiple keyshares (perhaps the first unsupported by server, second supported by server)
| PTR_ENSURE_REF(server_group); | ||
| if (s2n_kem_group_is_available(client_default) && s2n_kem_group_is_available(server_group) | ||
| && client_default->iana_id == server_group->iana_id | ||
| && s2n_kem_group_is_available(client_default) == s2n_kem_group_is_available(server_group)) { |
There was a problem hiding this comment.
nit: here and on L60, i think the equality check is redundant. s2n_kem_group_is_available's return type is boolean.
| const struct s2n_kem_group *client_default = client_prefs->tls13_kem_groups[0]; | ||
| for (int i = 0; i < server_prefs->tls13_kem_group_count; i++) { | ||
| const struct s2n_kem_group *server_group = server_prefs->tls13_kem_groups[i]; | ||
| PTR_ENSURE_REF(client_default); |
There was a problem hiding this comment.
nit: client_default is const so should be invariant across the loop. the assertion can be moved outside the loop.
| for (int i = 0; i < server_prefs->tls13_kem_group_count; i++) { | ||
| const struct s2n_kem_group *server_group = server_prefs->tls13_kem_groups[i]; | ||
|
|
||
| for (int j = 0; j < client_prefs->tls13_kem_group_count; j++) { |
There was a problem hiding this comment.
we've already tested client_prefs->tls13_kem_groups[0] against all server pref KEM groups. should j start at 1? if so, suggest returning NULL before this loop if client_prefs->tls13_kem_group_count == 1
| .client_policy = &security_policy_pq_tls_1_3_2023_06_01, | ||
| .server_policy = &security_policy_pq_tls_1_0_2021_05_24, | ||
| .expected_kem_group = &s2n_x25519_kyber_512_r3, | ||
| .expected_curve = NULL, |
There was a problem hiding this comment.
if PQ is disabled, wouldn't we expect a curve to be negotiated here?
There was a problem hiding this comment.
There is a post-processing step that updates these values further down on line 638 if PQ is disabled.
| POSIX_ENSURE_REF(predicted_kem_group); | ||
|
|
||
| /* Confirm that the expected KEM Group listed in the test vector matches the output of s2n_get_predicted_negotiated_kem_group() */ | ||
| POSIX_ENSURE_EQ(kem_group->iana_id, predicted_kem_group->iana_id); | ||
| } | ||
|
|
There was a problem hiding this comment.
should we add an assertion that precisely one of kem_group or curve is non-NULL?
There was a problem hiding this comment.
That check is the first check done within s2n_test_tls13_pq_handshake() just a few lines further down.
| /* Server and client have mutually supported groups, but the client did not send key | ||
| * shares for any of them. Send HRR indicating the server's preference. */ | ||
| /* Option 4: Server and client have at least 1 mutually supported group, but the client did not send key shares for | ||
| * any of them. Send a HelloRetryRequest indicating the server's preference. */ | ||
| POSIX_GUARD(s2n_set_hello_retry_required(conn)); | ||
| return S2N_SUCCESS; |
There was a problem hiding this comment.
what if client and server supported groups/curves have no overlap?
is that checked before s2n_extensions_server_key_share_select is called?
There was a problem hiding this comment.
If there's no overlap, the s2n server will have returned an error to the client earlier in the handshake. At this point there should be at least 1 SupportedGroup that is overlapping between client and server.
| @@ -354,44 +354,59 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) | |||
| POSIX_GUARD(s2n_connection_get_kem_preferences(conn, &kem_pref)); | |||
There was a problem hiding this comment.
neither ecc_pref nor kem_pref appear to be used anywhere
| /* When client sent valid keyshares | ||
| * only for ECC, server should choose curves[0] and not send HRR. */ | ||
| /* When client sent KeyShares only for ECC but also supports PQ, server should choose PQ and send HRR. */ |
There was a problem hiding this comment.
It makes sense to me to retry if the client sent an invalid PQ keyshare but supports valid PQ, but not to retry if the client didn't send any PQ. Are you sure about this behavior? Wouldn't no PQ share indicate that the client doesn't really want to do PQ?
It also seems like that would meet the usecase of upgrading from one PQ algorithm to another, without triggering retries for clients that might not care about PQ.
There was a problem hiding this comment.
Are you sure about this behavior? Wouldn't no PQ share indicate that the client doesn't really want to do PQ?
No, it wouldn't. There may be clients who do not want to waste unnecessary bandwidth sending PQ KeyShares if they've previously tested and confirmed the endpoints they are connecting to don't yet support PQ, but might support PQ at some point in the future. These clients can decide to indicate that they support PQ in there SupportedGroups extension (2 bytes added to the handshake), without including PQ KeyShare (800-1,568 bytes added) in their KeyShare extension, and only send an ECDHE KeyShare. This behavior is allowed by the TLS 1.3 RFC, and if a client adopt this behavior, those clients would never be able to gracefully upgrade to PQ when server-side PQ deployments occur.
tls/s2n_server_hello_retry.c
Outdated
| if (server_preferred_kem_group != NULL && s2n_kem_group_is_available(server_preferred_kem_group) && s2n_kem_preferences_includes_tls13_kem_group(kem_pref, server_preferred_kem_group->iana_id)) { | ||
| selected_group_in_supported_groups = true; |
There was a problem hiding this comment.
This line was long before but now it's REALLY long. Is there a clearer way to write this if?
alexw91
force-pushed
the
2-rtt-pq-handshake
branch
2 times, most recently
from
34b98d0
to
db306ee
Compare
s2n's current integration tests are fragile and hard to run and debug locally. I'd prefer to test this change using unit tests until this issue is resolved: #4342
These unit tests already exist in this test file. Found some that appear to be testing what you are asking: |
alexw91
force-pushed
the
2-rtt-pq-handshake
branch
from
db306ee
to
46ae0e0
Compare
Resolved issues:
N/A
Description of changes:
Updates s2n to always prefer upgrading to a PQ Hybrid KeyShare whenever possible, even if doing so would require a 2 round trip handshake. s2n's current behavior is to choose the best KeyShare option possible in a 1-RTT handshake first, and only if a 1-RTT is not possible will a KeyShare algorithm that requires a 2-RTT handshake be chosen. After this change, s2n's server-side negotiation logic will consider a Hybrid PQ KeyShare as higher priority than completing the handshake in 1-RTT. This is to ensure that Hybrid PQ KeyShares will be guaranteed to be negotiated during any transition periods between one PQ KeyShare algorithm and another PQ KeyShare algorithm (where a newer PQ algorithm isn't mutually supported yet, but the older PQ algorithm can be negotiated in 2 round trips).
There is further discussion of this change in the IETF mailing list, and in a draft RFC by David Benjamin from Google:
Call-outs:
N/A
Testing:
Unit Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.