Never resume sessions on renegotiations.
This cuts down on one config knob as well as one case in the renego
combinatorial explosion. Since the only case we care about with renego
is the client auth hack, there's no reason to ever do resumption.
Especially since, no matter what's in the session cache:
- OpenSSL will only ever offer the session it just established,
whether or not a newer one with client auth was since established.
- Chrome will never cache sessions created on a renegotiation, so
such a session would never make it to the session cache.
- The new_session + SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION
logic had a bug where it would unconditionally never offer tickets
(but would advertise support) on renego, so any server doing renego
resumption against an OpenSSL-derived client must not support
session tickets.
This also gets rid of s->new_session which is now pointless.
BUG=429450
Change-Id: I884bdcdc80bff45935b2c429b4bbc9c16b2288f8
Reviewed-on: https://boringssl-review.googlesource.com/4732
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 4ac7250..2e6ddf3 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -681,9 +681,9 @@
// fragments in DTLS.
SendEmptyFragments bool
- // NeverResumeOnRenego, if true, causes renegotiations to always be full
- // handshakes.
- NeverResumeOnRenego bool
+ // FailIfResumeOnRenego, if true, causes renegotiations to fail if the
+ // client offers a resumption or the server accepts one.
+ FailIfResumeOnRenego bool
// NoSignatureAlgorithmsOnRenego, if true, causes renegotiations to omit
// the signature_algorithms extension.
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 0dac05d..5129b8f 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -140,9 +140,6 @@
var session *ClientSessionState
var cacheKey string
sessionCache := c.config.ClientSessionCache
- if c.config.Bugs.NeverResumeOnRenego && c.cipherSuite != nil {
- sessionCache = nil
- }
if sessionCache != nil {
hello.ticketSupported = !c.config.SessionTicketsDisabled
@@ -727,6 +724,12 @@
}
if hs.serverResumedSession() {
+ // For test purposes, assert that the server never accepts the
+ // resumption offer on renegotiation.
+ if c.cipherSuite != nil && c.config.Bugs.FailIfResumeOnRenego {
+ return false, errors.New("tls: server resumed session on renegotiation")
+ }
+
// Restore masterSecret and peerCerts from previous state
hs.masterSecret = hs.session.masterSecret
c.peerCertificates = hs.session.serverCertificates
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 59ed9df..f159aff 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -333,6 +333,12 @@
_, hs.ecdsaOk = hs.cert.PrivateKey.(*ecdsa.PrivateKey)
+ // For test purposes, check that the peer never offers a session when
+ // renegotiating.
+ if c.cipherSuite != nil && len(hs.clientHello.sessionId) > 0 && c.config.Bugs.FailIfResumeOnRenego {
+ return false, errors.New("tls: offered resumption on renegotiation")
+ }
+
if hs.checkForResumption() {
return true, nil
}
@@ -382,10 +388,6 @@
func (hs *serverHandshakeState) checkForResumption() bool {
c := hs.c
- if c.config.Bugs.NeverResumeOnRenego && c.cipherSuite != nil {
- return false
- }
-
if len(hs.clientHello.sessionTicket) > 0 {
if c.config.SessionTicketsDisabled {
return false
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index ec2fede..6c16020 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2858,17 +2858,11 @@
func addRenegotiationTests() {
testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Renegotiate-Server",
- flags: []string{"-renegotiate"},
- shimWritesFirst: true,
- })
- testCases = append(testCases, testCase{
testType: serverTest,
- name: "Renegotiate-Server-Full",
+ name: "Renegotiate-Server",
config: Config{
Bugs: ProtocolBugs{
- NeverResumeOnRenego: true,
+ FailIfResumeOnRenego: true,
},
},
flags: []string{"-renegotiate"},
@@ -2943,7 +2937,6 @@
name: "Renegotiate-Server-NoSignatureAlgorithms",
config: Config{
Bugs: ProtocolBugs{
- NeverResumeOnRenego: true,
NoSignatureAlgorithmsOnRenego: true,
},
},
@@ -2952,14 +2945,10 @@
})
// TODO(agl): test the renegotiation info SCSV.
testCases = append(testCases, testCase{
- name: "Renegotiate-Client",
- renegotiate: true,
- })
- testCases = append(testCases, testCase{
- name: "Renegotiate-Client-Full",
+ name: "Renegotiate-Client",
config: Config{
Bugs: ProtocolBugs{
- NeverResumeOnRenego: true,
+ FailIfResumeOnRenego: true,
},
},
renegotiate: true,