feat!(stackable-certs): Support adding SAN entries #1057
Conversation
| Ok(GeneralName::DnsName(Ia5String::new(dns_name).context( | ||
| ParseSubjectAlternativeDnsNameSnafu { | ||
| subject_alternative_dns_name: dns_name.to_string(), | ||
| }, | ||
| )?)) |
There was a problem hiding this comment.
I find these a little bit of a handful and would tend to prefer this:
| Ok(GeneralName::DnsName(Ia5String::new(dns_name).context( | |
| ParseSubjectAlternativeDnsNameSnafu { | |
| subject_alternative_dns_name: dns_name.to_string(), | |
| }, | |
| )?)) | |
| let ia5_dns_name = Ia5String::new(dns_name).context( | |
| ParseSubjectAlternativeDnsNameSnafu { | |
| subject_alternative_dns_name: dns_name.to_string(), | |
| }, | |
| )?; | |
| Ok(GeneralName::DnsName(ia5_dns_name)) |
You're welcome to ignore this suggestion
There was a problem hiding this comment.
Even better if we had a TryFrom impl, we could then do:
let sans = subject_alterative_dns_names
.into_iter()
.map(GeneralName::try_from)
.collect::<Result<Vec<_>, Error>>()?;|
|
||
| - BREAKING: `CertificateAuthority::generate_leaf_certificate` (and `generate_rsa_leaf_certificate` and `generate_ecdsa_leaf_certificate`) | ||
| now take an additional parameter `subject_alterative_dns_names`. The passed SANs are added to the generated certificate, | ||
| this is needed for basically all modern TLS certificate validations when used with HTTPS. |
There was a problem hiding this comment.
I disagree that it is needed.
It is only needed when you want to use the certificate with multiple names (in addition to the commonName). So while it is common, it isn't correct to say that it is needed due to modern validation impls.
Unless I've been hiding under a rock and you have seen problems validating on commonName alone?
| this is needed for basically all modern TLS certificate validations when used with HTTPS. | |
| this is often needed. |
or
| this is needed for basically all modern TLS certificate validations when used with HTTPS. | |
| this is needed when the HTTPS server is accessible on multiple DNS names and/or IPs. |
| - BREAKING: The constant `DEFAULT_CA_VALIDITY_SECONDS` has been renamed to `DEFAULT_CA_VALIDITY` and now is of type `stackable_operator::time::Duration`. | ||
| Also, the constant `ROOT_CA_SUBJECT` has been renamed to `SDP_ROOT_CA_SUBJECT` ([#1057]). |
There was a problem hiding this comment.
| - BREAKING: The constant `DEFAULT_CA_VALIDITY_SECONDS` has been renamed to `DEFAULT_CA_VALIDITY` and now is of type `stackable_operator::time::Duration`. | |
| Also, the constant `ROOT_CA_SUBJECT` has been renamed to `SDP_ROOT_CA_SUBJECT` ([#1057]). | |
| - BREAKING: Constants have been renamed/retyped ([#1057]): | |
| - `DEFAULT_CA_VALIDITY_SECONDS` has been renamed to `DEFAULT_CA_VALIDITY` and now is of type `stackable_operator::time::Duration`. | |
| - `ROOT_CA_SUBJECT` has been renamed to `SDP_ROOT_CA_SUBJECT`. |
| ID_KP_CLIENT_AUTH, | ||
| ID_KP_SERVER_AUTH, |
There was a problem hiding this comment.
I know it was existing behaviour, so no change required now, but I don't think we should make any certificate usable as a Client Cert. It should be intentional.
| }) | ||
| .collect::<Result<Vec<_>, Error>>()?; | ||
| builder | ||
| .add_extension(&SubjectAltName(sans)) |
There was a problem hiding this comment.
Just checking what happens if there are no sans (note to self, see if tests show it).
Nice, there is a test for the empty sans.
Description
As #1044 didn't move along, splitting the relevant change out to unblock CRD versioning.
Definition of Done Checklist
Author
Reviewer
Acceptance