diff options
author | Brad Fitzpatrick <bradfitz@golang.org> | 2016-06-27 16:39:40 -0700 |
---|---|---|
committer | Brad Fitzpatrick <bradfitz@golang.org> | 2016-06-28 18:14:56 +0000 |
commit | b5f0aff49503e31002b33198e06708e263c445a7 (patch) | |
tree | 3e1c33f5ba496a71698ec6d242c97a6e48ac43a6 | |
parent | 996ed3be9a10ace6cd7a8a6a8080c0c8db7ab1fe (diff) | |
download | go-b5f0aff49503e31002b33198e06708e263c445a7.tar.gz go-b5f0aff49503e31002b33198e06708e263c445a7.zip |
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>
-rw-r--r-- | src/net/http/serve_test.go | 35 | ||||
-rw-r--r-- | src/net/http/server.go | 36 |
2 files changed, 68 insertions, 3 deletions
diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 62b558c2cf..139ce3eafc 100644 --- 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 a1c48272fd..7c3237c4cd 100644 --- 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() |