Skip to content

Conversation

sbernauer
Copy link
Member

@sbernauer sbernauer commented Sep 8, 2025

Description

Part of stackabletech/issues#764

Example usage

cargo r -p cert-tools -- generate-pkcs12-truststore --pkcs12 ./truststore.p12 --pem _test.pem --out ./out.p12
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.07s
     Running `target/debug/truststore-merger --pkcs12 ./truststore.p12 --pem _test.pem --out ./out.p12`
 INFO Added certificate subject=[commonName = "secret-operator self-signed"] not_before=Sep  2 08:03:58 2025 GMT not_after=Sep  2 08:08:58 2026 GMT serial="A79EC2461C6D66B1" source=Pem("_test.pem")
 INFO Added certificate subject=[commonName = "secret-operator self-signed"] not_before=Sep  3 11:02:40 2025 GMT not_after=Sep  3 11:08:40 2025 GMT serial="0E4BC78F87304678" source=Pkcs12(Pkcs12Source { path: "./truststore.p12", password: "" })
 INFO Added certificate subject=[commonName = "secret-operator self-signed"] not_before=Sep  3 11:03:12 2025 GMT not_after=Sep  3 11:09:12 2025 GMT serial="DD7DBA422B3966F8" source=Pkcs12(Pkcs12Source { path: "./truststore.p12", password: "" })
 WARN Skipped certificate as a cert with the same SHA256 hash was already added source=Pkcs12(Pkcs12Source { path: "./truststore.p12", password: "" }) sha25="ee7da620f1c7341c210aafad0e369b579a4e6060b26dea6f05fd871c4b362f42" existing.not_before=Sep  2 08:03:58 2025 GMT existing.not_after=Sep  2 08:08:58 2026 GMT existing.subject=[commonName = "secret-operator self-signed"] existing.serial="A79EC2461C6D66B1" new.not_before=Sep  2 08:03:58 2025 GMT new.not_after=Sep  2 08:08:58 2026 GMT new.subject=[commonName = "secret-operator self-signed"] new.serial="A79EC2461C6D66B1"
 INFO Added certificate subject=[commonName = "secret-operator self-signed"] not_before=Sep  3 11:03:59 2025 GMT not_after=Sep  3 11:08:59 2026 GMT serial="8246453548B78E11" source=Pkcs12(Pkcs12Source { path: "./truststore.p12", password: "" })
 INFO Added certificate subject=[commonName = "secret-operator self-signed"] not_before=Sep  3 11:02:01 2025 GMT not_after=Sep  3 11:08:01 2025 GMT serial="2ADEC7FB80162856" source=Pkcs12(Pkcs12Source { path: "./truststore.p12", password: "" })

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.
  • Helm chart can be installed and deployed operator works
  • Integration tests passed (for non trivial changes)
  • Changes need to be "offline" compatible
  • Links to generated (nightly) docs added
  • Release note snippet added

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
  • Links to generated (nightly) docs added
  • Release note snippet added
  • Add type/deprecation label & add to the deprecation schedule
  • Add type/experimental label & add to the experimental features tracker

@sbernauer sbernauer closed this Sep 9, 2025
@sbernauer sbernauer reopened this Sep 9, 2025
@sbernauer sbernauer marked this pull request as ready for review September 9, 2025 12:24
@sbernauer sbernauer changed the title feat: Add truststore-merger binary feat: Add cert-tool binary Sep 11, 2025
@sbernauer sbernauer changed the title feat: Add cert-tool binary feat: Add cert-tools binary Sep 11, 2025
@sbernauer sbernauer self-assigned this Sep 11, 2025
@sbernauer sbernauer moved this to Development: Waiting for Review in Stackable Engineering Sep 11, 2025
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

Just a partial review while on mobile.
Will do a proper one tomorrow.

@NickLarsenNZ NickLarsenNZ moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Sep 12, 2025
NickLarsenNZ
NickLarsenNZ previously approved these changes Sep 12, 2025
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +6 to +7
const HEADER: &[u8] = b"-----BEGIN CERTIFICATE-----";
const FOOTER: &[u8] = b"-----END CERTIFICATE-----";
Copy link
Member

Choose a reason for hiding this comment

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

There are a few variations of the header/footer: See: https://github.com/openssl/openssl/blob/ea85fbce9fac7bf87c5c0a82151dbde259bbfc4f/include/openssl/pem.h#L35-L62

At least we might care about:

BEGIN CERTIFICATE
BEGIN RSA PUBLIC KEY
BEGIN ECDSA PUBLIC KEY

Same for END.

I left out legacy stuff like BEGIN DSA PUBLIC KEY.

Copy link
Member Author

Choose a reason for hiding this comment

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

note: I copied that from existing code.

Also, https://docs.rs/openssl/latest/openssl/x509/struct.X509.html#method.from_pem states:

The input should have a header of -----BEGIN CERTIFICATE-----.

As we stuff the result into that openssl function, this is definitely not ideal but worked so far fine.
The good thing is that the SDP has most PEM files under it's own control, only rarely users add certificates to it.

Are you ok with merging as-is and improving it in case it causes any trouble? But I only see the option of adding error handling, as the openssl function seems to require this specific header

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Development: In Review
Development

Successfully merging this pull request may close these issues.

3 participants