Open high3eam opened 1 year ago
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.)
cc @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.
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.
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.
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.
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 :)
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.
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.
@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?
What version of Go are you using (
go version
)?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
OutputWhat 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
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.