fix: better errors for all client auth failures (#4492)

aws · Apr 8, 2024 · 0e57e94 · 0e57e94
1 parent 34ad914
commit 0e57e94
Show file tree

Hide file tree

Showing 5 changed files with 129 additions and 4 deletions.
diff --git a/error/s2n_errno.c b/error/s2n_errno.c
@@ -307,7 +307,9 @@ static const char *no_such_error = "Internal s2n error";
     ERR_ENTRY(S2N_ERR_KTLS_RENEG, "kTLS does not support secure renegotiation") \
     ERR_ENTRY(S2N_ERR_KTLS_KEYUPDATE, "Received KeyUpdate from peer, but kernel does not support updating tls keys") \
     ERR_ENTRY(S2N_ERR_KTLS_KEY_LIMIT, "Reached key encryption limit, but kernel does not support updating tls keys") \
-    ERR_ENTRY(S2N_ERR_UNEXPECTED_CERT_REQUEST, "Client does not support mutual authentication") \
+    ERR_ENTRY(S2N_ERR_UNEXPECTED_CERT_REQUEST, "Client forbids mutual authentication, but server requested a cert") \
+    ERR_ENTRY(S2N_ERR_MISSING_CERT_REQUEST, "Client requires mutual authentication, but server did not request a cert") \
+    ERR_ENTRY(S2N_ERR_MISSING_CLIENT_CERT, "Server requires client certificate") \
     ERR_ENTRY(S2N_INVALID_SERIALIZED_CONNECTION, "Serialized connection is invalid"); \
     /* clang-format on */
 

diff --git a/error/s2n_errno.h b/error/s2n_errno.h
@@ -72,6 +72,8 @@ typedef enum {
     S2N_ERR_DECRYPT,
     S2N_ERR_BAD_MESSAGE,
     S2N_ERR_UNEXPECTED_CERT_REQUEST,
+    S2N_ERR_MISSING_CERT_REQUEST,
+    S2N_ERR_MISSING_CLIENT_CERT,
     S2N_ERR_KEY_INIT,
     S2N_ERR_KEY_DESTROY,
     S2N_ERR_DH_SERIALIZING,

diff --git a/tests/unit/s2n_client_auth_handshake_test.c b/tests/unit/s2n_client_auth_handshake_test.c
@@ -390,6 +390,33 @@ int main(int argc, char **argv)
                 S2N_ERR_UNEXPECTED_CERT_REQUEST);
     };
 
+    /* Test missing certificate request */
+    {
+        DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
+                s2n_connection_ptr_free);
+        EXPECT_NOT_NULL(client);
+        EXPECT_SUCCESS(s2n_connection_set_blinding(client, S2N_SELF_SERVICE_BLINDING));
+
+        DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
+        EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(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, config));
+
+        /* Require client auth on the client, but not on the server */
+        EXPECT_SUCCESS(s2n_connection_set_client_auth_type(client, S2N_CERT_AUTH_REQUIRED));
+        EXPECT_SUCCESS(s2n_connection_set_client_auth_type(server, S2N_CERT_AUTH_NONE));
+
+        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_FAILURE_WITH_ERRNO(s2n_negotiate_test_server_and_client(server, client),
+                S2N_ERR_MISSING_CERT_REQUEST);
+    };
+
     /* By default, client accepts certificate requests */
     {
         DEFER_CLEANUP(struct s2n_config *client_config = s2n_config_new(), s2n_config_ptr_free);
@@ -416,5 +443,90 @@ int main(int argc, char **argv)
         EXPECT_SUCCESS(s2n_negotiate_test_server_and_client(server, client));
     };
 
+    /* Test: all combinations of client and server mutual auth settings produce useful errors
+     *
+     * Customers have struggled to correctly configure client auth in the past.
+     * We should provide very specific and helpful errors to facilitate debugging.
+     * Do not allow any generic errors like S2N_ERR_BAD_MESSAGE.
+     */
+    {
+        const int S2N_CERT_AUTH_DEFAULT = -255;
+        int all_options[] = {
+            S2N_CERT_AUTH_DEFAULT,
+            S2N_CERT_AUTH_NONE,
+            S2N_CERT_AUTH_OPTIONAL,
+            S2N_CERT_AUTH_REQUIRED
+        };
+
+        struct {
+            int client_auth_type;
+            int server_auth_type;
+            bool client_cert_exists;
+            uint8_t version;
+        } test_cases[100] = { 0 };
+        size_t test_case_count = 0;
+
+        for (size_t client_i = 0; client_i < s2n_array_len(all_options); client_i++) {
+            EXPECT_TRUE(test_case_count < s2n_array_len(test_cases));
+            for (size_t server_i = 0; server_i < s2n_array_len(all_options); server_i++) {
+                for (size_t cert_i = 0; cert_i <= 1; cert_i++) {
+                    for (size_t version = S2N_TLS12; version <= S2N_TLS13; version++) {
+                        test_cases[test_case_count].client_auth_type = all_options[client_i];
+                        test_cases[test_case_count].server_auth_type = all_options[server_i];
+                        test_cases[test_case_count].client_cert_exists = (cert_i == 1);
+                        test_cases[test_case_count].version = version;
+                        test_case_count++;
+                    }
+                }
+            }
+        }
+
+        for (size_t i = 0; i < test_case_count; i++) {
+            DEFER_CLEANUP(struct s2n_config *client_config = s2n_config_new(), s2n_config_ptr_free);
+            EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(client_config));
+            if (test_cases[i].client_cert_exists) {
+                EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(client_config, chain_and_key));
+            }
+
+            DEFER_CLEANUP(struct s2n_config *server_config = s2n_config_new(), s2n_config_ptr_free);
+            EXPECT_SUCCESS(s2n_config_set_unsafe_for_testing(server_config));
+            EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(server_config, chain_and_key));
+
+            DEFER_CLEANUP(struct s2n_connection *client = s2n_connection_new(S2N_CLIENT),
+                    s2n_connection_ptr_free);
+            EXPECT_NOT_NULL(client);
+            EXPECT_SUCCESS(s2n_connection_set_blinding(client, S2N_SELF_SERVICE_BLINDING));
+            EXPECT_SUCCESS(s2n_connection_set_config(client, 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_blinding(server, S2N_SELF_SERVICE_BLINDING));
+            EXPECT_SUCCESS(s2n_connection_set_config(server, server_config));
+            server->server_protocol_version = test_cases[i].version;
+
+            if (test_cases[i].client_auth_type != S2N_CERT_AUTH_DEFAULT) {
+                EXPECT_SUCCESS(s2n_connection_set_client_auth_type(client, test_cases[i].client_auth_type));
+            }
+
+            if (test_cases[i].server_auth_type != S2N_CERT_AUTH_DEFAULT) {
+                EXPECT_SUCCESS(s2n_connection_set_client_auth_type(server, test_cases[i].server_auth_type));
+            }
+
+            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));
+
+            int result = s2n_negotiate_test_server_and_client(server, client);
+            EXPECT_EQUAL(client->actual_protocol_version, test_cases[i].version);
+            EXPECT_EQUAL(server->actual_protocol_version, test_cases[i].version);
+            if (result != S2N_SUCCESS) {
+                EXPECT_TRUE(s2n_errno == S2N_ERR_MISSING_CERT_REQUEST
+                        || s2n_errno == S2N_ERR_UNEXPECTED_CERT_REQUEST
+                        || s2n_errno == S2N_ERR_MISSING_CLIENT_CERT);
+            }
+        }
+    };
+
     END_TEST();
 }
diff --git a/tls/s2n_alerts.c b/tls/s2n_alerts.c
@@ -43,6 +43,7 @@ static S2N_RESULT s2n_translate_protocol_error_to_alert(int error_code, uint8_t
     switch (error_code) {
         S2N_ALERT_CASE(S2N_ERR_MISSING_EXTENSION, S2N_TLS_ALERT_MISSING_EXTENSION);
         S2N_ALERT_CASE(S2N_ERR_NO_VALID_SIGNATURE_SCHEME, S2N_TLS_ALERT_HANDSHAKE_FAILURE);
+        S2N_ALERT_CASE(S2N_ERR_MISSING_CLIENT_CERT, S2N_TLS_ALERT_CERTIFICATE_REQUIRED);
 
         /* TODO: The ERR_BAD_MESSAGE -> ALERT_UNEXPECTED_MESSAGE mapping
          * isn't always correct. Sometimes s2n-tls uses ERR_BAD_MESSAGE
@@ -52,6 +53,7 @@ static S2N_RESULT s2n_translate_protocol_error_to_alert(int error_code, uint8_t
          */
         S2N_ALERT_CASE(S2N_ERR_BAD_MESSAGE, S2N_TLS_ALERT_UNEXPECTED_MESSAGE);
         S2N_ALERT_CASE(S2N_ERR_UNEXPECTED_CERT_REQUEST, S2N_TLS_ALERT_UNEXPECTED_MESSAGE);
+        S2N_ALERT_CASE(S2N_ERR_MISSING_CERT_REQUEST, S2N_TLS_ALERT_UNEXPECTED_MESSAGE);
 
         /* For errors involving secure renegotiation:
          *= https://tools.ietf.org/rfc/rfc5746#3.4

diff --git a/tls/s2n_handshake_io.c b/tls/s2n_handshake_io.c
@@ -1105,7 +1105,7 @@ int s2n_conn_set_handshake_no_client_cert(struct s2n_connection *conn)
 {
     s2n_cert_auth_type client_cert_auth_type;
     POSIX_GUARD(s2n_connection_get_client_auth_type(conn, &client_cert_auth_type));
-    S2N_ERROR_IF(client_cert_auth_type != S2N_CERT_AUTH_OPTIONAL, S2N_ERR_BAD_MESSAGE);
+    POSIX_ENSURE(client_cert_auth_type == S2N_CERT_AUTH_OPTIONAL, S2N_ERR_MISSING_CLIENT_CERT);
 
     POSIX_GUARD_RESULT(s2n_handshake_type_set_flag(conn, NO_CLIENT_CERT));
 
@@ -1499,8 +1499,9 @@ static int s2n_handshake_read_io(struct s2n_connection *conn)
         s2n_cert_auth_type client_cert_auth_type;
         POSIX_GUARD(s2n_connection_get_client_auth_type(conn, &client_cert_auth_type));
 
-        /* If we're a Client, and received a ClientCertRequest message, and ClientAuth
-         * is set to optional, then switch the State Machine that we're using to expect the ClientCertRequest. */
+        /* If client auth is optional, we initially assume it will not be requested.
+         * If we received a request, switch to a client auth handshake.
+         */
         if (conn->mode == S2N_CLIENT
                 && client_cert_auth_type != S2N_CERT_AUTH_REQUIRED
                 && message_type == TLS_CERT_REQ) {
@@ -1529,6 +1530,12 @@ static int s2n_handshake_read_io(struct s2n_connection *conn)
             continue;
         }
 
+        /* Check for missing Certificate Requests to surface a more specific error */
+        if (EXPECTED_MESSAGE_TYPE(conn) == TLS_CERT_REQ) {
+            POSIX_ENSURE(message_type == TLS_CERT_REQ,
+                    S2N_ERR_MISSING_CERT_REQUEST);
+        }
+
         POSIX_ENSURE(record_type == EXPECTED_RECORD_TYPE(conn), S2N_ERR_BAD_MESSAGE);
         POSIX_ENSURE(message_type == EXPECTED_MESSAGE_TYPE(conn), S2N_ERR_BAD_MESSAGE);
         POSIX_ENSURE(!CONNECTION_IS_WRITER(conn), S2N_ERR_BAD_MESSAGE);