Skip to content

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

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions crates/stackable-certs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,24 @@ All notable changes to this project will be documented in this file.

## [Unreleased]

### Added

- 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.
Pass an empty list (`[]`) to keep the existing behavior ([#1044]).
- 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` ([#1044]).
- Added the function `CertificateAuthority::ca_cert` to easily get the CA `Certificate` ([#1044]).

## [0.3.1] - 2024-07-10

### Changed

- Bump rust-toolchain to 1.79.0 ([#822]).

[#822]: https://github.com/stackabletech/operator-rs/pull/822
[#1044]: https://github.com/stackabletech/operator-rs/pull/1044

## [0.3.0] - 2024-05-08

Expand Down
6 changes: 4 additions & 2 deletions crates/stackable-certs/src/ca/consts.rs
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";
118 changes: 92 additions & 26 deletions crates/stackable-certs/src/ca/mod.rs
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;
Expand All @@ -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},
Expand Down Expand Up @@ -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,
},
Copy link
Member

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.

Suggested change
#[snafu(display("The subject alternative DNS name \"{dns_name}\" is not a Ia5String"))]
SaDnsNameNotAIa5String {
dns_name: String,
source: x509_cert::der::Error,
},
#[snafu(display("failed to parse subject alternative name {subject_alternative_name:?} as a Ia5 string"))]
ParseSubjectAlternativeName {
subject_alternative_name: String,
source: x509_cert::der::Error,
},

Copy link
Member Author

@sbernauer sbernauer May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to

    #[snafu(display(
        "failed to parse subject alternative DNS name \"{subject_alternative_dns_name}\" as a Ia5 string"
    ))]
    ParseSubjectAlternativeDnsName {
        subject_alternative_dns_name: String,
        source: x509_cert::der::Error,
    },

in 8f86a93

}

/// 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 {
Expand Down Expand Up @@ -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.
Expand All @@ -181,9 +188,8 @@ where
#[instrument(name = "create_certificate_authority", skip(signing_key_pair))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: The new implementation drops these instrumentations, why?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: While we are at it, I think this should use expect instead. This subject name must always pass.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 CN=<pod-name>, but maybe also a certificate we issue for users (user-provided subject?), so they can connect to e.g. mTLS Kafka.
Better safe than sorry I'd say

})?;

let spki_pem = signing_key_pair
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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");
Expand All @@ -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`].
Expand Down Expand Up @@ -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> {
Expand All @@ -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
);
}
}
9 changes: 6 additions & 3 deletions crates/stackable-webhook/src/tls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@ use hyper::{body::Incoming, service::service_fn};
use hyper_util::rt::{TokioExecutor, TokioIo};
use opentelemetry::trace::{FutureExt, SpanKind};
use snafu::{ResultExt, Snafu};
use stackable_certs::{CertificatePairError, ca::CertificateAuthority, keys::rsa};
use stackable_operator::time::Duration;
use stackable_certs::{
CertificatePairError,
ca::{CertificateAuthority, DEFAULT_CA_VALIDITY},
keys::rsa,
};
use tokio::net::TcpListener;
use tokio_rustls::{
TlsAcceptor,
Expand Down Expand Up @@ -106,7 +109,7 @@ impl TlsServer {
CertificateAuthority::new_rsa().context(CreateCertificateAuthoritySnafu)?;

let leaf_certificate = certificate_authority
.generate_rsa_leaf_certificate("Leaf", "webhook", Duration::from_secs(3600))
.generate_rsa_leaf_certificate("Leaf", "webhook", [], DEFAULT_CA_VALIDITY)
.context(GenerateLeafCertificateSnafu)?;

let certificate_der = leaf_certificate
Expand Down
Loading