add ecdsa certificate support at gateways #3466

ramaraochavali · 2025-03-12T11:26:34Z

Also needed for Gateway API

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn · 2025-03-12T15:37:35Z

networking/v1alpha3/gateway.proto

@@ -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;


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

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

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

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.

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 <rama.rao@salesforce.com>

ramaraochavali · 2025-03-18T15:27:17Z

@howardjohn can you PTAL when you get chance?

howardjohn · 2025-03-18T17:40:09Z

networking/v1alpha3/gateway.proto

@@ -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"


Suggested change

// +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

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

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

howardjohn · 2025-03-18T17:43:00Z

networking/v1alpha3/gateway.proto

+// +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)"


Do we intentionally allow tls_certificate + credential_names?

My vote would be no

Do we intentionally allow tls_certificate + credential_names?

No. only one of them. That was the oneof I added. Any issue with that?

nvm. I did not add that combination. Fixed now

keithmattix · 2025-03-18T17:46:15Z

networking/v1alpha3/gateway.proto

+// +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)"


My vote would be no

keithmattix · 2025-03-18T17:47:37Z

networking/v1alpha3/gateway.proto

  // 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`,


What's doing the validating? If the validation fails, what errors?

This is the existing SAN validation and you get certificate verify failed message.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn

LGTM just one minor comment

howardjohn · 2025-03-21T18:36:38Z

networking/v1alpha3/gateway.proto

@@ -380,6 +380,9 @@ message Port {
  uint32 target_port = 4 [deprecated=true];
 }

+// +kubebuilder:validation:XValidation:message="only one of credential_names or tls_certificates can be set",rule="oneof(self.tlsCertificates, self.credentialNames)"
+// +kubebuilder:validation:XValidation:message="only one of credential_name or credential_names can be set",rule="oneof(self.credentialName, self.credentialNames)"
+// +kubebuilder:validation:XValidation:message="only one of credential_name or tls_certificates can be set",rule="oneof(self.credentialNames, self.tlsCertificates)"


Suggested change

// +kubebuilder:validation:XValidation:message="only one of credential_name or tls_certificates can be set",rule="oneof(self.credentialNames, self.tlsCertificates)"

// +kubebuilder:validation:XValidation:message="only one of credential_name or tls_certificates can be set",rule="oneof(self.credentialName, self.tlsCertificates)"

nit: the error message probably should also be camel case, though that matters less

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2025-03-24T22:27:11Z

@howardjohn can you PTAL when you get chance?

howardjohn · 2025-03-25T15:23:44Z

When you implement this can you make sure to either add support in gateway-api as well (they have the API but we just allow 1 entry) or open an issue to track the TODO?

ramaraochavali · 2025-03-25T15:37:37Z

When you implement this can you make sure to either add support in gateway-api as well (they have the API but we just allow 1 entry) or open an issue to track the TODO?

Will do

ramaraochavali · 2025-03-26T14:13:41Z

@keithmattix can you PTAL when you get chance?

For istio#36181 Implements istio/api#3466

Address istio#36181 - Update istio gateway to support multiple server certificates (istio/api#3466) - Update gateway api support to allow at most 2 server certificates

add ecdsa certificate support at gateways

70c2aa8

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali added the release-notes-none Indicates a PR that does not require release notes. label Mar 12, 2025

ramaraochavali requested a review from a team as a code owner March 12, 2025 11:26

istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 12, 2025

add support for certs

166ea98

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn reviewed Mar 12, 2025

View reviewed changes

ramaraochavali added 2 commits March 14, 2025 14:16

add cel validations

f781c83

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

add comments for subject alt names

856a127

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 14, 2025

clean gen

3bea870

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn reviewed Mar 18, 2025

View reviewed changes

keithmattix reviewed Mar 18, 2025

View reviewed changes

fix validations

1fcc243

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn reviewed Mar 21, 2025

View reviewed changes

fix message

a25c2ce

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn approved these changes Mar 25, 2025

View reviewed changes

keithmattix approved these changes Mar 26, 2025

View reviewed changes

istio-testing merged commit ffa4a97 into istio:master Mar 26, 2025
5 checks passed

ramaraochavali deleted the fix/ecdsa_support branch March 26, 2025 14:22

raoaditya314 added a commit to raoaditya314/istio that referenced this pull request Mar 30, 2025

Add support for multiple server certificates in istio gateway

c075b33

For istio#36181 Implements istio/api#3466

raoaditya314 mentioned this pull request Mar 31, 2025

Support dual certificates in istio gateway and gateway-api istio/istio#55760

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ecdsa certificate support at gateways #3466

add ecdsa certificate support at gateways #3466

ramaraochavali commented Mar 12, 2025

howardjohn Mar 12, 2025

ramaraochavali Mar 12, 2025

howardjohn Mar 12, 2025 •

edited

Loading

ramaraochavali Mar 13, 2025

ramaraochavali Mar 14, 2025

ramaraochavali commented Mar 18, 2025

howardjohn Mar 18, 2025

howardjohn Mar 18, 2025

howardjohn Mar 18, 2025

howardjohn Mar 18, 2025

keithmattix Mar 18, 2025

ramaraochavali Mar 19, 2025

ramaraochavali Mar 19, 2025

keithmattix Mar 18, 2025

keithmattix Mar 18, 2025

ramaraochavali Mar 19, 2025

howardjohn left a comment

howardjohn Mar 21, 2025

ramaraochavali commented Mar 24, 2025

howardjohn commented Mar 25, 2025

ramaraochavali commented Mar 25, 2025

ramaraochavali commented Mar 26, 2025

	// +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"

add ecdsa certificate support at gateways #3466

add ecdsa certificate support at gateways #3466

Conversation

ramaraochavali commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Mar 24, 2025

howardjohn commented Mar 25, 2025

ramaraochavali commented Mar 25, 2025

ramaraochavali commented Mar 26, 2025

howardjohn Mar 12, 2025 •

edited

Loading