-
Notifications
You must be signed in to change notification settings - Fork 568
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
add ecdsa certificate support at gateways #3466
base: master
Are you sure you want to change the base?
Changes from 5 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
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.
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 |
---|---|---|
|
@@ -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" | ||
// +kubebuilder:validation:XValidation:message="cannot have more than two tls certificates",rule="default(self.tls_certificates, []).size() <= 2" | ||
// +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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we intentionally allow tls_certificate + credential_names? 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. My vote would be no 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.
No. only one of them. That was the oneof I added. Any issue with that? 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. nvm. I did not add that combination. Fixed now |
||
|
||
// If set to true, the load balancer will send a 301 redirect for | ||
// all http connections, asking the clients to use HTTPS. | ||
bool https_redirect = 1; | ||
|
@@ -471,9 +476,36 @@ 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. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Added validations. PTAL. one thing I could not add is |
||
|
||
// 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. | ||
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 | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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=1There 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.
So to clarify:
ServerTLSSettings
, since it needs access to the fields. It needs to change to camel case