Repository navigation
Conversation
When the server rejects ECH it answers the outer ClientHello, so the certificate it presents is for the ECH configuration's public_name, and the client must verify it against that name to obtain retry_configs (RFC 9849, section 6.1.6). verifyServerCertificate instead used the inner name from Config.ServerName, which almost always differs, so a correctly behaving client-facing server failed verification with a hostname mismatch and *ECHRejectionError was never returned. crypto/tls verifies against c.serverName, which holds the public name; uTLS had replaced it with the config name while adding InsecureServerNameToVerify. Both ClientHello builders set c.serverName to the public name: the Go path from the ECH config, and the custom spec path through the SNI extension in ApplyPreset. InsecureServerNameToVerify still overrides the name when set. Refs: refraction-networking#407
EncryptedClientHelloRejectionVerify exists so that a client can verify the ECH provider's certificate itself when the server rejects ECH, but it was invoked before the chain was recorded on the connection, so the ConnectionState it received had an empty PeerCertificates and the callback had nothing to verify. crypto/tls has the same ordering, tracked in golang/go#75242. Record the chain before calling the callback. VerifiedChains stays empty, since the callback replaces verification, and the field's documentation now states what the callback receives. Refs: golang/go#75242
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the server rejects ECH, the client must authenticate it for the ECH configuration's
public_nameto obtainretry_configs(RFC 9849, section 6.1.6).verifyServerCertificate'sechRejectedbranch instead verifies the presented certificate against the inner name inConfig.ServerName. The two names almost always differ, so a correctly behaving client-facing server, whose certificate covers only the public name, always fails with a hostname mismatch and the*ECHRejectionErrorcarryingretry_configsis never returned.crypto/tlsverifies againstc.serverName, which holds the public name; uTLS replaced it with the config name whenInsecureServerNameToVerifywas added. This is the same defect #407 fixes, with the same one-line change.Separately,
EncryptedClientHelloRejectionVerifyis called beforec.peerCertificatesis set, so the callback receives aConnectionStatewith an emptyPeerCertificatesand cannot verify the provider certificate as its documentation says.crypto/tlshas the same ordering (golang/go#75242, open). The second commit records the chain before invoking the callback and documents what it receives;VerifiedChainsstays empty since the callback replaces verification.Both ClientHello builders set
c.serverNameto the public name (the Go path from the ECH config, the custom spec path through the SNI extension inApplyPreset), and the new tests run each case against both. The existingTestTLS13ECHRejectionCallbacksdid not catch either defect because it sets the inner name equal to the public name; it is left unchanged, with its ECHConfigList literal moved into a named constant.Verified with Go 1.27.3:
go test ./...,go test -raceon the ECH tests,go vet,gofmt -s,go fix -diff, staticcheck and golangci-lint report nothing.Refs #407