feat!(stackable-certs): Use builder pattern and support SAN entries #1044

sbernauer · 2025-05-21T11:54:11Z

Description

Needed for CRD conversions, as the apiserver needs to trust our cert

Current example code

let ca = CertificateAuthority::builder_with_ecdsa()
    .build()
    .expect("failed to build CA");

let certificate = CertificateBuilder::builder()
    .subject("CN=trino-coordinator-default-0")
    .signed_by(&ca)
    .build()
    .expect("failed to build certificate");

Definition of Done Checklist

Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
Please make sure all these things are done and tick the boxes

# Author
- [ ] Changes are OpenShift compatible
- [ ] CRD changes approved
- [ ] Integration tests passed (for non trivial changes)

# Reviewer
- [ ] Code contains useful comments
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated
- [ ] Changelog updated
- [ ] Cargo.toml only contains references to git tags (not specific commits or branches)

# Acceptance
- [ ] Feature Tracker has been updated
- [ ] Proper release label has been added

NickLarsenNZ

Just commenting on a few things. Mostly thoughts.

crates/stackable-certs/src/ca/ca_builder.rs

NickLarsenNZ · 2025-05-23T09:52:03Z

crates/stackable-certs/src/ca/ca_builder.rs

+///     .build()
+///     .build_ca()


This feels a little awkward to me. ~~I'm not (yet) sure what the two build methods do differently?~~

It looks like the build() method still returns a builder, and build_ca() self-signs and returns a CertificateAuthority.

Point is, I don't find it intuitive and I think it should be.

I worked it out below, and recommended renaming the struct.

NickLarsenNZ · 2025-05-23T10:14:11Z

crates/stackable-certs/src/ca/ca_builder.rs

+///     .expect("failed to build CA");
+/// ```
+#[derive(Builder)]
+pub struct CertificateAuthorityBuilder<'a, SKP>


I think this should be:

Suggested change

pub struct CertificateAuthorityBuilder<'a, SKP>

pub struct CertificateAuthority<'a, SKP>

So when we call build(), we get back a CA and not a CA Builder.

Note, the Builder macro would otherwise generate CertificateAuthorityBuilderBuilder (ie: it generates the Builder, not us).

To illustrate further:

CertificateAuthority::builder() -> CertificateAuthorityBuilder

CertificateAuthorityBuilder::build() -> a valid CertificateAuthority

Oh, but we already have a struct: CertificateAuthority<SK> at

operator-rs/crates/stackable-certs/src/ca/mod.rs

Line 19 in 88d850d

pub struct CertificateAuthority<SK>

in my mind, that one should be the one that derives Builder. Otherwise we need to revise what each struct is, because the current CertificateAuthorityBuilder is not a builder (per previous message above).

crates/stackable-certs/src/ca/ca_builder.rs

Techassi · 2025-05-22T14:33:04Z

crates/stackable-certs/src/ca/mod.rs

+        let subject = Name::from_str(SDP_ROOT_CA_SUBJECT).context(ParseSubjectSnafu {
+            subject: SDP_ROOT_CA_SUBJECT,


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

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

Techassi · 2025-05-22T14:35:36Z

crates/stackable-certs/src/ca/mod.rs

+    #[snafu(display("The subject alternative DNS name \"{dns_name}\" is not a Ia5String"))]
+    SaDnsNameNotAIa5String {
+        dns_name: String,
+        source: x509_cert::der::Error,
+    },


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,

},

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

Techassi · 2025-05-23T11:09:18Z

crates/stackable-certs/src/ca/ca_builder.rs

+    /// Serial number of the generated certificate.
+    ///
+    /// If not specified a random serial will be generated.
+    serial_number: Option<u64>,


note: This is an internal detail we shouldn't allow being customized by the user of this library. I'm sure there is something like

Suggested change

/// Serial number of the generated certificate.

///

/// If not specified a random serial will be generated.

serial_number: Option<u64>,

/// Serial number of the generated certificate.

#[builder(skip)]

serial_number: Option<u64>,

Agreed. On that topic, if we want to have proper CAs... then serial should increment.
So, if we restore a CA (eg: on startup, read from a secret to get the CA keypair), we also need information for the certs that have been issued (not by reading available signed certs, since they might be gone already). We basically need an atomic counter.

I'm not 100% certain of the downside if we just always issue the same version though.

I copied the builder to mimic the current api, in which users can specify the serial:

operator-rs/crates/stackable-certs/src/ca/mod.rs

Line 196 in e0309b9

pub fn new_with(signing_key_pair: S, serial_number: u64, validity: Duration) -> Result<Self> {

secret-op generates random serials for cas and certs (https://github.com/stackabletech/secret-operator/blob/00909cfb92e61539a4baab9190235cd33b33a61b/rust/operator-binary/src/backend/tls/ca.rs#L258 and https://github.com/stackabletech/secret-operator/blob/00909cfb92e61539a4baab9190235cd33b33a61b/rust/operator-binary/src/backend/tls/mod.rs#L306), this seems to be fine.
I will update the PR to not let users choose the serial

Techassi · 2025-05-23T11:11:42Z

crates/stackable-certs/src/ca/ca_builder.rs

+    /// Cryptographic keypair used to sign leaf certificates.
+    ///
+    /// If not specified a random keypair will be generated.
+    signing_key_pair: Option<SKP>,


note: I would like to see the same high-level functions to create a RSA or ECDSA based CA.

Added in fd19a03

Techassi · 2025-05-23T11:19:06Z

crates/stackable-certs/src/ca/mod.rs

-    /// Creates a new CA certificate with many parameters set to their default
-    /// values.
-    ///
-    /// These parameters include:
-    ///
-    /// - a randomly generated serial number
-    /// - a default validity of one hour (see [`DEFAULT_CA_VALIDITY_SECONDS`])
-    ///
-    /// The CA contains the public half of the provided `signing_key` and is
-    /// signed by the private half of said key.
-    ///
-    /// If the default values for the serial number and validity don't satisfy
-    /// the requirements of the caller, use [`CertificateAuthority::new_with`]
-    /// instead.


note: Compared to these docs, the current docs lack in depth and technical explanations. I've spent a significant amount of time on these and would like to see them carried over to the new implementation as complete as possible.

I tried carrying over everything that is still relevant: 1322779

Techassi · 2025-05-23T11:21:48Z

crates/stackable-certs/src/cert_builder.rs

+    /// Serial number of the generated certificate.
+    ///
+    /// If not specified a random serial will be generated.
+    serial_number: Option<u64>,


note: Again, I would like this to be an internal details not exposed to the user.

Same as #1044 (comment)

Techassi · 2025-05-23T11:22:41Z

crates/stackable-certs/src/cert_builder.rs

+    /// Mandatorily sign the certificate using the provided [`CertificateAuthority`].
+    signed_by: &'a CertificateAuthority<KP>,


note: This should be part of the .build() method on the CertificateBuilder.

Techassi · 2025-05-23T11:24:59Z

crates/stackable-certs/src/cert_builder.rs

+        let ca_validity = self.signed_by.ca_cert().tbs_certificate.validity;
+        let ca_not_after = ca_validity.not_after.to_system_time();
+        let cert_not_after = validity.not_after.to_system_time();
+        if ca_not_after < cert_not_after {
+            warn!(
+                ca.validity = ?ca_validity,
+                cert.validity = ?validity,
+                ca.not_after = ?ca_not_after,
+                cert.not_after = ?cert_not_after,
+                subject = ?subject,
+                "The lifetime of certificate authority is shorted than the lifetime of the generated certificate",
+            );
+        }


praise: Thank you for adding this.

note: This should be a hard error instead.

Techassi · 2025-05-23T11:25:51Z

crates/stackable-certs/src/cert_builder.rs

+    /// Optional list of subject alternative names (SAN) DNS entries,
+    /// that are added to the certificate.
+    #[builder(default)]
+    subject_alterative_dns_names: &'a [&'a str],


suggestion: I would just go with

Suggested change

/// Optional list of subject alternative names (SAN) DNS entries,

/// that are added to the certificate.

#[builder(default)]

subject_alterative_dns_names: &'a [&'a str],

/// Optional list of subject alternative names (SAN) DNS entries,

/// that are added to the certificate.

#[builder(default)]

subject_alterative_names: &'a [&'a str],

Also, there is no value in carrying this as &str. They need to be converted into owned values below anyway. The API should reflect that to not hide (possibly) unexpected cloning/allocation.

Do we want to allow for more things, like IP's, email addresses, etc... (just so we avoid premature restrictions)?

Secret-op already does IP SANs, so yes.

Oh I misread, I though @nightkr requested to support IPs... I added support for that in 8f86a93 🙈

But as this is a builder I prefer to have a function to add DNs names and one to add IP addresses instead of passing a list of enums (which we first need to introduce).

Regarding the allocation, I see the point that this allocates. On the other hand I prefer the simple api, so that not every caller needs to deal with der::asn1::ia5_string::allocation::Ia5String

Oh I misread, I though @nightkr requested to support IPs... I added support for that in 8f86a93 🙈

I was. (At least from the perspective of "if we want to use it for secret-op at some point".)

Techassi · 2025-05-23T11:28:48Z

crates/stackable-certs/src/cert_builder.rs

+            .map(|dns_name| {
+                Ok(GeneralName::DnsName(Ia5String::new(dns_name).context(
+                    SaDnsNameNotAIa5StringSnafu {
+                        dns_name: dns_name.to_string(),
+                    },
+                )?))
+            })


note: Please flatten this a little bit. I'm not a huge fan of these nested code snippets.

The best I could come up was changing

let san_dns = self.subject_alterative_dns_names.iter().map(|dns_name| { Ok(GeneralName::DnsName( Ia5String::new(dns_name).with_context(|_| ParseSubjectAlternativeDnsNameSnafu { subject_alternative_dns_name: dns_name.to_string(), })?, )) });

to

let san_dns = self.subject_alterative_dns_names.iter().map( |dns_name| -> Result<_, CreateCertificateError<KP::Error>> { Ia5String::new(dns_name) .with_context(|_| ParseSubjectAlternativeDnsNameSnafu { subject_alternative_dns_name: dns_name.to_string(), }) .map(GeneralName::DnsName) }, );

Not sure if that really improves the situation

feat!(stackable-certs): Support adding SAN entries

651c78a

sbernauer self-assigned this May 21, 2025

changelog

6b79029

sbernauer added this to Stackable Engineering May 21, 2025

sbernauer moved this to Development: Waiting for Review in Stackable Engineering May 21, 2025

sbernauer mentioned this pull request May 21, 2025

tracking: CRD versioning stackabletech/issues#642

Open

80 tasks

sbernauer added 3 commits May 23, 2025 09:50

Rewrite CA and cert generation to use builder pattern

88d850d

changelog

3fd41d7

misc cleanup

d3e7643

sbernauer changed the title ~~feat!(stackable-certs): Support adding SAN entries~~ feat!(stackable-certs): Use builder pattern and support SAN entries May 23, 2025

sbernauer added 2 commits May 23, 2025 10:13

warn when cert lifes longer than CA

70dc741

Merge remote-tracking branch 'origin/main' into feat/support-sans

08c6bc1

sbernauer requested review from Techassi and NickLarsenNZ May 23, 2025 08:16

NickLarsenNZ reviewed May 23, 2025

View reviewed changes

Offer convenience functions

b8c667b

Techassi requested changes May 23, 2025

View reviewed changes

sbernauer added 2 commits May 23, 2025 14:39

Add builder() convenience function

07fcc89

Rename the convenience function ro build()

4c84aea

sbernauer moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering May 26, 2025

sbernauer added 8 commits May 26, 2025 16:12

Simply tests

ac18bd9

Support SAN IPs

8f86a93

Rename inner builder() to start_builder()

3995717

Dont let users choose serial

e9e8bac

Add some instrumentation

e946556

Cary over some docs

1322779

Add builder_with_rsa and builder_with_ecdsa helpers

fd19a03

Make cert outlives ca a hard error

3d3e0c1

sbernauer requested a review from Techassi May 27, 2025 12:01

clippy

5cf7c64

sbernauer added 2 commits May 27, 2025 14:14

Merge remote-tracking branch 'origin/main' into feat/support-sans

c955ae6

new fmt

c263ae7

	pub struct CertificateAuthorityBuilder<'a, SKP>
	pub struct CertificateAuthority<'a, SKP>

		let subject = Name::from_str(SDP_ROOT_CA_SUBJECT).context(ParseSubjectSnafu {
		subject: SDP_ROOT_CA_SUBJECT,

		/// Mandatorily sign the certificate using the provided [`CertificateAuthority`].
		signed_by: &'a CertificateAuthority<KP>,

Uh oh!

feat!(stackable-certs): Use builder pattern and support SAN entries #1044

Are you sure you want to change the base?

feat!(stackable-certs): Use builder pattern and support SAN entries #1044

Uh oh!

Conversation

sbernauer commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Definition of Done Checklist

Uh oh!

NickLarsenNZ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NickLarsenNZ May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickLarsenNZ May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbernauer May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickLarsenNZ May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbernauer commented May 21, 2025 •

edited

Loading

NickLarsenNZ May 23, 2025 •

edited

Loading

NickLarsenNZ May 23, 2025 •

edited

Loading

sbernauer May 26, 2025 •

edited

Loading

NickLarsenNZ May 23, 2025 •

edited

Loading