Fix DTLS cross-version resumption tests
For now, just have runner tolerate the PSK extension changing across
HVR. It's possible we'll want to change this, but I think we can resolve
that separately.
The tests required an additional fix to deal with a syntactic
impossibility in DTLS. (Also QUIC, if TLS 1.2 in QUIC existed.)
Bug: 42290594
Change-Id: Ia06975104933e049bcd2c62aa8a29fd74b20f474
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73827
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 7f890fd..2198ffd 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -323,7 +323,12 @@
if !bytes.Equal(newClientHello.cookie, helloVerifyRequest.cookie) {
return errors.New("dtls: invalid cookie")
}
- if err := checkClientHellosEqual(hs.clientHello.raw, newClientHello.raw, c.isDTLS, nil); err != nil {
+ // Allow the client to recompute the pre_shared_key extension. It is
+ // possible we'll want to remove this and instead require the client
+ // keep it unchanged. (Either by remembering it or hashing the
+ // ClientHello funny.) See discussion at
+ // https://mailarchive.ietf.org/arch/msg/tls/HSTgV2voS9tn03oasGmBAdwazWA/
+ if err := checkClientHellosEqual(hs.clientHello.raw, newClientHello.raw, c.isDTLS, []uint16{extensionPreSharedKey}); err != nil {
return err
}
hs.clientHello = newClientHello
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 84fc93e..25651f6 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -9245,13 +9245,6 @@
suffix := "-" + sessionVers.name + "-" + resumeVers.name
suffix += "-" + protocol.String()
- // TODO(crbug.com/42290594): Attempting to resume a DTLS 1.3 session
- // when the new connection is an older version is currently broken.
- // The current implementation recomputes the PSK binder value for the
- // second ClientHello (sent in response to HelloVerifyRequest), but
- // the runner expects all extension values to stay byte-for-byte the
- // same (a possible interpretation of RFC 6347 section 4.2.1).
- isBadDTLSResumption := protocol == dtls && sessionVers.version == VersionTLS13 && resumeVers.version < VersionTLS13
if sessionVers.version == resumeVers.version {
testCases = append(testCases, testCase{
protocol: protocol,
@@ -9270,7 +9263,37 @@
version: resumeVers.version,
},
})
- } else if !isBadDTLSResumption {
+ } else if protocol != tls && sessionVers.version >= VersionTLS13 && resumeVers.version < VersionTLS13 {
+ // In TLS 1.2 and below, the server indicates resumption by echoing
+ // the client's session ID, which is impossible if the client did
+ // not send a session ID. If the client offers a TLS 1.3 session, it
+ // only fills in session ID in TLS (not DTLS or QUIC) for middlebox
+ // compatibility mode. So, instead, test that the session ID was
+ // empty and it was indeed impossible to hit this path
+ testCases = append(testCases, testCase{
+ protocol: protocol,
+ name: "Resume-Client-Impossible" + suffix,
+ resumeSession: true,
+ config: Config{
+ MaxVersion: sessionVers.version,
+ },
+ expectations: connectionExpectations{
+ version: sessionVers.version,
+ },
+ resumeConfig: &Config{
+ MaxVersion: resumeVers.version,
+ Bugs: ProtocolBugs{
+ ExpectNoSessionID: true,
+ },
+ },
+ resumeExpectations: &connectionExpectations{
+ version: resumeVers.version,
+ },
+ expectResumeRejected: true,
+ })
+ } else {
+ // Test that the client rejects ServerHellos which resume
+ // sessions at inconsistent versions.
expectedError := ":OLD_SESSION_VERSION_NOT_RETURNED:"
if sessionVers.version < VersionTLS13 && resumeVers.version >= VersionTLS13 {
// The server will "resume" the session by sending pre_shared_key,
@@ -9278,6 +9301,7 @@
// should reject this because the extension was not allowed at all.
expectedError = ":UNEXPECTED_EXTENSION:"
}
+
testCases = append(testCases, testCase{
protocol: protocol,
name: "Resume-Client-Mismatch" + suffix,
@@ -9302,27 +9326,25 @@
})
}
- if !isBadDTLSResumption {
- testCases = append(testCases, testCase{
- protocol: protocol,
- name: "Resume-Client-NoResume" + suffix,
- resumeSession: true,
- config: Config{
- MaxVersion: sessionVers.version,
- },
- expectations: connectionExpectations{
- version: sessionVers.version,
- },
- resumeConfig: &Config{
- MaxVersion: resumeVers.version,
- },
- newSessionsOnResume: true,
- expectResumeRejected: true,
- resumeExpectations: &connectionExpectations{
- version: resumeVers.version,
- },
- })
- }
+ testCases = append(testCases, testCase{
+ protocol: protocol,
+ name: "Resume-Client-NoResume" + suffix,
+ resumeSession: true,
+ config: Config{
+ MaxVersion: sessionVers.version,
+ },
+ expectations: connectionExpectations{
+ version: sessionVers.version,
+ },
+ resumeConfig: &Config{
+ MaxVersion: resumeVers.version,
+ },
+ newSessionsOnResume: true,
+ expectResumeRejected: true,
+ resumeExpectations: &connectionExpectations{
+ version: resumeVers.version,
+ },
+ })
testCases = append(testCases, testCase{
protocol: protocol,
