New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/http2: bogus greeting when providing TLSClientConfig #21336

Open
jbowens opened this Issue Aug 8, 2017 · 9 comments

Comments

Projects
None yet
6 participants
@jbowens
Copy link

jbowens commented Aug 8, 2017

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

go version go1.9beta2 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jackson/"
GORACE=""
GOROOT="/Users/jackson/sdk/go1.9beta2"
GOTOOLDIR="/Users/jackson/sdk/go1.9beta2/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/df/tll2ynl125z01jxszx2fq2nw0000gn/T/go-build293310413=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config

What did you do?

Download https://gist.github.com/jbowens/3e1f7443d6cb1bf8071f4aea2397c4b1

go1.9beta2 run main.go

What did you expect to see?

The program should exit without panicking.

What did you see instead?

2017/08/07 18:17:01 http2: server: error reading preface from client [::1]:59163: bogus greeting "POST / HTTP/1.1\r\nHost: l"
panic: Post https://localhost:8080/: net/http: HTTP/1.x transport connection broken: malformed HTTP response "\x00\x00\x18\x04\x00\x00\x00\x00\x00\x00\x05\x00\x10\x00\x00\x00\x03\x00\x00\x00\xfa\x00\x06\x00\x10\x01@\x00\x04\x00\x10\x00\x00"

From my understanding, the presence of TLSClientConfig should cause the client to not use http2, but it appears to negotiate to http2 anyways.

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Aug 8, 2017

@jbowens, to avoid using http2, the documentation at https://golang.org/pkg/net/http says
screen shot 2017-08-07 at 7 32 07 pm
set Transport.TLSNextProto, so in your gist please set

config.NextProtos = []string{"h1"}

and that should use HTTP/1. Does this change your bug report?

@jbowens

This comment has been minimized.

Copy link

jbowens commented Aug 8, 2017

@odeke-em thanks, but I don't think it does. I can avoid this error by explicitly configuring the protocols (either including http2 or not). I was more reporting the fact that this configuration causes the client to negotiate http/2 but then request using http1.

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Aug 8, 2017

Aye aye, thanks for letting me know. I'll call in the big guns /cc @bradfitz @tombergan.

@tombergan

This comment has been minimized.

Copy link
Contributor

tombergan commented Aug 8, 2017

The behavior is definitely inconsistent. http.Transport says:

https://golang.org/pkg/net/http/#Transport

// TLSClientConfig specifies the TLS configuration to use with
// tls.Client.
// If nil, the default configuration is used.
// If non-nil, HTTP/2 support may not be enabled by default.
TLSClientConfig *tls.Config

Note "may not be enabled". As you point out, the code does not enable HTTP/2 when TLCClientConfig is specified.

http.Server makes no guarantees one way or the other and will happily use HTTP/2 if TLSConfig is specified, as long as the config doesn't include a forbidden cipher.
https://golang.org/pkg/net/http/#Server

I'm guessing the H2 server was written that way to allow callers to specify the cert, while it's less common for H2 clients to provide a cert. I think the Transport should behave like the server, to allow clients to provide certs where needed.

@tombergan tombergan self-assigned this Aug 8, 2017

@tombergan tombergan added this to the Go1.10 milestone Aug 8, 2017

@janoberst

This comment has been minimized.

Copy link

janoberst commented Aug 29, 2017

To add a little bit of context here: this is caused by the same tls.Config being used to create both the server and the client.

When the server starts serving, it correctly detects that it can indeed serve HTTP2. Because it has detected that it can serve HTTP2, it appends "h2" to the NextProtos list in its TLSConfig (https://github.com/golang/go/blob/master/src/net/http/h2_bundle.go#L3949).

haveNPN := false
for _, p := range s.TLSConfig.NextProtos {
	if p == http2NextProtoTLS {
		haveNPN = true
		break
	}
}
if !haveNPN {
	s.TLSConfig.NextProtos = append(s.TLSConfig.NextProtos, http2NextProtoTLS)
}

Note that it first clones the TLSConfig, so I believe the intent is that multiple servers and clients can be initialized using the same TLSConfig. But it only does a shallow clone (see https://github.com/golang/go/blob/master/src/crypto/tls/common.go#L545). The shallow clone means that the NextProtos list is shared between multiple TLSConfig objects.

Therefore, when the server decides that it can serve h2, it also adds h2 to the client's TLSConfig. The TLS negotiation therefore negotiates an HTTP2 connection with the server. However, the Transport decides to not use HTTP2, because it sees that a TLSConfig has been set (https://github.com/golang/go/blob/master/src/net/http/transport.go#L227).

The two protocol levels in the client disagree here -- TLS has been negotiated with HTTP2 being the next protocol, but the Transport decides that it can't use HTTP2.

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Jan 9, 2018

Happy New Year @tombergan! How's it going here?

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Jan 10, 2018

There's also a data race, since the goroutine running ListenAndServeTLS is also initializing the * tls.Config.NextProtos concurrently with the main goroutine's http Client reading it.

You can observe that by adding the lines:

        time.Sleep(500 * time.Millisecond)                                                                                                                                                                          
        log.Printf("TLS config NextProto = %q", config.NextProtos)

... to the main.go code above, adding it after the goroutine, before the client initialization. Comment-out the 500ms sleep and you'll need NextProtos sometimes be empty. Running with -race shows:

==================
WARNING: DATA RACE
Write at 0x00c4200844e0 by goroutine 6:
  net/http.http2ConfigureServer()
      /home/bradfitz/go/src/net/http/h2_bundle.go:3953 +0x580
  net/http.(*Server).onceSetNextProtoDefaults()
      /home/bradfitz/go/src/net/http/server.go:3062 +0x13e
  net/http.(*Server).(net/http.onceSetNextProtoDefaults)-fm()
      /home/bradfitz/go/src/net/http/server.go:3026 +0x41
  sync.(*Once).Do()
      /home/bradfitz/go/src/sync/once.go:44 +0xe1  
  net/http.(*Server).setupHTTP2_ServeTLS()
      /home/bradfitz/go/src/net/http/server.go:3026 +0x81
  net/http.(*Server).ServeTLS()
      /home/bradfitz/go/src/net/http/server.go:2806 +0x53
  net/http.(*Server).ListenAndServeTLS()
      /home/bradfitz/go/src/net/http/server.go:3019 +0x139
  main.main.func2()
      /home/bradfitz/x.go:50 +0x49

Previous read at 0x00c4200844e0 by main goroutine:
  runtime.convT2Eslice()
      /home/bradfitz/go/src/runtime/iface.go:365 +0x0
  main.main()
      /home/bradfitz/x.go:57 +0x699

Goroutine 6 (running) created at:
  main.main()
      /home/bradfitz/x.go:49 +0x66d
==================

To fix this all, I think we'd need to stop mutating the caller's provided *Server, at least via its public fields. We might need to add an addition private field (likely guarded by the existing private mutex) and have the http2.ConfigureServer set that instead, to a clone of the exported config, if any. But since that's in a different package, we'd need an exported setter method? (Gross.) Or we'd need to do some trickery that only works in the bundled x/net/http2-in-std version, which is probably what the answer will be.

Then whenever the net/http Server needs its config, it would get it via a private accessor method that prefers the unexported TLS config field over the exported one.

Unrelated, we could also be smarter in the HTTP/1 Transport to check its TLS connection's state and if it had negotiated ALPN "h2", either fail early with something useful ("you configured it wrong.") or go out of its way to actually speak HTTP/2, if that isn't too much work.

In any case, there's too much to do here for Go 1.10, which is due out soon. Sorry.

At least there are plenty of workarounds.

@bradfitz bradfitz added the NeedsFix label Jan 10, 2018

@bradfitz bradfitz self-assigned this Jan 10, 2018

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Jan 10, 2018

@bradfitz bradfitz modified the milestones: Go1.11, Unplanned May 23, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented May 23, 2018

/cc @FiloSottile

@asim

This comment has been minimized.

Copy link

asim commented Nov 29, 2018

Is there a temporary fix for this on the client side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment