crypto/tls: don't modify Config.Certificates in BuildNameToCertificate

The Config does not own the memory pointed to by the Certificate slice.
Instead, opportunistically use Certificate.Leaf and let the application
set it if it desires the performance gain.

This is a partial rollback of CL 107627. See the linked issue for the
full explanation.

Fixes #28744

Change-Id: I33ce9e6712e3f87939d9d0932a06d24e48ba4567
Reviewed-on: https://go-review.googlesource.com/c/149098
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 25e4a7d8860d88a15ccdd7de3b7ff20b74e0e393..3ba3aac86bb097444d1b6e9864293dc91905c0a8 100644 (file)
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -871,14 +871,14 @@ func (c *Config) BuildNameToCertificate() {
        c.NameToCertificate = make(map[string]*Certificate)
        for i := range c.Certificates {
                cert := &c.Certificates[i]
-               if cert.Leaf == nil {
-                       x509Cert, err := x509.ParseCertificate(cert.Certificate[0])
+               x509Cert := cert.Leaf
+               if x509Cert == nil {
+                       var err error
+                       x509Cert, err = x509.ParseCertificate(cert.Certificate[0])
                        if err != nil {
                                continue
                        }
-                       cert.Leaf = x509Cert
                }
-               x509Cert := cert.Leaf
                if len(x509Cert.Subject.CommonName) > 0 {
                        c.NameToCertificate[x509Cert.Subject.CommonName] = cert
                }

diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go
index e23068ce4357327e433878bccc45cce99a00422a..00bb6e4ef3754c176dfde3efdddd3341fdafa099 100644 (file)
--- a/src/crypto/tls/tls_test.go
+++ b/src/crypto/tls/tls_test.go
@@ -1087,3 +1087,25 @@ func TestEscapeRoute(t *testing.T) {
                t.Errorf("Client negotiated version %x, expected %x", cs.Version, VersionTLS12)
        }
 }
+
+// Issue 28744: Ensure that we don't modify memory
+// that Config doesn't own such as Certificates.
+func TestBuildNameToCertificate_doesntModifyCertificates(t *testing.T) {
+       c0 := Certificate{
+               Certificate: [][]byte{testRSACertificate},
+               PrivateKey:  testRSAPrivateKey,
+       }
+       c1 := Certificate{
+               Certificate: [][]byte{testSNICertificate},
+               PrivateKey:  testRSAPrivateKey,
+       }
+       config := testConfig.Clone()
+       config.Certificates = []Certificate{c0, c1}
+
+       config.BuildNameToCertificate()
+       got := config.Certificates
+       want := []Certificate{c0, c1}
+       if !reflect.DeepEqual(got, want) {
+               t.Fatalf("Certificates were mutated by BuildNameToCertificate\nGot: %#v\nWant: %#v\n", got, want)
+       }
+}