AGWA
commented
3 months ago I think most server operators who enable ECH will want to continue supporting non-ECH clients. I also think it will be desirable to use a hostname as both an ECH public name and a real SNI hostname. The proposed API makes this awkward by requiring the programmer to write a GetConfigForClient function that checks if the ECH extension is present (which requires #32936) to decide whether to return an EncryptedClientHelloConfig or not.
I'm not sure of the value in aborting a handshake if there is no ECH, since by that point, the private hostname has already been sent in the clear. There might be value in encouraging the user to use an ECH-supporting client for future connections, but that can be handled post-handshake by checking ECHAccepted in ConnectionState and sending a user-friendly error.
Note that the spec states that servers complete the handshake normally if no ECH extension is present (Section 7) and defines the ech_required alert for clients only.
To simplify configuration of a common case, and be more in line with the spec, I propose that crypto/tls complete a normal handshake if the ECH extension is absent, regardless of EncryptedClientHelloConfigs.
For simplicity, I also propose removing PublicName from EncryptedClientHelloConfig, since it seems redundant - if an operator wants to restrict the use of ECH configs to certain SNI hostnames, they can use GetConfigForClient. This is compliant with the spec, which says that checking the SNI hostname against the config's public name is "MAY" not "MUST" (Section 7.1).
This does mean that EncryptedClientHelloConfig can't be used to serialize an ECHConfigList, but the ECH config also includes a maximum_name_length that operators may want to set, so reusing the struct probably didn't make sense anyways. I'm not sure the standard library needs a function to serialize an ECHConfigList, but if it does, it could use a different struct that looks something like this:
struct {
ID uint8
PublicKey ecdh.PublicKey
PublicName string
MaximumNameLength uint8
}Finally, for some bikeshedding - having both EncryptedClientHelloConfigs and EncryptedClientHelloConfigList in tls.Config seems unfortunate. Maybe EncryptedClientHelloConfig(s) should be renamed to EncryptedClientHelloKey(s)? That would also free up the name EncryptedClientHelloConfig to be used for the above struct if desired.
HurricanKai
rsc
rolandshoemaker
gopherbot
With client support for ECH landing in 1.23, we should now move on to server side support, hopefully for 1.24.
After implementing client-side ECH, server-side seems a little more complicated, mainly from the configuration standpoint.
The more I think about it, the less I think we'll want to reuse the
EncryptedClientHelloConfigListfield we added for server-side support, since we need a mapping between configs and keys, and we'll likely want to provide some more options.Perhaps introducing a new struct
EncryptedClientHelloKeywith ID and private key fields, and a newEncryptedClientHelloKeysfield intls.Configwould be sufficient, hiding the rest of the HPKE cipher suite configuration etc as an internal implementation detail.Additionally we should probably include
func MarshalEncryptedClientHelloConfigList(configs []EncryptedClientHelloConfig) ([]byte, error)to allow servers to generate the config list necessary for clients and for retry configs.Concretely the new API would be:
~An open question for me is how we handle SNI requests for names not included in the
EncryptedClientHelloConfigslist. I think I'm erring on the side of saying we reject the handshake, and document that if the user wishes to support both ECH and non-ECH handshakes, they should use not configureEncryptedClientHelloConfigsin their top-levelConfigand instead useGetConfigForClientto only setEncryptedClientHelloConfigsfor names they explicitly want ECH for.~ See Discussion below for more context, in short we want to continue supporting both ECH and non-ECH connections in a single config.See #63369 for the client side proposal we implemented.