Skip to content

Commit

Permalink
Increment session hit counter for ticket resumptions (#1320)
Browse files Browse the repository at this point in the history
OpenSSL increments session hits in two places:

1. Handshake finish [on the client side](https://github.com/openssl/openssl/blob/1750689767cc922bdbe73358f7256475f0838c67/ssl/statem/statem_lib.c#L1494)
2. Fetching potentially pre-existing session [on the server side](https://github.com/openssl/openssl/blob/1750689767cc922bdbe73358f7256475f0838c67/ssl/ssl_sess.c#L665)

Prior to this change, we failed to increment the session "cache" hit counter for TLS 1.3 servers. I put "cache" in scare-quotes because whether that behavior is correct depends on how you define a cache, as TLS 1.3 ticket resumption is stateless and doesn't require a true session cache. Semantics aside, the change in this PR conforms to OpenSSL's behavior. OpenSSL [documents](https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_sess_hits.html) session hit counts as:

> SSL_CTX_sess_hits() returns the number of successfully reused sessions. In client mode a session set with [SSL_set_session(3)](https://www.openssl.org/docs/man1.1.1/man3/SSL_set_session.html) successfully reused is counted as a hit. In server mode a session successfully retrieved from internal or external cache is counted as a hit.
  • Loading branch information
WillChilds-Klein authored Nov 27, 2023
1 parent 89e1fd0 commit 4b18065
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 2 deletions.
3 changes: 2 additions & 1 deletion include/openssl/ssl.h
Original file line number Diff line number Diff line change
Expand Up @@ -5016,7 +5016,8 @@ OPENSSL_EXPORT int SSL_CTX_sess_accept_renegotiate(const SSL_CTX *ctx);
// SSL/TLS sessions in server mode.
OPENSSL_EXPORT int SSL_CTX_sess_accept_good(const SSL_CTX *ctx);

// SSL_CTX_sess_hits returns the number of successfully reused sessions.
// SSL_CTX_sess_hits returns the number of successfully reused sessions, from
// both session cache and session tickets.
OPENSSL_EXPORT int SSL_CTX_sess_hits(const SSL_CTX *ctx);

// SSL_CTX_sess_cb_hits returns the number of successfully retrieved sessions
Expand Down
5 changes: 5 additions & 0 deletions ssl/extensions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4019,6 +4019,11 @@ enum ssl_ticket_aead_result_t ssl_process_ticket(
// Other consumers may expect a non-empty session ID to indicate resumption.
session->session_id_length = SHA256_DIGEST_LENGTH;

// Ticket-based session resumption bypasses the session cache, but counts as
// session reuse, so update the session's "session hit" counter.
ssl_update_counter(ssl->session_ctx.get(), ssl->session_ctx->stats.sess_hit,
true);

*out_session = std::move(session);
return ssl_ticket_aead_success;
}
Expand Down
10 changes: 9 additions & 1 deletion ssl/ssl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9223,6 +9223,11 @@ TEST_P(SSLVersionTest, SameKeyResume) {
ClientConfig config;
config.session = session.get();

// No hits before we resume the connection
EXPECT_EQ(SSL_CTX_sess_hits(client_ctx_.get()), 0);
EXPECT_EQ(SSL_CTX_sess_hits(server_ctx_.get()), 0);
EXPECT_EQ(SSL_CTX_sess_hits(server_ctx2.get()), 0);

// Resuming with |server_ctx_| again works.
bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx_.get(),
Expand All @@ -9236,8 +9241,11 @@ TEST_P(SSLVersionTest, SameKeyResume) {
EXPECT_TRUE(SSL_session_reused(client.get()));
EXPECT_TRUE(SSL_session_reused(server.get()));

// By this point, the session has been resumed twice on the client side.
// By this point, the session has been resumed twice on the client side and
// once for each server context.
EXPECT_EQ(SSL_CTX_sess_hits(client_ctx_.get()), 2);
EXPECT_EQ(SSL_CTX_sess_hits(server_ctx_.get()), 1);
EXPECT_EQ(SSL_CTX_sess_hits(server_ctx2.get()), 1);
}

TEST_P(SSLVersionTest, DifferentKeyNoResume) {
Expand Down

0 comments on commit 4b18065

Please sign in to comment.