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

sbernauer · 2025-06-17T13:07:57Z

Description

As #1044 didn't move along, splitting the relevant change out to unblock CRD versioning.

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
CRD documentation for all fields, following the style guide.
Integration tests passed (for non trivial changes)
Changes need to be "offline" compatible

Reviewer

Code contains useful comments
Code contains useful logging statements
(Integration-)Test cases added
Documentation added or updated. Follows the style guide.
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 some changes to the changelog, and the rest (I think) are only suggestions.

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

crates/stackable-certs/CHANGELOG.md

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

Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com>

NickLarsenNZ

LGTM

sbernauer · 2025-06-25T07:01:06Z

Thanks for the review!

Techassi

I didn't take a close look at this, but I will soon.

Blocking it for now.

Techassi · 2025-06-25T08:11:51Z

crates/stackable-certs/CHANGELOG.md

+- 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 when the HTTPS server is accessible on multiple DNS names and/or IPs.
+  Pass an empty list (`[]`) to keep the existing behavior ([#1057]).


note: This is a change, not an addition. As such, this should live in the "Changed" section.

suggestion: Slight reword.

Suggested change

- 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 when the HTTPS server is accessible on multiple DNS names and/or IPs.

Pass an empty list (`[]`) to keep the existing behavior ([#1057]).

- BREAKING: The functions `generate_leaf_certificate`, `generate_rsa_leaf_certificate` and

`generate_ecdsa_leaf_certificate` of `CertificateAuthority` accept an additional parameter

`subject_alterative_dns_names` ([#1057]).

- The passed SANs are added to the generated certificate, this is needed when the HTTPS server is

accessible on multiple DNS names and/or IPs.

- Pass an empty list (`[]`) to keep the existing behavior.

Techassi · 2025-06-25T08:12:27Z

crates/stackable-certs/CHANGELOG.md

+- 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`.


note: This also a change and should be moved.

Techassi · 2025-06-25T08:13:01Z

crates/stackable-certs/CHANGELOG.md

+- 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`.
+- Added the function `CertificateAuthority::ca_cert` to easily get the CA `Certificate` ([#1057]).


suggestion: Slight reword.

Suggested change

- Added the function `CertificateAuthority::ca_cert` to easily get the CA `Certificate` ([#1057]).

- Add the function `CertificateAuthority::ca_cert` to easily get the CA `Certificate` ([#1057]).

Techassi · 2025-06-25T08:13:29Z

crates/stackable-certs/CHANGELOG.md

 ## [0.3.1] - 2024-07-10

 ### Changed

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

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


note: This link reference is in the wrong place.

Techassi · 2025-06-25T08:14:13Z

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

+    #[snafu(display(
+        "failed to parse subject alternative DNS name \"{subject_alternative_dns_name}\" as a Ia5 string"
+    ))]


note: Get rid of the escaped quotes.

Suggested change

#[snafu(display(

"failed to parse subject alternative DNS name \"{subject_alternative_dns_name}\" as a Ia5 string"

))]

#[snafu(display(

"failed to parse subject alternative DNS name {subject_alternative_dns_name:?} as a Ia5 string"

))]

Techassi · 2025-06-25T08:17:03Z

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

    use super::*;

+    const TEST_CERT_LIFETIME: Duration = Duration::from_hours_unchecked(1);
+    const TEST_SAN: &str = "airflow-0.airflow.default.svc.cluster.local";


question: Can we use a non-product specific test value here? This comes to mind:

Suggested change

const TEST_SAN: &str = "airflow-0.airflow.default.svc.cluster.local";

const TEST_SAN: &str = "example-0.example.default.svc.cluster.local";

Techassi · 2025-06-25T08:18:15Z

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

-        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)


note: In a similar fashion, we should replace "Airflow" with "Example" here as well.

Techassi · 2025-06-25T08:19:05Z

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

-            .unwrap();
+        let cert = ca
+            .generate_rsa_leaf_certificate("Airflow", "pod", [TEST_SAN], TEST_CERT_LIFETIME)
+            .expect("RSA certificate generation failed");


praise: Nicely done!

note: The message should instead read why this will never fail.

Techassi · 2025-06-25T08:20:50Z

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

+        );
+
+        // Test SAN extension is present
+        let extensions = cert.extensions.as_ref().expect("cert had no extension");


note: This should state why it should never fail. Something like this comes to mind:

Suggested change

let extensions = cert.extensions.as_ref().expect("cert had no extension");

let extensions = cert.extensions.as_ref().expect("cert must have extensions");

Techassi · 2025-06-25T08:21:46Z

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

+        assert_eq!(
+            not_after
+                .duration_since(not_before)
+                .expect("Failed to calculate duration between notBefore and notAfter"),


note: This should state why it should never fail.

sbernauer and others added 6 commits June 17, 2025 15:03

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

42ead5b

changelog

45a6a1b

changelog

2b17859

Improve error

d4d2d0d

changelog

e2901ab

Merge branch 'main' into feat/san-2

3880bd8

sbernauer added this to Stackable Engineering Jun 23, 2025

sbernauer moved this to Development: Waiting for Review in Stackable Engineering Jun 23, 2025

sbernauer requested review from NickLarsenNZ and Techassi June 23, 2025 08:26

NickLarsenNZ requested changes Jun 23, 2025

View reviewed changes

sbernauer and others added 3 commits June 24, 2025 09:20

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

67cba18

Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com>

Update crates/stackable-certs/CHANGELOG.md

4dc617f

Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com>

Update crates/stackable-certs/CHANGELOG.md

b5faf6d

Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com>

sbernauer requested a review from NickLarsenNZ June 24, 2025 07:26

NickLarsenNZ previously approved these changes Jun 25, 2025

View reviewed changes

sbernauer enabled auto-merge June 25, 2025 07:01

sbernauer disabled auto-merge June 25, 2025 07:01

Techassi requested changes Jun 25, 2025

View reviewed changes

lets try different formatting

a58677c

sbernauer dismissed NickLarsenNZ’s stale review via a58677c June 25, 2025 08:01

Merge branch 'main' into feat/san-2

e8b47b2

Techassi requested changes Jun 25, 2025

View reviewed changes

Techassi moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Jun 25, 2025

	- Added the function `CertificateAuthority::ca_cert` to easily get the CA `Certificate` ([#1057]).
	- Add the function `CertificateAuthority::ca_cert` to easily get the CA `Certificate` ([#1057]).

	const TEST_SAN: &str = "airflow-0.airflow.default.svc.cluster.local";
	const TEST_SAN: &str = "example-0.example.default.svc.cluster.local";

	let extensions = cert.extensions.as_ref().expect("cert had no extension");
	let extensions = cert.extensions.as_ref().expect("cert must have extensions");

Uh oh!

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

Are you sure you want to change the base?

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

Conversation

sbernauer commented Jun 17, 2025

Description

Definition of Done Checklist

Author

Reviewer

Acceptance

Uh oh!

NickLarsenNZ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NickLarsenNZ left a comment

Choose a reason for hiding this comment

Uh oh!

sbernauer commented Jun 25, 2025

Uh oh!

Techassi left a comment

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!

Uh oh!