-
-
Notifications
You must be signed in to change notification settings - Fork 16
feat!(stackable-certs): Use builder pattern and support SAN entries #1044
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 2 commits
651c78a
6b79029
88d850d
3fd41d7
d3e7643
70dc741
08c6bc1
b8c667b
07fcc89
4c84aea
ac18bd9
8f86a93
3995717
e9e8bac
e946556
1322779
fd19a03
3d3e0c1
5cf7c64
c955ae6
c263ae7
c235572
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
use stackable_operator::time::Duration; | ||
|
||
/// The default CA validity time span of one hour (3600 seconds). | ||
pub const DEFAULT_CA_VALIDITY_SECONDS: u64 = 3600; | ||
pub const DEFAULT_CA_VALIDITY: Duration = Duration::from_hours_unchecked(1); | ||
|
||
/// The root CA subject name containing only the common name. | ||
pub const ROOT_CA_SUBJECT: &str = "CN=Stackable Data Platform Internal CA"; | ||
pub const SDP_ROOT_CA_SUBJECT: &str = "CN=Stackable Data Platform Internal CA"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
//! Contains types and functions to generate and sign certificate authorities | ||
//! (CAs). | ||
use std::str::FromStr; | ||
use std::{fmt::Debug, str::FromStr}; | ||
|
||
use const_oid::db::rfc5280::{ID_KP_CLIENT_AUTH, ID_KP_SERVER_AUTH}; | ||
use k8s_openapi::api::core::v1::Secret; | ||
|
@@ -9,9 +9,10 @@ use snafu::{OptionExt, ResultExt, Snafu}; | |
use stackable_operator::{client::Client, commons::secret::SecretReference, time::Duration}; | ||
use tracing::{debug, instrument}; | ||
use x509_cert::{ | ||
Certificate, | ||
builder::{Builder, CertificateBuilder, Profile}, | ||
der::{DecodePem, pem::LineEnding, referenced::OwnedToRef}, | ||
ext::pkix::{AuthorityKeyIdentifier, ExtendedKeyUsage}, | ||
der::{DecodePem, asn1::Ia5String, pem::LineEnding, referenced::OwnedToRef}, | ||
ext::pkix::{AuthorityKeyIdentifier, ExtendedKeyUsage, SubjectAltName, name::GeneralName}, | ||
name::Name, | ||
serial_number::SerialNumber, | ||
spki::{EncodePublicKey, SubjectPublicKeyInfoOwned}, | ||
|
@@ -66,14 +67,20 @@ pub enum Error { | |
|
||
#[snafu(display("failed to parse AuthorityKeyIdentifier"))] | ||
ParseAuthorityKeyIdentifier { source: x509_cert::der::Error }, | ||
|
||
#[snafu(display("The subject alternative DNS name \"{dns_name}\" is not a Ia5String"))] | ||
SaDnsNameNotAIa5String { | ||
dns_name: String, | ||
source: x509_cert::der::Error, | ||
}, | ||
} | ||
|
||
/// Custom implementation of [`std::cmp::PartialEq`] because some inner types | ||
/// don't implement it. | ||
/// | ||
/// Note that this implementation is restritced to testing because there is a | ||
/// Note that this implementation is restricted to testing because there is a | ||
/// variant that is impossible to compare, and will cause a panic if it is | ||
/// attemped. | ||
/// attempted. | ||
#[cfg(test)] | ||
impl PartialEq for Error { | ||
fn eq(&self, other: &Self) -> bool { | ||
|
@@ -170,7 +177,7 @@ where | |
/// These parameters include: | ||
/// | ||
/// - a randomly generated serial number | ||
/// - a default validity of one hour (see [`DEFAULT_CA_VALIDITY_SECONDS`]) | ||
/// - a default validity of one hour (see [`DEFAULT_CA_VALIDITY`]) | ||
/// | ||
/// The CA contains the public half of the provided `signing_key` and is | ||
/// signed by the private half of said key. | ||
|
@@ -181,9 +188,8 @@ where | |
#[instrument(name = "create_certificate_authority", skip(signing_key_pair))] | ||
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. note: The new implementation drops these instrumentations, why? 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. I just didn't get to it, added it in e946556 |
||
pub fn new(signing_key_pair: S) -> Result<Self> { | ||
let serial_number = rand::random::<u64>(); | ||
let validity = Duration::from_secs(DEFAULT_CA_VALIDITY_SECONDS); | ||
|
||
Self::new_with(signing_key_pair, serial_number, validity) | ||
Self::new_with(signing_key_pair, serial_number, DEFAULT_CA_VALIDITY) | ||
} | ||
|
||
/// Creates a new CA certificate. | ||
|
@@ -200,8 +206,8 @@ where | |
// We don't allow customization of the CA subject by callers. Every CA | ||
// created by us should contain the same subject consisting a common set | ||
// of distinguished names (DNs). | ||
let subject = Name::from_str(ROOT_CA_SUBJECT).context(ParseSubjectSnafu { | ||
subject: ROOT_CA_SUBJECT, | ||
let subject = Name::from_str(SDP_ROOT_CA_SUBJECT).context(ParseSubjectSnafu { | ||
subject: SDP_ROOT_CA_SUBJECT, | ||
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. note: While we are at it, I think this should use 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. I think I tend to disagree here. My argumentation would be that stackable-certs doesn't know where the subject is coming from. It could be |
||
})?; | ||
|
||
let spki_pem = signing_key_pair | ||
|
@@ -267,15 +273,16 @@ where | |
/// authentication, because they include [`ID_KP_CLIENT_AUTH`] and | ||
/// [`ID_KP_SERVER_AUTH`] in the extended key usage extension. | ||
/// | ||
/// It is also possible to directly greate RSA or ECDSA-based leaf | ||
/// It is also possible to directly create RSA or ECDSA-based leaf | ||
/// certificates using [`CertificateAuthority::generate_rsa_leaf_certificate`] | ||
/// and [`CertificateAuthority::generate_ecdsa_leaf_certificate`]. | ||
#[instrument(skip(self, key_pair))] | ||
pub fn generate_leaf_certificate<T>( | ||
pub fn generate_leaf_certificate<'a, T>( | ||
&mut self, | ||
key_pair: T, | ||
name: &str, | ||
scope: &str, | ||
subject_alterative_dns_names: impl IntoIterator<Item = &'a str> + Debug, | ||
validity: Duration, | ||
) -> Result<CertificatePair<T>> | ||
where | ||
|
@@ -301,10 +308,6 @@ where | |
let spki = SubjectPublicKeyInfoOwned::from_pem(spki_pem.as_bytes()) | ||
.context(DecodeSpkiFromPemSnafu)?; | ||
|
||
// The leaf certificate can be used for WWW client and server | ||
// authentication. This is a base requirement for TLS certs. | ||
let eku = ExtendedKeyUsage(vec![ID_KP_CLIENT_AUTH, ID_KP_SERVER_AUTH]); | ||
|
||
let signer = self.certificate_pair.key_pair.signing_key(); | ||
let mut builder = CertificateBuilder::new( | ||
Profile::Leaf { | ||
|
@@ -325,9 +328,27 @@ where | |
) | ||
.context(CreateCertificateBuilderSnafu)?; | ||
|
||
// Again, add the extension created above. | ||
// The leaf certificate can be used for WWW client and server | ||
// authentication. This is a base requirement for TLS certs. | ||
builder | ||
.add_extension(&eku) | ||
.add_extension(&ExtendedKeyUsage(vec![ | ||
ID_KP_CLIENT_AUTH, | ||
ID_KP_SERVER_AUTH, | ||
])) | ||
.context(AddCertificateExtensionSnafu)?; | ||
|
||
let sans = subject_alterative_dns_names | ||
.into_iter() | ||
.map(|dns_name| { | ||
Ok(GeneralName::DnsName(Ia5String::new(dns_name).context( | ||
SaDnsNameNotAIa5StringSnafu { | ||
dns_name: dns_name.to_string(), | ||
}, | ||
)?)) | ||
}) | ||
.collect::<Result<Vec<_>, Error>>()?; | ||
builder | ||
.add_extension(&SubjectAltName(sans)) | ||
.context(AddCertificateExtensionSnafu)?; | ||
|
||
debug!("create and sign leaf certificate"); | ||
|
@@ -344,29 +365,31 @@ where | |
/// See [`CertificateAuthority::generate_leaf_certificate`] for more | ||
/// information. | ||
#[instrument(skip(self))] | ||
pub fn generate_rsa_leaf_certificate( | ||
pub fn generate_rsa_leaf_certificate<'a>( | ||
&mut self, | ||
name: &str, | ||
scope: &str, | ||
subject_alterative_dns_names: impl IntoIterator<Item = &'a str> + Debug, | ||
validity: Duration, | ||
) -> Result<CertificatePair<rsa::SigningKey>> { | ||
let key = rsa::SigningKey::new().context(GenerateRsaSigningKeySnafu)?; | ||
self.generate_leaf_certificate(key, name, scope, validity) | ||
self.generate_leaf_certificate(key, name, scope, subject_alterative_dns_names, validity) | ||
} | ||
|
||
/// Generates an ECDSAasync -based leaf certificate which is signed by this CA. | ||
/// | ||
/// See [`CertificateAuthority::generate_leaf_certificate`] for more | ||
/// information. | ||
#[instrument(skip(self))] | ||
pub fn generate_ecdsa_leaf_certificate( | ||
pub fn generate_ecdsa_leaf_certificate<'a>( | ||
&mut self, | ||
name: &str, | ||
scope: &str, | ||
subject_alterative_dns_names: impl IntoIterator<Item = &'a str> + Debug, | ||
validity: Duration, | ||
) -> Result<CertificatePair<ecdsa::SigningKey>> { | ||
let key = ecdsa::SigningKey::new().context(GenerateEcdsaSigningKeySnafu)?; | ||
self.generate_leaf_certificate(key, name, scope, validity) | ||
self.generate_leaf_certificate(key, name, scope, subject_alterative_dns_names, validity) | ||
} | ||
|
||
/// Create a [`CertificateAuthority`] from a Kubernetes [`Secret`]. | ||
|
@@ -443,6 +466,11 @@ where | |
|
||
Self::from_secret(secret, key_certificate, key_private_key) | ||
} | ||
|
||
/// Returns the ca certificate. | ||
pub fn ca_cert(&self) -> &Certificate { | ||
&self.certificate_pair.certificate | ||
} | ||
} | ||
|
||
impl CertificateAuthority<rsa::SigningKey> { | ||
|
@@ -468,19 +496,57 @@ fn format_leaf_certificate_subject(name: &str, scope: &str) -> Result<Name> { | |
|
||
#[cfg(test)] | ||
mod tests { | ||
use const_oid::ObjectIdentifier; | ||
|
||
use super::*; | ||
|
||
const TEST_CERT_LIFETIME: Duration = Duration::from_hours_unchecked(1); | ||
const TEST_SAN: &str = "airflow-0.airflow.default.svc.cluster.local"; | ||
|
||
#[tokio::test] | ||
async fn rsa_key_generation() { | ||
let mut ca = CertificateAuthority::new_rsa().unwrap(); | ||
ca.generate_rsa_leaf_certificate("Airflow", "pod", Duration::from_secs(3600)) | ||
.unwrap(); | ||
let cert = ca | ||
.generate_rsa_leaf_certificate("Airflow", "pod", [TEST_SAN], TEST_CERT_LIFETIME) | ||
.expect("RSA certificate generation failed"); | ||
|
||
assert_cert_attributes(cert.certificate()); | ||
} | ||
|
||
#[tokio::test] | ||
async fn ecdsa_key_generation() { | ||
let mut ca = CertificateAuthority::new_ecdsa().unwrap(); | ||
ca.generate_ecdsa_leaf_certificate("Airflow", "pod", Duration::from_secs(3600)) | ||
.unwrap(); | ||
let cert = ca | ||
.generate_ecdsa_leaf_certificate("Airflow", "pod", [TEST_SAN], TEST_CERT_LIFETIME) | ||
.expect("ecdsa certificate generation failed"); | ||
|
||
assert_cert_attributes(cert.certificate()); | ||
} | ||
|
||
fn assert_cert_attributes(cert: &Certificate) { | ||
let cert = &cert.tbs_certificate; | ||
// Test subject | ||
assert_eq!( | ||
cert.subject, | ||
Name::from_str("CN=Airflow Certificate for pod").unwrap() | ||
); | ||
|
||
// Test SAN extension is present | ||
let extensions = cert.extensions.as_ref().expect("cert had no extension"); | ||
assert!( | ||
extensions | ||
.iter() | ||
.any(|ext| ext.extn_id == ObjectIdentifier::new_unwrap("2.5.29.17")) | ||
); | ||
|
||
// Test lifetime | ||
let not_before = cert.validity.not_before.to_system_time(); | ||
let not_after = cert.validity.not_after.to_system_time(); | ||
assert_eq!( | ||
not_after | ||
.duration_since(not_before) | ||
.expect("Failed to calculate duration between notBefore and notAfter"), | ||
*TEST_CERT_LIFETIME | ||
); | ||
} | ||
} |
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.
suggestion: Rename the variant and adjust error message.
Uh oh!
There was an error while loading. Please reload this page.
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.
Changed to
in 8f86a93