Skip to content

proposal: crypto/tls: add GetEncryptedClientHelloKeys callback #71920

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? No Sign in to your account

Open
mholt opened this issue Feb 24, 2025 · 8 comments
Open

proposal: crypto/tls: add GetEncryptedClientHelloKeys callback #71920

mholt opened this issue Feb 24, 2025 · 8 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal
Milestone

Comments

@mholt
Copy link

mholt commented Feb 24, 2025

Go version

go version go1.24.0 linux/amd64

Output of go env in your module/workspace:

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/matt/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/matt/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1380606169=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/matt/Dev/caddyserver/caddy/go.mod'
GOMODCACHE='/home/matt/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/matt/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='on'
GOTELEMETRYDIR='/home/matt/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.0'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

I'm attempting to implement the ECH spec with regards to key rotation. It states in 10.10.5:

This design is not forward secret because the server's ECH key is static. However, the window of exposure is bound by the key lifetime. It is RECOMMENDED that servers rotate keys frequently.

The application (Caddy in this case) requires use of GetConfigForClient.

What did you see happen?

The problem is, tls.Config.EncryptedClientHelloKeys is a static field, not a callback like GetCertificate.

This means we have to start a new TLS server with the updated ECH keys, which is complex and can be tricky to do without downtime.

I also realized that GetConfigForClient is not recursive... at first I tried using GetConfigForClient to return a tls.Config populated especially for ECH:

// ECH keys loaded dynamically for the outer SNI, which
// solves the key rotation problem
// ...
return &tls.Config{
	MinVersion:               tls.VersionTLS13,
	GetConfigForClient:       getConfigForClient, // loads config for inner SNI
	EncryptedClientHelloKeys: stdECHKeys,
}, nil

But this results in "no certificates configured", because the returned Config's GetConfigForClient isn't called, and sure enough it has no certs. Relevant code:

https://cs.opensource.google/go/go/+/master:src/crypto/tls/handshake_server.go;l=148-168?q=GetConfigForClient&ss=go%2Fgo&start=11

But we can't return the final tls.Config yet (the one with certificates) because we don't have the inner SNI yet.

What did you expect to see?

I was hoping that GetConfigForClient + EncryptedClientHelloKeys could play well together... in the case of ECH, maybe allow GetConfigForClient to be used to unwrap the outer Hello, and also be used for the inner. (i.e. allow calling it twice, once before once after processing ECH) Or, make a new field, GetEncryptedClientHelloKeys callback function, much like GetCertificate.

  • We can't forfeit the use of GetConfigForClient due to dynamic configuration requirements.
  • But EncryptedClientHelloKeys must be changed after the server has started

Rotating keys is beneficial for security and encouraged by the spec.

Do you have any other suggestions for making this work as-is? Or is this something that can be enhanced?

Thanks again for server-side ECH 💯

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Feb 24, 2025
@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 24, 2025
@prattmic
Copy link
Member

cc @FiloSottile @rolandshoemaker @golang/security

@rolandshoemaker
Copy link
Member

Is the only dynamic configuration (other than the ECH keys) the certificates for the unwrapped SNI? If so can you just offload that logic to a GetCertificate callback (which can be populated in the config returned from GetConfigForClient, if that helps)?

@mholt
Copy link
Author

mholt commented Feb 26, 2025

I did consider that, and it's tempting to do that, but unfortunately, Caddy users can customize much about the TLS connection: https://caddyserver.com/docs/json/apps/http/servers/tls_connection_policies/

I guess in TLS 1.3 (especially with ECH enabled), many of the parameters have no effect or are overridden by hard-coded values to ensure compliance, but parameters like client authentication (ClientAuthMode) and, in some cases, even KeyLogWriter are customized by the user on a dynamic basis. (In Caddy, users can customize TLS configs based not only on ServerName, but on any property of the ClientHelloInfo.)

So, yes I think that would be a workaround, but I'd have to disclaim in our docs that enabling ECH essentially ignores all other TLS configuration properties.

I guess I'm hoping for a way to still have control over the TLS config for the unwrapped ClientHello when GetConfigForClient is used.

That'd be ideal -- but I don't want to try to force a square peg into a round hole. It could be an odd use case maybe.

@gstrauss
Copy link

gstrauss commented Feb 26, 2025

The problem is, tls.Config.EncryptedClientHelloKeys is a static field, not a callback like GetCertificate.

	// If a client attempts ECH, but it is rejected by the server, the server
	// will send a list of configs to retry based on the set of
	// EncryptedClientHelloKeys which have the SendAsRetry field set.

When a new (refreshed) TLS ECH key is created, it is desirable that the prior key still be valid for a short period, but the prior key should now have SendAsRetry field unset. Also, after a short period of time, the prior key should be removed from EncryptedClientHelloKeys.

[Edit: so not only is the list of keys dynamic, but it is desirable to be able to change the properties on each key. (Alt: Could remove key and re-add same key but with altered properties)]

@rolandshoemaker
Copy link
Member

My memory of how I actually implemented the client hello switching paged out of my brain almost immediately after 1.24 was finished, looking back at the actual code I see how this is problematic. We currently switch out the client hello before calling GetConfigForClient, so the returned config only ever applies to the inner hello.

I think this makes sense, since the outer config should never really require special config settings, except EncryptedClientHelloKeys. Unfortunately this doesn't leave any way to currently change EncryptedClientHelloKeys safely once the Config is passed to a Conn because we say the Config cannot be mutated. So unless the caller is manually creating configs with Server(..), which is a pretty niche usage of the package, they are forced to essentially restartthe server if they want to change the keys.

I think the only viable option here is to add a new callback, e.g. GetEncryptedClientHelloKeys func(*ClientHelloInfo) ([]EncryptedClientHelloKey, error), which lets the caller return a set of keys based on the outer client hello (we may actually want to reduce the scope of the client hello to the outer SNI, since the rest of the information in the outer hello should generally be ignored.)

@rolandshoemaker
Copy link
Member

Note, because this will be a new API, it'll need to wait for 1.25.

@rolandshoemaker rolandshoemaker changed the title crypto/tls: EncryptedClientHelloKeys can't be safely rotated when GetConfigForClient is used proposal: crypto/tls: add GetEncryptedClientHelloKeys callback Feb 26, 2025
@gopherbot gopherbot added this to the Proposal milestone Feb 26, 2025
@rolandshoemaker rolandshoemaker moved this to Incoming in Proposals Feb 26, 2025
@mholt
Copy link
Author

mholt commented Feb 26, 2025

FWIW I really like the design choice overall -- it makes generating, publishing, and serving ECH keys elegantly symmetric. The remaining puzzle piece is the on-line rotation.

A callback should do the trick. 💯

No particular hurry. I have it working without key rotation for now, as it's mainly a "good to have" feature but not strictly necessary. So Go 1.25 is fine with me.

ECH has been interesting in reshaping my view of TLS since, for the first time I think, a TLS connection may be established strictly for the purpose of TLS itself, rather than for application data. (The one case being, a TLS handshake for the purpose of conveying ECH retry-enabled configs. I think. Maybe there's others I haven't known about.) So I had to decide in my application that the outer handshake is abstracted away from the user, since it is not in any way related to or used for application data/use.

Given that paradigm, I think limiting the data populated into the outer ClientHelloInfo is acceptable, and worst case scenario, more data can be added later if needed. Although, how would you feel about preserving the underlying Conn? Since some filtering is done based on remote IP, for example.

No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal
Projects
Status: Incoming
Development

No branches or pull requests

6 participants