Add support for signing sub-CA with external keys#5337
Open
abbra wants to merge 15 commits intodogtagpki:masterfrom
Open
Add support for signing sub-CA with external keys#5337abbra wants to merge 15 commits intodogtagpki:masterfrom
abbra wants to merge 15 commits intodogtagpki:masterfrom
Conversation
…omCSR Extract the certificate enrollment logic from generateSigningCert() into a new generateSigningCertFromCSR() method that takes an already-prepared PKCS10 object and a profile ID, then have generateSigningCert() delegate to it after generating the key pair and CSR internally. The new method takes an explicit profileId parameter rather than hard-coding "caCACert", allowing callers to use a different CA signing profile (e.g. one that adds nameConstraints for ACME sub-CAs). No functional change for existing callers. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds support for creating lightweight sub‑CAs using an externally‑held private key: clients submit a PEM PKCS#10 CSR and optional profileId to obtain a signed sub‑CA certificate while Dogtag records the authority without storing the private key. Changes
Sequence Diagram(s)sequenceDiagram
participant ES as External System
participant REST as Dogtag REST API
participant ENGINE as CA Engine
participant PROFILE as Profile Processor
participant LDAP as LDAP Directory
ES->>ES: Generate keypair & PKCS#10 CSR
ES->>REST: POST /ca/v2/authorities (csrData + profileId)
REST->>ENGINE: createAuthorityRecord(parentAID, authToken, subjectDN, desc, csrData, profileId)
ENGINE->>ENGINE: Validate CSR and subject DN
ENGINE->>PROFILE: Apply signing profile & sign CSR
PROFILE-->>ENGINE: Signed sub‑CA certificate
ENGINE->>LDAP: Store authority entry (nickname=prefix+id, externalKey=true, ready=false)
ENGINE-->>REST: 201 Created (authority id)
ES->>REST: GET /ca/v2/authorities/{id}/cert
REST-->>ES: Signed sub‑CA certificate
ES->>ES: Verify chain and install cert on external HSM
ES->>REST: DELETE /ca/v2/authorities/{id}
REST->>ENGINE: deleteAuthority(id)
ENGINE->>LDAP: Remove authority entry
ENGINE-->>REST: 204 No Content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for creating lightweight sub-CAs with externally-held private keys. Key changes include the addition of a new certificate profile (caExternalKeyCACert), updates to the CAEngine to process externally provided PKCS#10 CSRs, and modifications to the REST API, CLI, and Python client to support csrData and profileId parameters. Documentation and unit tests for these features are also provided. Feedback was given regarding a missing null check in CAEngine when retrieving a certificate profile, which could lead to a NullPointerException if an invalid profile ID is provided.
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/AuthorityRepository.java (1)
839-855:⚠️ Potential issue | 🟡 Minor
externalKeycan be missing in modify responses.
readAuthorityData(AuthorityRecord)setsexternalKey, butmodifyCA()returnsreadAuthorityData(CertificateAuthority), which does not. This creates inconsistent API output for the same authority depending on endpoint path.✅ Suggested fix (set `externalKey` in CA-based mapper too)
private AuthorityData readAuthorityData(CertificateAuthority ca) throws PKIException { @@ - return new AuthorityData( + AuthorityData data = new AuthorityData( ca.isHostAuthority(), dn, ca.getAuthorityID().toString(), parentAID != null ? parentAID.toString() : null, issuerDN, serial, ca.getAuthorityEnabled(), ca.getAuthorityDescription(), ca.isReady() ); + + String nickname = ca.getNickname(); + if (nickname != null && nickname.startsWith(AuthorityRecord.EXTERNAL_KEY_NICKNAME_PREFIX)) { + data.setExternalKey(true); + } + return data; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/AuthorityRepository.java` around lines 839 - 855, modifyCA() returns AuthorityData produced by readAuthorityData(CertificateAuthority) which currently omits the externalKey flag, causing inconsistent API output; update the CA-based mapper (readAuthorityData(CertificateAuthority) or the code path used by modifyCA()) to set data.setExternalKey(true/false) based on the underlying CertificateAuthority/AuthorityRecord equivalent (e.g., call record.isExternalKey() or the CertificateAuthority getter) so AuthorityData.externalKey is always populated consistently like readAuthorityData(AuthorityRecord) does.
🧹 Nitpick comments (2)
base/ca/shared/profiles/ca/caExternalKeyCACert.cfg (1)
54-55: Consider narrowing KeyUsage to CA-only bits.
digitalSignatureandnonRepudiationare currently enabled. If not explicitly required for your downstream use cases, restricting tokeyCertSign+crlSigngives a tighter CA cert profile.🔒 Suggested hardening
-policyset.caCertSet.6.constraint.params.keyUsageDigitalSignature=true -policyset.caCertSet.6.constraint.params.keyUsageNonRepudiation=true +policyset.caCertSet.6.constraint.params.keyUsageDigitalSignature=false +policyset.caCertSet.6.constraint.params.keyUsageNonRepudiation=false ... -policyset.caCertSet.6.default.params.keyUsageDigitalSignature=true -policyset.caCertSet.6.default.params.keyUsageNonRepudiation=true +policyset.caCertSet.6.default.params.keyUsageDigitalSignature=false +policyset.caCertSet.6.default.params.keyUsageNonRepudiation=falseAlso applies to: 66-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/shared/profiles/ca/caExternalKeyCACert.cfg` around lines 54 - 55, The CA profile currently enables non-CA KeyUsage bits (policyset.caCertSet.6.constraint.params.keyUsageDigitalSignature and policyset.caCertSet.6.constraint.params.keyUsageNonRepudiation); change these to false and instead enable only the CA bits (set keyUsageKeyCertSign=true and keyUsageCrlSign=true) for the CA certificate constraint; apply the same change to the other CA block referenced by the comment (the similar policyset.caCertSet.* entries at lines 66-67) so CA certs are limited to keyCertSign + crlSign only.base/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCreateCLI.java (1)
86-90: Use explicit UTF-8 charset when reading CSR file.Platform-default charset can cause environment-dependent parsing behavior. CSR/PEM files should always be decoded with UTF-8 explicitly.
💡 Suggested change
+import java.nio.charset.StandardCharsets; @@ - csrData = new String(Files.readAllBytes(Paths.get(csrFile))); + csrData = new String(Files.readAllBytes(Paths.get(csrFile)), StandardCharsets.UTF_8);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCreateCLI.java` around lines 86 - 90, The code in AuthorityCreateCLI that reads the CSR into csrData uses the platform default charset; change the read to explicitly use UTF-8 (e.g., call Files.readString or new String(..., StandardCharsets.UTF_8)) when reading from Paths.get(csrFile) so csrData is always decoded as UTF-8. Update the csrData assignment in AuthorityCreateCLI to pass StandardCharsets.UTF_8 to the reader/constructor and import java.nio.charset.StandardCharsets if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/AuthorityRepository.java`:
- Around line 839-855: modifyCA() returns AuthorityData produced by
readAuthorityData(CertificateAuthority) which currently omits the externalKey
flag, causing inconsistent API output; update the CA-based mapper
(readAuthorityData(CertificateAuthority) or the code path used by modifyCA()) to
set data.setExternalKey(true/false) based on the underlying
CertificateAuthority/AuthorityRecord equivalent (e.g., call
record.isExternalKey() or the CertificateAuthority getter) so
AuthorityData.externalKey is always populated consistently like
readAuthorityData(AuthorityRecord) does.
---
Nitpick comments:
In `@base/ca/shared/profiles/ca/caExternalKeyCACert.cfg`:
- Around line 54-55: The CA profile currently enables non-CA KeyUsage bits
(policyset.caCertSet.6.constraint.params.keyUsageDigitalSignature and
policyset.caCertSet.6.constraint.params.keyUsageNonRepudiation); change these to
false and instead enable only the CA bits (set keyUsageKeyCertSign=true and
keyUsageCrlSign=true) for the CA certificate constraint; apply the same change
to the other CA block referenced by the comment (the similar
policyset.caCertSet.* entries at lines 66-67) so CA certs are limited to
keyCertSign + crlSign only.
In
`@base/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCreateCLI.java`:
- Around line 86-90: The code in AuthorityCreateCLI that reads the CSR into
csrData uses the platform default charset; change the read to explicitly use
UTF-8 (e.g., call Files.readString or new String(..., StandardCharsets.UTF_8))
when reading from Paths.get(csrFile) so csrData is always decoded as UTF-8.
Update the csrData assignment in AuthorityCreateCLI to pass
StandardCharsets.UTF_8 to the reader/constructor and import
java.nio.charset.StandardCharsets if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3324e96-c6d3-464f-a43a-f5dbddd50ca8
📒 Files selected for processing (15)
.github/workflows/ca-authority-external-key-test.yml.github/workflows/subca-tests.ymlbase/ca/shared/profiles/ca/caExternalKeyCACert.cfgbase/ca/src/main/java/com/netscape/ca/CertificateAuthority.javabase/ca/src/main/java/org/dogtagpki/server/ca/AuthorityRecord.javabase/ca/src/main/java/org/dogtagpki/server/ca/CAEngine.javabase/ca/src/main/java/org/dogtagpki/server/ca/rest/base/AuthorityRepository.javabase/ca/src/test/java/org/dogtagpki/server/ca/AuthorityRecordTest.javabase/common/python/pki/authority.pybase/common/src/main/java/com/netscape/certsrv/authority/AuthorityData.javabase/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCLI.javabase/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCreateCLI.javadocs/changes/v11.10.0/API-Changes.adocdocs/user/tools/Creating-Sub-CA-with-External-Key.adocdocs/user/tools/Creating-Sub-CA.adoc
abbra
force-pushed
the
acme-subca-external-csr
branch
from
b818e8f to
94504ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ca-authority-external-key-test.yml (1)
116-129: Add coverage for the newprofileIdinput.This workflow only exercises the default external-key profile. A regression anywhere in the new
AuthorityData.profileId→ REST →CAEngineplumbing would still pass here, even though profile selection is a primary goal of the PR. Please add a case that posts a non-defaultprofileIdand asserts a certificate property that is specific to that profile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ca-authority-external-key-test.yml around lines 116 - 129, Update the "Build authority creation JSON payload" step to include a non-default profileId in the generated payload (set AuthorityData.profileId to a known non-default value) so the body written to /tmp/authority-req.json exercises profile selection; then add a follow-up workflow step that posts this payload and asserts a certificate property specific to that profile (e.g., certificate extensions or key usage changed by that profile) to validate the CAEngine plumbing honors profileId. Reference the payload-building block (variables csr, parent_id, payload) and the AuthorityData.profileId / CAEngine flow when adding the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ca-authority-external-key-test.yml:
- Around line 250-253: The workflow step that verifies deletion uses `docker
exec pki pki ca-authority-show ${AID} && exit 1 || true`, which always exits
success; replace it with a conditional that fails the step when `pki
ca-authority-show` succeeds: run `docker exec pki cat /tmp/authority-id` to set
AID as before, then run `docker exec pki pki ca-authority-show ${AID}` and if
that command returns success, explicitly exit 1 (e.g., if ...; then echo
"authority still exists"; exit 1; fi) so the job fails when the authority still
exists. Ensure the step does not unconditionally swallow failures.
In `@base/ca/src/main/java/org/dogtagpki/server/ca/CAEngine.java`:
- Around line 1434-1455: Replace the direct use of
CertUtil.decodePKCS10(csrData) in the external-key CSR handling so the CA honors
ca.requestVerify and related config; call the class's parsePKCS10(Locale locale,
String certreq) method instead (passing the appropriate locale and csrData) so
the CSR goes through the same config-aware parsing and signing-token handling
used elsewhere before proceeding to generateSigningCertFromCSR; ensure you
preserve the existing subject DN comparison and subsequent use of
effectiveProfileId and authToken.
---
Nitpick comments:
In @.github/workflows/ca-authority-external-key-test.yml:
- Around line 116-129: Update the "Build authority creation JSON payload" step
to include a non-default profileId in the generated payload (set
AuthorityData.profileId to a known non-default value) so the body written to
/tmp/authority-req.json exercises profile selection; then add a follow-up
workflow step that posts this payload and asserts a certificate property
specific to that profile (e.g., certificate extensions or key usage changed by
that profile) to validate the CAEngine plumbing honors profileId. Reference the
payload-building block (variables csr, parent_id, payload) and the
AuthorityData.profileId / CAEngine flow when adding the assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39941342-1aab-414f-9ac0-1be0e9b8054b
📒 Files selected for processing (9)
.github/workflows/ca-authority-external-key-test.yml.github/workflows/subca-tests.ymlbase/ca/src/main/java/org/dogtagpki/server/ca/CAEngine.javabase/common/python/pki/authority.pybase/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCLI.javabase/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCreateCLI.javadocs/changes/v11.10.0/API-Changes.adocdocs/user/tools/Creating-Sub-CA-with-External-Key.adocdocs/user/tools/Creating-Sub-CA.adoc
✅ Files skipped from review due to trivial changes (4)
- base/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCLI.java
- docs/changes/v11.10.0/API-Changes.adoc
- docs/user/tools/Creating-Sub-CA.adoc
- docs/user/tools/Creating-Sub-CA-with-External-Key.adoc
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/subca-tests.yml
- base/common/python/pki/authority.py
Comment on lines
+250
to
+253
| - name: Verify authority no longer exists | ||
| run: | | ||
| AID=$(docker exec pki cat /tmp/authority-id) | ||
| docker exec pki pki ca-authority-show ${AID} && exit 1 || true |
There was a problem hiding this comment.
Make the post-delete assertion fail when the authority still exists.
docker exec ... && exit 1 || true always returns success, so this step no longer proves deletion. If ca-authority-show still succeeds, the workflow still passes.
Proposed fix
- name: Verify authority no longer exists
run: |
AID=$(docker exec pki cat /tmp/authority-id)
- docker exec pki pki ca-authority-show ${AID} && exit 1 || true
+ if docker exec pki pki ca-authority-show "${AID}"; then
+ echo "Authority ${AID} still exists after deletion" >&2
+ exit 1
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ca-authority-external-key-test.yml around lines 250 - 253,
The workflow step that verifies deletion uses `docker exec pki pki
ca-authority-show ${AID} && exit 1 || true`, which always exits success; replace
it with a conditional that fails the step when `pki ca-authority-show` succeeds:
run `docker exec pki cat /tmp/authority-id` to set AID as before, then run
`docker exec pki pki ca-authority-show ${AID}` and if that command returns
success, explicitly exit 1 (e.g., if ...; then echo "authority still exists";
exit 1; fi) so the job fails when the authority still exists. Ensure the step
does not unconditionally swallow failures.
94504ac to
ada1074
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/AuthorityRepository.java (1)
782-799:⚠️ Potential issue | 🔴 Critical
readAuthorityData(CertificateAuthority)still assumes a local signing unit.This method calls
ca.getCACert()before checking whether the authority is external-key. For the new external-key flow,CertificateAuthority.initSigningUnits()leavesmSigningUnitnull, so this path can throw before the response is built.modifyCA()already returns through this method, which means updating an external-key authority will fail instead of returning the updated resource. Detect the external-key case first and derive issuer/serial without touchinggetCACert().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/AuthorityRepository.java` around lines 782 - 799, readAuthorityData(CertificateAuthority) assumes a local signing unit and calls ca.getCACert() before checking for external-key, which can throw when mSigningUnit is null; update the method to detect external-key by checking ca.getNickname() against AuthorityRecord.EXTERNAL_KEY_NICKNAME_PREFIX (as done where data.setExternalKey(true) is used) before any call to ca.getCACert(), and for external-key derive issuerDN and serial from available metadata (or null/placeholder) without invoking getCACert(), so modifyCA() and external-key authorities return the built AuthorityData without triggering getCACert().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base/ca/src/main/java/com/netscape/ca/CertificateAuthority.java`:
- Around line 692-700: When an external-key authority is detected in
CertificateAuthority (mNickname startsWith
AuthorityRecord.EXTERNAL_KEY_NICKNAME_PREFIX) do not return early; instead
initialize a minimal certificate-only signing unit (set mSigningUnit to a
SigningUnit instance with no private key but populated certificate, nickname and
token metadata and hasKeys=false/isReady()=false) by loading the authority
certificate from the repository using authoritySerial; this preserves downstream
assumptions (so CAEngine.revokeAuthority() and CAEngine.deleteAuthorityNSSDB()
can dereference mSigningUnit safely). Ensure the minimal SigningUnit exposes any
accessors used by those callers while still marking it as not capable of
signing, or alternatively update those callers (isExternalKey() checks) to fall
back to repository/authoritySerial if mSigningUnit is null—prefer initializing
the lightweight signing unit in CertificateAuthority to minimize caller changes.
---
Outside diff comments:
In
`@base/ca/src/main/java/org/dogtagpki/server/ca/rest/base/AuthorityRepository.java`:
- Around line 782-799: readAuthorityData(CertificateAuthority) assumes a local
signing unit and calls ca.getCACert() before checking for external-key, which
can throw when mSigningUnit is null; update the method to detect external-key by
checking ca.getNickname() against AuthorityRecord.EXTERNAL_KEY_NICKNAME_PREFIX
(as done where data.setExternalKey(true) is used) before any call to
ca.getCACert(), and for external-key derive issuerDN and serial from available
metadata (or null/placeholder) without invoking getCACert(), so modifyCA() and
external-key authorities return the built AuthorityData without triggering
getCACert().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cac6dc98-a888-4212-84e3-2134912bcda6
📒 Files selected for processing (15)
.github/workflows/ca-authority-external-key-test.yml.github/workflows/subca-tests.ymlbase/ca/shared/profiles/ca/caExternalKeyCACert.cfgbase/ca/src/main/java/com/netscape/ca/CertificateAuthority.javabase/ca/src/main/java/org/dogtagpki/server/ca/AuthorityRecord.javabase/ca/src/main/java/org/dogtagpki/server/ca/CAEngine.javabase/ca/src/main/java/org/dogtagpki/server/ca/rest/base/AuthorityRepository.javabase/ca/src/test/java/org/dogtagpki/server/ca/AuthorityRecordTest.javabase/common/python/pki/authority.pybase/common/src/main/java/com/netscape/certsrv/authority/AuthorityData.javabase/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCLI.javabase/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCreateCLI.javadocs/changes/v11.10.0/API-Changes.adocdocs/user/tools/Creating-Sub-CA-with-External-Key.adocdocs/user/tools/Creating-Sub-CA.adoc
✅ Files skipped from review due to trivial changes (5)
- base/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCLI.java
- .github/workflows/subca-tests.yml
- docs/user/tools/Creating-Sub-CA.adoc
- docs/user/tools/Creating-Sub-CA-with-External-Key.adoc
- base/ca/shared/profiles/ca/caExternalKeyCACert.cfg
🚧 Files skipped from review as they are similar to previous changes (6)
- base/ca/src/main/java/org/dogtagpki/server/ca/AuthorityRecord.java
- base/tools/src/main/java/com/netscape/cmstools/authority/AuthorityCreateCLI.java
- docs/changes/v11.10.0/API-Changes.adoc
- base/common/src/main/java/com/netscape/certsrv/authority/AuthorityData.java
- .github/workflows/ca-authority-external-key-test.yml
- base/common/python/pki/authority.py
abbra
force-pushed
the
acme-subca-external-csr
branch
from
ada1074 to
7f42d59
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ca-authority-external-key-test.yml (1)
245-248:⚠️ Potential issue | 🟠 MajorMake the post-delete check fail when the authority still exists.
docker exec ... && exit 1 || trueexits successfully in both branches, so this step does not prove deletion. Ifca-authority-showstill succeeds, the workflow still stays green.Suggested fix
- name: Verify authority no longer exists run: | AID=$(docker exec pki cat /tmp/authority-id) - docker exec pki pki ca-authority-show ${AID} && exit 1 || true + if docker exec pki pki ca-authority-show "${AID}"; then + echo "Authority ${AID} still exists after deletion" >&2 + exit 1 + fi#!/bin/bash ( true && exit 1 || true ) >/dev/null 2>&1 status_when_show_succeeds=$? ( false && exit 1 || true ) >/dev/null 2>&1 status_when_show_fails=$? printf 'status when first command succeeds: %s\n' "$status_when_show_succeeds" printf 'status when first command fails: %s\n' "$status_when_show_fails" # Expected: both are 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ca-authority-external-key-test.yml around lines 245 - 248, The step that checks deletion currently masks failures because `&& exit 1 || true` always yields a successful exit; change the check to run `docker exec pki pki ca-authority-show ${AID}` and then inspect its exit status: if the command exits with 0 (meaning the authority still exists) explicitly fail the step (exit 1) and print a clear error, otherwise succeed; use the existing AID retrieval and the `pki ca-authority-show` invocation to locate where to capture the exit code and branch on it.
🧹 Nitpick comments (1)
.github/workflows/ca-authority-external-key-test.yml (1)
211-233: Exercise the explicit signing-profile path before asserting it is not persisted.This is a good read-back check for
csrData, butprofileIdis never set anywhere in the workflow. As written, Line 232 only proves “absent when omitted,” so a regression in the new profile-selection path from#5336could still pass CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ca-authority-external-key-test.yml around lines 211 - 233, Before asserting profileId is not persisted, exercise the explicit signing-profile path by submitting a request that selects a signing profile for the authority (use the same AID variable already used); for example, perform the API call that exercises profile selection (a POST to the authority's signing/sign or CSR endpoint including a profileId payload), then re-run the GET into /tmp/authority-get-resp.json and assert that data does not contain 'profileId' or 'csrData'. Ensure you reference the same AID variable and the same /tmp/authority-get-resp.json file and checks for 'profileId' and 'csrData' so the test actually verifies the transient profile-selection path is not persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ca-authority-external-key-test.yml:
- Around line 123-130: The pipeline running inside docker exec uses bash -c
wrapping the pki -n caadmin ca-authority-create command piped to tee, but
pipefail is not enabled so tee's exit status hides failures from pki; update the
inner shell invocation (the bash -c that runs pki -n caadmin ca-authority-create
| tee ...) to enable pipefail (e.g., prepend a shell setting like set -o
pipefail or use bash -o pipefail) so that the exit status of pki is preserved
and non-zero failures propagate through the pipeline.
---
Duplicate comments:
In @.github/workflows/ca-authority-external-key-test.yml:
- Around line 245-248: The step that checks deletion currently masks failures
because `&& exit 1 || true` always yields a successful exit; change the check to
run `docker exec pki pki ca-authority-show ${AID}` and then inspect its exit
status: if the command exits with 0 (meaning the authority still exists)
explicitly fail the step (exit 1) and print a clear error, otherwise succeed;
use the existing AID retrieval and the `pki ca-authority-show` invocation to
locate where to capture the exit code and branch on it.
---
Nitpick comments:
In @.github/workflows/ca-authority-external-key-test.yml:
- Around line 211-233: Before asserting profileId is not persisted, exercise the
explicit signing-profile path by submitting a request that selects a signing
profile for the authority (use the same AID variable already used); for example,
perform the API call that exercises profile selection (a POST to the authority's
signing/sign or CSR endpoint including a profileId payload), then re-run the GET
into /tmp/authority-get-resp.json and assert that data does not contain
'profileId' or 'csrData'. Ensure you reference the same AID variable and the
same /tmp/authority-get-resp.json file and checks for 'profileId' and 'csrData'
so the test actually verifies the transient profile-selection path is not
persisted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d64fbbbb-cfa5-420e-a11d-5840f4682746
📒 Files selected for processing (1)
.github/workflows/ca-authority-external-key-test.yml
Comment on lines
+123
to
+130
| docker exec pki bash -c " | ||
| pki -n caadmin ca-authority-create \ | ||
| --parent ${PARENT_ID} \ | ||
| --csr-file /tmp/external-ca.csr \ | ||
| --desc 'Integration test external sub-CA' \ | ||
| 'CN=Test External Sub-CA,O=EXAMPLE' \ | ||
| | tee /tmp/authority-create-output.txt | ||
| " |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
bash -c 'false | tee /dev/null' >/dev/null 2>&1
status_plain=$?
bash -o pipefail -c 'false | tee /dev/null' >/dev/null 2>&1
status_pipefail=$?
printf 'plain bash -c status: %s\n' "$status_plain"
printf 'bash -o pipefail -c status: %s\n' "$status_pipefail"
# Expected: plain=0, pipefail!=0Repository: dogtagpki/pki
Length of output: 114
🏁 Script executed:
head -n 130 .github/workflows/ca-authority-external-key-test.yml | tail -n 20Repository: dogtagpki/pki
Length of output: 895
🏁 Script executed:
grep -n "authority-create-output" .github/workflows/ca-authority-external-key-test.ymlRepository: dogtagpki/pki
Length of output: 182
🏁 Script executed:
sed -n '130,150p' .github/workflows/ca-authority-external-key-test.ymlRepository: dogtagpki/pki
Length of output: 971
Add -o pipefail to preserve ca-authority-create failures through the tee pipeline.
The inner bash -c does not enable pipefail, so this step returns tee's status instead of pki's. If pki ca-authority-create fails, the command exit status will be 0, and the workflow continues until the output validation fails one step later.
Suggested fix
- docker exec pki bash -c "
+ docker exec pki bash -o pipefail -c "
pki -n caadmin ca-authority-create \
--parent ${PARENT_ID} \
--csr-file /tmp/external-ca.csr \
--desc 'Integration test external sub-CA' \
'CN=Test External Sub-CA,O=EXAMPLE' \
| tee /tmp/authority-create-output.txt
"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| docker exec pki bash -c " | |
| pki -n caadmin ca-authority-create \ | |
| --parent ${PARENT_ID} \ | |
| --csr-file /tmp/external-ca.csr \ | |
| --desc 'Integration test external sub-CA' \ | |
| 'CN=Test External Sub-CA,O=EXAMPLE' \ | |
| | tee /tmp/authority-create-output.txt | |
| " | |
| docker exec pki bash -o pipefail -c " | |
| pki -n caadmin ca-authority-create \ | |
| --parent ${PARENT_ID} \ | |
| --csr-file /tmp/external-ca.csr \ | |
| --desc 'Integration test external sub-CA' \ | |
| 'CN=Test External Sub-CA,O=EXAMPLE' \ | |
| | tee /tmp/authority-create-output.txt | |
| " |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ca-authority-external-key-test.yml around lines 123 - 130,
The pipeline running inside docker exec uses bash -c wrapping the pki -n caadmin
ca-authority-create command piped to tee, but pipefail is not enabled so tee's
exit status hides failures from pki; update the inner shell invocation (the bash
-c that runs pki -n caadmin ca-authority-create | tee ...) to enable pipefail
(e.g., prepend a shell setting like set -o pipefail or use bash -o pipefail) so
that the exit status of pki is preserved and non-zero failures propagate through
the pipeline.
605c959 to
ba2304f
Compare
Introduce a dedicated signing profile for lightweight CA authorities whose private key is held by a remote caller (e.g. an ACME server with its own HSM). The profile differs from caCACert in three ways that are appropriate for programmatic sub-CA creation: * RSA-1024 is removed from keyParameters: sub-CA keys shorter than 2048 bits are rejected by keyConstraintImpl. * basicConstraintsPathLen is set to 0 and basicConstraintsMaxPathLen is set to 0: the issued sub-CA cannot itself act as an intermediate CA and sign further sub-CA certificates. * Policy 11 adds userExtensionDefaultImpl: extensions embedded in the submitted CSR (e.g. NameConstraints) are copied verbatim into the issued certificate rather than being silently discarded. The profile is not visible in the enrollment UI (visible=false) because it is intended to be used only by the authority-creation REST path. No changes to caCACert.cfg: local-key sub-CA creation is unaffected. Fixes: dogtagpki#5336 Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Allow POST /v2/authorities to accept an optional csrData field containing
a PEM-encoded PKCS#10 CSR. When present, Dogtag signs the CSR as a
sub-CA certificate without generating a local key pair. The authority's
private key remains with the caller (e.g. in an HSM attached to a remote
ACME server).
The authority record stores a sentinel value in authorityKeyNickname
("#external#:<uuid>") so Dogtag can distinguish external-key authorities
from local-key ones. Such authorities report externalKey=true and
ready=false via the REST API and never have a signing unit initialised.
An optional profileId field in the same request body selects the signing
profile used to issue the sub-CA certificate. When absent, the engine
defaults to caExternalKeyCACert, which enforces pathLen=0, rejects RSA
keys shorter than 2048 bits, and copies CSR-embedded extensions (e.g.
NameConstraints) into the issued certificate.
Changes:
AuthorityData: add csrData (input-only CSR field), externalKey
(read-only output), and profileId (optional profile selection;
not stored or returned after creation)
AuthorityRecord: EXTERNAL_KEY_NICKNAME_PREFIX constant +
isExternalKey() derived method
AuthorityRepository: sentinel detection in readAuthorityData();
csrData and profileId forwarded to engine in createCA()
CAEngine: two-branch createAuthorityRecord() (external vs local key);
new generateSigningCertFromCSR(profileId) method; 4-param overload
preserved for internal callers that do not supply csrData
CertificateAuthority: guard against signing unit initialisation for
external-key CAs (authorityKeyNickname starts with "#external#:")
Fixes: dogtagpki#5336
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Add a user guide explaining how to create a lightweight CA authority where the private key is held externally (e.g. in an ACME server's HSM) by supplying a PKCS#10 CSR at creation time. Update the v11.10.0 API change log to describe the new csrData input field and externalKey output field on the /v2/authorities endpoint. Fixes: dogtagpki#5336 Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Cover the sentinel prefix detection introduced for externally-keyed lightweight CAs: true for the #external#: prefix, false for real token:nickname strings, false for null, and a constant-value check to guard against the prefix losing its leading '#'. Fixes: dogtagpki#5336 Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Fixes: dogtagpki#5336 Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Extend pki ca-authority-create with two new options:
--csr-file <path> Read a PEM-encoded PKCS#10 CSR from the given file
and submit it as the csrData field. When present,
Dogtag signs the CSR without generating a local key
pair; the authority's private key remains with the
caller (e.g. in an HSM).
--profile <id> Select the signing profile for the sub-CA certificate.
Valid for both local-key and external-key creation:
- with --csr-file: defaults to caExternalKeyCACert
- without --csr-file: defaults to caCACert
AuthorityCLI.printAuthorityData() is extended to print "External key: true"
when the returned authority has externalKey set.
CAEngine.generateSigningCert() is updated to accept a profileId parameter
so that local-key sub-CA creation goes through the same profile-selection
path as the external-key path.
Fixes: dogtagpki#5336
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
AuthorityData.json_attribute_names now maps the four new JSON fields: csrData → csr_data (input-only: PEM CSR for external-key CA creation) profileId → profile_id (input-only: signing profile override) externalKey → external_key (output: True when Dogtag does not hold the key) ready → ready (output: False for external-key authorities) The __init__ signature and __repr__ are updated accordingly. Callers can now pass csr_data and profile_id to AuthorityClient.create_ca() and inspect external_key / ready on the returned AuthorityData object. Fixes: dogtagpki#5336 Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Add docs/user/tools/Creating-Sub-CA.adoc documenting the standard local-key lightweight CA creation workflow, including: - Obtaining the parent CA ID - Creating a sub-CA with pki ca-authority-create - Using --profile to select a signing profile (default: caCACert) - Common management operations (enable/disable/delete) Update Creating-Sub-CA-with-External-Key.adoc to cross-link the new document from its --profile NOTE and from the See Also section. Update API-Changes.adoc to reference both guides from the profileId API change description. Fixes: dogtagpki#5336 Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
abbra
force-pushed
the
acme-subca-external-csr
branch
from
ba2304f to
f743dab
Compare
curl --cert-type P12 does not authenticate to Tomcat's TLS client certificate layer; the user principal is never set, so ACLFilter returns 403 "No user principal provided." for the POST /v2/authorities endpoint. Replace the curl-based creation step with pki -n caadmin ca-authority-create --csr-file, which authenticates via the NSS database imported in the "Install CA admin cert" step — consistent with how all other LWCA tests authenticate. Verification of the response fields (externalKey, ready, csrData absent, profileId absent) is moved to the unauthenticated GET /v2/authorities/<id> step, which already worked correctly. Fixes: dogtagpki#5336 Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
abbra
force-pushed
the
acme-subca-external-csr
branch
from
f743dab to
4d0fac0
Compare
GET /ca/v2/authorities/{id}/cert returned 404 for external-key
authorities because getCaX509Cert() returns null when the signing unit
is not initialised (the private key lives outside Dogtag).
The signed sub-CA certificate is stored in the certificate repository
by the enrollment processor and its serial number is recorded in the
authority LDAP record (authoritySerial). When getCaX509Cert() returns
null and a serial number is available, fall back to
certRepository.getX509Certificate(serial) to retrieve and return the
certificate.
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
readLine() returns null when stdin is closed or the command runs in a
non-interactive context (pipeline, CI). The subsequent
line.equalsIgnoreCase("Y") call then throws a NullPointerException.
Treat null the same as "N": abort the deletion when no confirmation
can be read, so the --force flag remains the only way to delete without
an interactive prompt.
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Use 'echo Y | docker exec -i' so that the delete command receives the confirmation via stdin rather than running non-interactively with a closed stdin. This exercises the fixed null-check code path in AuthorityRemoveCLI and avoids the --force flag, keeping the test consistent with real administrative usage. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Dogtag requires a CA to be explicitly disabled before it can be deleted (CAEngine rejects the delete with CAEnabledException if the authority is still enabled). Add a ca-authority-disable step before ca-authority-del to satisfy this precondition and exercise the full disable-then-delete lifecycle. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
getCACert() unconditionally dereferenced mSigningUnit, which is null for external-key authorities (initSigningUnits() returns early on the #external#: sentinel). The resulting NullPointerException was not caught by the EBaseException handlers in callers such as AuthorityRepository.readAuthorityData(), causing ca-authority-disable (and any other operation that calls getCACert() after modify) to fail with a blank PKIException. Apply the same repository fallback already used by getBinaryCert(): when mSigningUnit is null and authoritySerial is known, retrieve the X509CertImpl directly from certRepository. This makes getCACert() work correctly for external-key CAs everywhere it is called. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: #5336
Summary by CodeRabbit
New Features
Tests
Documentation