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

@@ -585,16 +585,7 @@ static uint32_t ssl_session_hash(const SSL_SESSION *sess) {
   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

@@ -670,7 +670,6 @@ static enum ssl_hs_wait_t ssl_lookup_session(
   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

@@ -7609,6 +7609,27 @@ func addResumptionVersionTests() {
 						"-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,
+					})
+				}
 			}
 		}
 	}