crypto/tls: set ServerName and unset TLSUnique in ConnectionState in TLS 1.3

Fix a couple overlooked ConnectionState fields noticed by net/http
tests, and add a test in crypto/tls. Spun off CL 147638 to keep that one
cleanly about enabling TLS 1.3.

Change-Id: I9a6c2e68d64518a44be2a5d7b0b7b8d78c98c95d
Reviewed-on: https://go-review.googlesource.com/c/148900
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 62d786aeae7c4ccbd1f68f79fa3ebfb4f35883e7..fefb68adc7855a718a45ac4a5f5f24005e2e6ae2 100644 (file)
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -208,8 +208,8 @@ type ConnectionState struct {
        ServerName                  string                // server name requested by client, if any (server side only)
        PeerCertificates            []*x509.Certificate   // certificate chain presented by remote peer
        VerifiedChains              [][]*x509.Certificate // verified chains built from PeerCertificates
-       SignedCertificateTimestamps [][]byte              // SCTs from the server, if any
-       OCSPResponse                []byte                // stapled OCSP response from server, if any
+       SignedCertificateTimestamps [][]byte              // SCTs from the peer, if any
+       OCSPResponse                []byte                // stapled OCSP response from peer, if any
 
        // ekm is a closure exposed via ExportKeyingMaterial.
        ekm func(label string, context []byte, length int) ([]byte, error)
@@ -219,7 +219,7 @@ type ConnectionState struct {
        // because resumption does not include enough context (see
        // https://mitls.org/pages/attacks/3SHAKE#channelbindings). This will
        // change in future versions of Go once the TLS master-secret fix has
-       // been standardized and implemented.
+       // been standardized and implemented. It is not defined in TLS 1.3.
        TLSUnique []byte
 }
 

diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go
index 6786d19748e069c25a04c1d20eb77f10207125bc..f61d43203fd93333cca4a838404a03627f07ee2a 100644 (file)
--- a/src/crypto/tls/conn.go
+++ b/src/crypto/tls/conn.go
@@ -1378,7 +1378,7 @@ func (c *Conn) ConnectionState() ConnectionState {
                state.VerifiedChains = c.verifiedChains
                state.SignedCertificateTimestamps = c.scts
                state.OCSPResponse = c.ocspResponse
-               if !c.didResume {
+               if !c.didResume && c.vers != VersionTLS13 {
                        if c.clientFinishedIsFirst {
                                state.TLSUnique = c.clientFinished[:]
                        } else {

diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go
index 4d13ff39d942dd83b3f5530319537aed8234e757..a689096caecc245326528ef7e17b1fbc1ceb0d47 100644 (file)
--- a/src/crypto/tls/handshake_server_tls13.go
+++ b/src/crypto/tls/handshake_server_tls13.go
@@ -213,6 +213,7 @@ GroupSelection:
                return errors.New("tls: invalid client key share")
        }
 
+       c.serverName = hs.clientHello.serverName
        return nil
 }
 

diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go
index e9abe01280fd21ab0b48d6b2eecfbdc822accf39..fac3522b7de20f9ada2ca2155649e3af73e79b8d 100644 (file)
--- a/src/crypto/tls/tls_test.go
+++ b/src/crypto/tls/tls_test.go
@@ -916,3 +916,119 @@ func TestConnectionStateMarshal(t *testing.T) {
                t.Errorf("json.Marshal failed on ConnectionState: %v", err)
        }
 }
+
+func TestConnectionState(t *testing.T) {
+       issuer, err := x509.ParseCertificate(testRSACertificateIssuer)
+       if err != nil {
+               panic(err)
+       }
+       rootCAs := x509.NewCertPool()
+       rootCAs.AddCert(issuer)
+
+       now := func() time.Time { return time.Unix(1476984729, 0) }
+
+       const alpnProtocol = "golang"
+       const serverName = "example.golang"
+       var scts = [][]byte{[]byte("dummy sct 1"), []byte("dummy sct 2")}
+       var ocsp = []byte("dummy ocsp")
+
+       for _, v := range []uint16{VersionTLS12, VersionTLS13} {
+               var name string
+               switch v {
+               case VersionTLS12:
+                       name = "TLSv12"
+               case VersionTLS13:
+                       name = "TLSv13"
+               }
+               t.Run(name, func(t *testing.T) {
+                       config := &Config{
+                               Time:         now,
+                               Rand:         zeroSource{},
+                               Certificates: make([]Certificate, 1),
+                               MaxVersion:   v,
+                               RootCAs:      rootCAs,
+                               ClientCAs:    rootCAs,
+                               ClientAuth:   RequireAndVerifyClientCert,
+                               NextProtos:   []string{alpnProtocol},
+                               ServerName:   serverName,
+                       }
+                       config.Certificates[0].Certificate = [][]byte{testRSACertificate}
+                       config.Certificates[0].PrivateKey = testRSAPrivateKey
+                       config.Certificates[0].SignedCertificateTimestamps = scts
+                       config.Certificates[0].OCSPStaple = ocsp
+
+                       ss, cs, err := testHandshake(t, config, config)
+                       if err != nil {
+                               t.Fatalf("Handshake failed: %v", err)
+                       }
+
+                       if ss.Version != v || cs.Version != v {
+                               t.Errorf("Got versions %x (server) and %x (client), expected %x", ss.Version, cs.Version, v)
+                       }
+
+                       if !ss.HandshakeComplete || !cs.HandshakeComplete {
+                               t.Errorf("Got HandshakeComplete %v (server) and %v (client), expected true", ss.HandshakeComplete, cs.HandshakeComplete)
+                       }
+
+                       if ss.DidResume || cs.DidResume {
+                               t.Errorf("Got DidResume %v (server) and %v (client), expected false", ss.DidResume, cs.DidResume)
+                       }
+
+                       if ss.CipherSuite == 0 || cs.CipherSuite == 0 {
+                               t.Errorf("Got invalid cipher suite: %v (server) and %v (client)", ss.CipherSuite, cs.CipherSuite)
+                       }
+
+                       if ss.NegotiatedProtocol != alpnProtocol || cs.NegotiatedProtocol != alpnProtocol {
+                               t.Errorf("Got negotiated protocol %q (server) and %q (client), expected %q", ss.NegotiatedProtocol, cs.NegotiatedProtocol, alpnProtocol)
+                       }
+
+                       if !cs.NegotiatedProtocolIsMutual {
+                               t.Errorf("Got false NegotiatedProtocolIsMutual on the client side")
+                       }
+                       // NegotiatedProtocolIsMutual on the server side is unspecified.
+
+                       if ss.ServerName != serverName {
+                               t.Errorf("Got server name %q, expected %q", ss.ServerName, serverName)
+                       }
+                       if cs.ServerName != "" {
+                               t.Errorf("Got unexpected server name on the client side")
+                       }
+
+                       if len(ss.PeerCertificates) != 1 || len(cs.PeerCertificates) != 1 {
+                               t.Errorf("Got %d (server) and %d (client) peer certificates, expected %d", len(ss.PeerCertificates), len(cs.PeerCertificates), 1)
+                       }
+
+                       if len(ss.VerifiedChains) != 1 || len(cs.VerifiedChains) != 1 {
+                               t.Errorf("Got %d (server) and %d (client) verified chains, expected %d", len(ss.VerifiedChains), len(cs.VerifiedChains), 1)
+                       } else if len(ss.VerifiedChains[0]) != 2 || len(cs.VerifiedChains[0]) != 2 {
+                               t.Errorf("Got %d (server) and %d (client) long verified chain, expected %d", len(ss.VerifiedChains[0]), len(cs.VerifiedChains[0]), 2)
+                       }
+
+                       if len(cs.SignedCertificateTimestamps) != 2 {
+                               t.Errorf("Got %d SCTs, expected %d", len(cs.SignedCertificateTimestamps), 2)
+                       }
+                       if !bytes.Equal(cs.OCSPResponse, ocsp) {
+                               t.Errorf("Got OCSPs %x, expected %x", cs.OCSPResponse, ocsp)
+                       }
+                       // Only TLS 1.3 supports OCSP and SCTs on client certs.
+                       if v == VersionTLS13 {
+                               if len(ss.SignedCertificateTimestamps) != 2 {
+                                       t.Errorf("Got %d client SCTs, expected %d", len(ss.SignedCertificateTimestamps), 2)
+                               }
+                               if !bytes.Equal(ss.OCSPResponse, ocsp) {
+                                       t.Errorf("Got client OCSPs %x, expected %x", ss.OCSPResponse, ocsp)
+                               }
+                       }
+
+                       if v == VersionTLS13 {
+                               if ss.TLSUnique != nil || cs.TLSUnique != nil {
+                                       t.Errorf("Got TLSUnique %x (server) and %x (client), expected nil in TLS 1.3", ss.TLSUnique, cs.TLSUnique)
+                               }
+                       } else {
+                               if ss.TLSUnique == nil || cs.TLSUnique == nil {
+                                       t.Errorf("Got TLSUnique %x (server) and %x (client), expected non-nil", ss.TLSUnique, cs.TLSUnique)
+                               }
+                       }
+               })
+       }
+}