crypto/tls: fix ConnectionState().VerifiedChains for resumed connection

Strengthening VerifyHostname exposed the fact that for resumed
connections, ConnectionState().VerifiedChains was not being saved
and restored during the ClientSessionCache operations.
Do that.

This change just saves the verified chains in the client's session
cache. It does not re-verify the certificates when resuming a
connection.

There are arguments both ways about this: we want fast, light-weight
resumption connections (thus suggesting that we shouldn't verify) but
it could also be a little surprising that, if the verification config
is changed, that would be ignored if the same session cache is used.

On the server side we do re-verify client-auth certificates, but the
situation is a little different there. The client session cache is an
object in memory that's reset each time the process restarts. But the
server's session cache is a conceptual object, held by the clients, so
can persist across server restarts. Thus the chance of a change in
verification config being surprisingly ignored is much higher in the
server case.

Fixes #12024.

Change-Id: I3081029623322ce3d9f4f3819659fdd9a381db16
Reviewed-on: https://go-review.googlesource.com/13164
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 12500ab72ad4f5795bd4b99bde916fce48368147..a3d75d69cbfadf33acc4a6726cd0f340c799f32f 100644 (file)
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -191,11 +191,12 @@ const (
 // ClientSessionState contains the state needed by clients to resume TLS
 // sessions.
 type ClientSessionState struct {
-       sessionTicket      []uint8             // Encrypted ticket used for session resumption with server
-       vers               uint16              // SSL/TLS version negotiated for the session
-       cipherSuite        uint16              // Ciphersuite negotiated for the session
-       masterSecret       []byte              // MasterSecret generated by client on a full handshake
-       serverCertificates []*x509.Certificate // Certificate chain presented by the server
+       sessionTicket      []uint8               // Encrypted ticket used for session resumption with server
+       vers               uint16                // SSL/TLS version negotiated for the session
+       cipherSuite        uint16                // Ciphersuite negotiated for the session
+       masterSecret       []byte                // MasterSecret generated by client on a full handshake
+       serverCertificates []*x509.Certificate   // Certificate chain presented by the server
+       verifiedChains     [][]*x509.Certificate // Certificate chains we built for verification
 }
 
 // ClientSessionCache is a cache of ClientSessionState objects that can be used

diff --git a/src/crypto/tls/handshake_client.go b/src/crypto/tls/handshake_client.go
index 6b092649c373ebc408c4c958e37ea226627c5056..0b591d7309c2592b9067b8ba796519806260e1fd 100644 (file)
--- a/src/crypto/tls/handshake_client.go
+++ b/src/crypto/tls/handshake_client.go
@@ -547,6 +547,7 @@ func (hs *clientHandshakeState) processServerHello() (bool, error) {
                // Restore masterSecret and peerCerts from previous state
                hs.masterSecret = hs.session.masterSecret
                c.peerCertificates = hs.session.serverCertificates
+               c.verifiedChains = hs.session.verifiedChains
                return true, nil
        }
        return false, nil
@@ -604,6 +605,7 @@ func (hs *clientHandshakeState) readSessionTicket() error {
                cipherSuite:        hs.suite.id,
                masterSecret:       hs.masterSecret,
                serverCertificates: c.peerCertificates,
+               verifiedChains:     c.verifiedChains,
        }
 
        return nil

diff --git a/src/crypto/tls/handshake_client_test.go b/src/crypto/tls/handshake_client_test.go
index 5fc57b0f17db890a409cb1c737775a3de0fd2ed5..664fe8de6a0c73b0439cffcc5621955f49c955fd 100644 (file)
--- a/src/crypto/tls/handshake_client_test.go
+++ b/src/crypto/tls/handshake_client_test.go
@@ -406,10 +406,20 @@ func TestClientResumption(t *testing.T) {
                CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_SHA, TLS_ECDHE_RSA_WITH_RC4_128_SHA},
                Certificates: testConfig.Certificates,
        }
+
+       issuer, err := x509.ParseCertificate(testRSACertificateIssuer)
+       if err != nil {
+               panic(err)
+       }
+
+       rootCAs := x509.NewCertPool()
+       rootCAs.AddCert(issuer)
+
        clientConfig := &Config{
                CipherSuites:       []uint16{TLS_RSA_WITH_RC4_128_SHA},
-               InsecureSkipVerify: true,
                ClientSessionCache: NewLRUClientSessionCache(32),
+               RootCAs:            rootCAs,
+               ServerName:         "example.golang",
        }
 
        testResumeState := func(test string, didResume bool) {
@@ -420,6 +430,9 @@ func TestClientResumption(t *testing.T) {
                if hs.DidResume != didResume {
                        t.Fatalf("%s resumed: %v, expected: %v", test, hs.DidResume, didResume)
                }
+               if didResume && (hs.PeerCertificates == nil || hs.VerifiedChains == nil) {
+                       t.Fatalf("expected non-nil certificates after resumption. Got peerCertificates: %#v, verifedCertificates: %#v", hs.PeerCertificates, hs.VerifiedChains)
+               }
        }
 
        getTicket := func() []byte {

diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go
index 8e22c9cafa3351a5bd064c8c5cd497a31bc10d35..c45c10378d76344ed34e1795fbec10391a628ebe 100644 (file)
--- a/src/crypto/tls/tls_test.go
+++ b/src/crypto/tls/tls_test.go
@@ -307,3 +307,28 @@ func TestVerifyHostname(t *testing.T) {
                t.Fatalf("verify www.google.com succeeded with InsecureSkipVerify=true")
        }
 }
+
+func TestVerifyHostnameResumed(t *testing.T) {
+       testenv.MustHaveExternalNetwork(t)
+
+       config := &Config{
+               ClientSessionCache: NewLRUClientSessionCache(32),
+       }
+       for i := 0; i < 2; i++ {
+               c, err := Dial("tcp", "www.google.com:https", config)
+               if err != nil {
+                       t.Fatalf("Dial #%d: %v", i, err)
+               }
+               cs := c.ConnectionState()
+               if i > 0 && !cs.DidResume {
+                       t.Fatalf("Subsequent connection unexpectedly didn't resume")
+               }
+               if cs.VerifiedChains == nil {
+                       t.Fatalf("Dial #%d: cs.VerifiedChains == nil", i)
+               }
+               if err := c.VerifyHostname("www.google.com"); err != nil {
+                       t.Fatalf("verify www.google.com #%d: %v", i, err)
+               }
+               c.Close()
+       }
+}