From 640067f28a95bbc32aa701ceea204e28e510b04f Mon Sep 17 00:00:00 2001 From: Chance Zibolski Date: Fri, 7 Jun 2024 15:24:08 +0000 Subject: [PATCH] net/http: check GetConfigForClient in server.ServeTLS Just like for tls.Config.GetCertificate the http.Server.ServeTLS method should be checking tls.Config.GetConfigForClient before trying top open the specified certFile/keyFile. This was previously fixed for crypto/tls when using tls.Listen in CL205059, but the same change for net/http was missed. I've added a comment src/crypto/tls/tls.go in the relevant section in the hope that any future changes of a similar nature consider will consider updating net/http as needed as well. Change-Id: I312303bc497d92aa2f4627fe2620c70779cbcc99 GitHub-Last-Rev: 6ed29a900816a13690a9f3e26476d9bc1055a6f7 GitHub-Pull-Request: golang/go#66795 Reviewed-on: https://go-review.googlesource.com/c/go/+/578396 Reviewed-by: Filippo Valsorda Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Cherry Mui --- src/crypto/tls/tls.go | 1 + src/net/http/serve_test.go | 18 ++++++++++++++++++ src/net/http/server.go | 5 +++-- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/crypto/tls/tls.go b/src/crypto/tls/tls.go index b30f0b8fe4..f3089f0ed6 100644 --- a/src/crypto/tls/tls.go +++ b/src/crypto/tls/tls.go @@ -87,6 +87,7 @@ func NewListener(inner net.Listener, config *Config) net.Listener { // The configuration config must be non-nil and must include // at least one certificate or else set GetCertificate. func Listen(network, laddr string, config *Config) (net.Listener, error) { + // If this condition changes, consider updating http.Server.ServeTLS too. if config == nil || len(config.Certificates) == 0 && config.GetCertificate == nil && config.GetConfigForClient == nil { return nil, errors.New("tls: neither Certificates, GetCertificate, nor GetConfigForClient set in Config") diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 34b7d57f40..06bf5089d8 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -1748,6 +1748,24 @@ func TestAutomaticHTTP2_ListenAndServe_GetCertificate(t *testing.T) { }) } +func TestAutomaticHTTP2_ListenAndServe_GetConfigForClient(t *testing.T) { + cert, err := tls.X509KeyPair(testcert.LocalhostCert, testcert.LocalhostKey) + if err != nil { + t.Fatal(err) + } + conf := &tls.Config{ + // GetConfigForClient requires specifying a full tls.Config so we must set + // NextProtos ourselves. + NextProtos: []string{"h2"}, + Certificates: []tls.Certificate{cert}, + } + testAutomaticHTTP2_ListenAndServe(t, &tls.Config{ + GetConfigForClient: func(clientHello *tls.ClientHelloInfo) (*tls.Config, error) { + return conf, nil + }, + }) +} + func testAutomaticHTTP2_ListenAndServe(t *testing.T, tlsConf *tls.Config) { CondSkipHTTP2(t) // Not parallel: uses global test hooks. diff --git a/src/net/http/server.go b/src/net/http/server.go index b9a6edd7ad..190f565013 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -3370,7 +3370,8 @@ func (srv *Server) Serve(l net.Listener) error { // // Files containing a certificate and matching private key for the // server must be provided if neither the [Server]'s -// TLSConfig.Certificates nor TLSConfig.GetCertificate are populated. +// TLSConfig.Certificates, TLSConfig.GetCertificate nor +// config.GetConfigForClient are populated. // If the certificate is signed by a certificate authority, the // certFile should be the concatenation of the server's certificate, // any intermediates, and the CA's certificate. @@ -3389,7 +3390,7 @@ func (srv *Server) ServeTLS(l net.Listener, certFile, keyFile string) error { config.NextProtos = append(config.NextProtos, "http/1.1") } - configHasCert := len(config.Certificates) > 0 || config.GetCertificate != nil + configHasCert := len(config.Certificates) > 0 || config.GetCertificate != nil || config.GetConfigForClient != nil if !configHasCert || certFile != "" || keyFile != "" { var err error config.Certificates = make([]tls.Certificate, 1) -- 2.48.1