Clean up following new SNI tests

- Only send SNI in SNI tests. This allows us to test handshakes without
  the SNI extension as well.
- Move all handshake-specific machinery to handshake_helper.c
- Use enum types to represent the enum everywhere
  (Resorting to plain ints can end in sign mismatch when the enum is
  represented by an unsigned type.)
Reviewed-by: NRich Salz <rsalz@openssl.org>

test/README.ssltest.md

@@ -64,8 +64,9 @@ The test section supports the following options:
   - AcceptAll - accepts all certificates.
   - RejectAll - rejects all certificates.
 
-* ServerName - the server the client is expected to successfully connect to
-  - server1 - the initial context (default)
+* ServerName - the server the client should attempt to connect to. One of
+  - None - do not use SNI (default)
+  - server1 - the initial context
   - server2 - the secondary context
 
 * SessionTicketExpected - whether or not a session ticket is expected

test/handshake_helper.c

@@ -41,6 +41,23 @@ static void info_callback(const SSL *s, int where, int ret)
     }
 }
 
+static int servername_callback(SSL *s, int *ad, void *arg)
+{
+    const char *servername = SSL_get_servername(s, TLSEXT_NAMETYPE_host_name);
+    if (servername != NULL && !strcmp(servername, "server2")) {
+        SSL_CTX *new_ctx = (SSL_CTX*)arg;
+        SSL_set_SSL_CTX(s, new_ctx);
+        /*
+         * Copy over all the SSL_CTX options - reasonable behavior
+         * allows testing of cases where the options between two
+         * contexts differ/conflict
+         */
+        SSL_clear_options(s, 0xFFFFFFFFL);
+        SSL_set_options(s, SSL_CTX_get_options(new_ctx));
+    }
+    return SSL_TLSEXT_ERR_OK;
+}
+
 static int verify_reject_callback(X509_STORE_CTX *ctx, void *arg) {
     X509_STORE_CTX_set_error(ctx, X509_V_ERR_APPLICATION_VERIFICATION);
     return 0;
@@ -56,8 +73,10 @@ static int broken_session_ticket_callback(SSL* s, unsigned char* key_name, unsig
     return 0;
 }
 
-int do_not_call_session_ticket_callback(SSL* s, unsigned char* key_name, unsigned char *iv,
-                                        EVP_CIPHER_CTX *ctx, HMAC_CTX *hctx, int enc)
+static int do_not_call_session_ticket_callback(SSL* s, unsigned char* key_name,
+                                               unsigned char *iv,
+                                               EVP_CIPHER_CTX *ctx,
+                                               HMAC_CTX *hctx, int enc)
 {
     HANDSHAKE_EX_DATA *ex_data =
         (HANDSHAKE_EX_DATA*)(SSL_get_ex_data(s, ex_data_idx));
@@ -69,7 +88,8 @@ int do_not_call_session_ticket_callback(SSL* s, unsigned char* key_name, unsigne
  * Configure callbacks and other properties that can't be set directly
  * in the server/client CONF.
  */
-static void configure_handshake_ctx(SSL_CTX *server_ctx, SSL_CTX *client_ctx,
+static void configure_handshake_ctx(SSL_CTX *server_ctx, SSL_CTX *server2_ctx,
+                                    SSL_CTX *client_ctx,
                                     const SSL_TEST_CTX *test_ctx)
 {
     switch (test_ctx->client_verify_callback) {
@@ -84,6 +104,17 @@ static void configure_handshake_ctx(SSL_CTX *server_ctx, SSL_CTX *client_ctx,
     default:
         break;
     }
+
+    /* link the two contexts for SNI purposes */
+    SSL_CTX_set_tlsext_servername_callback(server_ctx, servername_callback);
+    SSL_CTX_set_tlsext_servername_arg(server_ctx, server2_ctx);
+    /*
+     * The initial_ctx/session_ctx always handles the encrypt/decrypt of the
+     * session ticket. This ticket_key callback is assigned to the second
+     * session (assigned via SNI), and should never be invoked
+     */
+    SSL_CTX_set_tlsext_ticket_key_cb(server2_ctx, do_not_call_session_ticket_callback);
+
     if (test_ctx->session_ticket_expected == SSL_TEST_SESSION_TICKET_BROKEN) {
         SSL_CTX_set_tlsext_ticket_key_cb(server_ctx, broken_session_ticket_callback);
     }
@@ -96,7 +127,9 @@ static void configure_handshake_ctx(SSL_CTX *server_ctx, SSL_CTX *client_ctx,
 static void configure_handshake_ssl(SSL *server, SSL *client,
                                     const SSL_TEST_CTX *test_ctx)
 {
-    SSL_set_tlsext_host_name(client, ssl_servername_name(test_ctx->servername));
+    if (test_ctx->servername != SSL_TEST_SERVERNAME_NONE)
+        SSL_set_tlsext_host_name(client,
+                                 ssl_servername_name(test_ctx->servername));
 }
 
 
@@ -199,8 +232,8 @@ static handshake_status_t handshake_status(peer_status_t last_status,
     return INTERNAL_ERROR;
 }
 
-HANDSHAKE_RESULT do_handshake(SSL_CTX *server_ctx, SSL_CTX *client_ctx,
-                              const SSL_TEST_CTX *test_ctx)
+HANDSHAKE_RESULT do_handshake(SSL_CTX *server_ctx, SSL_CTX *server2_ctx,
+                              SSL_CTX *client_ctx, const SSL_TEST_CTX *test_ctx)
 {
     SSL *server, *client;
     BIO *client_to_server, *server_to_client;
@@ -213,7 +246,7 @@ HANDSHAKE_RESULT do_handshake(SSL_CTX *server_ctx, SSL_CTX *client_ctx,
     size_t len = 0;
     SSL_SESSION* sess = NULL;
 
-    configure_handshake_ctx(server_ctx, client_ctx, test_ctx);
+    configure_handshake_ctx(server_ctx, server2_ctx, client_ctx, test_ctx);
 
     server = SSL_new(server_ctx);
     client = SSL_new(client_ctx);

test/handshake_helper.h

@@ -27,18 +27,15 @@ typedef struct handshake_result {
     int server_protocol;
     int client_protocol;
     /* Server connection */
-    int servername;
+    ssl_servername_t servername;
     /* Session ticket status */
-    int session_ticket;
+    ssl_session_ticket_t session_ticket;
     /* Was this called on the second context? */
     int session_ticket_do_not_call;
 } HANDSHAKE_RESULT;
 
 /* Do a handshake and report some information about the result. */
-HANDSHAKE_RESULT do_handshake(SSL_CTX *server_ctx, SSL_CTX *client_ctx,
-                              const SSL_TEST_CTX *test_ctx);
-
-int do_not_call_session_ticket_callback(SSL* s, unsigned char* key_name, unsigned char *iv,
-                                        EVP_CIPHER_CTX *ctx, HMAC_CTX *hctx, int enc);
+HANDSHAKE_RESULT do_handshake(SSL_CTX *server_ctx, SSL_CTX *server2_ctx,
+                              SSL_CTX *client_ctx, const SSL_TEST_CTX *test_ctx);
 
 #endif  /* HEADER_HANDSHAKE_HELPER_H */

test/ssl_test.c

@@ -125,7 +125,8 @@ static int check_protocol(HANDSHAKE_RESULT result, SSL_TEST_CTX *test_ctx)
 
 static int check_servername(HANDSHAKE_RESULT result, SSL_TEST_CTX *test_ctx)
 {
-    if (result.servername != test_ctx->servername) {
+    if (test_ctx->servername != SSL_TEST_SERVERNAME_NONE
+        && result.servername != test_ctx->servername) {
         fprintf(stderr, "Client ServerName mismatch, expected %s, got %s\n.",
                 ssl_servername_name(test_ctx->servername),
                 ssl_servername_name(result.servername));
@@ -134,7 +135,7 @@ static int check_servername(HANDSHAKE_RESULT result, SSL_TEST_CTX *test_ctx)
     return 1;
 }
 
-static int check_session_ticket_expected(HANDSHAKE_RESULT result, SSL_TEST_CTX *test_ctx)
+static int check_session_ticket(HANDSHAKE_RESULT result, SSL_TEST_CTX *test_ctx)
 {
     if (test_ctx->session_ticket_expected == SSL_TEST_SESSION_TICKET_IGNORE)
         return 1;
@@ -143,8 +144,8 @@ static int check_session_ticket_expected(HANDSHAKE_RESULT result, SSL_TEST_CTX *
         return 1;
     if (result.session_ticket != test_ctx->session_ticket_expected) {
         fprintf(stderr, "Client SessionTicketExpected mismatch, expected %s, got %s\n.",
-                ssl_session_ticket_expected_name(test_ctx->session_ticket_expected),
-                ssl_session_ticket_expected_name(result.session_ticket));
+                ssl_session_ticket_name(test_ctx->session_ticket_expected),
+                ssl_session_ticket_name(result.session_ticket));
         return 0;
     }
     return 1;
@@ -163,29 +164,12 @@ static int check_test(HANDSHAKE_RESULT result, SSL_TEST_CTX *test_ctx)
     if (result.result == SSL_TEST_SUCCESS) {
         ret &= check_protocol(result, test_ctx);
         ret &= check_servername(result, test_ctx);
-        ret &= check_session_ticket_expected(result, test_ctx);
+        ret &= check_session_ticket(result, test_ctx);
         ret &= (result.session_ticket_do_not_call == 0);
     }
     return ret;
 }
 
-static int servername_callback(SSL *s, int *ad, void *arg)
-{
-    const char *servername = SSL_get_servername(s, TLSEXT_NAMETYPE_host_name);
-    if (servername != NULL && !strcmp(servername, "server2")) {
-        SSL_CTX *new_ctx = (SSL_CTX*)arg;
-        SSL_set_SSL_CTX(s, new_ctx);
-        /*
-         * Copy over all the SSL_CTX options - reasonable behavior
-         * allows testing of cases where the options between two
-         * contexts differ/conflict
-         */
-        SSL_clear_options(s, 0xFFFFFFFFL);
-        SSL_set_options(s, SSL_CTX_get_options(new_ctx));
-    }
-    return SSL_TLSEXT_ERR_OK;
-}
-
 static int execute_test(SSL_TEST_FIXTURE fixture)
 {
     int ret = 0;
@@ -206,21 +190,11 @@ static int execute_test(SSL_TEST_FIXTURE fixture)
         goto err;
     }
 
-    /* link the two contexts for SNI purposes */
-    SSL_CTX_set_tlsext_servername_callback(server_ctx, servername_callback);
-    SSL_CTX_set_tlsext_servername_arg(server_ctx, server2_ctx);
-    /*
-     * The initial_ctx/session_ctx always handles the encrypt/decrypt of the
-     * session ticket. This ticket_key callback is assigned to the second
-     * session (assigned via SNI), and should never be invoked
-     */
-    SSL_CTX_set_tlsext_ticket_key_cb(server2_ctx, do_not_call_session_ticket_callback);
-
     test_ctx = SSL_TEST_CTX_create(conf, fixture.test_app);
     if (test_ctx == NULL)
         goto err;
 
-    result = do_handshake(server_ctx, client_ctx, test_ctx);
+    result = do_handshake(server_ctx, server2_ctx, client_ctx, test_ctx);
 
     ret = check_test(result, test_ctx);
 

test/ssl_test_ctx.c

@@ -159,6 +159,7 @@ const char *ssl_verify_callback_name(ssl_verify_callback_t callback)
 /**************/
 
 static const test_enum ssl_servername[] = {
+    {"None", SSL_TEST_SERVERNAME_NONE},
     {"server1", SSL_TEST_SERVERNAME_SERVER1},
     {"server2", SSL_TEST_SERVERNAME_SERVER2},
 };
@@ -185,18 +186,17 @@ const char *ssl_servername_name(ssl_servername_t server)
 /* SessionTicketExpected */
 /*************************/
 
-static const test_enum ssl_session_ticket_expected[] = {
+static const test_enum ssl_session_ticket[] = {
     {"Ignore", SSL_TEST_SESSION_TICKET_IGNORE},
     {"Yes", SSL_TEST_SESSION_TICKET_YES},
     {"No", SSL_TEST_SESSION_TICKET_NO},
     {"Broken", SSL_TEST_SESSION_TICKET_BROKEN},
 };
 
-__owur static int parse_session_ticket_expected(SSL_TEST_CTX *test_ctx,
-                                                const char *value)
+__owur static int parse_session_ticket(SSL_TEST_CTX *test_ctx, const char *value)
 {
     int ret_value;
-    if (!parse_enum(ssl_session_ticket_expected, OSSL_NELEM(ssl_session_ticket_expected),
+    if (!parse_enum(ssl_session_ticket, OSSL_NELEM(ssl_session_ticket),
                     &ret_value, value)) {
         return 0;
     }
@@ -204,10 +204,10 @@ __owur static int parse_session_ticket_expected(SSL_TEST_CTX *test_ctx,
     return 1;
 }
 
-const char *ssl_session_ticket_expected_name(ssl_session_ticket_expected_t server)
+const char *ssl_session_ticket_name(ssl_session_ticket_t server)
 {
-    return enum_name(ssl_session_ticket_expected,
-                     OSSL_NELEM(ssl_session_ticket_expected),
+    return enum_name(ssl_session_ticket,
+                     OSSL_NELEM(ssl_session_ticket),
                      server);
 }
 
@@ -227,7 +227,7 @@ static const ssl_test_ctx_option ssl_test_ctx_options[] = {
     { "Protocol", &parse_protocol },
     { "ClientVerifyCallback", &parse_client_verify_callback },
     { "ServerName", &parse_servername },
-    { "SessionTicketExpected", &parse_session_ticket_expected },
+    { "SessionTicketExpected", &parse_session_ticket },
 };
 
 

test/ssl_test_ctx.h

@@ -27,7 +27,8 @@ typedef enum {
 } ssl_verify_callback_t;
 
 typedef enum {
-    SSL_TEST_SERVERNAME_SERVER1 = 0, /* Default */
+    SSL_TEST_SERVERNAME_NONE = 0, /* Default */
+    SSL_TEST_SERVERNAME_SERVER1,
     SSL_TEST_SERVERNAME_SERVER2
 } ssl_servername_t;
 
@@ -36,7 +37,7 @@ typedef enum {
     SSL_TEST_SESSION_TICKET_YES,
     SSL_TEST_SESSION_TICKET_NO,
     SSL_TEST_SESSION_TICKET_BROKEN, /* Special test */
-} ssl_session_ticket_expected_t;
+} ssl_session_ticket_t;
 
 typedef struct ssl_test_ctx {
     /* Test expectations. */
@@ -55,7 +56,7 @@ typedef struct ssl_test_ctx {
     ssl_verify_callback_t client_verify_callback;
     /* One of a number of predefined server names use by the client */
     ssl_servername_t servername;
-    ssl_session_ticket_expected_t session_ticket_expected;
+    ssl_session_ticket_t session_ticket_expected;
 } SSL_TEST_CTX;
 
 const char *ssl_test_result_name(ssl_test_result_t result);
@@ -63,7 +64,7 @@ const char *ssl_alert_name(int alert);
 const char *ssl_protocol_name(int protocol);
 const char *ssl_verify_callback_name(ssl_verify_callback_t verify_callback);
 const char *ssl_servername_name(ssl_servername_t server);
-const char *ssl_session_ticket_expected_name(ssl_session_ticket_expected_t server);
+const char *ssl_session_ticket_name(ssl_session_ticket_t server);
 
 /*
  * Load the test case context from |conf|.

test/ssl_test_ctx_test.c

@@ -72,8 +72,8 @@ static int SSL_TEST_CTX_equal(SSL_TEST_CTX *ctx, SSL_TEST_CTX *ctx2)
     }
     if (ctx->session_ticket_expected != ctx2->session_ticket_expected) {
         fprintf(stderr, "SessionTicketExpected mismatch: %s vs %s.\n",
-                ssl_session_ticket_expected_name(ctx->session_ticket_expected),
-                ssl_session_ticket_expected_name(ctx2->session_ticket_expected));
+                ssl_session_ticket_name(ctx->session_ticket_expected),
+                ssl_session_ticket_name(ctx2->session_ticket_expected));
         return 0;
     }