Test that we reject Certificate or CertificateRequest in resumption

I was going to add a corresponding test for PAKEs and noticed we
neglected this for PSKs. This also fixes a bug in runner where client
auth + resumption handshakes as a server didn't work right. (I guess
none of our tests exercise this case.)

Change-Id: I9e82dcbca54aedba4059e45c3e40a39b390de34e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/75667
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

ssl/test/runner/common.go

diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 8a76672..36914ad 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1769,6 +1769,15 @@
 	// extension in a TLS 1.3 CertificateRequest.
 	SendCustomCertificateRequest uint16
 
+	// AlwaysSendCertificateRequest, if true, causes the server to send
+	// CertificateRequest in TLS 1.3, even in handshakes where it is not
+	// allowed, such as resumption.
+	AlwaysSendCertificateRequest bool
+
+	// AlwaysSendCertificate, if true, causes the server to send Certificate in
+	// TLS 1.3, even in handshakes where it is not allowed, such as resumption.
+	AlwaysSendCertificate bool
+
 	// SendSNIWarningAlert, if true, causes the server to send an
 	// unrecognized_name alert before the ServerHello.
 	SendSNIWarningAlert bool

ssl/test/runner/handshake_server.go

diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 00a6287..3ba14e3 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -1068,32 +1068,33 @@
 		c.writeRecord(recordTypeHandshake, encryptedExtensions.marshal())
 	}
 
-	if hs.sessionState == nil {
-		if config.ClientAuth >= RequestClientCert {
-			// Request a client certificate
-			certReq := &certificateRequestMsg{
-				vers:                  c.wireVersion,
-				hasSignatureAlgorithm: !config.Bugs.OmitCertificateRequestAlgorithms,
-				hasRequestContext:     true,
-				requestContext:        config.Bugs.SendRequestContext,
-				customExtension:       config.Bugs.SendCustomCertificateRequest,
-			}
-			if !config.Bugs.NoSignatureAlgorithms {
-				certReq.signatureAlgorithms = config.verifySignatureAlgorithms()
-			}
-
-			// An empty list of certificateAuthorities signals to
-			// the client that it may send any certificate in response
-			// to our request. When we know the CAs we trust, then
-			// we can send them down, so that the client can choose
-			// an appropriate certificate to give to us.
-			if config.ClientCAs != nil {
-				certReq.certificateAuthorities = config.ClientCAs.Subjects()
-			}
-			hs.writeServerHash(certReq.marshal())
-			c.writeRecord(recordTypeHandshake, certReq.marshal())
+	requestClientCert := config.ClientAuth >= RequestClientCert && (hs.sessionState == nil || config.Bugs.AlwaysSendCertificateRequest)
+	if requestClientCert {
+		// Request a client certificate
+		certReq := &certificateRequestMsg{
+			vers:                  c.wireVersion,
+			hasSignatureAlgorithm: !config.Bugs.OmitCertificateRequestAlgorithms,
+			hasRequestContext:     true,
+			requestContext:        config.Bugs.SendRequestContext,
+			customExtension:       config.Bugs.SendCustomCertificateRequest,
+		}
+		if !config.Bugs.NoSignatureAlgorithms {
+			certReq.signatureAlgorithms = config.verifySignatureAlgorithms()
 		}
 
+		// An empty list of certificateAuthorities signals to
+		// the client that it may send any certificate in response
+		// to our request. When we know the CAs we trust, then
+		// we can send them down, so that the client can choose
+		// an appropriate certificate to give to us.
+		if config.ClientCAs != nil {
+			certReq.certificateAuthorities = config.ClientCAs.Subjects()
+		}
+		hs.writeServerHash(certReq.marshal())
+		c.writeRecord(recordTypeHandshake, certReq.marshal())
+	}
+
+	if hs.sessionState == nil || config.Bugs.AlwaysSendCertificate {
 		certMsg := &certificateMsg{
 			hasRequestContext: true,
 		}
@@ -1308,7 +1309,7 @@
 
 	// If we requested a client certificate, then the client must send a
 	// certificate message, even if it's empty.
-	if config.ClientAuth >= RequestClientCert {
+	if requestClientCert {
 		certMsg, err := readHandshakeType[certificateMsg](c)
 		if err != nil {
 			return err

ssl/test/runner/runner.go

diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 4a58458..b16f43b 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -17015,6 +17015,41 @@
 		expectedLocalError: "remote error: unexpected message",
 	})
 
+	// PSK/resumption handshakes should not accept CertificateRequest or
+	// Certificate messages.
+	testCases = append(testCases, testCase{
+		testType: clientTest,
+		name:     "CertificateInResumption-TLS13",
+		config: Config{
+			MinVersion: VersionTLS13,
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				AlwaysSendCertificate: true,
+			},
+		},
+		resumeSession:      true,
+		shouldFail:         true,
+		expectedError:      ":UNEXPECTED_MESSAGE:",
+		expectedLocalError: "remote error: unexpected message",
+	})
+	testCases = append(testCases, testCase{
+		testType: clientTest,
+		name:     "CertificateRequestInResumption-TLS13",
+		config: Config{
+			MinVersion: VersionTLS13,
+			MaxVersion: VersionTLS13,
+			ClientAuth: RequireAnyClientCert,
+			Bugs: ProtocolBugs{
+				AlwaysSendCertificateRequest: true,
+			},
+		},
+		shimCertificate:    &rsaCertificate,
+		resumeSession:      true,
+		shouldFail:         true,
+		expectedError:      ":UNEXPECTED_MESSAGE:",
+		expectedLocalError: "remote error: unexpected message",
+	})
+
 	// If the client or server has 0-RTT enabled but disabled TLS 1.3, it should
 	// report a reason of protocol_version.
 	testCases = append(testCases, testCase{