Skip to content

Commit ea6781b

Browse files
mikedanesebradfitz
mikedanese
authored and
bradfitz
committed
[release-branch.go1.8] 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 golang#19264 Change-Id: Ie214f9f0039ac7c3a2dab8ffd14d30668bdb4c71 Signed-off-by: Mike Danese <[email protected]> Reviewed-on: https://go-review.googlesource.com/37541 Reviewed-by: Filippo Valsorda <[email protected]> Reviewed-by: Adam Langley <[email protected]> Run-TryBot: Adam Langley <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> (cherry picked from commit 87649d3) Reviewed-on: https://go-review.googlesource.com/37946 Run-TryBot: Brad Fitzpatrick <[email protected]> Reviewed-by: Tom Bergan <[email protected]>
1 parent 2327d69 commit ea6781b

2 files changed

Lines changed: 69 additions & 22 deletions

File tree

‎src/crypto/tls/common.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,7 @@ func (c *Config) Clone() *Config {
563563
Certificates: c.Certificates,
564564
NameToCertificate: c.NameToCertificate,
565565
GetCertificate: c.GetCertificate,
566+
GetClientCertificate: c.GetClientCertificate,
566567
GetConfigForClient: c.GetConfigForClient,
567568
VerifyPeerCertificate: c.VerifyPeerCertificate,
568569
RootCAs: c.RootCAs,

‎src/crypto/tls/tls_test.go‎

Lines changed: 68 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,11 @@ import (
1313
"io"
1414
"io/ioutil"
1515
"math"
16-
"math/rand"
1716
"net"
1817
"os"
1918
"reflect"
2019
"strings"
2120
"testing"
22-
"testing/quick"
2321
"time"
2422
)
2523

@@ -568,11 +566,50 @@ func TestConnCloseWrite(t *testing.T) {
568566
}
569567
}
570568

571-
func TestClone(t *testing.T) {
569+
func TestCloneFuncFields(t *testing.T) {
570+
const expectedCount = 5
571+
called := 0
572+
573+
c1 := Config{
574+
Time: func() time.Time {
575+
called |= 1 << 0
576+
return time.Time{}
577+
},
578+
GetCertificate: func(*ClientHelloInfo) (*Certificate, error) {
579+
called |= 1 << 1
580+
return nil, nil
581+
},
582+
GetClientCertificate: func(*CertificateRequestInfo) (*Certificate, error) {
583+
called |= 1 << 2
584+
return nil, nil
585+
},
586+
GetConfigForClient: func(*ClientHelloInfo) (*Config, error) {
587+
called |= 1 << 3
588+
return nil, nil
589+
},
590+
VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
591+
called |= 1 << 4
592+
return nil
593+
},
594+
}
595+
596+
c2 := c1.Clone()
597+
598+
c2.Time()
599+
c2.GetCertificate(nil)
600+
c2.GetClientCertificate(nil)
601+
c2.GetConfigForClient(nil)
602+
c2.VerifyPeerCertificate(nil, nil)
603+
604+
if called != (1<<expectedCount)-1 {
605+
t.Fatalf("expected %d calls but saw calls %b", expectedCount, called)
606+
}
607+
}
608+
609+
func TestCloneNonFuncFields(t *testing.T) {
572610
var c1 Config
573611
v := reflect.ValueOf(&c1).Elem()
574612

575-
rnd := rand.New(rand.NewSource(time.Now().Unix()))
576613
typ := v.Type()
577614
for i := 0; i < typ.NumField(); i++ {
578615
f := v.Field(i)
@@ -581,40 +618,49 @@ func TestClone(t *testing.T) {
581618
continue
582619
}
583620

584-
// testing/quick can't handle functions or interfaces.
585-
fn := typ.Field(i).Name
586-
switch fn {
621+
// testing/quick can't handle functions or interfaces and so
622+
// isn't used here.
623+
switch fn := typ.Field(i).Name; fn {
587624
case "Rand":
588625
f.Set(reflect.ValueOf(io.Reader(os.Stdin)))
589-
continue
590626
case "Time", "GetCertificate", "GetConfigForClient", "VerifyPeerCertificate", "GetClientCertificate":
591-
// DeepEqual can't compare functions.
592-
continue
627+
// DeepEqual can't compare functions. If you add a
628+
// function field to this list, you must also change
629+
// TestCloneFuncFields to ensure that the func field is
630+
// cloned.
593631
case "Certificates":
594632
f.Set(reflect.ValueOf([]Certificate{
595633
{Certificate: [][]byte{{'b'}}},
596634
}))
597-
continue
598635
case "NameToCertificate":
599636
f.Set(reflect.ValueOf(map[string]*Certificate{"a": nil}))
600-
continue
601637
case "RootCAs", "ClientCAs":
602638
f.Set(reflect.ValueOf(x509.NewCertPool()))
603-
continue
604639
case "ClientSessionCache":
605640
f.Set(reflect.ValueOf(NewLRUClientSessionCache(10)))
606-
continue
607641
case "KeyLogWriter":
608642
f.Set(reflect.ValueOf(io.Writer(os.Stdout)))
609-
continue
610-
611-
}
612-
613-
q, ok := quick.Value(f.Type(), rnd)
614-
if !ok {
615-
t.Fatalf("quick.Value failed on field %s", fn)
643+
case "NextProtos":
644+
f.Set(reflect.ValueOf([]string{"a", "b"}))
645+
case "ServerName":
646+
f.Set(reflect.ValueOf("b"))
647+
case "ClientAuth":
648+
f.Set(reflect.ValueOf(VerifyClientCertIfGiven))
649+
case "InsecureSkipVerify", "SessionTicketsDisabled", "DynamicRecordSizingDisabled", "PreferServerCipherSuites":
650+
f.Set(reflect.ValueOf(true))
651+
case "MinVersion", "MaxVersion":
652+
f.Set(reflect.ValueOf(uint16(VersionTLS12)))
653+
case "SessionTicketKey":
654+
f.Set(reflect.ValueOf([32]byte{}))
655+
case "CipherSuites":
656+
f.Set(reflect.ValueOf([]uint16{1, 2}))
657+
case "CurvePreferences":
658+
f.Set(reflect.ValueOf([]CurveID{CurveP256}))
659+
case "Renegotiation":
660+
f.Set(reflect.ValueOf(RenegotiateOnceAsClient))
661+
default:
662+
t.Errorf("all fields must be accounted for, but saw unknown field %q", fn)
616663
}
617-
f.Set(q)
618664
}
619665

620666
c2 := c1.Clone()

0 commit comments

Comments
 (0)