From b5f0aff49503e31002b33198e06708e263c445a7 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 27 Jun 2016 16:39:40 -0700 Subject: [PATCH] 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 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/net/http/serve_test.go | 35 ++++++++++++++++++++++++++++++++++- 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() -- 2.50.0