feature: allow overriding tls config based on client hello #3303
Conversation
* This method allows looking up custom config based on client hello * Expand certificate registry type to allow for both certs and config
|
@szuecs could we get some early feedback? We'd like to know if implementing this in the CertificateRegistry seems like a good idea. |
* Create function to parse client auth from annotation * Set tls config when adding tls spec from ingress * functions to parse and add configs to registry
|
|
||
| if cr != nil { | ||
| config.GetCertificate = cr.GetCertFromHello | ||
| config.GetConfigForClient = cr.GetConfigFromHello |
There was a problem hiding this comment.
problem: the tls.Config from GetConfigForClient will override the previously used GetCertificate. So the two are mutually exclusive, from the docs:
Server configurations must set one of Certificates, GetCertificate or GetConfigForClient.
There was a problem hiding this comment.
Check if the new one is non nil and if so only use that one, else fallback to current behavior.
Does it make sense?
There was a problem hiding this comment.
Makes sense.
I was also thinking we could also use GetConfigForClient only. This would allow us to use custom certs and custom TLS config. Config could be set to a "default" based on global skipper flags allowing it to be overridden. This approach shouldn't change the current behavior since it will effectively be the same functionality. Wdyt?
There was a problem hiding this comment.
I have no strong opinion on that onre
* Store tls.Config in registry to allow for certs and config to be stored * Refactor registry to handle setting configs with hash for comparison * Only ClientAuth and certs can be set with option for more tls configs
| // ParseTLSClientAuth parses the string representations of different | ||
| // client auth types. | ||
| func ParseTLSClientAuth(s string) (tls.ClientAuthType, error) { | ||
| switch s { |
There was a problem hiding this comment.
| switch s { | |
| switch strings.ToLower(s) { |
just an idea to support casing as well
| } | ||
|
|
||
| if found && !bytes.Equal(curr.hash, configHash) { | ||
| if !config.Certificate.Leaf.NotBefore.After(curr.config.Certificates[0].Leaf.NotBefore) { |
There was a problem hiding this comment.
problem: this causes config to be skipped when separate ingresses have different annotations. We'd need to set the annotation for all hosts regardless of the ingress.
There was a problem hiding this comment.
I am not sure why you compare the "NotBefore" values.
As far as I understand config.Certificate is the global default and curr.config.Certificates[0] is the looked up one. So here we compare if default is not newer than the looked up one.
Should not we match the "better" one?
For example better match like *.foo.example vs www.foo.example ?
| return nil | ||
| } | ||
|
|
||
| if found && !bytes.Equal(curr.hash, configHash) { |
There was a problem hiding this comment.
it's a duplicate check for found so maybe reafctor to have only one check for found.
4 participants
Fixes #3295
This feature allows setting a custom TLS config based on the client's request. It uses tls GetConfigForClient to obtain the correct config for the specific client's hello.
This feature allows the use of ingress annotations or other methods to add custom configs per routes.
This is especially useful for the issue being fixed where we would like to setup client auth per ingress and/or routegroup.