commit 63c79122e094358dfca206471831df6f51ff28bf
author David Benjamin <davidben@google.com> Fri Jun 29 19:28:17 2018 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue Jul 03 22:54:16 2018 +0000
tree 8852cc39f8997638d914b4837b3eba028858826d
parent 53d2c7a84ad9918562df202eca7a975e742361db

Remove the redundant version check in ssl_session_cmp.

This partitions the session ID space of the internal cache by version,
which is nominally something we want, but we must check the version
externally anyway for both tickets and external session cache. That
makes this measure redundant. (Servers generate session IDs and 2^256 is
huge, so there would never accidentally be a collision.)

This cuts down on the "key" in the internal session cache, which will
simplify adding something like an lh_SSL_SESSION_retrieve_key function.
(LHASH is currently lax about keys because it can freely stack-allocate
partially-initialized structs. C++ is a bit more finicky about this.)

Change-Id: I656fd9dbf023dccb163d2e8049eff8f1f9a0e21b
Reviewed-on: https://boringssl-review.googlesource.com/29585
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>

ssl/ssl_lib.cc

diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 0bf58a4..222a316 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -585,16 +585,7 @@
   return hash;
 }
 
-// NB: If this function (or indeed the hash function which uses a sort of
-// coarser function than this one) is changed, ensure
-// SSL_CTX_has_matching_session_id() is checked accordingly. It relies on being
-// able to construct an SSL_SESSION that will collide with any existing session
-// with a matching session ID.
 static int ssl_session_cmp(const SSL_SESSION *a, const SSL_SESSION *b) {
-  if (a->ssl_version != b->ssl_version) {
-    return 1;
-  }
-
   if (a->session_id_length != b->session_id_length) {
     return 1;
   }

ssl/ssl_session.cc

diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 1a098fb..adec8dc 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -670,7 +670,6 @@
   if (!(ssl->session_ctx->session_cache_mode &
         SSL_SESS_CACHE_NO_INTERNAL_LOOKUP)) {
     SSL_SESSION data;
-    data.ssl_version = ssl->version;
     data.session_id_length = session_id_len;
     OPENSSL_memcpy(data.session_id, session_id, session_id_len);
 

ssl/test/runner/runner.go

diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index d9e4d36..53f6ac8 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -7609,6 +7609,27 @@
 						"-on-resume-tls13-variant", strconv.Itoa(resumeVers.tls13Variant),
 					},
 				})
+
+				// Repeat the test using session IDs, rather than tickets.
+				if sessionVers.version < VersionTLS13 && resumeVers.version < VersionTLS13 {
+					testCases = append(testCases, testCase{
+						protocol:      protocol,
+						testType:      serverTest,
+						name:          "Resume-Server-NoTickets" + suffix,
+						resumeSession: true,
+						config: Config{
+							MaxVersion:             sessionVers.version,
+							SessionTicketsDisabled: true,
+						},
+						expectedVersion:      sessionVers.version,
+						expectResumeRejected: sessionVers != resumeVers,
+						resumeConfig: &Config{
+							MaxVersion:             resumeVers.version,
+							SessionTicketsDisabled: true,
+						},
+						expectedResumeVersion: resumeVers.version,
+					})
+				}
 			}
 		}
 	}