Skip to content
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: call conn.Handshake before accessing ConnectionState #33304

Open
sebsebmc opened this issue Jul 26, 2019 · 5 comments
Open

x/net/http2: call conn.Handshake before accessing ConnectionState #33304

sebsebmc opened this issue Jul 26, 2019 · 5 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@sebsebmc
Copy link

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

$ go version
go1.12.7 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\schlopeki\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\schlopeki\go
set GOPROXY=
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\schlopeki\Projects\doh\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\SCHLOP~1\AppData\Local\Temp\go-build767447546=/tmp/go-build -gno-record-gcc-switches

What did you do?

I attempted to make a simple http2 only client/server but the server rejects all connections as having "INADEQUATE_SECURITY, TLS version too low". When inspecting the TLS version connection claims to have a TLS version of 0 but in Wireshark I see that a TLS 1.2 negotiation has already started.

If I manually perform the TLS handshake the http2.Server.ServeConn no longer hangs up the connection.

Here is the minimal server:

package main

import (
	"crypto/tls"
	"log"
	"net/http"

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

func main() {
	cert, err := tls.LoadX509KeyPair("server.crt", "server.key")
	if err != nil {
		panic("Cannot load cert")
	}
	tlsCfg := &tls.Config{
		Certificates: []tls.Certificate{cert},
		NextProtos:   []string{http2.NextProtoTLS},
	}

	srv := http2.Server{}
	srvOpt := &http2.ServeConnOpts{
		Handler:    http.HandlerFunc(handler),
		BaseConfig: &http.Server{TLSConfig: tlsCfg},
	}
	listener, err := tls.Listen("tcp", "127.0.0.1:8942", tlsCfg)
	if err != nil {
		panic("Cant listen")
	}

	for {
		conn, err := listener.Accept()
		if err != nil {
			panic("connection error")
		}
		// Uncommenting the next 2 lines make the server work
		// tlsConn := conn.(*tls.Conn)
		// tlsConn.Handshake()

		go func() { srv.ServeConn(conn, srvOpt) }()
	}
}

func handler(wrt http.ResponseWriter, req *http.Request) {
	if req.ProtoMajor != 2 {
		wrt.WriteHeader(500)
		flusher, _ := wrt.(http.Flusher)
		flusher.Flush()
		log.Printf("Not HTTP2")
		return
	}
	wrt.WriteHeader(200)
	flusher, _ := wrt.(http.Flusher)
	flusher.Flush()
}

Modern versions of Chrome/Firefox should be able to test this server. Testing server.crt and server.key need to be created and I used a host file to point test.local at the loopback address.

What did you expect to see?

The connection would have already completed the TLS handshake once listen.Accept returned it for use, or http2.Server.ServerConn would be able to see that the TLS version is at least negotiated.

What did you see instead?

When passing the connection directly from Accept to the http2.Server.ServeConn, the TLS state of the connection is not set. This results in ServeConn seeing a TLS version of 0 and therefore it sends GOAWAY and kills the connection.

@julieqiu julieqiu changed the title x/net/http2 crypto/tls Fails to complete TLS handshake before http2 server kills the connection crypto/tls: TLS handshake fails before http2 server kills the connection Jul 29, 2019
@julieqiu
Copy link
Member

/cc @FiloSottile

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 29, 2019
@sebsebmc
Copy link
Author

The TLS handshake doesn't fail as far as I can tell, it just isn't complete in the tls.Conn I get from Accept.

ServeConn looks at the TLS ConnectionState and checks the HandshakeComplete, which is false and then ServeConn has to drop the connection according to the spec because the TLS version doesn't appear to be TLS 1.2 or higher.

If it would help I can provide a packet capture as well.

@sebsebmc sebsebmc changed the title crypto/tls: TLS handshake fails before http2 server kills the connection crypto/tls: TLS handshake doesn't complete before http2 server kills the connection Aug 7, 2019
@MicOestergaard
Copy link

I have the same issue. It seems it is not related to http2, but has something to do with how tls connections are established. Here is a simple server program to demonstrate the issue.

package main

import (
	"crypto/tls"
	"encoding/base64"
	"log"
)

func main() {
	certPEMBlock, _ := base64.StdEncoding.DecodeString("LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUJ6ekNDQVZXZ0F3SUJBZ0lVUlRvOXBnMTI5TG50SFB4Wm4zZ0dNU2swaStJd0NnWUlLb1pJemowRUF3SXcKSWpFZ01CNEdBMVVFQXd3WFEyVnlkR2xtYVdOaGRHVWdRWFYwYUc5eWFYUjVJREF3SUJjTk1Ua3dPREl5TURndwpOREV6V2hnUE1qRXhPVEE0TWpNd09EQTBNVE5hTUF3eENqQUlCZ05WQkFNTUFUQXdkakFRQmdjcWhrak9QUUlCCkJnVXJnUVFBSWdOaUFBU2RQVUZKQytiUyt4RlJzQUFHT3BkS0k4bVdVZ1VRWVZjdjNnRjlwSExZd2QrR3RxS3AKdmJIdXR0eUhQSkZHcGxHdVpUbjMyakdPV3pMMDJhQklFQjdpc3dpSVptc3hmZUxSdkFzeTdHd1B0Wkc1ZTlnQgp3TUF5WW0wTFRadjZ0VzJqWURCZU1Bd0dBMVVkRXdFQi93UUNNQUF3RVFZSllJWklBWWI0UWdFQkJBUURBZ2JBCk1BNEdBMVVkRHdFQi93UUVBd0lENkRBZEJnTlZIU1VFRmpBVUJnZ3JCZ0VGQlFjREFnWUlLd1lCQlFVSEF3RXcKREFZRFZSMFJCQVV3QTRJQk1EQUtCZ2dxaGtqT1BRUURBZ05vQURCbEFqQWM1WlZtL0pJaVRoZTQ1Z29Va3J5NgpmeGh5Z0hjS0NyaFByQTJjZDl6ZWt0SnBwd0VXU3d5ejRRcHJRczNhTHJnQ01RRE5PZHp3b2JlUW45dDk5TmduCityYlBObC9BWmVtdElER3pQU3dFb3dXR0RONitCTms3ZmVRd2Mwbmxnb0VOV0NzPQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tCg==")
	keyPEMBlock, _ := base64.StdEncoding.DecodeString("LS0tLS1CRUdJTiBQUklWQVRFIEtFWS0tLS0tCk1JRzJBZ0VBTUJBR0J5cUdTTTQ5QWdFR0JTdUJCQUFpQklHZU1JR2JBZ0VCQkRDMG1zam11TGVNSlNXZjJ6QnQKNFhpVVVMNWRkODh5d3ZueVVtQ3NCR01kZDJWSnNzdThnK2svMkdFYnc4MnhlbnFoWkFOaUFBU2RQVUZKQytiUworeEZSc0FBR09wZEtJOG1XVWdVUVlWY3YzZ0Y5cEhMWXdkK0d0cUtwdmJIdXR0eUhQSkZHcGxHdVpUbjMyakdPCld6TDAyYUJJRUI3aXN3aUlabXN4ZmVMUnZBc3k3R3dQdFpHNWU5Z0J3TUF5WW0wTFRadjZ0VzA9Ci0tLS0tRU5EIFBSSVZBVEUgS0VZLS0tLS0K")
	cert, _ := tls.X509KeyPair(certPEMBlock, keyPEMBlock)

	tlsConfig := tls.Config{
		Certificates: []tls.Certificate{cert},
	}

	listener, _ := tls.Listen("tcp", ":8443", &tlsConfig)
	for {
		conn, _ := listener.Accept()
		defer conn.Close()
		log.Printf("server: accepted from %s", conn.RemoteAddr())

		tlsConn := *conn.(*tls.Conn)
		//If you uncomment the line below it works
		//tlsConn.Handshake()
		log.Printf("server: handshake complete: %t", tlsConn.ConnectionState().HandshakeComplete)
	}
}

@FiloSottile
Copy link
Contributor

No reads can happen in Accept(), as that would block the accept loop. That means the Conn returned by Accept is not initialized: HandshakeComplete is false, Version is unset.

What looks like a problem here is that ServeConn should know to run a Read or call Handshake before reaching into ConnectionState.

@FiloSottile FiloSottile changed the title crypto/tls: TLS handshake doesn't complete before http2 server kills the connection x/net/http2: call conn.Handshake before accessing ConnectionState Aug 23, 2019
@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 23, 2019
@FiloSottile FiloSottile added this to the Go1.14 milestone Aug 23, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 23, 2019
@sebsebmc
Copy link
Author

sebsebmc commented Sep 3, 2019

Adding a line to the docs would also help prevent issues like this in the future, and would have helped me diagnose my issue much faster.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants