fix(pki): honor CA-issuance policy denial on certificate renewal#7027
fix(pki): honor CA-issuance policy denial on certificate renewal#7027Abdul-Moiz31 wants to merge 1 commit into
Conversation
Certificate renewal built its certificateRequest without basicConstraints, so validateRequestAgainstPolicy's CA-denial check (gated on request.basicConstraints?.isCA) was always falsy on renewal. A policy that denied CA issuance was silently bypassed when renewing a previously-issued CA certificate, and the renewed cert was issued to internalCaService.issueCertFromCa without basicConstraints/pathLength at all, so it would have lost its CA extension regardless. Renewal now derives shouldIssueAsCA from originalCert.isCA, populates certificateRequest.basicConstraints for policy validation, runs the same explicit CA-denial guard the other three issuance paths run, and passes basicConstraints/pathLength through to issueCertFromCa so the renewed certificate keeps its original CA status (re-validated against the current policy's max path length and the issuing CA's own constraints). Also extracts the CA-denial check duplicated identically across direct issuance, CSR/profile issuance, and approval finalization into a single assertCaIssuancePolicyAllowed helper in certificate-issuance-utils.ts, so renewal (and any future issuance path) reuses the same guard instead of risking another inline copy that misses it.
|
| Filename | Overview |
|---|---|
| backend/src/services/certificate-common/certificate-issuance-utils.ts | New assertCaIssuancePolicyAllowed helper correctly centralizes the CA-denial check; return type accurately reflects the resolved policy state, though callers in the service layer ignore it. |
| backend/src/services/certificate-v3/certificate-v3-service.ts | Core fix: renewCertificate now populates basicConstraints in certificateRequest, calls assertCaIssuancePolicyAllowed, and passes basicConstraints/pathLength through to issueCertFromCa; a null original pathLength under a policy with maxPathLength yields a confusing error (see comment). |
| backend/src/services/certificate-v3/certificate-approval-fns.ts | Refactored to use shared assertCaIssuancePolicyAllowed; return value correctly captured for subsequent path-length logic; behavior unchanged from previous inline check. |
| backend/src/services/certificate-v3/certificate-v3-service.test.ts | Two well-structured unit tests added: one verifying the denial path stops before issueCertFromCa, one verifying the allowed path threads basicConstraints/pathLength through correctly. |
Reviews (1): Last reviewed commit: "fix(pki): honor CA-issuance policy denia..." | Re-trigger Greptile
| basicConstraints: shouldIssueAsCA | ||
| ? { isCA: true, pathLength: policy?.basicConstraints?.maxPathLength } | ||
| : undefined, | ||
| pathLength: originalCert.pathLength ?? undefined, |
There was a problem hiding this comment.
When
originalCert.pathLength is null (a CA issued with unlimited path length, stored as null in the DB) and policy.basicConstraints.maxPathLength is defined (e.g. 2), the call to $createBasicConstraintsExtension sees policyMaxPathLength = 2 and pathLength = undefined, and throws "Path length is required when issuing CA certificates because the policy only allows a maximum path length of 2." This error is confusing during renewal because the user never chose a path length — it was inherited from the original cert. Defaulting to policy?.basicConstraints?.maxPathLength when the original cert has no stored path length would be a natural renewal semantic (the renewed cert gets the policy cap rather than failing).
| basicConstraints: shouldIssueAsCA | |
| ? { isCA: true, pathLength: policy?.basicConstraints?.maxPathLength } | |
| : undefined, | |
| pathLength: originalCert.pathLength ?? undefined, | |
| basicConstraints: shouldIssueAsCA | |
| ? { isCA: true, pathLength: policy?.basicConstraints?.maxPathLength } | |
| : undefined, | |
| pathLength: originalCert.pathLength ?? policy?.basicConstraints?.maxPathLength ?? undefined, |
Context
Fixes #6971.
Certificate renewal in
certificateV3Service.renewCertificateruns the samecertificatePolicyService.validateCertificateRequestthat direct issuance runs, but thecertificateRequestit built never populatedbasicConstraints.validateRequestAgainstPolicy's CA-related checks are gated onrequest.basicConstraints?.isCA === true, which was always falsy on renewal — so a policy that denies CA issuance was silently bypassed when renewing a previously-issued CA certificate.Separately, even once validation is fixed, the renewal's call to
internalCaService.issueCertFromCanever passedbasicConstraints/pathLengthat all — so the renewed certificate would have lost its CA extension regardless of what the policy said.Repro from the issue:
Fix
renewCertificatenow derivesshouldIssueAsCAfromoriginalCert.isCAand populatescertificateRequest.basicConstraints(withoriginalCert.pathLength) before the generic policy validation runs.assertCaIssuancePolicyAllowed(policy, shouldIssueAsCA)guard right after that validation — the same defense-in-depth check the other three issuance paths (direct issuance, CSR/profile issuance, approval finalization) already run.issueCertFromCacall now passesbasicConstraints: { isCA: true, pathLength: policy?.basicConstraints?.maxPathLength }andpathLength: originalCert.pathLength, so a renewed CA certificate keeps its CA status and gets re-validated against the current policy's max path length and the issuing CA's own constraints — not just blindly copied forward.Refactor (per the issue's suggestion)
The CA-denial check existed as three near-identical inline copies in
certificate-v3-service.ts(direct issuance, CSR/profile issuance) andcertificate-approval-fns.ts(approval finalization). Extracted them into a singleassertCaIssuancePolicyAllowed(policy, shouldIssueAsCA)helper incertificate-issuance-utils.ts, used by all four issuance paths now (including renewal). This also normalizes the error message, which previously differed slightly between the approval path and the other two.Tests
Added two unit tests to
certificate-v3-service.test.ts:issueCertFromCais never called.basicConstraints/pathLengththrough toissueCertFromCa.npx vitest run -c vitest.unit.config.mts src/services/certificate→ 198/198 passing. Type-check and lint are clean for all changed files (verified against the project's pre-existing, unrelated lint/type-check debt — error counts are identical before/after this diff).Type
Checklist