add ecdsa certificate support at gateways #3466
Conversation
Signed-off-by: Rama Chavali <[email protected]>
ramaraochavali
added
the
release-notes-none
istio-testing
added
the
size/S
Signed-off-by: Rama Chavali <[email protected]>
| @@ -471,6 +471,31 @@ message ServerTLSSettings { | |||
| // or credentialName can be specified. | |||
| string credential_name = 10; | |||
|
|
|||
| // Same as CredentialName but for multiple certificates. Mainly used for specifying | |||
| // RSA and ECDSA certificates for the same server. | |||
| repeated string credential_names = 14; | |||
There was a problem hiding this comment.
Are we going to support an arbitrary number or only 2? If a limit, can we add CEL validation?
Do we expect the certificates to be identical except in algorithm? Or could they, for instance, be multiple distinct SANs
There was a problem hiding this comment.
Only 2. I will add CEL validation.
Do we expect the certificates to be identical except in algorithm? Or could they, for instance, be multiple distinct SANs
Yes. I was n't expecting/not thinking of use case for different SANs
There was a problem hiding this comment.
Can we also make sure you set either credential_name or credential_names but not both (and same for tls_certificate(s) I think?)
Yes. I was n't expecting/not thinking of use case for different SANs
OK, I think we cannot actually validate it but we can just comment that is the intended usage maybe?
What happens in envoy if they don't match
There was a problem hiding this comment.
What happens in envoy if they don't match
Whatever certificate Envoy picked for TLS termination, it will validate the configured SAN's with available SAN's on that certificate.
There was a problem hiding this comment.
Added validations. PTAL. one thing I could not add is
only one of cred*, (server_certificate, private_key),tls_certificates can be specified. Do you know if it is possible?
Signed-off-by: Rama Chavali <[email protected]>
Signed-off-by: Rama Chavali <[email protected]>
istio-testing
added
size/M
Signed-off-by: Rama Chavali <[email protected]>
|
@howardjohn can you PTAL when you get chance? |
networking/v1alpha3/gateway.proto
Outdated
| @@ -381,6 +381,11 @@ message Port { | |||
| } | |||
|
|
|||
| message ServerTLSSettings { | |||
| // +kubebuilder:validation:XValidation:message="credential_names cannot have more than two credentials",rule="default(self.credential_names, []).size() <= 2" | |||
There was a problem hiding this comment.
| // +kubebuilder:validation:XValidation:message="credential_names cannot have more than two credentials",rule="default(self.credential_names, []).size() <= 2" | |
| // +kubebuilder:validation:XValidation:message="credential_names cannot have more than two credentials",rule="default(self.credentialName, []).size() <= 2" |
The CEL works on the CRD naming, not the proto, which is camelCase. Might be worth adding that note into GUIDELINES.md.
We could add a test under tests/testdata/ to catch this
There was a problem hiding this comment.
Also this would need to be over message ServerTLSSettings to work, not under it.
But even better, this could be only the specific field itself (ex over tls_certificates).
or even better, we do not need CEL for this; can use kubebuilder:validation:items:MaxItems. We should also probably put MinItems=1
There was a problem hiding this comment.
So to clarify:
- The two oneof checks need to be on
ServerTLSSettings, since it needs access to the fields. It needs to change to camel case - The two checks of size should move to the field level and use MinItems/MaxItems
networking/v1alpha3/gateway.proto
Outdated
| // +kubebuilder:validation:XValidation:message="only one of credential_names or tls_certificates can be set",rule="oneof(self.tls_certificates, self.credential_names)" | ||
| // +kubebuilder:validation:XValidation:message="only one of credential_name or credential_names can be set",rule="oneof(self.credential_name, self.credential_names)" |
There was a problem hiding this comment.
Do we intentionally allow tls_certificate + credential_names?
There was a problem hiding this comment.
Do we intentionally allow tls_certificate + credential_names?
No. only one of them. That was the oneof I added. Any issue with that?
There was a problem hiding this comment.
nvm. I did not add that combination. Fixed now
networking/v1alpha3/gateway.proto
Outdated
| // +kubebuilder:validation:XValidation:message="only one of credential_names or tls_certificates can be set",rule="oneof(self.tls_certificates, self.credential_names)" | ||
| // +kubebuilder:validation:XValidation:message="only one of credential_name or credential_names can be set",rule="oneof(self.credential_name, self.credential_names)" |
| // A list of alternate names to verify the subject identity in the | ||
| // certificate presented by the client. | ||
| // Requires TLS mode to be set to `MUTUAL`. | ||
| // When multiple certificates are provided via `credential_names` or `tls_certificates`, |
There was a problem hiding this comment.
What's doing the validating? If the validation fails, what errors?
There was a problem hiding this comment.
This is the existing SAN validation and you get certificate verify failed message.
Signed-off-by: Rama Chavali <[email protected]>
For istio/istio#36181.
Also needed for Gateway API