feat: add PKCS#11/HSM support for Fulcio and CTLog signing - #2128
feat: add PKCS#11/HSM support for Fulcio and CTLog signing#2128sampras343 wants to merge 2 commits into
Conversation
sampras343
changed the title
sampras343
changed the title
PR Summary by Qodofeat: PKCS#11/HSM support for CTLog signing (SECURESIGN-5021)
AI Description
Diagram
High-Level Assessment
Files changed (28)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2128 +/- ##
==========================================
- Coverage 57.13% 55.94% -1.19%
==========================================
Files 284 286 +2
Lines 15991 16735 +744
==========================================
+ Hits 9136 9362 +226
- Misses 5907 6389 +482
- Partials 948 984 +36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review by Qodo
1.
|
![qodo-for-securesign[bot]](https://avatars.githubusercontent.com/in/3596402?s=60&v=4){kind=small}
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 8bb1ae8 |
acd89d3 to
0447898
Compare
0447898 to
b404dbf
Compare
8d68073 to
053afe7
Compare
osmman
left a comment
osmman
left a comment
There was a problem hiding this comment.
Request changes — API isn't released yet, let's fix the shape now.
-
Reuse
Authinstead ofCredentialsRef/ServerEnv. We already have this pattern (Rekor/TSA/Trillian) — addAuthonFulcioCert, dropCredentialsRef/ServerEnv. Exception: CTLog'sPinSecretRefstays, since the operator reads that value itself to build ct_server's config. Also: don't auto-inject the PIN into every init container — most don't need it, let users add it explicitly where they do. -
Move
InitContainers/Volumes/ServerVolumeMountsout ofpkcs11config toFulcioSpec/CTlogSpectop level. These are pod-wide regardless of nesting, and not PKCS#11-specific. RenameServerVolumeMounts→VolumeMountswhile moving it (no longer needs the "Server" qualifier once it's not sitting next to per-init-container mounts). -
Drop the "at least one initContainer required for pkcs11" CEL rule. It assumes one specific way of provisioning the HSM library — other valid approaches exist (CSI driver, mutating webhook) that wouldn't populate
initContainersat all. Let pod startup failure be the real signal if provisioning didn't work. -
Drop
CTlogPKCS11Status/FulcioPKCS11Statusentirely. Every field is a required spec value copied verbatim — the operator never generates or modifies any of them, so they don't belong in status. TheirdriftDetected/hasPKCS11ConfigDriftchecks are redundant too (ObservedGenerationalready covers it). -
(Discuss) Consider
spec.signershape, matching Rekor/TSA. Fulcio usescertificate, CTLog uses flat fields — worth converging while unreleased. Bigger diff than 1-4, especially for Fulcio — fine as a fast-follow if too much for this PR.
Happy to go through any of these in more detail — the reasoning behind them.
053afe7 to
f65b798
Compare
Enable the operator to deploy Fulcio and CTLog with HSM-backed signing keys via PKCS#11, using a vendor-agnostic init container plugin model. Integrates cleanly into the spec.signer hierarchy. Fulcio PKCS#11 (spec.fulcio.signer): - type: pkcs11 on FulcioSigner with CEL mutual-exclusion rules - FulcioPKCS11Config: configRef (crypto11.conf) + keyConfig (HSM key ID) - signer.auth (Auth struct) for server container env vars - signer.certificateChain.certificateChainRef for pre-provisioned root CA - Top-level initContainers/volumes/volumeMounts on FulcioSpec CTLog PKCS#11 (spec.ctlog.signer): - type: pkcs11 on CTlogSigner with CEL mutual-exclusion rules - CTlogPKCS11Config: pinSecretRef, publicKeyRef, tokenLabel, modulePath - signer.auth for server container env vars - Config-driven dispatch: keyspb.PKCS11Config in protobuf config - Top-level initContainers/volumes/volumeMounts on CTlogSpec - Status.PublicKeyRef populated for trust material resolution Design decisions (per review feedback): - Reuse Auth struct instead of custom CredentialsRef/ServerEnv - Move initContainers/volumes/volumeMounts to top-level spec (pod-wide) - Drop PKCS11 status structs and drift detection (ObservedGeneration) - Rename configRef/modulePath (drop redundant PKCS11 prefix) - Drop initContainers CEL rule (allow CSI/webhook provisioning) - Stop auto-injecting HSM_PIN into init containers - Auth on Signer (uniform for Fulcio and CTLog) - Call Signer.SetDefaults() from SecureSign.SetDefaults() so signer.type: file is explicit on the parent CR Shared: - PKCS11InitContainerSpec: curated corev1.Container subset - v1alpha1 conversion preserves PKCS#11 fields via MarshalData E2E validated on OCP 4.22: dual PKCS#11 cosign sign+verify PASS, file mode regression PASS. Jira: SECURESIGN-5014 Signed-off-by: Sachin Sampras M <sacm@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
f65b798 to
71a6670
Compare
sampras343
left a comment
sampras343
left a comment
There was a problem hiding this comment.
All five points addressed in the latest push (71a6670a). Here is how each was resolved:
1. Reuse Auth instead of CredentialsRef/ServerEnv
Done. CredentialsRef and ServerEnv removed from both FulcioPKCS11Config and CTlogPKCS11Config. Auth *Auth added on FulcioSigner and CTlogSigner (uniform placement on both — uses the existing Auth{Env, SecretMount} type from common.go). CTLog PinSecretRef stays as requested — operator reads it server-side to embed in the protobuf config. HSM_PIN auto-injection into init containers removed (HSMPinEnvVar constant deleted) — users set it explicitly in initContainers[].env if their vendor image needs it. Auth env vars injected idempotently using FindEnvByNameOrCreate to prevent duplicate env vars on reconciliation.
2. Move InitContainers/Volumes/ServerVolumeMounts to top-level
Done. All three fields moved from FulcioPKCS11Config/CTlogPKCS11Config to FulcioSpec/CTlogSpec. ServerVolumeMounts renamed to VolumeMounts. Not placed on PodRequirements — only Fulcio/CTLog get these fields. Controller code updated to read from instance.Spec.InitContainers/instance.Spec.Volumes/instance.Spec.VolumeMounts instead of pkcs11Config.*.
3. Drop the initContainers CEL rule
Done. The has(self.initContainers) && size(self.initContainers) > 0 rule removed from CTlogPKCS11Config. The initContainers field changed back from required to optional. Vault Agent Injector, CSI drivers, and cert-manager can now provision the PKCS#11 library without declaring init containers in the CR.
4. Drop CTlogPKCS11Status/FulcioPKCS11Status
Done. Both status structs deleted. Status.PKCS11 field removed from CTlogStatus and FulcioStatus. hasPKCS11ConfigDrift/driftDetected functions deleted — ObservedGeneration check in CanHandle handles all spec changes. Status.PublicKeyRef (on CTlogStatus directly, not on the deleted PKCS11 sub-struct) retained for trust material resolution. All conversion code updated — Status.PKCS11 restoration lines removed.
5. spec.signer shape
Done in this PR (not deferred). Both Fulcio and CTLog now use the spec.signer hierarchy introduced by the signer restructure PRs (#2155, #2156). PKCS#11 config is at signer.pkcs11, type selector is signer.type: pkcs11. Additionally, SecureSign.SetDefaults() now calls Signer.SetDefaults() for both Fulcio and CTLog, so signer.type: file is explicit on the parent CR — CEL rules do not need !has(self.type) guards.
Also fixed from Qodo review:
- Hardcoded CTLog prefix in
CreateCtlogPKCS11Config→ now acceptslogPrefixparameter - Field renames:
pkcs11ConfigRef→configRef,pkcs11ModulePath→modulePath(already scoped underpkcs11:)
|
/retest |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 71a6670 |
54c3f35 to
eadd047
Compare
|
/retest |
eadd047 to
3dba406
Compare
|
Qodo findings #5 and #6 addressed in 3dba406: #5. CTLog auth misplaced in sample CR — Fixed. The sample CR had #6. Config key not validated — Fixed. Replaced |
- CTLog PIN content hashing for rotation detection - ObservedGeneration on all PKCS11 error conditions - Fulcio hsm-lib ReadOnly, rotation handling - Auth SecretMount via ensure.ContainerAuth - CTLog persistence idempotency - modulePath CRD validation (.so pattern) Signed-off-by: Sachin Sampras M <sampras343@gmail.com>
3dba406 to
a9a8e53
Compare
osmman
left a comment
osmman
left a comment
There was a problem hiding this comment.
Summary
API-redesign feedback is fully addressed. Remaining issues inline — headlines:
Blocking:
- PKCS#11 "rotation" fires on any spec change, not just PKCS#11 fields — spurious config regen + pod restarts (Fulcio + CTLog).
InitContainers/Volumes/VolumeMounts/Authare silent no-ops outsidetype: pkcs11, despite being moved to spec top level to be signer-agnostic.PersistenceonCTlogPKCS11Configdoesn't provision a PVC and silently overwrites a user-definedhsm-tokensvolume every reconcile — conflicts with the newVolumesmechanism this PR introduced.- No CTLog unit tests, no controller/e2e coverage — would have caught the bugs above.
Worth fixing: fragile != pkcs11 check in generate_signer.go; duplicated/drifting helpers between Fulcio and CTLog; roundtrip fuzzer skips testing the restore path it already has.
Recommendation: split this PR
Too large (~15k lines) to review the PKCS#11 logic as deeply as it needs. Please split into:
- Generic
InitContainers/Volumes/VolumeMounts/Authextension, signer-agnostic. - PKCS#11 signer support on top of (1).
Would also make proper test coverage realistic for the PKCS#11 piece.
| return true | ||
| } | ||
| // Fire if CR generation changed (e.g., tokenLabel updated) | ||
| if cond.ObservedGeneration != instance.GetGeneration() { |
There was a problem hiding this comment.
PKCS#11 "rotation" fires on any spec change, not just PKCS#11 field changes. CanHandle() returns true whenever ObservedGeneration != Generation — true for any spec edit (resources, TLS, monitoring, etc.), not just a PKCS#11 field change. Combined with Handle() unconditionally calling handleRotation() once PKCS11Condition is True, any unrelated spec bump on a PKCS#11-mode CTlog regenerates the server config Secret (new GenerateName) and restarts the pod. Consider comparing the actual PKCS#11 fields (tokenLabel, modulePath, configRef, etc.) against a stored/observed value before triggering rotation. Same gap in fulcio/actions/ensure_pkcs11_config.go.
| if cond == nil { | ||
| return true | ||
| } | ||
| if cond.ObservedGeneration != instance.GetGeneration() { |
There was a problem hiding this comment.
Same gap as CTLog's ensure_pkcs11_config.go: this only checks ObservedGeneration != Generation, which fires on any spec change, not specifically a PKCS#11 field change. Handle() then calls handleRotation() whenever PKCS11Condition is already True — so any unrelated Fulcio spec edit regenerates the server config Secret and restarts the pod.
| // or omitted entirely if the custom Fulcio image bundles the vendor SDK). | ||
| if !hasVolume(&template.Spec, HSMTokensVolumeName) { | ||
| hsmTokensVol := kubernetes.FindVolumeByNameOrCreate(&template.Spec, HSMTokensVolumeName) | ||
| hsmTokensVol.EmptyDir = &core.EmptyDirVolumeSource{} |
There was a problem hiding this comment.
Drift vs. CTLog's equivalent path. Unlike ctlog/actions/deployment.go (which resets tokensVol.VolumeSource = core.VolumeSource{} before assigning EmptyDir/PVC), this sets EmptyDir/PersistentVolumeClaim directly without clearing the existing VolumeSource first — if a volume switches source type across reconciles, stale fields could linger. Also: hasVolume/hasMountPath/ensureVolumeDefaultMode (lines 538-568 below) are copy-pasted verbatim from the CTLog package; consider moving them into internal/utils/kubernetes next to FindVolumeByNameOrCreate so both stay in sync.
| } | ||
|
|
||
| // Validate required refs | ||
| if p.PinSecretRef == nil { |
There was a problem hiding this comment.
The PinSecretRef nil-check+error (61-77) and PublicKeyRef nil-check+error (79-95) blocks are near-identical, as are the two GetSecretData validation blocks below (98-134). Worth collapsing into a small validateSecretRef(...)-style helper to cut the duplication.
| "/var/run/fulcio-secrets/cert.pem", | ||
| fmt.Sprintf("--ct-log-url=%s", ctlogUrl), | ||
| } | ||
| func (i deployAction) ensureFileCADeployment(instance *rhtasv1.Fulcio, sa string, labels map[string]string, dp *v1.Deployment) error { |
There was a problem hiding this comment.
ensureFileCADeployment and ensurePKCS11Deployment (line 291) both re-derive the same Replicas/Selector/template labels/ServiceAccountName/AutomountServiceAccountToken, http/grpc/monitoring port setup, and the fulcio-config/oidc-info volume+mount wiring. Consider extracting the shared pod scaffolding into a helper (similar to the existing setProbes) so the two paths can't silently diverge.
| } | ||
| } | ||
|
|
||
| func (i deployAction) ensurePKCS11Deployment(instance *rhtasv1.CTlog, template *core.PodTemplateSpec, container *core.Container) { |
There was a problem hiding this comment.
I'd prefer removing the //nolint:actionlint suppressions below rather than keeping them. InitContainers/Volumes/VolumeMounts/Auth are cross-cutting pod concerns, not CTLog-specific — internal/utils/kubernetes/ensure already exists for exactly this (see ensure.ContainerAuth, used two lines below). Adding ensure.InitContainers(...)/ensure.Volumes(...)/ensure.VolumeMounts(...) there, alongside ContainerAuth, and calling them from both Fulcio and CTLog would remove the duplication flagged above and the nolint noise as a side effect (free functions in ensure aren't methods on an action-type receiver, so the analyzer never looks at them).
| AlignStatus: alignStatus, | ||
| IsEnabled: func(i *rhtasv1.CTlog) bool { | ||
| // PKCS#11 mode manages keys on the HSM — no file-based signer secret needed. | ||
| return i.Spec.Signer.Type != rhtasv1.CTlogSignerTypePKCS11 |
There was a problem hiding this comment.
Negative check on signer type is fragile for future signer types. IsEnabled returns true whenever Type != CTlogSignerTypePKCS11 — today that's equivalent to Type == CTlogSignerTypeFile since those are the only two types, but it silently breaks the moment a third type (e.g. kms) is added: this would then generate/manage a file-based private key secret for a KMS-backed signer too, which is wrong. Please check Type == rhtasv1.CTlogSignerTypeFile instead, so unknown/future types fail closed (disabled) rather than fail open (enabled). Fulcio's generate_signer.go avoids this by not needing an IsEnabled gate at all and checking == FulcioSignerTypePKCS11 positively wherever it branches — worth mirroring that style here.
|
|
||
| // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch | ||
|
|
||
| func NewEnsurePKCS11ConfigAction() action.Action[*rhtasv1.CTlog] { |
There was a problem hiding this comment.
Missing test coverage: no unit tests for CTLog's PKCS#11 support, and no integration/e2e coverage for the feature at all. This PR adds unit tests for Fulcio's PKCS#11 pieces (fulcio/actions/ensure_pkcs11_config_test.go, fulcio/actions/fulcio_deployment_test.go) but this file and ctlog/actions/deployment.go have none. More importantly, every test added here (including the Fulcio ones) exercises a single action in isolation — nothing runs the full reconcile flow through the existing envtest infra (ctlog_controller_test.go/fulcio_controller_test.go, testonly.ControllerSuite), and there's no test/e2e coverage either. This is a multi-action state machine (config validation → server config generation → deployment, with rotation/drift handling) — at least one controller-level test that creates a signer.type: pkcs11 CR and asserts it reaches Ready with the expected init containers/volumes/mounts would exercise the interactions between actions that unit tests can't, and would have caught the rotation false-trigger and the ignored-outside-PKCS11-mode bugs flagged elsewhere in this review.
| // Persistent storage for HSM tokens (key survives pod restarts). | ||
| // When nil, an emptyDir is used (key is regenerated on pod restart). | ||
| //+optional | ||
| Persistence *Pvc `json:"persistence,omitempty"` |
There was a problem hiding this comment.
Persistence *Pvc doesn't do what the shared Pvc type implies, and duplicates the generic Volumes/VolumeMounts mechanism you already added. Unlike Rekor/TUF/Trillian's own pvc.go actions (which actually provision a PersistentVolumeClaim using Size/StorageClass/Retain), there's no equivalent action for CTLog — internal/controller/ctlog/actions/deployment.go only ever reads Persistence.Name and plugs it into PersistentVolumeClaimVolumeSource{ClaimName: ...} (lines 263-265, 274-276). Size, StorageClass, Retain (and their CEL immutability rules on Pvc) are dead for this field — a user setting them would reasonably expect the operator to provision storage, and it won't. Since this is just "reference an existing PVC by name," it seems fully replaceable by having the user define their own hsm-tokens volume via the generic spec.ctlog.volumes/volumeMounts you already introduced, with no dedicated Persistence field needed at all.
That also surfaces a real bug in the current precedence: the comment at deployment.go:248-249 says user-defined volumes are processed first "so operator-managed volumes take precedence for reserved names," but both the if !hasVolume(...) and else branches (256-280) unconditionally do tokensVol.VolumeSource = core.VolumeSource{} and rebuild it from Persistence/EmptyDir — so a user-defined hsm-tokens volume (e.g. a different PVC, or even a Secret) gets silently discarded and replaced every reconcile, regardless of what they configured. The two branches are functionally identical, so the hasVolume check isn't actually gating anything today.
| # | ||
| # Replace all <placeholder> values with your environment-specific configuration. | ||
| # This sample is vendor-agnostic. For SoftHSM dev/test or production HSM examples, | ||
| # see docs/pkcs11-hsm-support.md. |
There was a problem hiding this comment.
This sample is 172 lines — by far the largest in config/samples/ (every other one is 9-73 lines). Files here are meant to feed the CSV's alm-examples annotation (see config/samples/kustomization.yaml: "Append samples you want in your CSV to this file as resources"), which gets stored as CR data in etcd — we intentionally keep these small. This one isn't wired into kustomization.yaml yet, but it shouldn't live here at all: please move it under docs/ and link it from documentation instead, matching the pattern of fulcio-key-rotation.md/ctlog-key-rotation.md/etc. Also, this comment already points at docs/pkcs11-hsm-support.md, but that file doesn't exist anywhere in the repo — please add it (or fix the reference) as part of the move.
3 participants
Summary
Add PKCS#11/HSM support to the operator for both Fulcio (CA certificate signing) and CTLog (Signed Tree Head signing), enabling enterprises to use Hardware Security Modules for all signing keys via a vendor-agnostic plugin model. Integrates into the
spec.signerhierarchy.Fulcio PKCS#11 (
spec.fulcio.signer)type: pkcs11onFulcioSignerwith CEL mutual-exclusion rulesFulcioPKCS11Config:configRef(crypto11.conf) +keyConfig(HSM key ID/label)signer.auth(Authstruct) for server container env vars — replaces customCredentialsRef/ServerEnvsigner.certificateChain.certificateChainReffor pre-provisioned root CA from key ceremonyinitContainers/volumes/volumeMountsonFulcioSpec(pod-wide, not nested under pkcs11)CTLog PKCS#11 (
spec.ctlog.signer)type: pkcs11onCTlogSignerwith CEL mutual-exclusion rulesCTlogPKCS11Config:pinSecretRef,publicKeyRef,tokenLabel,modulePath,persistencesigner.authfor server container env vars (uniform with Fulcio)keyspb.PKCS11Configprotobuf instead ofkeyspb.PEMKeyFileStatus.PublicKeyRefpopulated fromEnsurePKCS11Configfor trust material resolutioninitContainers/volumes/volumeMountsonCTlogSpecDesign decisions (per review feedback)
Authstruct instead of customCredentialsRef/ServerEnv— matches TSA's patterninitContainers/volumes/volumeMountsto top-level spec — pod-wide resources, not backend-specificPKCS11Statusstructs and drift detection —ObservedGenerationsufficesconfigRef/modulePath— drop redundantPKCS11prefix (already scoped underpkcs11:)initContainersCEL rule — allow CSI drivers/webhook injectors to provision libraries without declaring init containersHSM_PIN— users set it explicitly ininitContainers[].envif their vendor image needs itSigner— uniform placement for both Fulcio and CTLogSigner.SetDefaults()fromSecureSign.SetDefaults()—signer.type: fileis explicit on the parent CR, CEL rules don't need!has(self.type)guardsShared
PKCS11InitContainerSpec— curatedcorev1.Containersubset shared by both componentsMarshalDataannotations (v1 API only)<placeholder>valuesE2E Validation (OCP 4.22)
signer.type: filedefaulted on SecureSign CRDependency
SECURESIGN-5021: RHTAS ct_server image must be built with
CGO_ENABLED=1 -tags=pkcs11,no_openssl. Thego-toolsetbase image already includesgccandglibc-devel— nodnf installneeded (hermetic build compatible).Test plan
make buildandmake testpasssigner.type: filedefaulted, no PKCS#11 conditions/args/init containerscosign sign + verifypasses in both file and PKCS#11 modessigner.typefrompkcs11tofile— PKCS#11 artifacts cleaned upJira: SECURESIGN-5014