Caddy: v2: panic when using HTTP3 with self-signed certificates

[damnever]

Caddy version: v2.0.0-beta.14

Stack:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x17912e9]

goroutine 63 [running]:
github.com/mholt/certmagic.(*Config).getCertificate(0xc0007e8000, 0xc0007e20b0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /Users/damnever/.go/pkg/mod/github.com/mholt/[email protected]/handshake.go:101 +0x2d9
github.com/mholt/certmagic.(*Config).getCertDuringHandshake(0xc0007e8000, 0xc0007e20b0, 0x101, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /Users/damnever/.go/pkg/mod/github.com/mholt/[email protected]/handshake.go:206 +0xd2
github.com/mholt/certmagic.(*Config).GetCertificate(0xc0007e8000, 0xc0007e20b0, 0x0, 0x0, 0x0)
        /Users/damnever/.go/pkg/mod/github.com/mholt/[email protected]/handshake.go:72 +0x17d
github.com/caddyserver/caddy/v2/modules/caddytls.(*ConnectionPolicy).buildStandardTLSConfig.func1(0xc0007e20b0, 0xc000677a10, 0x1045128, 0xb0)
        /Users/damnever/.go/pkg/mod/github.com/caddyserver/caddy/[email protected]/modules/caddytls/connpolicy.go:169 +0x36a
github.com/marten-seemann/qtls.(*Config).getCertificate(0xc0007e4000, 0xc0007e20b0, 0xc0007e20b0, 0xc00048a2e0, 0x5)
        /Users/damnever/.go/pkg/mod/github.com/marten-seemann/[email protected]/common.go:896 +0x5ac
github.com/marten-seemann/qtls.(*serverHandshakeStateTLS13).pickCertificate(0xc0005e5d88, 0x0, 0x0)
        /Users/damnever/.go/pkg/mod/github.com/marten-seemann/[email protected]/handshake_server_tls13.go:372 +0xf6
github.com/marten-seemann/qtls.(*serverHandshakeStateTLS13).handshake(0xc0005e5d88, 0xc0007e0000, 0x0)
        /Users/damnever/.go/pkg/mod/github.com/marten-seemann/[email protected]/handshake_server_tls13.go:53 +0xb7
github.com/marten-seemann/qtls.(*Conn).serverHandshake(0xc0007a4000, 0x0, 0x18)
        /Users/damnever/.go/pkg/mod/github.com/marten-seemann/[email protected]/handshake_server.go:54 +0x157
github.com/marten-seemann/qtls.(*Conn).Handshake(0xc0007a4000, 0x0, 0x0)
        /Users/damnever/.go/pkg/mod/github.com/marten-seemann/[email protected]/conn.go:1415 +0x3cd
github.com/lucas-clemente/quic-go/internal/handshake.(*cryptoSetup).RunHandshake.func1(0xc00079a120, 0xc0000c6540, 0xc0002cb7a0)
        /Users/damnever/.go/pkg/mod/github.com/lucas-clemente/[email protected]/internal/handshake/crypto_setup.go:239 +0x9f
created by github.com/lucas-clemente/quic-go/internal/handshake.(*cryptoSetup).RunHandshake
        /Users/damnever/.go/pkg/mod/github.com/lucas-clemente/[email protected]/internal/handshake/crypto_setup.go:237 +0xbd

I have found that the hello.Conn.LocalAddr() is nil

&{CipherSuites:[49199 49200 49195 49196 52392 52393 49171 49161 49172 49162 156 157 47 53 49170 10 4865 4867 4866] ServerName: SupportedCurves:[29 23 24 25] SupportedPoints:[0] SignatureSchemes:[2052 1027 2055 2053 2054 1025 1281 15
37 1283 1539 513 515] SupportedProtos:[h3-24] SupportedVersions:[772] Conn:0xc0005c03f0}

Local:  <nil>
Remote:  127.0.0.1:51409

Hope the above information is useful.

[francislavoie]

FYI @marten-seemann just in case this is an upstream issue

[marten-seemann]

Looks like certmagic is looking at the LocalAddr() in https://github.com/mholt/certmagic/blob/adb47e0d77ddb642dd5191ae31b36958197c61d3/handshake.go#L95-L101, which quic-go currently doesn't set.

I'm not sure if I really understand the logic in certmagic and why it makes sense to use the LocalAddr() there. Would it be a good idea if quic-go returned the actual local address there?

[damnever]

This PR fixed the issue in my local environment.

[mholt]

@marten-seemann

I'm not sure if I really understand the logic in certmagic and why it makes sense to use the LocalAddr() there.

If a client doesn't send ServerName in the TLS ClientHello part of the handshake, we use the IP address the client is connecting to instead, in case any certificates match the IP address. Otherwise, certificates for IPs (usually just local/dev/internal certs, but still, we see quite a lot of those in practice) would never be served. So yes, we expect and require that net.Conn always be able to return the address. :+1:

Thank you for the patch! Looks like it will solve the problem.

@damnever Thanks for finding the problem!