Add test for PKI CLI with Kryoptic#5341
Open
edewata wants to merge 1 commit intodogtagpki:masterfrom
Open
Conversation
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
📝 WalkthroughWalkthroughAdds a reusable Kryoptic-backed HSM GitHub Actions workflow, updates the SoftHSM workflow with explicit install/validation steps and minor command adjustments, and wires the Kryoptic test into the tools-tests workflow as a new job. Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant Runner as PKI Runner Container
participant Kry as Kryoptic HSM
participant NSS as NSS CLI
participant DB as NSS Internal DB
GHA->>Runner: invoke reusable workflow
Runner->>Kry: install kryoptic & tooling, init token (SO/user PIN)
Runner->>DB: create & open internal password-protected NSS DB
rect rgba(70, 130, 180, 0.5)
Note over Runner,Kry: CA signing flow
Runner->>Kry: generate CA signing key
Runner->>NSS: create CA CSR (HSM-backed)
Runner->>Kry: issue/self-sign CA certificate
Runner->>Kry: import CA cert with trust flags
Runner->>NSS: verify cert/key present in HSM token
Runner->>DB: verify absent in internal DB
end
rect rgba(144, 238, 144, 0.5)
Note over Runner,Kry: SSL server & audit signing flow
Runner->>Kry: generate SSL & audit keys
Runner->>NSS: create CSRs (HSM-backed)
Runner->>Kry: issue certificates
Runner->>Kry: import certs with nicknames/trust flags
Runner->>NSS: verify HSM presence and trust flags
Runner->>DB: verify absence in internal DB
end
rect rgba(255, 165, 0, 0.5)
Note over Runner,Kry: Teardown
Runner->>Kry: delete certs/keys from HSM
Runner->>DB: attempt deletes in internal DB (expect "not found" where applicable)
Runner->>Kry: remove CA signing key, clean kryoptic config
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![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.
Actionable comments posted: 1
🤖 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/pki-nss-kryoptic-test.yml:
- Around line 650-655: The diff command at the end of the Kryoptic trust-flag
check is being masked by "|| true", so failures from comparing expected vs
actual (for HSM:audit_signing trust flags produced by nss-cert-mod) are ignored;
remove the "|| true" and either change the expected value from "u,u,Pu" to the
current known output "u,u,u" (write that into expected) so the test asserts the
known behavior, or mark the assertion as an expected failure (with a clear
comment referencing issue `#445`) until nss-cert-mod is fixed; ensure the sed
invocation that extracts HSM:audit_signing into actual and the subsequent diff
expected actual report a non-zero exit code on mismatch rather than being
silenced.
🪄 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: 5ca9f4a2-be94-4298-8f02-11874052bb01
📒 Files selected for processing (3)
.github/workflows/pki-nss-kryoptic-test.yml.github/workflows/pki-nss-softhsm-test.yml.github/workflows/tools-tests.yml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/pki-nss-kryoptic-test.yml (1)
610-668:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe Kryoptic trust-flag path is still effectively untested.
This block documents that
nss-cert-modis currently broken, then both assertions at Lines 655 and 668 ignore mismatches with|| true. That means the workflow stays green whether the trust flags change or not, so this step is not actually validating Kryoptic behavior. Either assert the current known output untillatchset/kryoptic#445is fixed, or mark the check as an explicit expected failure instead of swallowing the diffs.Suggested way to keep the test meaningful for the current behavior
- name: Check audit signing cert trust flags in HSM run: | - # trust flags should be u,u,Pu - # https://github.com/latchset/kryoptic/issues/445 - echo "u,u,Pu" > expected + # trust flags currently remain unchanged until + # https://github.com/latchset/kryoptic/issues/445 is fixed + echo "u,u,u" > expected sed -n 's/^HSM:audit_signing\s*\(\S\+\)\s*$/\1/p' output > actual - diff expected actual || true + diff expected actual docker exec pki pki \ -f $SHARED/password.conf \ nss-cert-show \ HSM:audit_signing \ | tee output - # trust flags should be u,u,Pu - # https://github.com/latchset/kryoptic/issues/445 - echo "u,u,Pu" > expected + # trust flags currently remain unchanged until + # https://github.com/latchset/kryoptic/issues/445 is fixed + echo "u,u,u" > expected sed -n 's/\s*Trust Flags:\s*\(\S\+\)\s*$/\1/p' output > actual - diff expected actual || true + diff expected actual🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pki-nss-kryoptic-test.yml around lines 610 - 668, The test currently swallows failures for the Kryoptic trust-flag assertions because the two diffs in the "Check audit signing cert trust flags in HSM" step end with "|| true", so the workflow will stay green even if the trust flags change; remove the "|| true" from the two diff commands that compare expected vs actual (the sed/diff pairs that produce actual from output and then run diff expected actual) so the step fails on mismatch, or if you want to explicitly document the known broken behavior instead, replace the step with an explicit expected-failure by adding a step-level "continue-on-error: true" and a clear comment referencing nss-cert-mod/Kryoptic issue latchset/kryoptic#445 and keep the current expected values (the sed invocations and "echo 'u,u,Pu' > expected") so the test either asserts the actual current output or is marked as an intentional expected failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/pki-nss-kryoptic-test.yml:
- Around line 610-668: The test currently swallows failures for the Kryoptic
trust-flag assertions because the two diffs in the "Check audit signing cert
trust flags in HSM" step end with "|| true", so the workflow will stay green
even if the trust flags change; remove the "|| true" from the two diff commands
that compare expected vs actual (the sed/diff pairs that produce actual from
output and then run diff expected actual) so the step fails on mismatch, or if
you want to explicitly document the known broken behavior instead, replace the
step with an explicit expected-failure by adding a step-level
"continue-on-error: true" and a clear comment referencing nss-cert-mod/Kryoptic
issue latchset/kryoptic#445 and keep the current expected values (the sed
invocations and "echo 'u,u,Pu' > expected") so the test either asserts the
actual current output or is marked as an intentional expected failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 908b713c-1122-4bee-836a-1c9e97864861
📒 Files selected for processing (3)
.github/workflows/pki-nss-kryoptic-test.yml.github/workflows/pki-nss-softhsm-test.yml.github/workflows/tools-tests.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tools-tests.yml
A new test has been added to check PKI CLI against Kryoptic. The test is similar to the existing test for SoftHSM, but there are some differences in how the trust flags are handled in Kryoptic.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/pki-nss-kryoptic-test.yml (1)
72-73: ⚡ Quick winTurn the NSS smoke checks into real Kryoptic assertions.
These
modutil -nocertdb -listcalls only prove that NSS is reachable; they do not fail if the Kryoptic module is absent. Adding a grep/diff for the expected Kryoptic module name would make setup failures obvious and keep the workflow from drifting into a later, harder-to-diagnose failure.Also applies to: 116-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pki-nss-kryoptic-test.yml around lines 72 - 73, Replace the loose NSS smoke checks that run "modutil -nocertdb -list" with assertions that verify the Kryoptic module is present: run "modutil -nocertdb -list" and pipe/scan its output for the expected module name (e.g., "kryoptic" or the exact module label used in your build), then fail the step if the grep/scan does not find the string (non-zero exit); update both occurrences referenced (the modutil call around line 72 and the second instance around 116) so the workflow explicitly checks for the Kryoptic module presence rather than only verifying NSS is reachable..github/workflows/pki-nss-softhsm-test.yml (1)
67-68: ⚡ Quick winMake the NSS checks assert that SoftHSM is actually registered.
modutil -nocertdb -listwill exit successfully even when the SoftHSM module is missing, so these steps currently print diagnostics rather than verify the setup. Please grep/diff the output for the expected SoftHSM module name so an NSS/p11-kit registration break fails here instead of later in the PKI CLI flow.Also applies to: 97-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pki-nss-softhsm-test.yml around lines 67 - 68, Replace the diagnostic-only commands that run "docker exec pki modutil -nocertdb -list" with an assertion that the SoftHSM module is registered: run the modutil list inside the container via "docker exec pki modutil -nocertdb -list" and check its output for the expected SoftHSM identifier (e.g., "SoftHSM" or the library filename like "libsofthsm" depending on your environment); if the grep fails, return a non-zero exit so the workflow fails. Apply the same change to the other occurrences of the modutil check (the ones referenced around lines 97-98) so both places verify presence rather than just printing diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/pki-nss-kryoptic-test.yml:
- Around line 72-73: Replace the loose NSS smoke checks that run "modutil
-nocertdb -list" with assertions that verify the Kryoptic module is present: run
"modutil -nocertdb -list" and pipe/scan its output for the expected module name
(e.g., "kryoptic" or the exact module label used in your build), then fail the
step if the grep/scan does not find the string (non-zero exit); update both
occurrences referenced (the modutil call around line 72 and the second instance
around 116) so the workflow explicitly checks for the Kryoptic module presence
rather than only verifying NSS is reachable.
In @.github/workflows/pki-nss-softhsm-test.yml:
- Around line 67-68: Replace the diagnostic-only commands that run "docker exec
pki modutil -nocertdb -list" with an assertion that the SoftHSM module is
registered: run the modutil list inside the container via "docker exec pki
modutil -nocertdb -list" and check its output for the expected SoftHSM
identifier (e.g., "SoftHSM" or the library filename like "libsofthsm" depending
on your environment); if the grep fails, return a non-zero exit so the workflow
fails. Apply the same change to the other occurrences of the modutil check (the
ones referenced around lines 97-98) so both places verify presence rather than
just printing diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5a4f2e7-25e6-467c-9286-ced1804a3675
📒 Files selected for processing (3)
.github/workflows/pki-nss-kryoptic-test.yml.github/workflows/pki-nss-softhsm-test.yml.github/workflows/tools-tests.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/tools-tests.yml
|
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.
A new test has been added to check PKI CLI against Kryoptic. The test is similar to the existing test for SoftHSM, but there are some differences in how the trust flags are handled in Kryoptic.
Summary by CodeRabbit