net/http/httptest: NewTLSServer doesn't support HTTP/2 requests #34939

rhysh · 2019-10-16T20:29:48Z

What version of Go are you using (`go version`)?

$ go1.13 version
go version go1.13.1 darwin/amd64
$ go-tip version
go version devel +f6c624a22a Wed Oct 16 15:58:33 2019 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes, go1.13.1 and tip are affected.

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go1.13 env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/rhys/Library/Caches/go-build"
GOENV="/Users/rhys/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY="*"
GONOSUMDB="*"
GOOS="darwin"
GOPATH="/Users/rhys/go"
GOPRIVATE="*"
GOPROXY="direct"
GOROOT="/Users/rhys/go/version/go1.13"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/Users/rhys/go/version/go1.13/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/49/zmds5zsn75z1283vtzxyfr5hj7yjq4/T/go-build721053994=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I called httptest.NewTLSServer and used the attached http.Client to make requests to it.

What did you expect to see?

I expected the requests to the local test server to use HTTP/2. (This is important for me at the moment because I'm writing reproducers for bugs I've seen in HTTP/2 client requests.)

What did you see instead?

The requests are sent over HTTP/1.1.

It looks like this is because the httptest package's TLS setup sets a tls.Config on the http.Transport before its first use, which defeats net/http's usual HTTP/2 autoconfiguration.

$  go1.13 test -v ./repro_test.go
=== RUN   TestHTTPTestHTTP2
=== RUN   TestHTTPTestHTTP2/basic
=== RUN   TestHTTPTestHTTP2/advertise-h2
=== RUN   TestHTTPTestHTTP2/transport-hacks
2019/10/16 13:23:20 http: TLS handshake error from 127.0.0.1:64474: remote error: tls: bad certificate
--- FAIL: TestHTTPTestHTTP2 (0.14s)
    --- FAIL: TestHTTPTestHTTP2/basic (0.00s)
        repro_test.go:17: Request is not http/2: "HTTP/1.1"
    --- FAIL: TestHTTPTestHTTP2/advertise-h2 (0.00s)
        repro_test.go:37: Request is not http/2: "HTTP/1.1"
    --- PASS: TestHTTPTestHTTP2/transport-hacks (0.14s)
FAIL
FAIL    command-line-arguments  0.193s
FAIL

$ go-tip test -v ./repro_test.go
=== RUN   TestHTTPTestHTTP2
=== RUN   TestHTTPTestHTTP2/basic
=== RUN   TestHTTPTestHTTP2/advertise-h2
=== RUN   TestHTTPTestHTTP2/transport-hacks
2019/10/16 13:23:55 http: TLS handshake error from 127.0.0.1:64490: remote error: tls: bad certificate
--- FAIL: TestHTTPTestHTTP2 (0.14s)
    --- FAIL: TestHTTPTestHTTP2/basic (0.00s)
        repro_test.go:17: Request is not http/2: "HTTP/1.1"
    --- FAIL: TestHTTPTestHTTP2/advertise-h2 (0.00s)
        repro_test.go:37: Request is not http/2: "HTTP/1.1"
    --- PASS: TestHTTPTestHTTP2/transport-hacks (0.14s)
FAIL
FAIL    command-line-arguments  1.254s
FAIL

package repro

import (
	"crypto/tls"
	"net/http"
	"net/http/httptest"
	"sync/atomic"
	"testing"
)

func TestHTTPTestHTTP2(t *testing.T) {
	t.Run("basic", func(t *testing.T) {
		var calls int64
		s := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		defer s.Close()

		resp, err := s.Client().Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})

	t.Run("advertise-h2", func(t *testing.T) {
		var calls int64
		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		s.TLS = &tls.Config{
			NextProtos: []string{"h2"},
		}
		s.StartTLS()
		defer s.Close()

		resp, err := s.Client().Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})

	t.Run("transport-hacks", func(t *testing.T) {
		var calls int64
		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		s.TLS = &tls.Config{
			NextProtos: []string{"h2"},
		}
		s.StartTLS()
		defer s.Close()

		transport := s.Client().Transport.(*http.Transport)
		clientConfig := transport.TLSClientConfig
		transport.TLSClientConfig = nil

		// make a request to trigger HTTP/2 autoconfiguration
		resp, err := s.Client().Get(s.URL)
		if err == nil {
			t.Errorf(`Expected harmless "certificate signed by unknown authority" error`)
			resp.Body.Close()
		}
		// now allow the client to connect to the ad-hoc test server
		transport.TLSClientConfig.RootCAs = clientConfig.RootCAs

		resp, err = s.Client().Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})
}

The text was updated successfully, but these errors were encountered:

odeke-em · 2019-10-16T20:53:38Z

@rhysh thank you for filing this issue and great to catch you here!
Here is how you can enable HTTP/2 for httptest but also for other servers

--- before_test.go	2019-10-16 13:42:17.000000000 -0700
+++ fixed_test.go	2019-10-16 13:48:43.000000000 -0700
@@ -6,20 +6,34 @@
 	"net/http/httptest"
 	"sync/atomic"
 	"testing"
+
+	"golang.org/x/net/http2"
 )
 
 func TestHTTPTestHTTP2(t *testing.T) {
 	t.Run("basic", func(t *testing.T) {
 		var calls int64
-		s := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			atomic.AddInt64(&calls, 1)
 			if !r.ProtoAtLeast(2, 0) {
 				t.Errorf("Request is not http/2: %q", r.Proto)
 			}
 		}))
+		if err := http2.ConfigureServer(s.Config, new(http2.Server)); err != nil {
+			t.Fatalf("Failed to configure HTTP/2 server: %v", err)
+		}
+		s.TLS = s.Config.TLSConfig
+		s.StartTLS()
 		defer s.Close()
 
-		resp, err := s.Client().Get(s.URL)
+		tr := &http.Transport{TLSClientConfig: s.Config.TLSConfig}
+		if err := http2.ConfigureTransport(tr); err != nil {
+			t.Fatalf("Failed to configure http2 transport: %v", err)
+		}
+		tr.TLSClientConfig.InsecureSkipVerify = true
+		client := &http.Client{Transport: tr}
+
+		resp, err := client.Get(s.URL)
 		if err != nil {
 			t.Fatalf("HTTP GET: %v", err)
 		}
@@ -37,13 +51,21 @@
 				t.Errorf("Request is not http/2: %q", r.Proto)
 			}
 		}))
-		s.TLS = &tls.Config{
-			NextProtos: []string{"h2"},
+		if err := http2.ConfigureServer(s.Config, new(http2.Server)); err != nil {
+			t.Fatalf("Failed to configure HTTP/2 server: %v", err)
 		}
+		s.TLS = s.Config.TLSConfig
 		s.StartTLS()
 		defer s.Close()
 
-		resp, err := s.Client().Get(s.URL)
+		tr := &http.Transport{TLSClientConfig: s.Config.TLSConfig}
+		if err := http2.ConfigureTransport(tr); err != nil {
+			t.Fatalf("Failed to configure http2 transport: %v", err)
+		}
+		tr.TLSClientConfig.InsecureSkipVerify = true
+		client := &http.Client{Transport: tr}
+
+		resp, err := client.Get(s.URL)
 		if err != nil {
 			t.Fatalf("HTTP GET: %v", err)
 		}

and I have encountered this is in the wild which is why I shared this gist https://gist.github.com/odeke-em/cacfe2ea5efc57c5a7a562aba1478378

to finally give you the file

package repro

import (
	"crypto/tls"
	"net/http"
	"net/http/httptest"
	"sync/atomic"
	"testing"

	"golang.org/x/net/http2"
)

func TestHTTPTestHTTP2(t *testing.T) {
	t.Run("basic", func(t *testing.T) {
		var calls int64
		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		if err := http2.ConfigureServer(s.Config, new(http2.Server)); err != nil {
			t.Fatalf("Failed to configure HTTP/2 server: %v", err)
		}
		s.TLS = s.Config.TLSConfig
		s.StartTLS()
		defer s.Close()

		tr := &http.Transport{TLSClientConfig: s.Config.TLSConfig}
		if err := http2.ConfigureTransport(tr); err != nil {
			t.Fatalf("Failed to configure http2 transport: %v", err)
		}
		tr.TLSClientConfig.InsecureSkipVerify = true
		client := &http.Client{Transport: tr}

		resp, err := client.Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})

	t.Run("advertise-h2", func(t *testing.T) {
		var calls int64
		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		if err := http2.ConfigureServer(s.Config, new(http2.Server)); err != nil {
			t.Fatalf("Failed to configure HTTP/2 server: %v", err)
		}
		s.TLS = s.Config.TLSConfig
		s.StartTLS()
		defer s.Close()

		tr := &http.Transport{TLSClientConfig: s.Config.TLSConfig}
		if err := http2.ConfigureTransport(tr); err != nil {
			t.Fatalf("Failed to configure http2 transport: %v", err)
		}
		tr.TLSClientConfig.InsecureSkipVerify = true
		client := &http.Client{Transport: tr}

		resp, err := client.Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})

	t.Run("transport-hacks", func(t *testing.T) {
		var calls int64
		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		s.TLS = &tls.Config{
			NextProtos: []string{"h2"},
		}
		s.StartTLS()
		defer s.Close()

		transport := s.Client().Transport.(*http.Transport)
		clientConfig := transport.TLSClientConfig
		transport.TLSClientConfig = nil

		// make a request to trigger HTTP/2 autoconfiguration
		resp, err := s.Client().Get(s.URL)
		if err == nil {
			t.Errorf(`Expected harmless "certificate signed by unknown authority" error`)
			resp.Body.Close()
		}
		// now allow the client to connect to the ad-hoc test server
		transport.TLSClientConfig.RootCAs = clientConfig.RootCAs

		resp, err = s.Client().Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})
}

but anyways the crux of it is that you'll need to:
a) Make an Unstarted server
b) Invoke http2.ConfigureServer
c) do s.TLS = s.Config.TLSConfig and then s.StartTLS
d) For the client: make an http.Transport using s.Config.TLSConfig and obviously insecureSkipVerify

What I think we can do though is for the net/http/httptest package is add a new helper for example NewTLSServerWithHTTP2 which under the hood encompasses all those steps I listed out in a) to d), @bradfitz what do you think?

odeke-em · 2019-10-16T20:57:30Z

Or perhaps an option for NewTLSServer that enables HTTP/2 instead of the entirely new function NewTLSServerWithHTTP2

bradfitz · 2019-10-16T21:10:36Z

Howa about a new EnableHTTP2 bool on https://golang.org/pkg/net/http/httptest/#Server that then does the setup in Server.Start? (With any necessary changes to the Client method to return a configured client)

And an example, if docs aren't sufficient.

So the flow would be NewUnstartedServer, set the bool to true, and Start.

odeke-em · 2019-10-16T21:32:05Z

Perfect, SGTM, thank you @bradfitz! CL coming up shortly with an example too and tests :)

rhysh · 2019-10-16T21:51:17Z

Thanks @odeke-em and @bradfitz . A new boolean field sounds like the right level of complexity for the user. Do we need even that?

Should http/2 be the default for httptest setups just as it is for production net/http clients and servers? When would a user want to opt out of http/2?

httptest has a mechanism for that opt-out already: setting the server's NextProtos to not include "h2". Do we need a new specialized field?

gopherbot · 2019-10-16T23:10:21Z

Change https://golang.org/cl/201557 mentions this issue: net/http/httptest: add EnableHTTP2 to Server

odeke-em · 2019-10-16T23:16:44Z

Should http/2 be the default for httptest setups just as it is for production net/http clients and servers? When would a user want to opt out of http/2?

net/http/httptest has been around for ages before HTTP/2 and the norm when testing is to use HTTP/1.1, and as you can see configuring HTTP/2 on these servers is a little tricky. Changing it to always use HTTP/2 might break lots of people's code who've been using this package for years. I think we can perhaps schedule a gradual roll out to invert the behavior, and like you mention, instead make HTTP/1.1 toggleable by the opt-out but after a while of having EnableHTTP2 around.

gopherbot · 2020-01-31T03:16:35Z

Change https://golang.org/cl/217131 mentions this issue: doc/go1.14: mention new field net/http/httptest/Server.EnableHTTP2

Updates #34939 Updates #36878 Change-Id: Ifa9a17b5b16bfcfbfe1d113a2b66a63ea3a6b59c Reviewed-on: https://go-review.googlesource.com/c/go/+/217131 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

odeke-em self-assigned this Oct 16, 2019

gopherbot closed this as completed in 5375c71 Oct 18, 2019

catalinaperalta mentioned this issue Oct 12, 2020

Add NewTLSHTTP2Server Azure/azure-sdk-for-go#12726

Merged

golang locked and limited conversation to collaborators Jan 30, 2021

gopherbot added the FrozenDueToAge label Jan 30, 2021

rsc unassigned odeke-em Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http/httptest: NewTLSServer doesn't support HTTP/2 requests #34939

net/http/httptest: NewTLSServer doesn't support HTTP/2 requests #34939

rhysh commented Oct 16, 2019

odeke-em commented Oct 16, 2019 •

edited

odeke-em commented Oct 16, 2019

bradfitz commented Oct 16, 2019

odeke-em commented Oct 16, 2019

rhysh commented Oct 16, 2019

gopherbot commented Oct 16, 2019

odeke-em commented Oct 16, 2019

gopherbot commented Jan 31, 2020

net/http/httptest: NewTLSServer doesn't support HTTP/2 requests #34939

net/http/httptest: NewTLSServer doesn't support HTTP/2 requests #34939

Comments

rhysh commented Oct 16, 2019

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

What did you do?

What did you expect to see?

What did you see instead?

odeke-em commented Oct 16, 2019 • edited

odeke-em commented Oct 16, 2019

bradfitz commented Oct 16, 2019

odeke-em commented Oct 16, 2019

rhysh commented Oct 16, 2019

gopherbot commented Oct 16, 2019

odeke-em commented Oct 16, 2019

gopherbot commented Jan 31, 2020

What version of Go are you using (`go version`)?

What operating system and processor architecture are you using (`go env`)?

odeke-em commented Oct 16, 2019 •

edited