[release-branch.go1.8] crypto/tls: make Config.Clone also clone the GetClientCertific...

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>
(cherry picked from commit 87649d32ad16a9a0b7bd5dbd1c124b2032a270f1)
Reviewed-on: https://go-review.googlesource.com/37946
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 276d1761ea04033672d5ad0354327fa36bd34a4c..de833a905637259351433976f1c06b36d713a5f9 100644 (file)
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -563,6 +563,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()