caddy: v2.7.x: panic when TLS-SNI is received for an unknown domain

@marten-seemann I dunno if any of this info is helpful, but I’m starting to wonder if there’s another code path we missed. I’m not too familiar with the quic-go library though to know.

Good thinking! In fact there is: tls.Config.GetCertificate also passes in a ClientHelloInfo. I feel really stupid for missing this 😔. For some reason I thought that tls.Config.GetConfigForClient would be the only place, and didn’t even bother checking. This means we’ll need to wrap GetCertificate as well, equivalent to what we do in https://github.com/quic-go/quic-go/pull/4001/files#diff-c1eebd1c52f66da524b99c57f22669500f22784ae80475fbf2b786bc0d1ba278.

Fix coming later today. This will be included in the v0.37.2 patch release.

@W0n9 A fix for both panics is in the works, and will be included in the v0.37.2 release.

Okay! I have a way to replicate it.

Simple config:

{
    debug
    admin off
    http_port 8881
}

https://localhost:8882 {
    respond "foo"
}

Run with ./caddy run --config Caddyfile as normal.

(I’m using non-standard ports and turning off admin because I have another Caddy instance running already)

Making a request for the wrong SNI (i.e. foo.localhost when Caddy only has localhost) with this command (container with a custom curl with http3 support, because it’s the easiest way for me to try http3 reliably):

$ docker run -it --rm --net=host ymuski/curl-http3 curl -vsk -o/dev/null --http3-only https://foo.localhost:8882

@marten-seemann hopefully that should be enough for you to be able to do some fmt.Printf() tracing and such?

Stacktrace with Go 1.21 if it helps at all (i.e. using stdlib TLS stack instead of qtls):

goroutine 82 [running]:
github.com/caddyserver/certmagic.(*Config).getCertDuringHandshake(0xc0001db380, {0x1e4e970, 0x2b323a0}, _, _)
	/home/francis/go/pkg/mod/github.com/caddyserver/certmagic@v0.19.1/handshake.go:378 +0x1340
github.com/caddyserver/certmagic.(*Config).GetCertificateWithContext(0xc0001db380, {0x1e4e970, 0x2b323a0}, 0xc0001db110)
	/home/francis/go/pkg/mod/github.com/caddyserver/certmagic@v0.19.1/handshake.go:84 +0xbc5
github.com/caddyserver/certmagic.(*Config).GetCertificate(...)
	/home/francis/go/pkg/mod/github.com/caddyserver/certmagic@v0.19.1/handshake.go:50
github.com/caddyserver/caddy/v2/modules/caddytls.(*ConnectionPolicy).buildStandardTLSConfig.func1(0xc0001db110)
	/home/francis/repos/caddy/modules/caddytls/connpolicy.go:232 +0x170
crypto/tls.(*Config).getCertificate(0xc0004e2680, 0xc0001db110)
	/home/francis/sdk/gotip/src/crypto/tls/common.go:1116 +0x3b
crypto/tls.(*serverHandshakeStateTLS13).pickCertificate(0xc000735bf8)
	/home/francis/sdk/gotip/src/crypto/tls/handshake_server_tls13.go:435 +0x314
crypto/tls.(*serverHandshakeStateTLS13).handshake(0xc000735bf8)
	/home/francis/sdk/gotip/src/crypto/tls/handshake_server_tls13.go:59 +0x53
crypto/tls.(*Conn).serverHandshake(0xc000216e00, {0x1e4e8c8, 0xc000a04190})
	/home/francis/sdk/gotip/src/crypto/tls/handshake_server.go:53 +0x185
crypto/tls.(*Conn).handshakeContext(0xc000216e00, {0x1e4e890, 0xc000130210})
	/home/francis/sdk/gotip/src/crypto/tls/conn.go:1547 +0x3d3
crypto/tls.(*Conn).HandshakeContext(0xc0005077d0?, {0x1e4e890?, 0xc000130210?})
	/home/francis/sdk/gotip/src/crypto/tls/conn.go:1487 +0x1d
created by crypto/tls.(*QUICConn).Start in goroutine 69
	/home/francis/sdk/gotip/src/crypto/tls/quic.go:177 +0xc9

Baader–Meinhof phenomenon 😉

I did a quick test as well:

docker run -it --rm ymuski/curl-http3 curl -v https://google.com --connect-to google.com:443:192.168.1.25:443 --http3-only

2.7.2 fails but the updated build works.

Hi guys, can you please test with https://github.com/quic-go/quic-go/pull/4016. I’d like to get confirmation that this actually works before cutting yet another patch release again.

@W0n9 can you try a build with quic-go/quic-go#4018 ? You can use --with github.com/quic-go/quic-go=github.com/quic-go/quic-go@ack-after-handshake-complete

I have tried caddy 2.7.3, it fixed for me👍

Testing my server with curl -v https://google.com --connect-to google.com:443:MY_SERVER_IP:443 --http3-only

Caddy crash: 2.7.2 built with xcaddy build --with github.com/caddyserver/cache-handler --with github.com/caddyserver/replace-response
Works: 2.7.2 built with xcaddy build --with github.com/caddyserver/cache-handler --with github.com/caddyserver/replace-response --with github.com/quic-go/quic-go=github.com/quic-go/quic-go@280054c

@mholt Ah, I understand what you were asking now. Following the earlier example with Docker and curl-http3 (substituting for justdanz/curl-http3 because I’m on arm64), I tried with Caddy v2.7.2 as a control and it did indeed crash.

I then tried with GOOS=linux GOARCH=arm64 ./xcaddy build --with github.com/caddy-dns/cloudflare --with github.com/quic-go/quic-go=github.com/quic-go/quic-go@280054c and there is no crash after several minutes of running. I hope this is helpful.

Thanks @marten-seemann, I can confirm my reproduce case from https://github.com/caddyserver/caddy/issues/5680#issuecomment-1664889820 is fixed by https://github.com/quic-go/quic-go/pull/4016.

I think the panic @W0n9 reported is a separate issue though. I don’t know how to replicate that problem. A new issue should probably be opened for that.

Fix is ready at https://github.com/quic-go/quic-go/pull/4014.

You don’t have a certificate for the raw IP, so that is expected and only logged with level debug.

Right, I should not stay up this late, was just focusing on watching the errors, thats from the server and not cloudflare, so all good.

You don’t have a certificate for the raw IP, so that is expected and only logged with level debug.

Oh, as I just checked again I got handshake error now?

{“level”:“debug”,“ts”:1691191817.8557942,“logger”:“http.stdlib”,“msg”:“http: TLS handshake error from 192.168.1.26:56616: no certificate available for ‘192.168.1.4’”}

For whatever its worth, I tested on windows as well and I no longer receive TLS handshake error spam after building with

xcaddy build master --with github.com/caddy-dns/cloudflare --with github.com/caddyserver/transform-encoder --with github.com/WeidiDeng/caddy-cloudflare-ip --with github.com/quic-go/quic-go=github.com/quic-go/quic-go@280054c

Excellent, thank you both!

@marten-seemann We now have numerous confirmations of the patch working, so I’m confident with your fix. 💯 Thank you thank you.

Awesome, thank you. Confirmations of the fix from anyone else experiencing the issue would be valuable as we await the patch release! 💯

I’ll try to get a fix out for the recursive GetConfigForClient / GetCertificate some time tonight.

@marten-seemann Dang, there’s still a code path we’re missing.

@RainmakerRaw Thank you so much for trying it out. Sorry to keep you up 😅 Get some good sleep, hopefully we’ll have it figured out soon.

@mholt Sorry Matt it’s 2am here, I only just saw this by chance. Compiling now…

Yeah, I hate bugs like this as it happened to me twice on one box and zero times on my other box. Same build process, identical config minus 2 different site names.

Aug  3 07:45:49 gemini caddy[27228]: panic: runtime error: invalid memory address or nil pointer dereference
Aug  3 07:49:28 gemini caddy[27478]: panic: runtime error: invalid memory address or nil pointer dereference

and since those 2, I’ve tried restarting ~30 times and I can’t get it to reproduce.

lmk if there’s anything you need from the quic-go side. I’ll be holding off on cutting the RSA patch release until we’ve reached a conclusion here.

Oh sorry, one second, I forgot I downgraded my phoenix back back to the older version, let me update it and reproduce.

Here are the two:

[felix@gemini caddy]$ ./caddy build-info | grep quic
dep	github.com/quic-go/qpack	v0.4.0	h1:Cr9BXA1sQS2SmDUWjSofMPNKmvF6IiIfDRmgU0w1ZCo=
dep	github.com/quic-go/qtls-go1-20	v0.3.0	h1:NrCXmDl8BddZwO67vlvEpBTwT89bJfKYygxv4HQvuDk=
dep	github.com/quic-go/quic-go	v0.37.1	h1:M+mcsFq9KoxVjCetIwH65TvusW1UdRBc6zmxI6pkeD0=
[felix@gemini caddy]$ ./caddy version
v2.7.2 h1:QqThyoyUFAv1B7A2NMeaWlz7xmgKqU49PXBX08A+6xg=
[felix@gemini caddy]$

and

[felix@phoenix caddy]$ ./caddy build-info | head -5
go	go1.20.1
path	caddy
mod	caddy	(devel)
dep	filippo.io/edwards25519	v1.0.0	h1:0wAIcmJUqRdI8IJ/3eGi5/HwXZWPujYXXlkrQogz0Ek=
dep	github.com/AndreasBriese/bbloom	v0.0.0-20190825152654-46b345b51c96	h1:cTp8I5+VIoKjsnZuH8vjyaysT/ses3EvZeaV/1UkF2M=
[felix@phoenix caddy]$ ./caddy build-info | grep quic
dep	github.com/quic-go/qpack	v0.4.0	h1:Cr9BXA1sQS2SmDUWjSofMPNKmvF6IiIfDRmgU0w1ZCo=
dep	github.com/quic-go/qtls-go1-20	v0.1.0	h1:d1PK3ErFy9t7zxKsG3NXBJXZjp/kMLoIb3y/kV54oAI=
dep	github.com/quic-go/quic-go	v0.32.0	h1:lY02md31s1JgPiiyfqJijpu/UX/Iun304FI3yUqX7tA=
[felix@phoenix caddy]$ ./caddy version
v2.6.4 h1:2hwYqiRwk1tf3VruhMpLcYTg+11fCdr8S3jhNAdnPy8=

My config is small I think as I only use the security plugin to do SSO auth and that protects my site.

[felix@phoenix caddy]$ cat Caddyfile
{
	email {env.EMAIL}
	storage file_system {
		root /opt/caddy/ssl
	}
	order authenticate before respond
	order authorize before basicauth

	security {
		oauth identity provider github {env.GITHUB_CLIENT_ID} {env.GITHUB_CLIENT_SECRET}

		authentication portal myportal {
			crypto default token lifetime 604800
			crypto key sign-verify {env.JWT_SHARED_KEY}
			cookie domain blah.us
			cookie lifetime 604800
			enable identity provider github
			ui {
				links {
					"My Identity" "/whoami" icon "las la-user"
				}
			}

			transform user {
				match realm github
				match sub github.com/user1234
				action add role authp/admin
			}
		}

		authorization policy mypolicy {
			set auth url https://auth.blah.us/oauth2/github
			crypto key verify {env.JWT_SHARED_KEY}
			allow roles authp/admin authp/user
			validate bearer header
			inject headers with claims
		}
	}
}

auth.blah.us {
	tls {
		dns cloudflare {env.CLOUDFLARE_API_TOKEN}
		resolvers 1.1.1.1
	}
	authenticate with myportal
}

I wasn’t doing anything specific to generate as the automatic SSL renewal would kick in and that would produce the error as it happened right after startup.

Oh, we built with Go 1.20.6 though 🤔 did we need to build with 1.20.7?

Edit: Nevermind that shouldn’t have mattered according to the changes in https://github.com/golang/go/issues?q=milestone%3AGo1.20.7