Cleanup

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));

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,19 +648,29 @@ 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);
         }
 
         if (!s2n_kem_group_is_available(kem_group)) {
             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));
     }
 

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));
     }

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);

tls/extensions/s2n_server_key_share.c

@@ -388,15 +388,15 @@ 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;
         POSIX_GUARD(s2n_set_hello_retry_required(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;
 }