From 1bb4a269654c236611c91d0ba1e28e3f07c93ad5 Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Wed, 1 Mar 2017 18:33:05 +0100 Subject: [PATCH 1/3] Fix reading of tls.Config certificates This commit mostly copies the getCertificate function from crypto/tls to align our certificate reading with the standard library. Should fix #458. --- crypto/cert_chain.go | 58 +++++++++++++++++++++++++-------------- crypto/cert_chain_test.go | 2 ++ 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/crypto/cert_chain.go b/crypto/cert_chain.go index 0fd905a77..69c00d5f6 100644 --- a/crypto/cert_chain.go +++ b/crypto/cert_chain.go @@ -55,30 +55,46 @@ func (c *certChain) GetLeafCert(sni string) ([]byte, error) { return cert.Certificate[0], nil } -func (c *certChain) getCertForSNI(sni string) (*tls.Certificate, error) { - if c.config.GetCertificate != nil { - cert, err := c.config.GetCertificate(&tls.ClientHelloInfo{ServerName: sni}) - if err != nil { - return nil, err +func (cc *certChain) getCertForSNI(sni string) (*tls.Certificate, error) { + c := cc.config + // The rest of this function is mostly copied from crypto/tls.getCertificate + + if c.GetCertificate != nil { + cert, err := c.GetCertificate(&tls.ClientHelloInfo{ServerName: sni}) + if cert != nil || err != nil { + return cert, err } - if cert != nil { + } + + if len(c.Certificates) == 0 { + return nil, errNoMatchingCertificate + } + + if len(c.Certificates) == 1 || c.NameToCertificate == nil { + // There's only one choice, so no point doing any work. + return &c.Certificates[0], nil + } + + name := strings.ToLower(sni) + for len(name) > 0 && name[len(name)-1] == '.' { + name = name[:len(name)-1] + } + + if cert, ok := c.NameToCertificate[name]; ok { + return cert, nil + } + + // try replacing labels in the name with wildcards until we get a + // match. + labels := strings.Split(name, ".") + for i := range labels { + labels[i] = "*" + candidate := strings.Join(labels, ".") + if cert, ok := c.NameToCertificate[candidate]; ok { return cert, nil } } - if len(c.config.NameToCertificate) != 0 { - if cert, ok := c.config.NameToCertificate[sni]; ok { - return cert, nil - } - wildcardSNI := "*" + strings.TrimLeftFunc(sni, func(r rune) bool { return r != '.' }) - if cert, ok := c.config.NameToCertificate[wildcardSNI]; ok { - return cert, nil - } - } - - if len(c.config.Certificates) != 0 { - return &c.config.Certificates[0], nil - } - - return nil, errNoMatchingCertificate + // If nothing matches, return the first certificate. + return &c.Certificates[0], nil } diff --git a/crypto/cert_chain_test.go b/crypto/cert_chain_test.go index 6ad5ada8d..8216d8d36 100644 --- a/crypto/cert_chain_test.go +++ b/crypto/cert_chain_test.go @@ -84,6 +84,7 @@ var _ = Describe("Proof", func() { }) It("uses NameToCertificate entries", func() { + config.Certificates = []tls.Certificate{cert, cert} // two entries so the long path is used config.NameToCertificate = map[string]*tls.Certificate{ "quic.clemente.io": &cert, } @@ -94,6 +95,7 @@ var _ = Describe("Proof", func() { }) It("uses NameToCertificate entries with wildcard", func() { + config.Certificates = []tls.Certificate{cert, cert} // two entries so the long path is used config.NameToCertificate = map[string]*tls.Certificate{ "*.clemente.io": &cert, } From 219ce60a5e1eb8629a4388b1496bc392eeee1df9 Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Wed, 1 Mar 2017 21:11:52 +0100 Subject: [PATCH 2/3] Call GetConfigForClient in tls.Config if present --- crypto/cert_chain.go | 9 +++++++++ crypto/cert_chain_test.go | 11 +++++++++++ 2 files changed, 20 insertions(+) diff --git a/crypto/cert_chain.go b/crypto/cert_chain.go index 69c00d5f6..db7b5c350 100644 --- a/crypto/cert_chain.go +++ b/crypto/cert_chain.go @@ -57,6 +57,15 @@ func (c *certChain) GetLeafCert(sni string) ([]byte, error) { func (cc *certChain) getCertForSNI(sni string) (*tls.Certificate, error) { c := cc.config + if c.GetConfigForClient != nil { + var err error + c, err = c.GetConfigForClient(&tls.ClientHelloInfo{ + ServerName: sni, + }) + if err != nil { + return nil, err + } + } // The rest of this function is mostly copied from crypto/tls.getCertificate if c.GetCertificate != nil { diff --git a/crypto/cert_chain_test.go b/crypto/cert_chain_test.go index 8216d8d36..d78f7982d 100644 --- a/crypto/cert_chain_test.go +++ b/crypto/cert_chain_test.go @@ -127,5 +127,16 @@ var _ = Describe("Proof", func() { _, err := cc.GetLeafCert("invalid domain") Expect(err).To(MatchError(errNoMatchingCertificate)) }) + + It("respects GetConfigForClient", func() { + nestedConfig := &tls.Config{Certificates: []tls.Certificate{cert}} + config.GetConfigForClient = func(chi *tls.ClientHelloInfo) (*tls.Config, error) { + Expect(chi.ServerName).To(Equal("quic.clemente.io")) + return nestedConfig, nil + } + resultCert, err := cc.getCertForSNI("quic.clemente.io") + Expect(err).NotTo(HaveOccurred()) + Expect(*resultCert).To(Equal(cert)) + }) }) }) From 723f86c725c75463724d54f3db46f72a50f16ea1 Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Thu, 2 Mar 2017 10:40:20 +0100 Subject: [PATCH 3/3] Don't use GetConfigForClient on go < 1.8 --- crypto/cert_chain.go | 11 +++-------- crypto/cert_chain_test.go | 8 +++++++- crypto/config_for_client_1.8.go | 14 ++++++++++++++ crypto/config_for_client_pre1.8.go | 9 +++++++++ 4 files changed, 33 insertions(+), 9 deletions(-) create mode 100644 crypto/config_for_client_1.8.go create mode 100644 crypto/config_for_client_pre1.8.go diff --git a/crypto/cert_chain.go b/crypto/cert_chain.go index db7b5c350..96d0ecd01 100644 --- a/crypto/cert_chain.go +++ b/crypto/cert_chain.go @@ -57,14 +57,9 @@ func (c *certChain) GetLeafCert(sni string) ([]byte, error) { func (cc *certChain) getCertForSNI(sni string) (*tls.Certificate, error) { c := cc.config - if c.GetConfigForClient != nil { - var err error - c, err = c.GetConfigForClient(&tls.ClientHelloInfo{ - ServerName: sni, - }) - if err != nil { - return nil, err - } + c, err := maybeGetConfigForClient(c, sni) + if err != nil { + return nil, err } // The rest of this function is mostly copied from crypto/tls.getCertificate diff --git a/crypto/cert_chain_test.go b/crypto/cert_chain_test.go index d78f7982d..a4ee57a3e 100644 --- a/crypto/cert_chain_test.go +++ b/crypto/cert_chain_test.go @@ -5,6 +5,7 @@ import ( "compress/flate" "compress/zlib" "crypto/tls" + "reflect" "github.com/lucas-clemente/quic-go/testdata" @@ -129,11 +130,16 @@ var _ = Describe("Proof", func() { }) It("respects GetConfigForClient", func() { + if !reflect.ValueOf(tls.Config{}).FieldByName("GetConfigForClient").IsValid() { + // Pre 1.8, we don't have to do anything + return + } nestedConfig := &tls.Config{Certificates: []tls.Certificate{cert}} - config.GetConfigForClient = func(chi *tls.ClientHelloInfo) (*tls.Config, error) { + l := func(chi *tls.ClientHelloInfo) (*tls.Config, error) { Expect(chi.ServerName).To(Equal("quic.clemente.io")) return nestedConfig, nil } + reflect.ValueOf(config).Elem().FieldByName("GetConfigForClient").Set(reflect.ValueOf(l)) resultCert, err := cc.getCertForSNI("quic.clemente.io") Expect(err).NotTo(HaveOccurred()) Expect(*resultCert).To(Equal(cert)) diff --git a/crypto/config_for_client_1.8.go b/crypto/config_for_client_1.8.go new file mode 100644 index 000000000..452fe0e08 --- /dev/null +++ b/crypto/config_for_client_1.8.go @@ -0,0 +1,14 @@ +// +build go1.8 + +package crypto + +import "crypto/tls" + +func maybeGetConfigForClient(c *tls.Config, sni string) (*tls.Config, error) { + if c.GetConfigForClient == nil { + return c, nil + } + return c.GetConfigForClient(&tls.ClientHelloInfo{ + ServerName: sni, + }) +} diff --git a/crypto/config_for_client_pre1.8.go b/crypto/config_for_client_pre1.8.go new file mode 100644 index 000000000..612b94a11 --- /dev/null +++ b/crypto/config_for_client_pre1.8.go @@ -0,0 +1,9 @@ +// +build !go1.8 + +package crypto + +import "crypto/tls" + +func maybeGetConfigForClient(c *tls.Config, sni string) (*tls.Config, error) { + return c, nil +}