crypto/tls: make Config.Clone also clone the GetClientCertificate field

Using GetClientCertificate with the http client is currently completely
broken because inside the transport we clone the tls.Config and pass it
off to the tls.Client. Since tls.Config.Clone() does not pass forward
the GetClientCertificate field, GetClientCertificate is ignored in this
context.

Fixes #19264

Change-Id: Ie214f9f0039ac7c3a2dab8ffd14d30668bdb4c71
Signed-off-by: Mike Danese <mikedanese@google.com>
Reviewed-on: https://go-review.googlesource.com/37541
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index f0e30dcc5db1811c8aedd0ef2de81ab147c47a56..6bf9dc193e57a3b80e4d1ddc3f10c34a8fc03b86 100644 (file)
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -564,6 +564,7 @@ func (c *Config) Clone() *Config {
                Certificates:                c.Certificates,
                NameToCertificate:           c.NameToCertificate,
                GetCertificate:              c.GetCertificate,
+               GetClientCertificate:        c.GetClientCertificate,
                GetConfigForClient:          c.GetConfigForClient,
                VerifyPeerCertificate:       c.VerifyPeerCertificate,
                RootCAs:                     c.RootCAs,

diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go
index 8933f4f2015e1be457e55d2ad7fb8e66e23cb460..86812f0c974cf36076d133e46106596f668f9654 100644 (file)
--- a/src/crypto/tls/tls_test.go
+++ b/src/crypto/tls/tls_test.go
@@ -13,13 +13,11 @@ import (
        "io"
        "io/ioutil"
        "math"
-       "math/rand"
        "net"
        "os"
        "reflect"
        "strings"
        "testing"
-       "testing/quick"
        "time"
 )
 
@@ -568,11 +566,50 @@ func TestConnCloseWrite(t *testing.T) {
        }
 }
 
-func TestClone(t *testing.T) {
+func TestCloneFuncFields(t *testing.T) {
+       const expectedCount = 5
+       called := 0
+
+       c1 := Config{
+               Time: func() time.Time {
+                       called |= 1 << 0
+                       return time.Time{}
+               },
+               GetCertificate: func(*ClientHelloInfo) (*Certificate, error) {
+                       called |= 1 << 1
+                       return nil, nil
+               },
+               GetClientCertificate: func(*CertificateRequestInfo) (*Certificate, error) {
+                       called |= 1 << 2
+                       return nil, nil
+               },
+               GetConfigForClient: func(*ClientHelloInfo) (*Config, error) {
+                       called |= 1 << 3
+                       return nil, nil
+               },
+               VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
+                       called |= 1 << 4
+                       return nil
+               },
+       }
+
+       c2 := c1.Clone()
+
+       c2.Time()
+       c2.GetCertificate(nil)
+       c2.GetClientCertificate(nil)
+       c2.GetConfigForClient(nil)
+       c2.VerifyPeerCertificate(nil, nil)
+
+       if called != (1<<expectedCount)-1 {
+               t.Fatalf("expected %d calls but saw calls %b", expectedCount, called)
+       }
+}
+
+func TestCloneNonFuncFields(t *testing.T) {
        var c1 Config
        v := reflect.ValueOf(&c1).Elem()
 
-       rnd := rand.New(rand.NewSource(time.Now().Unix()))
        typ := v.Type()
        for i := 0; i < typ.NumField(); i++ {
                f := v.Field(i)
@@ -581,40 +618,49 @@ func TestClone(t *testing.T) {
                        continue
                }
 
-               // testing/quick can't handle functions or interfaces.
-               fn := typ.Field(i).Name
-               switch fn {
+               // testing/quick can't handle functions or interfaces and so
+               // isn't used here.
+               switch fn := typ.Field(i).Name; fn {
                case "Rand":
                        f.Set(reflect.ValueOf(io.Reader(os.Stdin)))
-                       continue
                case "Time", "GetCertificate", "GetConfigForClient", "VerifyPeerCertificate", "GetClientCertificate":
-                       // DeepEqual can't compare functions.
-                       continue
+                       // DeepEqual can't compare functions. If you add a
+                       // function field to this list, you must also change
+                       // TestCloneFuncFields to ensure that the func field is
+                       // cloned.
                case "Certificates":
                        f.Set(reflect.ValueOf([]Certificate{
                                {Certificate: [][]byte{{'b'}}},
                        }))
-                       continue
                case "NameToCertificate":
                        f.Set(reflect.ValueOf(map[string]*Certificate{"a": nil}))
-                       continue
                case "RootCAs", "ClientCAs":
                        f.Set(reflect.ValueOf(x509.NewCertPool()))
-                       continue
                case "ClientSessionCache":
                        f.Set(reflect.ValueOf(NewLRUClientSessionCache(10)))
-                       continue
                case "KeyLogWriter":
                        f.Set(reflect.ValueOf(io.Writer(os.Stdout)))
-                       continue
-
-               }
-
-               q, ok := quick.Value(f.Type(), rnd)
-               if !ok {
-                       t.Fatalf("quick.Value failed on field %s", fn)
+               case "NextProtos":
+                       f.Set(reflect.ValueOf([]string{"a", "b"}))
+               case "ServerName":
+                       f.Set(reflect.ValueOf("b"))
+               case "ClientAuth":
+                       f.Set(reflect.ValueOf(VerifyClientCertIfGiven))
+               case "InsecureSkipVerify", "SessionTicketsDisabled", "DynamicRecordSizingDisabled", "PreferServerCipherSuites":
+                       f.Set(reflect.ValueOf(true))
+               case "MinVersion", "MaxVersion":
+                       f.Set(reflect.ValueOf(uint16(VersionTLS12)))
+               case "SessionTicketKey":
+                       f.Set(reflect.ValueOf([32]byte{}))
+               case "CipherSuites":
+                       f.Set(reflect.ValueOf([]uint16{1, 2}))
+               case "CurvePreferences":
+                       f.Set(reflect.ValueOf([]CurveID{CurveP256}))
+               case "Renegotiation":
+                       f.Set(reflect.ValueOf(RenegotiateOnceAsClient))
+               default:
+                       t.Errorf("all fields must be accounted for, but saw unknown field %q", fn)
                }
-               f.Set(q)
        }
 
        c2 := c1.Clone()