From e923ece831f2e637fe91a03cbb9664f8d9c00035 Mon Sep 17 00:00:00 2001 From: Alex Weibel Date: Mon, 20 May 2024 16:14:13 -0700 Subject: [PATCH] Cleanup --- ..._extensions_server_key_share_select_test.c | 1 - tests/unit/s2n_tls13_pq_handshake_test.c | 25 +++++++++---------- tls/extensions/s2n_client_key_share.c | 6 ++--- tls/extensions/s2n_client_supported_groups.c | 2 ++ tls/extensions/s2n_server_key_share.c | 8 +++--- 5 files changed, 20 insertions(+), 22 deletions(-) diff --git a/tests/unit/s2n_extensions_server_key_share_select_test.c b/tests/unit/s2n_extensions_server_key_share_select_test.c index 08aa1439b10..04cf1f6cb7e 100644 --- a/tests/unit/s2n_extensions_server_key_share_select_test.c +++ b/tests/unit/s2n_extensions_server_key_share_select_test.c @@ -186,7 +186,6 @@ int main() EXPECT_EQUAL(server_params->kem_group, kem_group1); EXPECT_EQUAL(server_params->kem_params.kem, kem_group1->kem); EXPECT_EQUAL(server_params->ecc_params.negotiated_curve, kem_group1->curve); - EXPECT_NULL(server_conn->kex_params.client_kem_group_params.kem_group); EXPECT_NULL(server_conn->kex_params.server_ecc_evp_params.negotiated_curve); EXPECT_TRUE(s2n_is_hello_retry_handshake(server_conn)); diff --git a/tests/unit/s2n_tls13_pq_handshake_test.c b/tests/unit/s2n_tls13_pq_handshake_test.c index 5e1a7a4a084..839e3cfbc6a 100644 --- a/tests/unit/s2n_tls13_pq_handshake_test.c +++ b/tests/unit/s2n_tls13_pq_handshake_test.c @@ -110,12 +110,6 @@ int s2n_test_tls13_pq_handshake(const struct s2n_security_policy *client_sec_pol /* XOR check: can expect to negotiate either a KEM group, or a classic EC curve, but not both/neither */ POSIX_ENSURE((expected_kem_group == NULL) != (expected_curve == NULL), S2N_ERR_SAFETY); - if (expected_kem_group != NULL) { - const struct s2n_kem_group *predicted_kem_group = s2n_get_predicted_negotiated_kem_group(client_sec_policy->kem_preferences, server_sec_policy->kem_preferences); - POSIX_ENSURE_REF(predicted_kem_group); - POSIX_ENSURE_EQ(expected_kem_group->iana_id, predicted_kem_group->iana_id); - } - /* Set up connections */ struct s2n_connection *client_conn = NULL, *server_conn = NULL; POSIX_ENSURE_REF(client_conn = s2n_connection_new(S2N_CLIENT)); @@ -642,12 +636,7 @@ int main() bool len_prefix_expected = vector->len_prefix_expected; if (!s2n_pq_is_enabled()) { - /* If PQ is disabled, for older policies we expect to negotiate - * x25519 ECDH if available. Newer policies only include NIST - * curves, so if the server policy doesn't contain x25519, modify - * that expectation to a NIST curve. - */ - + EXPECT_TRUE(client_policy->ecc_preferences->count > 0); const struct s2n_ecc_named_curve *client_default = client_policy->ecc_preferences->ecc_curves[0]; const struct s2n_ecc_named_curve *predicted_curve = s2n_get_predicted_negotiated_ecdhe_curve(client_policy, server_policy); @@ -659,12 +648,14 @@ int main() curve = &s2n_ecc_curve_secp256r1; } - /* The client's preferred curve will be a higher priority than the default if both sides support TLS 1.3 since it can be chosen in 1-RTT. */ + /* The client's preferred curve will be a higher priority than the default if both sides support TLS 1.3, + * and if the client's default can be chosen by the server in 1-RTT. */ if (s2n_security_policy_supports_tls13(client_policy) && s2n_security_policy_supports_tls13(server_policy) && s2n_ecc_preferences_includes_curve(server_policy->ecc_preferences, client_default->iana_id)) { curve = client_default; } + /* Finally, confirm that the expected curve listed in the test vector matches the output of s2n_get_predicted_negotiated_ecdhe_curve() */ EXPECT_EQUAL(curve->iana_id, predicted_curve->iana_id); } @@ -672,6 +663,14 @@ int main() kem_group = NULL; } + if (kem_group != NULL) { + const struct s2n_kem_group *predicted_kem_group = s2n_get_predicted_negotiated_kem_group(client_policy->kem_preferences, server_policy->kem_preferences); + 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); + } + EXPECT_SUCCESS(s2n_test_tls13_pq_handshake(client_policy, server_policy, kem_group, curve, hrr_expected, len_prefix_expected)); } diff --git a/tls/extensions/s2n_client_key_share.c b/tls/extensions/s2n_client_key_share.c index e7d77f1379b..cb066556fcd 100644 --- a/tls/extensions/s2n_client_key_share.c +++ b/tls/extensions/s2n_client_key_share.c @@ -438,10 +438,6 @@ static int s2n_client_key_share_recv(struct s2n_connection *conn, struct s2n_stu /* During a retry, the client should only have sent one keyshare */ POSIX_ENSURE(!s2n_is_hello_retry_handshake(conn) || keyshare_count == 1, S2N_ERR_BAD_MESSAGE); - /* Get the client's preferred params for the KeyShares that were actually sent */ - struct s2n_ecc_evp_params *client_ecc_params = &conn->kex_params.client_ecc_evp_params; - struct s2n_kem_group_params *client_pq_params = &conn->kex_params.client_kem_group_params; - /** * If there were no matching key shares, then we received an empty key share extension * or we didn't match a key share with a supported group. We should send a retry. @@ -451,6 +447,8 @@ static int s2n_client_key_share_recv(struct s2n_connection *conn, struct s2n_stu *# compatible "key_share" extension in the initial ClientHello, the *# server MUST respond with a HelloRetryRequest (Section 4.1.4) message. **/ + struct s2n_ecc_evp_params *client_ecc_params = &conn->kex_params.client_ecc_evp_params; + struct s2n_kem_group_params *client_pq_params = &conn->kex_params.client_kem_group_params; if (!client_pq_params->kem_group && !client_ecc_params->negotiated_curve) { POSIX_GUARD(s2n_set_hello_retry_required(conn)); } diff --git a/tls/extensions/s2n_client_supported_groups.c b/tls/extensions/s2n_client_supported_groups.c index e5f2a71a597..cc30f0c9dff 100644 --- a/tls/extensions/s2n_client_supported_groups.c +++ b/tls/extensions/s2n_client_supported_groups.c @@ -106,6 +106,7 @@ S2N_RESULT s2n_supported_groups_parse_count(struct s2n_stuffer *extension, uint1 static int s2n_client_supported_groups_recv_iana_id(struct s2n_connection *conn, uint16_t iana_id) { POSIX_ENSURE_REF(conn); + const struct s2n_ecc_preferences *ecc_pref = NULL; POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref)); POSIX_ENSURE_REF(ecc_pref); @@ -141,6 +142,7 @@ static int s2n_client_supported_groups_recv_iana_id(struct s2n_connection *conn, static int s2n_choose_supported_group(struct s2n_connection *conn) { POSIX_ENSURE_REF(conn); + const struct s2n_ecc_preferences *ecc_pref = NULL; POSIX_GUARD(s2n_connection_get_ecc_preferences(conn, &ecc_pref)); POSIX_ENSURE_REF(ecc_pref); diff --git a/tls/extensions/s2n_server_key_share.c b/tls/extensions/s2n_server_key_share.c index 5cac32684f6..bd2ef8acc09 100644 --- a/tls/extensions/s2n_server_key_share.c +++ b/tls/extensions/s2n_server_key_share.c @@ -388,7 +388,7 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) return S2N_SUCCESS; } - /* Option 2: Otherwise, if any PQ Hybrid Groups can be negotiated in 2-RTT's select it.*/ + /* Option 2: Otherwise, if any PQ Hybrid Groups can be negotiated in 2-RTT's select that one. */ if (server_kem_group != NULL) { /* Null out any available ECC curves so that they won't be sent in the ClientHelloRetry */ conn->kex_params.server_ecc_evp_params.negotiated_curve = NULL; @@ -396,7 +396,7 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) return S2N_SUCCESS; } - /* Option 3: Otherwise, select a mutually supported classical ECC curve that can be negotiated in 1-RTT */ + /* Option 3: Otherwise, if there is a mutually supported classical ECDHE-only group can be negotiated in 1-RTT, select that one */ if (client_curve) { conn->kex_params.server_ecc_evp_params.negotiated_curve = conn->kex_params.client_ecc_evp_params.negotiated_curve; conn->kex_params.server_kem_group_params.kem_group = NULL; @@ -405,8 +405,8 @@ int s2n_extensions_server_key_share_select(struct s2n_connection *conn) return S2N_SUCCESS; } - /* Option 4: Server and client have mutually supported groups, but the client did not send key shares for any of - * them. Send a HelloRetryRequest 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; }