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

crypto/tls: cannot enforce "h2" with ALPN #59734

Open
jfgiorgi opened this issue Apr 20, 2023 · 4 comments
Open

crypto/tls: cannot enforce "h2" with ALPN #59734

jfgiorgi opened this issue Apr 20, 2023 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@jfgiorgi
Copy link

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

$ go version
go version go1.20.3 linux/amd64

Does this issue reproduce with the latest release?

yes

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

go env
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/user/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1335295081=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import (
	"crypto/tls"
	"flag"
	"log"
	"os"
)

var localAddr = flag.String("a", ":8765", "local address to listen ([ip]:port)")
var optCert = flag.String("cert", "", "certificate file")
var optCertKey = flag.String("key", "", "certificate key file")

func main() {
	log.SetFlags(log.Lshortfile)
	flag.Parse()
	if *optCert == "" || *optCertKey == "" {
		flag.Usage()
		os.Exit(1)
	}
	cer, err := tls.LoadX509KeyPair(*optCert, *optCertKey)
	if err != nil {
		log.Println(err)
		return
	}
	config := &tls.Config{Certificates: []tls.Certificate{cer}}
	config.NextProtos = []string{"h2"}
	ln, err := tls.Listen("tcp", *localAddr, config)
	if err != nil {
		log.Println(err)
		return
	}
	defer ln.Close()

	log.Printf("Listening: %v\n", ln.Addr().String())

	for {
		conn, err := ln.Accept()
		if err != nil {
			log.Println(err)
			continue
		}
		log.Printf("connection from %s\n", conn.RemoteAddr())
		tlsconn, ok := conn.(*tls.Conn)
		if !ok {
			log.Fatal("not tls conn!")
		}
		err = tlsconn.Handshake()
		if err != nil {
			log.Println(err)
			continue
		}
		connState := tlsconn.ConnectionState()
		log.Printf("ALPN = %s", connState.NegotiatedProtocol)
		// simple echo
		go func(c *tls.Conn) {
			defer c.Close()
			b := make([]byte, 1024)
			for {
				_, err := c.Read(b)
				if err != nil {
					break
				}
				_, err = c.Write(b)
				if err != nil {
					break
				}
			}
		}(tlsconn)
	}
}
go run main.go cert.crt cert.key then from another terminal: echo HELLO | openssl s_client -connect localhost:8765 echo HELLO | openssl s_client -connect localhost:8765 -alpn http/1.1 echo HELLO | openssl s_client -connect localhost:8765 -alpn foo echo HELLO | openssl s_client -connect localhost:8765 -alpn h2

What did you expect to see?

main.go:50: tls: client requested unsupported application protocols ([]) ... main.go:50: tls: client requested unsupported application protocols ([http/1.1]) ... main.go:50: tls: client requested unsupported application protocols ([foo]) ... main.go:54: negociated protocol = h2

What did you see instead?

main.go:54: negociated protocol = ... main.go:54: negociated protocol = ... main.go:50: tls: client requested unsupported application protocols ([foo]) ... main.go:54: negociated protocol = h2

With current cpypto/tls and since https://go-review.googlesource.com/c/go/+/325432 ( see #46310 ) we cannot enforce h2 and drop clients asking for http/1.1. cpypto/tls has been specialized/harcoded for net/http instead of been agnostic.
Also there should be an option to accept or not client with no alpn when the server has alpn.
In current state, we have to either clone crypto/tls and modify it or drop the connection at a higher level (which leads to different behaviors/tests, log messages, impact on client & server codes, etc)

@seankhliao
Copy link
Member

cc @golang/security

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 20, 2023
@rolandshoemaker
Copy link
Member

You should be able to enforce the selection of h2 with a Config.GetConfigForClient or Config.VerifyConnection callback, either by requiring the presence of h2 in the ClientHelloInfo.SupportedProtos or requiring ConnecitonState.NegotiatedProtocol is h2 respectively.

@jfgiorgi
Copy link
Author

jfgiorgi commented Apr 21, 2023

You should be able to enforce the selection of h2 with a Config.GetConfigForClient or Config.VerifyConnection callback, either by requiring the presence of h2 in the ClientHelloInfo.SupportedProtos or requiring ConnecitonState.NegotiatedProtocol is h2 respectively.

thx this helped a lot.

I've tested both:

Config.VerifyConnection let the client go further and fails with an error number 80 (alertInternalError). Server side we don't know the proto(s) requested by the client so we can't recreate the same exact error message.

Config.GetConfigForClient can do a better the job but client side we still get an error 42 (alertBadCertificate) instead of the wanted error 120 (alertNoApplicationProtocol). It's very close but not quiet there.

What is missing is a callback Config.NegotiateALPN so we can replace the "too specialized/legacy" default one ...

@jefferai
Copy link

jefferai commented Jan 4, 2024

May be related to #16588 where I identified some issues with NextProtos handling and h2, namely that it's impossible to force h2 when using custom ALPN. There is a workaround documented there; the Go team did not want to change the code in the stdlib even though IMO it wouldn't be a backwards compat issue because it literally doesn't work (unless they have since fixed it).

Basically: manually configure the transport with the x/net http2 logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants