Change the default number of NewSessionTickets we send to 2

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5227)

7 files changed, 185 insertions, 54 deletions

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 2c29d7f61c..c38fc58a5d 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -591,6 +591,7 @@ int SSL_clear(SSL *s)
     s->psksession_id = NULL;
     s->psksession_id_len = 0;
     s->hello_retry_request = 0;
+    s->sent_tickets = 0;
 
     s->error = 0;
     s->hit = 0;
@@ -3034,8 +3035,8 @@ SSL_CTX *SSL_CTX_new(const SSL_METHOD *meth)
      */
     ret->max_early_data = 0;
 
-    /* By default we send one session ticket automatically in TLSv1.3 */
-    ret->num_tickets = 1;
+    /* By default we send two session tickets automatically in TLSv1.3 */
+    ret->num_tickets = 2;
 
     ssl_ctx_system_config(ret);
 
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 60e987adb1..6c0f8be564 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -2590,7 +2590,6 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt)
      * cache.
      */
     if (SSL_IS_TLS13(s) || s->session->session_id_length > 0) {
-        int i = s->session_ctx->session_cache_mode;
         SSL_SESSION *new_sess;
         /*
          * We reused an existing session, so we need to replace it with a new
@@ -2603,13 +2602,6 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt)
             goto err;
         }
 
-        if (i & SSL_SESS_CACHE_CLIENT) {
-            /*
-             * Remove the old session from the cache. We carry on if this fails
-             */
-            SSL_CTX_remove_session(s->session_ctx, s->session);
-        }
-
         SSL_SESSION_free(s->session);
         s->session = new_sess;
     }
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index dfeba173a7..ce8cec185a 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -507,6 +507,9 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s)
         /* Fall through */
 
     case TLS_ST_SW_KEY_UPDATE:
+        st->hand_state = TLS_ST_OK;
+        return WRITE_TRAN_CONTINUE;
+
     case TLS_ST_SW_SESSION_TICKET:
         /* In a resumption we only ever send a maximum of one new ticket.
          * Following an initial handshake we send the number of tickets we have
@@ -708,7 +711,7 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst)
         return WORK_FINISHED_CONTINUE;
 
     case TLS_ST_SW_SESSION_TICKET:
-        if (SSL_IS_TLS13(s)) {
+        if (SSL_IS_TLS13(s) && s->sent_tickets == 0) {
             /*
              * Actually this is the end of the handshake, but we're going
              * straight into writing the session ticket out. So we finish off
@@ -3687,12 +3690,16 @@ MSG_PROCESS_RETURN tls_process_client_certificate(SSL *s, PACKET *pkt)
     sk = NULL;
 
     /* Save the current hash state for when we receive the CertificateVerify */
-    if (SSL_IS_TLS13(s)
-            && !ssl_handshake_hash(s, s->cert_verify_hash,
-                                   sizeof(s->cert_verify_hash),
-                                   &s->cert_verify_hash_len)) {
-        /* SSLfatal() already called */
-        goto err;
+    if (SSL_IS_TLS13(s)) {
+        if (!ssl_handshake_hash(s, s->cert_verify_hash,
+                                sizeof(s->cert_verify_hash),
+                                &s->cert_verify_hash_len)) {
+            /* SSLfatal() already called */
+            goto err;
+        }
+
+        /* Resend session tickets */
+        s->sent_tickets = 0;
     }
 
     ret = MSG_PROCESS_CONTINUE_READING;
@@ -3989,7 +3996,6 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt)
         goto err;
     }
     if (SSL_IS_TLS13(s)) {
-        ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
         if (!tls_construct_extensions(s, pkt,
                                       SSL_EXT_TLS1_3_NEW_SESSION_TICKET,
                                       NULL, 0)) {
@@ -3997,6 +4003,7 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt)
             goto err;
         }
         s->sent_tickets++;
+        ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
     }
     EVP_CIPHER_CTX_free(ctx);
     HMAC_CTX_free(hctx);
diff --git a/test/handshake_helper.c b/test/handshake_helper.c
index b3d94bb1ee..3ebf64dfe3 100644
--- a/test/handshake_helper.c
+++ b/test/handshake_helper.c
@@ -1403,7 +1403,7 @@ static HANDSHAKE_RESULT *do_handshake_internal(
     HANDSHAKE_EX_DATA server_ex_data, client_ex_data;
     CTX_DATA client_ctx_data, server_ctx_data, server2_ctx_data;
     HANDSHAKE_RESULT *ret = HANDSHAKE_RESULT_new();
-    int client_turn = 1, client_turn_count = 0;
+    int client_turn = 1, client_turn_count = 0, client_wait_count = 0;
     connect_phase_t phase = HANDSHAKE;
     handshake_status_t status = HANDSHAKE_RETRY;
     const unsigned char* tick = NULL;
@@ -1586,9 +1586,19 @@ static HANDSHAKE_RESULT *do_handshake_internal(
                     ret->result = SSL_TEST_INTERNAL_ERROR;
                     goto err;
                 }
-
-                /* Continue. */
-                client_turn ^= 1;
+                if (client_turn && server.status == PEER_SUCCESS) {
+                    /*
+                     * The server may finish before the client because the
+                     * client spends some turns processing NewSessionTickets.
+                     */
+                    if (client_wait_count++ >= 2) {
+                        ret->result = SSL_TEST_INTERNAL_ERROR;
+                        goto err;
+                    }
+                } else {
+                    /* Continue. */
+                    client_turn ^= 1;
+                }
             }
             break;
         }
diff --git a/test/sslapitest.c b/test/sslapitest.c
index 06d6cb2b68..626e26f52b 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -882,10 +882,14 @@ static int execute_test_session(int maxprot, int use_int_cache,
     SSL *serverssl3 = NULL, *clientssl3 = NULL;
 # endif
     SSL_SESSION *sess1 = NULL, *sess2 = NULL;
-    int testresult = 0;
+    int testresult = 0, numnewsesstick = 1;
 
     new_called = remove_called = 0;
 
+    /* TLSv1.3 sends 2 NewSessionTickets */
+    if (maxprot == TLS1_3_VERSION)
+        numnewsesstick = 2;
+
     if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(), TLS_client_method(),
                                        TLS1_VERSION, TLS_MAX_VERSION,
                                        &sctx, &cctx, cert, privkey)))
@@ -923,7 +927,9 @@ static int execute_test_session(int maxprot, int use_int_cache,
     if (use_int_cache && !TEST_false(SSL_CTX_add_session(cctx, sess1)))
         goto end;
     if (use_ext_cache
-            && (!TEST_int_eq(new_called, 1) || !TEST_int_eq(remove_called, 0)))
+            && (!TEST_int_eq(new_called, numnewsesstick)
+
+                || !TEST_int_eq(remove_called, 0)))
         goto end;
 
     new_called = remove_called = 0;
@@ -938,11 +944,11 @@ static int execute_test_session(int maxprot, int use_int_cache,
     if (maxprot == TLS1_3_VERSION) {
         /*
          * In TLSv1.3 we should have created a new session even though we have
-         * resumed. The original session should also have been removed.
+         * resumed.
          */
         if (use_ext_cache
                 && (!TEST_int_eq(new_called, 1)
-                    || !TEST_int_eq(remove_called, 1)))
+                    || !TEST_int_eq(remove_called, 0)))
             goto end;
     } else {
         /*
@@ -972,7 +978,8 @@ static int execute_test_session(int maxprot, int use_int_cache,
         goto end;
 
     if (use_ext_cache
-            && (!TEST_int_eq(new_called, 1) || !TEST_int_eq(remove_called, 0)))
+            && (!TEST_int_eq(new_called, numnewsesstick)
+                || !TEST_int_eq(remove_called, 0)))
         goto end;
 
     new_called = remove_called = 0;
@@ -1072,7 +1079,7 @@ static int execute_test_session(int maxprot, int use_int_cache,
     if (use_ext_cache) {
         SSL_SESSION *tmp = sess2;
 
-        if (!TEST_int_eq(new_called, 1)
+        if (!TEST_int_eq(new_called, numnewsesstick)
                 || !TEST_int_eq(remove_called, 0)
                 || !TEST_int_eq(get_called, 0))
             goto end;
@@ -1105,10 +1112,6 @@ static int execute_test_session(int maxprot, int use_int_cache,
             goto end;
 
         if (maxprot == TLS1_3_VERSION) {
-            /*
-             * Every time we issue a NewSessionTicket we are creating a new
-             * session for next time in TLSv1.3
-             */
             if (!TEST_int_eq(new_called, 1)
                     || !TEST_int_eq(get_called, 0))
                 goto end;
@@ -1181,6 +1184,101 @@ static int test_session_with_both_cache(void)
 #endif
 }
 
+SSL_SESSION *sesscache[9];
+
+static int new_cachesession_cb(SSL *ssl, SSL_SESSION *sess)
+{
+    sesscache[new_called++] = sess;
+
+    return 1;
+}
+
+static int test_tickets(int idx)
+{
+    SSL_CTX *sctx = NULL, *cctx = NULL;
+    SSL *serverssl = NULL, *clientssl = NULL;
+    int testresult = 0, i;
+    size_t j;
+
+    /* idx is the test number, but also the number of tickets we want */
+
+    new_called = 0;
+
+    if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(), TLS_client_method(),
+                                       TLS1_VERSION, TLS_MAX_VERSION, &sctx,
+                                       &cctx, cert, privkey))
+            || !TEST_true(SSL_CTX_set_num_tickets(sctx, idx)))
+        goto end;
+
+    SSL_CTX_set_session_cache_mode(cctx, SSL_SESS_CACHE_CLIENT
+                                         | SSL_SESS_CACHE_NO_INTERNAL_STORE);
+    SSL_CTX_sess_set_new_cb(cctx, new_cachesession_cb);
+
+    if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl,
+                                          &clientssl, NULL, NULL)))
+        goto end;
+
+    SSL_force_post_handshake_auth(clientssl);
+
+    if (!TEST_true(create_ssl_connection(serverssl, clientssl,
+                                                SSL_ERROR_NONE))
+               /* Check we got the number of tickets we were expecting */
+            || !TEST_int_eq(idx, new_called))
+        goto end;
+
+    /* After a post-handshake authentication we should get new tickets issued */
+    SSL_set_verify(serverssl, SSL_VERIFY_PEER, NULL);
+    if (!TEST_true(SSL_verify_client_post_handshake(serverssl)))
+        goto end;
+
+    /* Start handshake on the server and client */
+    if (!TEST_int_eq(SSL_do_handshake(serverssl), 1)
+            || !TEST_int_le(SSL_read(clientssl, NULL, 0), 0)
+            || !TEST_int_le(SSL_read(serverssl, NULL, 0), 0)
+            || !TEST_true(create_ssl_connection(serverssl, clientssl,
+                                                SSL_ERROR_NONE))
+            || !TEST_int_eq(idx * 2, new_called))
+        goto end;
+
+    SSL_CTX_sess_set_new_cb(cctx, NULL);
+    SSL_shutdown(clientssl);
+    SSL_shutdown(serverssl);
+    SSL_free(serverssl);
+    SSL_free(clientssl);
+    serverssl = clientssl = NULL;
+
+    /* Test that we can resume with all the tickets we got given */
+    for (i = 0; i < new_called; i++) {
+        if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl,
+                                              &clientssl, NULL, NULL))
+                || !TEST_true(SSL_set_session(clientssl, sesscache[i]))
+                || !TEST_true(create_ssl_connection(serverssl, clientssl,
+                                                    SSL_ERROR_NONE))
+                || !TEST_true(SSL_session_reused(clientssl)))
+            goto end;
+
+        SSL_shutdown(clientssl);
+        SSL_shutdown(serverssl);
+        SSL_free(serverssl);
+        SSL_free(clientssl);
+        serverssl = clientssl = NULL;
+        SSL_SESSION_free(sesscache[i]);
+        sesscache[i] = NULL;
+    }
+
+    testresult = 1;
+
+ end:
+    SSL_free(serverssl);
+    SSL_free(clientssl);
+    for (j = 0; j < OSSL_NELEM(sesscache); j++)
+        SSL_SESSION_free(sesscache[j]);
+    SSL_CTX_free(sctx);
+    SSL_CTX_free(cctx);
+
+    return testresult;
+}
+
 #define USE_NULL            0
 #define USE_BIO_1           1
 #define USE_BIO_2           2
@@ -1198,7 +1296,6 @@ static int test_session_with_both_cache(void)
 # define TOTAL_CONN_FAIL_SSL_SET_BIO_TESTS       0
 #endif
 
-
 #define TOTAL_SSL_SET_BIO_TESTS TOTAL_NO_CONN_SSL_SET_BIO_TESTS \
                                 + TOTAL_CONN_SUCCESS_SSL_SET_BIO_TESTS \
                                 + TOTAL_CONN_FAIL_SSL_SET_BIO_TESTS
@@ -1933,10 +2030,13 @@ static int test_early_data_read_write(int idx)
         goto end;
 
     /*
-     * Make sure we process the NewSessionTicket. This arrives post-handshake.
-     * We attempt a read which we do not expect to return any data.
+     * Make sure we process the two NewSessionTickets. These arrive
+     * post-handshake. We attempt reads which we do not expect to return any
+     * data.
      */
-    if (!TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf), &readbytes)))
+    if (!TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf), &readbytes))
+            || !TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf),
+                           &readbytes)))
         goto end;
 
     /* Server should be able to write normal data */
@@ -3392,9 +3492,10 @@ static int test_custom_exts(int tst)
                 || (tst == 2 && snicb != 1))
             goto end;
     } else {
+        /* In this case there 2 NewSessionTicket messages created */
         if (clntaddnewcb != 1
-                || clntparsenewcb != 4
-                || srvaddnewcb != 4
+                || clntparsenewcb != 5
+                || srvaddnewcb != 5
                 || srvparsenewcb != 1)
             goto end;
     }
@@ -3438,10 +3539,13 @@ static int test_custom_exts(int tst)
                 || srvparsenewcb != 2)
             goto end;
     } else {
-        /* No Certificate message extensions in the resumption handshake */
+        /*
+         * No Certificate message extensions in the resumption handshake,
+         * 2 NewSessionTickets in the initial handshake, 1 in the resumption
+         */
         if (clntaddnewcb != 2
-                || clntparsenewcb != 7
-                || srvaddnewcb != 7
+                || clntparsenewcb != 8
+                || srvaddnewcb != 8
                 || srvparsenewcb != 2)
             goto end;
     }
@@ -4205,14 +4309,16 @@ static struct info_cb_states_st {
         {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"},
         {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
         {SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
-        {SSL_CB_EXIT, NULL}, {SSL_CB_ALERT, NULL},
-        {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
-        {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"}, {SSL_CB_LOOP, "TWSH"},
-        {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWFIN"},
-        {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"},
-        {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
         {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TWST"},
-        {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL},
+        {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
+        {SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
+        {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"},
+        {SSL_CB_LOOP, "TWSH"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"},
+        {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL},
+        {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"},
+        {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
+        {SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
+        {SSL_CB_EXIT, NULL}, {0, NULL},
     }, {
         /* TLSv1.3 client followed by resumption */
         {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
@@ -4223,6 +4329,9 @@ static struct info_cb_states_st {
         {SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
         {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
         {SSL_CB_HANDSHAKE_DONE, NULL},  {SSL_CB_EXIT, NULL},
+        {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "SSLOK "},
+        {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
+        {SSL_CB_HANDSHAKE_DONE, NULL},  {SSL_CB_EXIT, NULL},
         {SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
         {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TWCH"}, {SSL_CB_EXIT, NULL},
         {SSL_CB_LOOP, "TWCH"}, {SSL_CB_LOOP, "TRSH"},  {SSL_CB_LOOP, "TREE"},
@@ -4856,6 +4965,9 @@ int setup_tests(void)
     ADD_TEST(test_session_with_only_int_cache);
     ADD_TEST(test_session_with_only_ext_cache);
     ADD_TEST(test_session_with_both_cache);
+#ifndef OPENSSL_NO_TLS1_3
+    ADD_ALL_TESTS(test_tickets, 3);
+#endif
     ADD_ALL_TESTS(test_ssl_set_bio, TOTAL_SSL_SET_BIO_TESTS);
     ADD_TEST(test_ssl_bio_pop_next_bio);
     ADD_TEST(test_ssl_bio_pop_ssl_bio);
diff --git a/test/ssltestlib.c b/test/ssltestlib.c
index c7689631f1..2ef4b5d432 100644
--- a/test/ssltestlib.c
+++ b/test/ssltestlib.c
@@ -682,7 +682,7 @@ int create_ssl_objects(SSL_CTX *serverctx, SSL_CTX *clientctx, SSL **sssl,
 
 int create_ssl_connection(SSL *serverssl, SSL *clientssl, int want)
 {
-    int retc = -1, rets = -1, err, abortctr = 0;
+    int retc = -1, rets = -1, err, abortctr = 0, i;
     int clienterr = 0, servererr = 0;
     unsigned char buf;
     size_t readbytes;
@@ -741,13 +741,16 @@ int create_ssl_connection(SSL *serverssl, SSL *clientssl, int want)
     /*
      * We attempt to read some data on the client side which we expect to fail.
      * This will ensure we have received the NewSessionTicket in TLSv1.3 where
-     * appropriate.
+     * appropriate. We do this twice because there are 2 NewSesionTickets.
      */
-    if (SSL_read_ex(clientssl, &buf, sizeof(buf), &readbytes) > 0) {
-        if (!TEST_ulong_eq(readbytes, 0))
+    for (i = 0; i < 2; i++) {
+        if (SSL_read_ex(clientssl, &buf, sizeof(buf), &readbytes) > 0) {
+            if (!TEST_ulong_eq(readbytes, 0))
+                return 0;
+        } else if (!TEST_int_eq(SSL_get_error(clientssl, 0),
+                                SSL_ERROR_WANT_READ)) {
             return 0;
-    } else if (!TEST_int_eq(SSL_get_error(clientssl, 0), SSL_ERROR_WANT_READ)) {
-        return 0;
+        }
     }
 
     return 1;
diff --git a/util/perl/TLSProxy/Proxy.pm b/util/perl/TLSProxy/Proxy.pm
index 8df0153d24..8c13520ec6 100644
--- a/util/perl/TLSProxy/Proxy.pm
+++ b/util/perl/TLSProxy/Proxy.pm
@@ -220,6 +220,12 @@ sub start
 
     my $execcmd = $self->execute
         ." s_server -max_protocol TLSv1.3 -no_comp -rev -engine ossltest"
+        #In TLSv1.3 we issue two session tickets. The default session id
+        #callback gets confused because the ossltest engine causes the same
+        #session id to be created twice due to the changed random number
+        #generation. Using "-ext_cache" replaces the default callback with a
+        #different one that doesn't get confused.
+        ." -ext_cache"
         ." -accept $self->{server_addr}:0"
         ." -cert ".$self->cert." -cert2 ".$self->cert
         ." -naccept ".$self->serverconnects;