-
Notifications
You must be signed in to change notification settings - Fork 571
add ecdsa certificate support at gateways #3466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “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? Sign in to your account
Changes from all commits
70c2aa8
166ea98
f781c83
856a127
3bea870
1fcc243
a25c2ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -380,6 +380,9 @@ message Port { | |
uint32 target_port = 4 [deprecated=true]; | ||
} | ||
|
||
// +kubebuilder:validation:XValidation:message="only one of credentialNames or tlsCertificates can be set",rule="oneof(self.tlsCertificates, self.credentialNames)" | ||
// +kubebuilder:validation:XValidation:message="only one of credentialName or credentialNames can be set",rule="oneof(self.credentialName, self.credentialNames)" | ||
// +kubebuilder:validation:XValidation:message="only one of credentialName or tlsCertificates can be set",rule="oneof(self.credentialNames, self.tlsCertificates)" | ||
message ServerTLSSettings { | ||
// If set to true, the load balancer will send a 301 redirect for | ||
// all http connections, asking the clients to use HTTPS. | ||
|
@@ -471,9 +474,40 @@ 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. | ||
// +kubebuilder:validation:MaxItems=2 | ||
// +kubebuilder:validation:MinItems=1 | ||
repeated string credential_names = 14; | ||
|
||
// TLSCertificate describes the server's TLS certificate. | ||
message TLSCertificate { | ||
// REQUIRED if mode is `SIMPLE` or `MUTUAL`. The path to the file | ||
// holding the server-side TLS certificate to use. | ||
string server_certificate = 1; | ||
|
||
// REQUIRED if mode is `SIMPLE` or `MUTUAL`. The path to the file | ||
// holding the server's private key. | ||
string private_key = 2; | ||
|
||
// REQUIRED if mode is `MUTUAL` or `OPTIONAL_MUTUAL`. The path to a file | ||
// containing certificate authority certificates to use in verifying a presented | ||
// client side certificate. | ||
string ca_certificates = 3; | ||
} | ||
|
||
// Only one of `server_certificate`, `private_key`, `ca_certificates` or `credential_name` | ||
// or `credential_names` or `tls_certificates` should be specified. | ||
// This is mainly used for specifying RSA and ECDSA certificates for the same server. | ||
// +kubebuilder:validation:MaxItems=2 | ||
// +kubebuilder:validation:MinItems=1 | ||
repeated TLSCertificate tls_certificates = 15; | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's doing the validating? If the validation fails, what errors? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the existing SAN validation and you get certificate verify failed message. |
||
// the subject alternate names are validated against the selected certificate. | ||
repeated string subject_alt_names = 6; | ||
|
||
// An optional list of base64-encoded SHA-256 hashes of the SPKIs of | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only 2. I will add CEL validation.
Yes. I was n't expecting/not thinking of use case for different SANs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also make sure you set either credential_name or credential_names but not both (and same for tls_certificate(s) I think?)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?