crypto/tls: ClientHelloInfo.Conn field is nil (or return value of RemoteAddr())

[high3eam]

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

$ go version
go version go1.21rc3 linux/amd64

Does this issue reproduce with the latest release?

Reproducable with go1.21rcX
Not reproducable with <go1.20.6

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

Debian 12.1 amd64

go env Output

$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/hnrk/.cache/go-build'
GOENV='/home/hnrk/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/hnrk/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/hnrk/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/hnrk/go-bins/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/hnrk/go-bins/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21rc3'
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 -ffile-prefix-map=/tmp/go-build1381339882=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Trying to run Caddy webserver (Version v2.7.0-beta.2.0.20230725185021-d7d16360d411 h1:Hq2Ph3i47imGFwMmyEb8g8ExG2G9ISJlQJ6R73ddb6E=) with go1.21rc3 as described here but it panics after some time..

Logs

Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: panic: runtime error: invalid memory address or nil pointer dereference
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: [signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x8d5d60]
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: goroutine 108749 [running]:
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: github.com/caddyserver/certmagic.(*Config).getCertDuringHandshake(0xc00137a5b0, {0x1e2ab90, 0x2ae9520}, _, _)
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         github.com/caddyserver/certmagic@v0.19.1/handshake.go:378 +0x1340
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: github.com/caddyserver/certmagic.(*Config).GetCertificateWithContext(0xc00137a5b0, {0x1e2ab90, 0x2ae9520}, 0xc00137a4e0)
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         github.com/caddyserver/certmagic@v0.19.1/handshake.go:84 +0xbc5
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: github.com/caddyserver/certmagic.(*Config).GetCertificate(...)
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         github.com/caddyserver/certmagic@v0.19.1/handshake.go:50
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: github.com/caddyserver/caddy/v2/modules/caddytls.(*ConnectionPolicy).buildStandardTLSConfig.func1(0xc00137a4e0)
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         github.com/caddyserver/caddy/v2@v2.7.0-beta.2.0.20230725185021-d7d16360d411/modules/caddytls/connpolicy.go:232 +>
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: crypto/tls.(*Config).getCertificate(0xc0008e8ea0, 0xc00137a4e0)
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         crypto/tls/common.go:1116 +0x3b
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: crypto/tls.(*serverHandshakeStateTLS13).pickCertificate(0xc0009d9bf8)
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         crypto/tls/handshake_server_tls13.go:435 +0x314
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: crypto/tls.(*serverHandshakeStateTLS13).handshake(0xc0009d9bf8)
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         crypto/tls/handshake_server_tls13.go:59 +0x53
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: crypto/tls.(*Conn).serverHandshake(0xc000774380, {0x1e2aae8, 0xc00146e230})
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         crypto/tls/handshake_server.go:53 +0x185
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: crypto/tls.(*Conn).handshakeContext(0xc000774380, {0x1e2aab0, 0xc0006fee40})
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         crypto/tls/conn.go:1547 +0x3d3
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: crypto/tls.(*Conn).HandshakeContext(0xc000afafd0?, {0x1e2aab0?, 0xc0006fee40?})
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         crypto/tls/conn.go:1487 +0x1d
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]: created by crypto/tls.(*QUICConn).Start in goroutine 109988
Jul 28 13:43:03 www.hnrk.io v2caddy[2690046]:         crypto/tls/quic.go:177 +0xc9
Jul 28 13:43:03 www.hnrk.io systemd[1]: v2caddy.service: Main process exited, code=exited, status=2/INVALIDARGUMENT
Jul 28 13:43:03 www.hnrk.io systemd[1]: v2caddy.service: Failed with result 'exit-code'.
Jul 28 13:43:03 www.hnrk.io systemd[1]: v2caddy.service: Consumed 2min 26.066s CPU time.

It looks like "ClientHelloInfo.Conn field is nil (or maybe just the return value of RemoteAddr() is)" (Thanks @mholt)

What did you expect to see?

Caddy webserver in operating state without any panics.

What did you see instead?

Caddy webserver keeps panicking irregularly.

[mholt]

To be clear, the stack trace indicates the execution of crypto/tls/quic.go:177 so I assume this may be an HTTP/3 request? Either way, there's an unexpected nil value in the tls.ClientHelloInfo -- I believe that isn't intentional. It's new with the Go 1.21 tree. (It doesn't reproduce with older versions of Go.)

[seankhliao]

cc @neild

[neild]

This is an HTTP/3 request.

ClientHelloInfo.Conn is nil because there is no net.Conn associated with this connection. The underlying connection is QUIC, and a QUIC connection is not a net.Conn.

We should document that ClientHelloInfo.Conn is not usable for QUIC connections.

Perhaps we should also populate ClientHelloInfo.Conn with a Conn that returns errors or zero addrs from all of its methods, so anything that inadvertently tries to manipulate it gets an error rather than a panic.

[marten-seemann]

A QUIC stack needs to do better than this. There needs to be a way to retrieve the remote (and maybe local) address, since certificate selection (or even rejection of the handshake) might depend on these.

In quic-go, I used to solve this by creating a fake net.Conn that would return the correct addresses on LocalAddr and RemoteAddr (although switching to crypto/tls in the latest release caused a regression, which I believe is the reason this issue was opened). The documentation for ClientHelloInfo.Conn already forbids reading from or writing to that connection.

If crypto/tls doesn't add this function, I expect that QUIC stacks built on top of crypto/tls will need to wrap GetClientHelloInfo to return such a fake net.Conn as I described above.

[mholt]

Thanks both.

It's really important to get the remote IP, whether it's UDP or TCP. This is crucial for DoS protection, rate limiting, etc. Our users sometimes do vary certificate selection on the remote host, or reject handshakes.

My opinion is that that since QUIC has "virtual" connections (basically?), I'd still expect at least some of the methods to work on net.Conn.

[neild]

QUIC connections don't necessarily have a stable local or remote address, but they do during the handshake and exposing those addresses seems reasonable.

A couple options I can think of:

type QUICConfig struct { // contains existing fields
  // Conn provides a net.Conn for the ClientHelloInfo.Conn field.
  // It is not used in any other way.
  Conn net.Conn
}

// SetLocalAddr sets the local address of the connection.
// The ClientHelloInfo.Conn will contain a net.Conn that returns this address from LocalAddr.
func (*QUICConn) SetLocalAddr(net.Addr) {}

// SetRemoteAddr sets the remote address of the connection.
// The ClientHelloInfo.Conn will contain a net.Conn that returns this address from RemoteAddr.
func (*QUICConn) SetRemoteAddr(net.Addr) {}

We could also have LocalAddr/RemoteAddr fields or methods on ClientHelloInfo, to give the local/remote address without needing to go through a possibly-fake connection that you aren't supposed to touch anyway.

[mholt]

Thanks for the careful consideration on this. I'll let Marten give his thoughts on the proposed API, since we use it only indirectly through quic-go at this time; for us, as long as the ClientHelloInfo.Conn is not nil and can return a RemoteAddr, that is all we need :)

[neild]

We don't have time to add any API for this in 1.21, but that should be fine; quic-go can wrap the GetClientHelloInfo callback for now and inject its own fake net.Conn. We can take our time and figure out something better for 1.22.

An immediate question for 1.21 and the future is what ClientHelloInfo.Conn should be by default for QUIC connections: nil, because there is no connection, or a fake net.Conn that returns errors from the various Conn methods? Assume in both cases that there's some way to get the local/remote addrs.

[mholt]

Gotcha, makes sense.

I'd personally prefer to have a "fake" net.Conn as long as it returns usable address values for RemoteAddr() and LocalAddr(). I think applications will continue to expect that, at least during a handshake. Looking at the other methods, returning an error / making them no-ops would be fine with me.

[marten-seemann]

@neild Should we aim to resolve this issue for Go 1.22? This issue is one of the reasons why QUIC implementations need to clone the tls.Config, and it's surprisingly subtle due the recursive nature of GetConfigForClient. How do we make a decision which solution to implement?