diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 12db981e8..7a356f7a1 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1793,8 +1793,10 @@ OPENSSL_EXPORT int SSL_SESSION_set1_id_context(SSL_SESSION *session, // used without leaking a correlator. OPENSSL_EXPORT int SSL_SESSION_should_be_single_use(const SSL_SESSION *session); -// SSL_SESSION_is_resumable returns one if |session| is resumable and zero -// otherwise. +// SSL_SESSION_is_resumable returns one if |session| is complete and contains a +// session ID or ticket. It returns zero otherwise. Note this function does not +// ensure |session| will be resumed. It may be expired, dropped by the server, +// or associated with incompatible parameters. OPENSSL_EXPORT int SSL_SESSION_is_resumable(const SSL_SESSION *session); // SSL_SESSION_has_ticket returns one if |session| has a ticket and zero diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 8f23d959e..1a14c110b 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -402,9 +402,7 @@ static enum ssl_hs_wait_t do_start_connect(SSL_HANDSHAKE *hs) { if (ssl->session != nullptr) { if (ssl->session->is_server || !ssl_supports_version(hs, ssl->session->ssl_version) || - (ssl->session->session_id_length == 0 && - ssl->session->ticket.empty()) || - ssl->session->not_resumable || + !SSL_SESSION_is_resumable(ssl->session.get()) || !ssl_session_is_time_valid(ssl, ssl->session.get()) || (ssl->quic_method != nullptr) != ssl->session->is_quic || ssl->s3->initial_handshake_complete) { @@ -1666,8 +1664,8 @@ static enum ssl_hs_wait_t do_read_session_ticket(SSL_HANDSHAKE *hs) { } session->ticket_lifetime_hint = ticket_lifetime_hint; - // Generate a session ID for this session. Some callers expect all sessions to - // have a session ID. + // Historically, OpenSSL filled in fake session IDs for ticket-based sessions. + // TODO(davidben): Are external callers relying on this? Try removing this. SHA256(CBS_data(&ticket), CBS_len(&ticket), session->session_id); session->session_id_length = SHA256_DIGEST_LENGTH; diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index e89929609..5f3614a30 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -275,9 +275,7 @@ ssl_open_record_t ssl_open_app_data(SSL *ssl, Span *out, void ssl_update_cache(SSL_HANDSHAKE *hs, int mode) { SSL *const ssl = hs->ssl; SSL_CTX *ctx = ssl->session_ctx.get(); - // Never cache sessions with empty session IDs. - if (ssl->s3->established_session->session_id_length == 0 || - ssl->s3->established_session->not_resumable || + if (!SSL_SESSION_is_resumable(ssl->s3->established_session.get()) || (ctx->session_cache_mode & mode) != mode) { return; } diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index 41df63f93..a52ec3dd7 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc @@ -1004,7 +1004,8 @@ int SSL_SESSION_should_be_single_use(const SSL_SESSION *session) { } int SSL_SESSION_is_resumable(const SSL_SESSION *session) { - return !session->not_resumable; + return !session->not_resumable && + (session->session_id_length != 0 || !session->ticket.empty()); } int SSL_SESSION_has_ticket(const SSL_SESSION *session) { diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index dae4bef10..0fd656705 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc @@ -1053,8 +1053,8 @@ UniquePtr tls13_create_session_with_ticket(SSL *ssl, CBS *body) { } } - // Generate a session ID for this session. Some callers expect all sessions to - // have a session ID. + // Historically, OpenSSL filled in fake session IDs for ticket-based sessions. + // TODO(davidben): Are external callers relying on this? Try removing this. SHA256(CBS_data(&ticket), CBS_len(&ticket), session->session_id); session->session_id_length = SHA256_DIGEST_LENGTH;