Make S2N_CERT_AUTH_OPTIONAL the default for clients (aws#4390)

tests/saw/spec/handshake/handshake_io_lowlevel.saw

@@ -154,8 +154,13 @@ let setup_connection_common chosen_psk_null = do {
    cork_val <- crucible_fresh_var "corked" (llvm_int 2);
    crucible_ghost_value corked cork_val;
 
+   // We assume that the client_cert_auth_type_overridden flag in s2n_connection is set.
+   // If that flag is not set, auth_type / "cca_type" could be determined by the config
+   // or just set to a default value. Since we're primarily interested in the handshake
+   // here and the end result is the same, just consider the connection auth_type.
    cca_ov <- crucible_fresh_var "cca_ov" (llvm_int 8);
    crucible_points_to (cca_type_ov pconn) (crucible_term cca_ov);
+   crucible_equal (crucible_term cca_ov) (crucible_term {{1 : [8]}});
 
    cca <- crucible_fresh_var "cca" (llvm_int 32);
    crucible_points_to (cca_type pconn) (crucible_term cca);
@@ -175,9 +180,6 @@ let setup_connection_common chosen_psk_null = do {
    config <- crucible_alloc (llvm_struct "struct.s2n_config");
    crucible_points_to (conn_config pconn) config;
 
-   config_cca <- crucible_fresh_var "config_cca" (llvm_int 32);
-   crucible_points_to (config_cca_type config) (crucible_term config_cca);
-
    cak <- crucible_alloc (llvm_struct "struct.s2n_cert_chain_and_key");
    crucible_points_to (conn_chain_and_key pconn) cak;
 
@@ -226,8 +228,6 @@ let setup_connection_common chosen_psk_null = do {
 
    no_client_cert <- crucible_fresh_var "no_client_cert" (llvm_int 8);
 
-   let client_cert_auth_type = {{ if cca_ov != 0 then cca else config_cca }};
-
    npn_negotiated <- llvm_fresh_var "npn_negotiated" (llvm_int 1);
    llvm_points_to_bitfield pconn "npn_negotiated" (llvm_term npn_negotiated);
    let npn_negotiated_bit = {{ bv1_to_bit npn_negotiated }};
@@ -246,8 +246,8 @@ let setup_connection_common chosen_psk_null = do {
                             ((ocsp_flag == 1) && (status_size > 0)) ||
                             ((mode == 1) && (ocsp_flag == 1))
                      ,resume_from_cache = False
-                     ,client_auth_flag = if mode == S2N_CLIENT then client_cert_auth_type == S2N_CERT_AUTH_REQUIRED else
-                                         if mode == S2N_SERVER then client_cert_auth_type != S2N_CERT_AUTH_NONE     else False
+                     ,client_auth_flag = if mode == S2N_CLIENT then cca == S2N_CERT_AUTH_REQUIRED else
+                                         if mode == S2N_SERVER then cca != S2N_CERT_AUTH_NONE     else False
                      ,no_client_cert = no_client_cert != zero
                      ,actual_protocol_version = version
                      ,early_data_state = early_data_state
@@ -292,9 +292,7 @@ let s2n_allowed_to_cache_connection_spec = do {
     crucible_return (crucible_term {{ 0 : [32] }});
 };
 
-
-let s2n_connection_get_client_auth_type_spec = do{
-
+let s2n_connection_get_client_auth_type_spec = do {
     pconn <- crucible_alloc_readonly (llvm_struct "struct.s2n_connection");
     auth_type <- crucible_alloc (llvm_int 32);
 
@@ -304,18 +302,28 @@ let s2n_connection_get_client_auth_type_spec = do{
     config <- crucible_alloc_readonly (llvm_struct "struct.s2n_config");
     crucible_points_to (conn_config pconn) config;
 
+    config_cca_ov <- llvm_fresh_var "config_cca_ov" (llvm_int 8);
+    crucible_points_to (crucible_field config "client_cert_auth_type_overridden") (crucible_term config_cca_ov);
+
     config_cca <- crucible_fresh_var "config_cca" (llvm_int 32);
     crucible_points_to (config_cca_type config) (crucible_term config_cca);
 
     cca <- crucible_fresh_var "cca" (llvm_int 32);
     crucible_points_to (cca_type pconn) (crucible_term cca);
 
+    mode <- crucible_fresh_var "mode" (llvm_int 32);
+    crucible_points_to (conn_mode pconn) (crucible_term mode);
+
     crucible_execute_func [pconn, auth_type];
 
-    crucible_points_to (auth_type) (crucible_term {{if cca_ov != zero then cca else config_cca}});
+    crucible_points_to (auth_type) (crucible_term {{
+        if cca_ov != zero then cca else
+        if config_cca_ov != zero then config_cca else
+        if mode == S2N_CLIENT then S2N_CERT_AUTH_OPTIONAL else
+        S2N_CERT_AUTH_NONE
+    }});
 
     crucible_return (crucible_term {{ 0 : [32] }});
-
 };
 
 // Specification for s2n_conn_set_handshake_type that sets up simulation of it

tests/saw/spec/handshake/s2n_handshake_io.cry

@@ -1112,6 +1112,7 @@ S2N_CLIENT = 1
 //S2N cert auth types
 S2N_CERT_AUTH_NONE = 0
 S2N_CERT_AUTH_REQUIRED = 1
+S2N_CERT_AUTH_OPTIONAL = 2
 
 //S2N early data states
 S2N_EARLY_DATA_ACCEPTED = 3

tests/unit/s2n_client_auth_handshake_test.c

@@ -379,6 +379,7 @@ int main(int argc, char **argv)
         EXPECT_SUCCESS(s2n_connection_set_config(server, config));
 
         /* Enable client auth on the server, but not on the client */
+        EXPECT_SUCCESS(s2n_connection_set_client_auth_type(client, S2N_CERT_AUTH_NONE));
         EXPECT_SUCCESS(s2n_connection_set_client_auth_type(server, S2N_CERT_AUTH_OPTIONAL));
 
         DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close);
@@ -389,5 +390,31 @@ int main(int argc, char **argv)
                 S2N_ERR_UNEXPECTED_CERT_REQUEST);
     };
 
+    /* By default, client accepts certificate requests */
+    {
+        DEFER_CLEANUP(struct s2n_config *client_config = s2n_config_new(), s2n_config_ptr_free);
+        EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(client_config));
+        DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(client);
+        EXPECT_SUCCESS(s2n_connection_set_config(client, client_config));
+
+        DEFER_CLEANUP(struct s2n_config *server_config = s2n_config_new(), s2n_config_ptr_free);
+        EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_config, chain_and_key));
+        DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(server);
+        EXPECT_SUCCESS(s2n_connection_set_config(server, server_config));
+
+        /* Enable client auth on the server */
+        EXPECT_SUCCESS(s2n_connection_set_client_auth_type(server, S2N_CERT_AUTH_OPTIONAL));
+
+        DEFER_CLEANUP(struct s2n_test_io_pair io_pair = { 0 }, s2n_io_pair_close);
+        EXPECT_SUCCESS(s2n_io_pair_init_non_blocking(&io_pair));
+        EXPECT_SUCCESS(s2n_connections_set_io_pair(client, server, &io_pair));
+
+        EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server, client));
+    };
+
     END_TEST();
 }

tests/unit/s2n_connection_test.c

@@ -929,6 +929,59 @@ int main(int argc, char **argv)
         EXPECT_EQUAL(recv_count, expected_recv_count);
     }
 
+    /* Test s2n_connection_get_client_auth_type */
+    {
+        DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
+
+        DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(client);
+        EXPECT_SUCCESS(s2n_connection_set_config(client, config));
+
+        DEFER_CLEANUP(struct s2n_connection *server = s2n_connection_new(S2N_SERVER),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(server);
+        EXPECT_SUCCESS(s2n_connection_set_config(server, config));
+
+        /* Test: use defaults if no overrides set */
+        {
+            /* Ensure that defaults are still used if the override flags are not set,
+             * even if client_cert_auth_type is somehow set.
+             */
+            server->client_cert_auth_type = S2N_CERT_AUTH_REQUIRED;
+            client->client_cert_auth_type = S2N_CERT_AUTH_REQUIRED;
+
+            s2n_cert_auth_type auth_type = S2N_CERT_AUTH_REQUIRED;
+            EXPECT_SUCCESS(s2n_connection_get_client_auth_type(client, &auth_type));
+            EXPECT_EQUAL(auth_type, S2N_CERT_AUTH_OPTIONAL);
+            EXPECT_SUCCESS(s2n_connection_get_client_auth_type(server, &auth_type));
+            EXPECT_EQUAL(auth_type, S2N_CERT_AUTH_NONE);
+        };
+
+        /* Test: use config overrides if set */
+        {
+            EXPECT_SUCCESS(s2n_config_set_client_auth_type(config, S2N_CERT_AUTH_REQUIRED));
+
+            s2n_cert_auth_type auth_type = S2N_CERT_AUTH_NONE;
+            EXPECT_SUCCESS(s2n_connection_get_client_auth_type(client, &auth_type));
+            EXPECT_EQUAL(auth_type, S2N_CERT_AUTH_REQUIRED);
+            EXPECT_SUCCESS(s2n_connection_get_client_auth_type(server, &auth_type));
+            EXPECT_EQUAL(auth_type, S2N_CERT_AUTH_REQUIRED);
+        };
+
+        /* Test: use connection overrides if set */
+        {
+            EXPECT_SUCCESS(s2n_connection_set_client_auth_type(client, S2N_CERT_AUTH_NONE));
+            EXPECT_SUCCESS(s2n_connection_set_client_auth_type(server, S2N_CERT_AUTH_OPTIONAL));
+
+            s2n_cert_auth_type auth_type = S2N_CERT_AUTH_REQUIRED;
+            EXPECT_SUCCESS(s2n_connection_get_client_auth_type(client, &auth_type));
+            EXPECT_EQUAL(auth_type, S2N_CERT_AUTH_NONE);
+            EXPECT_SUCCESS(s2n_connection_get_client_auth_type(server, &auth_type));
+            EXPECT_EQUAL(auth_type, S2N_CERT_AUTH_OPTIONAL);
+        };
+    };
+
     EXPECT_SUCCESS(s2n_cert_chain_and_key_free(ecdsa_chain_and_key));
     EXPECT_SUCCESS(s2n_cert_chain_and_key_free(rsa_chain_and_key));
     END_TEST();

tests/unit/s2n_handshake_test.c

@@ -384,7 +384,6 @@ int main(int argc, char **argv)
             server_config->security_policy = &security_policy;
 
             EXPECT_NOT_NULL(client_config = s2n_config_new());
-            client_config->client_cert_auth_type = S2N_CERT_AUTH_NONE;
             client_config->check_ocsp = 0;
             client_config->disable_x509_validation = 1;
             client_config->security_policy = &security_policy;
@@ -420,7 +419,6 @@ int main(int argc, char **argv)
 
             EXPECT_NOT_NULL(client_config = s2n_config_new());
             EXPECT_SUCCESS(s2n_config_set_cipher_preferences(client_config, "20200207"));
-            client_config->client_cert_auth_type = S2N_CERT_AUTH_NONE;
             client_config->check_ocsp = 0;
             client_config->disable_x509_validation = 1;
 

tests/unit/s2n_tls13_handshake_state_machine_test.c

@@ -762,7 +762,7 @@ int main(int argc, char **argv)
         conn->session_ticket_status = S2N_NEW_TICKET;
 
         /* Ensure CLIENT_AUTH is set */
-        conn->config->client_cert_auth_type = S2N_CERT_AUTH_REQUIRED;
+        EXPECT_SUCCESS(s2n_connection_set_client_auth_type(conn, S2N_CERT_AUTH_REQUIRED));
 
         /* Ensure TLS12_PERFECT_FORWARD_SECRECY is set by choosing a cipher suite with is_ephemeral=1 on the kex */
         conn->secure->cipher_suite = &s2n_dhe_rsa_with_chacha20_poly1305_sha256;

tls/s2n_config.c

@@ -96,10 +96,6 @@ static int s2n_config_init(struct s2n_config *config)
     config->encrypt_decrypt_key_lifetime_in_nanos = S2N_TICKET_ENCRYPT_DECRYPT_KEY_LIFETIME_IN_NANOS;
     config->decrypt_key_lifetime_in_nanos = S2N_TICKET_DECRYPT_KEY_LIFETIME_IN_NANOS;
     config->async_pkey_validation_mode = S2N_ASYNC_PKEY_VALIDATION_FAST;
-
-    /* By default, only the client will authenticate the Server's Certificate. The Server does not request or
-     * authenticate any client certificates. */
-    config->client_cert_auth_type = S2N_CERT_AUTH_NONE;
     config->check_ocsp = 1;
 
     config->client_hello_cb_mode = S2N_CLIENT_HELLO_CB_BLOCKING;
@@ -222,7 +218,6 @@ struct s2n_config *s2n_fetch_default_config(void)
 int s2n_config_set_unsafe_for_testing(struct s2n_config *config)
 {
     POSIX_ENSURE(s2n_in_test(), S2N_ERR_NOT_IN_TEST);
-    config->client_cert_auth_type = S2N_CERT_AUTH_NONE;
     config->check_ocsp = 0;
     config->disable_x509_validation = 1;
 
@@ -415,6 +410,7 @@ int s2n_config_get_client_auth_type(struct s2n_config *config, s2n_cert_auth_typ
 int s2n_config_set_client_auth_type(struct s2n_config *config, s2n_cert_auth_type client_auth_type)
 {
     POSIX_ENSURE_REF(config);
+    config->client_cert_auth_type_overridden = 1;
     config->client_cert_auth_type = client_auth_type;
     return 0;
 }

tls/s2n_config.h

@@ -144,6 +144,16 @@ struct s2n_config {
 
     s2n_ct_support_level ct_type;
 
+    /* Track whether the application has overriden the default client auth type.
+     * Clients and servers have different default client auth behavior, and this
+     * config could apply to either.
+     * This should be a bitflag, but that change is blocked on the SAW proofs.
+     */
+    uint8_t client_cert_auth_type_overridden;
+
+    /* Whether or not the client should authenticate itself to the server.
+     * Only used if client_cert_auth_type_overridden is true.
+     */
     s2n_cert_auth_type client_cert_auth_type;
 
     s2n_alert_behavior alert_behavior;

tls/s2n_connection.c

@@ -59,6 +59,9 @@
 #define S2N_SET_KEY_SHARE_LIST_EMPTY(keyshares) (keyshares |= 1)
 #define S2N_SET_KEY_SHARE_REQUEST(keyshares, i) (keyshares |= (1 << (i + 1)))
 
+static S2N_RESULT s2n_connection_and_config_get_client_auth_type(const struct s2n_connection *conn,
+        const struct s2n_config *config, s2n_cert_auth_type *client_cert_auth_type);
+
 /* Allocates and initializes memory for a new connection.
  *
  * Since customers can reuse a connection, ensure that values on the connection are
@@ -296,11 +299,8 @@ int s2n_connection_set_config(struct s2n_connection *conn, struct s2n_config *co
 
     s2n_x509_validator_wipe(&conn->x509_validator);
 
-    s2n_cert_auth_type auth_type = config->client_cert_auth_type;
-
-    if (conn->client_cert_auth_type_overridden) {
-        auth_type = conn->client_cert_auth_type;
-    }
+    s2n_cert_auth_type auth_type = S2N_CERT_AUTH_NONE;
+    POSIX_GUARD_RESULT(s2n_connection_and_config_get_client_auth_type(conn, config, &auth_type));
 
     int8_t dont_need_x509_validation = (conn->mode == S2N_SERVER) && (auth_type == S2N_CERT_AUTH_NONE);
 
@@ -748,19 +748,37 @@ int s2n_connection_get_protocol_preferences(struct s2n_connection *conn, struct
     return 0;
 }
 
-int s2n_connection_get_client_auth_type(struct s2n_connection *conn, s2n_cert_auth_type *client_cert_auth_type)
+static S2N_RESULT s2n_connection_and_config_get_client_auth_type(const struct s2n_connection *conn,
+        const struct s2n_config *config, s2n_cert_auth_type *client_cert_auth_type)
 {
-    POSIX_ENSURE_REF(conn);
-    POSIX_ENSURE_REF(client_cert_auth_type);
+    RESULT_ENSURE_REF(conn);
+    RESULT_ENSURE_REF(config);
+    RESULT_ENSURE_REF(client_cert_auth_type);
 
     if (conn->client_cert_auth_type_overridden) {
         *client_cert_auth_type = conn->client_cert_auth_type;
+    } else if (config->client_cert_auth_type_overridden) {
+        *client_cert_auth_type = config->client_cert_auth_type;
+    } else if (conn->mode == S2N_CLIENT) {
+        /* Clients should default to "Optional" so that they handle any
+         * CertificateRequests sent by the server.
+         */
+        *client_cert_auth_type = S2N_CERT_AUTH_OPTIONAL;
     } else {
-        POSIX_ENSURE_REF(conn->config);
-        *client_cert_auth_type = conn->config->client_cert_auth_type;
+        /* Servers should default to "None" so that they send no CertificateRequests. */
+        *client_cert_auth_type = S2N_CERT_AUTH_NONE;
     }
 
-    return 0;
+    return S2N_RESULT_OK;
+}
+
+int s2n_connection_get_client_auth_type(struct s2n_connection *conn,
+        s2n_cert_auth_type *client_cert_auth_type)
+{
+    POSIX_ENSURE_REF(conn);
+    POSIX_GUARD_RESULT(s2n_connection_and_config_get_client_auth_type(
+            conn, conn->config, client_cert_auth_type));
+    return S2N_SUCCESS;
 }
 
 int s2n_connection_set_client_auth_type(struct s2n_connection *conn, s2n_cert_auth_type client_cert_auth_type)

tls/s2n_connection.h

@@ -224,15 +224,15 @@ struct s2n_connection {
     /* The PRF needs some storage elements to work with */
     struct s2n_prf_working_space *prf_space;
 
-    /* Whether to use client_cert_auth_type stored in s2n_config or in this s2n_connection.
-     *
-     * By default the s2n_connection will defer to s2n_config->client_cert_auth_type on whether or not to use Client Auth.
-     * But users can override Client Auth at the connection level using s2n_connection_set_client_auth_type() without mutating
-     * s2n_config since s2n_config can be shared between multiple s2n_connections. */
+    /* Indicates whether the application has overridden the client auth behavior
+     * inherited from the config.
+     * This should be a bitflag, but that change is blocked on the SAW proofs.
+     */
     uint8_t client_cert_auth_type_overridden;
 
-    /* Whether or not the s2n_connection should require the Client to authenticate itself to the server. Only used if
-     * client_cert_auth_type_overridden is non-zero. */
+    /* Whether or not the client should authenticate itself to the server.
+     * Only used if client_cert_auth_type_overridden is true.
+     */
     s2n_cert_auth_type client_cert_auth_type;
 
     /* Our workhorse stuffers, used for buffering the plaintext