net/http: conditionally configure HTTP/2 in Server.Serve(Listener)

Don't configure HTTP/2 in http.Server.Serve(net.Listener) if the
Server's TLSConfig is set and doesn't include the "h2" NextProto
value. This avoids mutating a *tls.Config already in use if
previously passed to tls.NewListener.

Also document this. (it's come up a few times now)

Fixes #15908

Change-Id: I283eed82fdb29a791f80d801aadd9f75db244de0
Reviewed-on: https://go-review.googlesource.com/24508
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 62b558c2cf90b3dd0820f3e4173eb45e95f4b061..139ce3eafc7e1ff4a5de2d3ea35afe296a9dd015 100644 (file)
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -1107,11 +1107,44 @@ func TestTLSServer(t *testing.T) {
        })
 }
 
-func TestAutomaticHTTP2_Serve(t *testing.T) {
+// Issue 15908
+func TestAutomaticHTTP2_Serve_NoTLSConfig(t *testing.T) {
+       testAutomaticHTTP2_Serve(t, nil, true)
+}
+
+func TestAutomaticHTTP2_Serve_NonH2TLSConfig(t *testing.T) {
+       testAutomaticHTTP2_Serve(t, &tls.Config{}, false)
+}
+
+func TestAutomaticHTTP2_Serve_H2TLSConfig(t *testing.T) {
+       testAutomaticHTTP2_Serve(t, &tls.Config{NextProtos: []string{"h2"}}, true)
+}
+
+func testAutomaticHTTP2_Serve(t *testing.T, tlsConf *tls.Config, wantH2 bool) {
        defer afterTest(t)
        ln := newLocalListener(t)
        ln.Close() // immediately (not a defer!)
        var s Server
+       s.TLSConfig = tlsConf
+       if err := s.Serve(ln); err == nil {
+               t.Fatal("expected an error")
+       }
+       gotH2 := s.TLSNextProto["h2"] != nil
+       if gotH2 != wantH2 {
+               t.Errorf("http2 configured = %v; want %v", gotH2, wantH2)
+       }
+}
+
+func TestAutomaticHTTP2_Serve_WithTLSConfig(t *testing.T) {
+       defer afterTest(t)
+       ln := newLocalListener(t)
+       ln.Close() // immediately (not a defer!)
+       var s Server
+       // Set the TLSConfig. In reality, this would be the
+       // *tls.Config given to tls.NewListener.
+       s.TLSConfig = &tls.Config{
+               NextProtos: []string{"h2"},
+       }
        if err := s.Serve(ln); err == nil {
                t.Fatal("expected an error")
        }

diff --git a/src/net/http/server.go b/src/net/http/server.go
index a1c48272fd6b7f22b88142f398c5a0d3678aebf1..7c3237c4cd4e6404403de76c938797ca5d53bc82 100644 (file)
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2222,9 +2222,37 @@ func (srv *Server) ListenAndServe() error {
 
 var testHookServerServe func(*Server, net.Listener) // used if non-nil
 
+// shouldDoServeHTTP2 reports whether Server.Serve should configure
+// automatic HTTP/2. (which sets up the srv.TLSNextProto map)
+func (srv *Server) shouldConfigureHTTP2ForServe() bool {
+       if srv.TLSConfig == nil {
+               // Compatibility with Go 1.6:
+               // If there's no TLSConfig, it's possible that the user just
+               // didn't set it on the http.Server, but did pass it to
+               // tls.NewListener and passed that listener to Serve.
+               // So we should configure HTTP/2 (to set up srv.TLSNextProto)
+               // in case the listener returns an "h2" *tls.Conn.
+               return true
+       }
+       // The user specified a TLSConfig on their http.Server.
+       // In this, case, only configure HTTP/2 if their tls.Config
+       // explicitly mentions "h2". Otherwise http2.ConfigureServer
+       // would modify the tls.Config to add it, but they probably already
+       // passed this tls.Config to tls.NewListener. And if they did,
+       // it's too late anyway to fix it. It would only be potentially racy.
+       // See Issue 15908.
+       return strSliceContains(srv.TLSConfig.NextProtos, http2NextProtoTLS)
+}
+
 // Serve accepts incoming connections on the Listener l, creating a
 // new service goroutine for each. The service goroutines read requests and
 // then call srv.Handler to reply to them.
+//
+// For HTTP/2 support, srv.TLSConfig should be initialized to the
+// provided listener's TLS Config before calling Serve. If
+// srv.TLSConfig is non-nil and doesn't include the string "h2" in
+// Config.NextProtos, HTTP/2 support is not enabled.
+//
 // Serve always returns a non-nil error.
 func (srv *Server) Serve(l net.Listener) error {
        defer l.Close()
@@ -2232,9 +2260,13 @@ func (srv *Server) Serve(l net.Listener) error {
                fn(srv, l)
        }
        var tempDelay time.Duration // how long to sleep on accept failure
-       if err := srv.setupHTTP2(); err != nil {
-               return err
+
+       if srv.shouldConfigureHTTP2ForServe() {
+               if err := srv.setupHTTP2(); err != nil {
+                       return err
+               }
        }
+
        // TODO: allow changing base context? can't imagine concrete
        // use cases yet.
        baseCtx := context.Background()