feat(security): docs and hooks to support cryptographic signing of commits#155
feat(security): docs and hooks to support cryptographic signing of commits#155ryanheffernan wants to merge 15 commits into
Conversation
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
📝 WalkthroughWalkthroughThe PR adds repository-local commit-signing readiness checks to hook installation and pre-commit execution, documents signing and approval workflows, and updates pull request instructions for verified commits and maintainer-approved testing. ChangesCommit-signing workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
scripts/configure-gpg-signing.sh (1)
55-65: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated GPG program discovery.
This
GPG_PROGRAM/gpg/gpg2discovery block is re-implemented almost verbatim inscripts/hooks/pre-commit(lines 56-70). Extracting a shared helper would prevent the two copies from drifting (see consolidated comment).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/configure-gpg-signing.sh` around lines 55 - 65, Extract the duplicated GPG executable discovery logic from the configure script and scripts/hooks/pre-commit into a shared helper, then update both callers to reuse it. Preserve the existing GPG_PROGRAM validation, gpg/gpg2 fallback order, and failure messages or equivalent behavior through the shared helper.scripts/tests/configure-gpg-signing.sh (1)
120-127: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSign-failure path is untested.
The stub already supports simulating a signing failure via
GPG_STUB_CAN_SIGN=0(lines 67-69), but no test case sets it, soconfigure-gpg-signing.sh's "GnuPG could not sign" branch (its lines 100-104) has no coverage alongside the missing-key case already tested here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/configure-gpg-signing.sh` around lines 120 - 127, Add a test case in the configure-gpg-signing test flow that sets GPG_STUB_CAN_SIGN=0 while providing an available key, invokes configure-gpg-signing.sh, and asserts it fails with the expected signing-error path. Keep the existing missing-key case unchanged and use the established test helpers and environment setup.scripts/hooks/pre-commit (1)
27-90: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicates GPG program/key-validation logic from
configure-gpg-signing.sh.The
gpg/gpg2discovery (lines 56-70) and secret-key existence check (lines 72-84) closely mirrorscripts/configure-gpg-signing.sh(lines 55-65, 78-84). A shared helper sourced by both scripts would avoid duplicated maintenance and the kind of drift already visible inscripts/install-hooks.sh's weaker status check (see consolidated comment below).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/hooks/pre-commit` around lines 27 - 90, Extract the shared GPG program discovery and secret-key validation used by check_gpg_signing and configure-gpg-signing.sh into a reusable helper sourced by both scripts. Update check_gpg_signing to call that helper instead of maintaining its own gpg/gpg2 lookup and list-secret-keys checks, while preserving the existing failure behavior and messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 120: Update the pull request checklist associated with “Submit a pull
request” to remove the stale “all commits signed off” wording, replacing it with
the repository’s current cryptographic signing requirement or removing the
checklist item entirely so it no longer references DCO sign-off.
- Around line 88-93: Complete the DCO-to-GPG migration consistently: in
CONTRIBUTING.md lines 88-93, align the documented installed hooks with the
installer by removing commit-msg enforcement or explicitly documenting its
continued use; in CONTRIBUTING.md line 120, remove or rewrite the stale “all
commits signed off” checklist requirement; and in README.md lines 67-85, make
the documented hook set match the installer and finalized contribution policy.
In `@scripts/configure-gpg-signing.sh`:
- Around line 119-136: Update the public-key export command in the heredoc to
use the resolved GPG_PROGRAM_PATH variable instead of hardcoded gpg, preserving
the existing fingerprint argument and user guidance.
In `@scripts/install-hooks.sh`:
- Around line 49-61: The GPG signing status check in the install-hooks script
omits the format requirement enforced by check_gpg_signing. Read the
repository’s gpg.format configuration and report signing as configured only when
it is unset or set to openpgp, in addition to commit.gpgsign being true and
user.signingkey being present.
---
Nitpick comments:
In `@scripts/configure-gpg-signing.sh`:
- Around line 55-65: Extract the duplicated GPG executable discovery logic from
the configure script and scripts/hooks/pre-commit into a shared helper, then
update both callers to reuse it. Preserve the existing GPG_PROGRAM validation,
gpg/gpg2 fallback order, and failure messages or equivalent behavior through the
shared helper.
In `@scripts/hooks/pre-commit`:
- Around line 27-90: Extract the shared GPG program discovery and secret-key
validation used by check_gpg_signing and configure-gpg-signing.sh into a
reusable helper sourced by both scripts. Update check_gpg_signing to call that
helper instead of maintaining its own gpg/gpg2 lookup and list-secret-keys
checks, while preserving the existing failure behavior and messages.
In `@scripts/tests/configure-gpg-signing.sh`:
- Around line 120-127: Add a test case in the configure-gpg-signing test flow
that sets GPG_STUB_CAN_SIGN=0 while providing an available key, invokes
configure-gpg-signing.sh, and asserts it fails with the expected signing-error
path. Keep the existing missing-key case unchanged and use the established test
helpers and environment setup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 274a0fbb-93f8-4593-b14c-1a7608ba3aa3
📒 Files selected for processing (8)
CONTRIBUTING.mdGOVERNANCE.mdMakefileREADME.mdscripts/configure-gpg-signing.shscripts/hooks/pre-commitscripts/install-hooks.shscripts/tests/configure-gpg-signing.sh
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)
scripts/hooks/pre-commit (1)
34-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse local Git config for signing checks
git config --getcan read global/system settings, so these checks can pass even when this repository has no local signing config. Switch them to--localto matchscripts/configure-gpg-signing.sh(including thegpg.programlookup inscripts/hooks/pre-commit).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/hooks/pre-commit` around lines 34 - 43, Update the signing checks in the pre-commit hook to read repository-local Git configuration by adding --local to the commit.gpgsign, user.signingkey, and gpg.format lookups, and apply the same local-scope lookup to the gpg.program check in the hook. Keep the existing OpenPGP defaulting and validation behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/PULL_REQUEST_TEMPLATE.md:
- Line 43: Restore the DCO checklist requirement in
.github/PULL_REQUEST_TEMPLATE.md alongside the GitHub Verified-signature item,
keeping the existing Signed-off-by enforcement in scripts/hooks/commit-msg and
related CI policy consistent.
---
Outside diff comments:
In `@scripts/hooks/pre-commit`:
- Around line 34-43: Update the signing checks in the pre-commit hook to read
repository-local Git configuration by adding --local to the commit.gpgsign,
user.signingkey, and gpg.format lookups, and apply the same local-scope lookup
to the gpg.program check in the hook. Keep the existing OpenPGP defaulting and
validation behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3896673f-3391-424f-99d6-95e68182c367
📒 Files selected for processing (5)
.github/PULL_REQUEST_TEMPLATE.mdCONTRIBUTING.mdREADME.mdscripts/hooks/pre-commitscripts/install-hooks.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/install-hooks.sh
- README.md
- CONTRIBUTING.md
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/tests/configure-gpg-signing.sh (1)
40-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated stub-environment prefix.
PATH="$test_root/bin:$PATH" HOME="$test_root/home"is repeated ~8 times across this function. A small helper reduces duplication and the risk of one call site drifting (e.g. missing a stub var).♻️ Suggested helper
+with_stub_env() { + PATH="$test_root/bin:$PATH" HOME="$test_root/home" "$@" +} + output="$( cd "$repo" - PATH="$test_root/bin:$PATH" HOME="$test_root/home" \ - "$bash_path" "$CONFIGURE_SCRIPT" 89ABCDEF01234567 + with_stub_env "$bash_path" "$CONFIGURE_SCRIPT" 89ABCDEF01234567 )"Given the function is already flagged high-complexity (170+ lines covering ~6 scenarios), consider also splitting it into scenario-specific functions (e.g.
test_configure_success,test_email_mismatch,test_hook_readiness,test_installer_commit_msg) sharing a common fixture setup, for easier debugging when one assertion fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/configure-gpg-signing.sh` around lines 40 - 213, Extract the repeated PATH and HOME setup from run_suite into a small reusable helper, then use it at every configure-gpg-signing.sh and pre-commit invocation while preserving each call’s scenario-specific environment variables. Keep the existing test behavior unchanged; splitting run_suite into scenario-specific functions is optional and outside the required cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/tests/configure-gpg-signing.sh`:
- Around line 40-213: Extract the repeated PATH and HOME setup from run_suite
into a small reusable helper, then use it at every configure-gpg-signing.sh and
pre-commit invocation while preserving each call’s scenario-specific environment
variables. Keep the existing test behavior unchanged; splitting run_suite into
scenario-specific functions is optional and outside the required cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cec314ce-6308-4e27-b24d-bf7d00b0b84f
📒 Files selected for processing (8)
.github/PULL_REQUEST_TEMPLATE.mdCONTRIBUTING.mdREADME.mdscripts/configure-gpg-signing.shscripts/hooks/commit-msgscripts/hooks/pre-commitscripts/install-hooks.shscripts/tests/configure-gpg-signing.sh
💤 Files with no reviewable changes (1)
- scripts/hooks/commit-msg
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/PULL_REQUEST_TEMPLATE.md
- README.md
- scripts/configure-gpg-signing.sh
- CONTRIBUTING.md
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
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 (2)
scripts/install-hooks.sh (2)
57-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not report signing ready from configuration alone.
A deleted secret key or missing configured
gpg.programstill produces this success message, whereasscripts/hooks/pre-commitwarns that signing is unavailable. Either perform the same availability checks or describe this as configuration being set rather than operational signing readiness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-hooks.sh` around lines 57 - 58, Update the signing message in the configuration check around GPG_SIGNING_ENABLED, GPG_SIGNING_KEY, and GPG_SIGNING_FORMAT so it does not claim operational signing readiness without verifying the secret key and configured gpg.program are available. Reuse the availability checks used by scripts/hooks/pre-commit, or change the message to state only that signing configuration is set.
30-30: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the retired
commit-msghook from the installation contract.The PR objective says this hook is retired, but these changes still install, advertise, and test its DCO enforcement—so it will continue blocking commits.
scripts/install-hooks.sh#L30-L30: install only the supported hook(s).scripts/install-hooks.sh#L46-L46: remove the retired-hook status entry.scripts/tests/configure-gpg-signing.sh#L22-L22: removeCOMMIT_MSG_HOOK.scripts/tests/configure-gpg-signing.sh#L50-L51: remove commit-message test state.scripts/tests/configure-gpg-signing.sh#L186-L216: remove copying and enforcement assertions for the retired hook.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-hooks.sh` at line 30, Remove the retired commit-msg hook from the installation and DCO test contract: update scripts/install-hooks.sh lines 30-30 to install only supported hooks and lines 46-46 to remove its status entry; in scripts/tests/configure-gpg-signing.sh remove COMMIT_MSG_HOOK at lines 22-22, commit-message test state at lines 50-51, and the retired-hook copying and enforcement assertions at lines 186-216.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 161-165: Remove the retired commit-msg hook from the documented
hook set and update the installer and hook files so installation no longer
creates or enforces it. In CONTRIBUTING.md lines 161-165 and README.md lines
74-76, remove or revise the corresponding installation and DCO-enforcement
descriptions while keeping documentation consistent with the remaining hooks.
---
Outside diff comments:
In `@scripts/install-hooks.sh`:
- Around line 57-58: Update the signing message in the configuration check
around GPG_SIGNING_ENABLED, GPG_SIGNING_KEY, and GPG_SIGNING_FORMAT so it does
not claim operational signing readiness without verifying the secret key and
configured gpg.program are available. Reuse the availability checks used by
scripts/hooks/pre-commit, or change the message to state only that signing
configuration is set.
- Line 30: Remove the retired commit-msg hook from the installation and DCO test
contract: update scripts/install-hooks.sh lines 30-30 to install only supported
hooks and lines 46-46 to remove its status entry; in
scripts/tests/configure-gpg-signing.sh remove COMMIT_MSG_HOOK at lines 22-22,
commit-message test state at lines 50-51, and the retired-hook copying and
enforcement assertions at lines 186-216.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 64e39011-5570-45db-8567-1f605a8e3735
📒 Files selected for processing (5)
.github/PULL_REQUEST_TEMPLATE.mdCONTRIBUTING.mdREADME.mdscripts/install-hooks.shscripts/tests/configure-gpg-signing.sh
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
…/configure-gpg-signing Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
5fdc8dd to
c88dcee
Compare
ryanheffernan
changed the title
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)
CONTRIBUTING.md (1)
21-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language identifiers to the fenced code blocks.
Use
textfor these prose examples to satisfy Markdown linting.Proposed fix
-``` +```text ... -``` +```textAlso applies to: 61-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.md` at line 21, Update the fenced code blocks in CONTRIBUTING.md, including the blocks at the referenced prose examples, to specify the text language identifier (```text) while preserving their contents.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@CONTRIBUTING.md`:
- Line 21: Update the fenced code blocks in CONTRIBUTING.md, including the
blocks at the referenced prose examples, to specify the text language identifier
(```text) while preserving their contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4f5bba39-cd7a-468d-92c1-f749376e26ce
📒 Files selected for processing (6)
CONTRIBUTING.mdMakefileREADME.mdscripts/hooks/pre-commitscripts/install-hooks.shscripts/tests/configure-gpg-signing.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/install-hooks.sh
- Makefile
- README.md
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
ryanheffernan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install-hooks.sh`:
- Around line 53-68: Update the signing readiness branch in
scripts/install-hooks.sh to validate the configured signer executable and
required OpenPGP secret key using the same read-only checks as the pre-commit
hook before reporting signing as configured. Keep the incomplete message for
missing or invalid readiness, including unsupported formats, so “configured” is
emitted only when signing can actually be used.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7d00912f-1f8c-4ddf-b5be-1cd48cbe292f
📒 Files selected for processing (4)
CONTRIBUTING.mdREADME.mdscripts/hooks/pre-commitscripts/install-hooks.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- CONTRIBUTING.md
- scripts/hooks/pre-commit
Signed-off-by: Ryan Heffernan <rheffernan@nvidia.com>
ryanheffernan
changed the title
1 participant
Description
Document and validate cryptographic commit signing for trustees who rely on
copy-pr-bot automatic sync, without prompting external contributors who receive
no auto-sync benefit.
Copy-pr-bot uses GitHub's
commit.verification.verifiedstatus when decidingwhether a trusted pull request can auto-sync. Verified commits from trustees will automatically sync to the protected
pull-request/*branch and trigger CI runs without needing to prompt withok to test. Note that all commits in a PR must be verified for this to work.Commits from non-trustees or any commits that aren't verified will still require a trustee to approve the sync via
/ok to test.Validation
No need to run kind, docs and git hooks update only.
Local validation
The kind integration test is manual due to taking ~30 min to complete. Ready pull requests from
configured trustees auto-sync when every commit is GitHub
Verified. Other pull requests,including draft pull requests, require an authorized maintainer to approve the exact current
commit. Approval must be repeated after the pull request is updated.
For a pull request that requires approval, an authorized maintainer first comments:
After copy-pr-bot has synced the current commit, start the suite with:
As a fallback, run Actions -> Kind Integration -> Run workflow against the copy-pr-bot generated
pull-request/<PR_NUMBER>branch. Use the defaulttest_pathfor the full suite, or narrow itonly while debugging.
The completed workflow updates this PR description with its conclusion and exact run URL.
Passing Kind Integration run, if not automatically reported:
Checklist
CONTRIBUTING.md.docs screenshots, or Helm/rendered outputs.
Summary by CodeRabbit
Summary by CodeRabbit
Documentation
New Features
commit-msgcontinues enforcing DCOSigned-off-bytrailers.Workflow
/ok to test <sha>approval before running Kind tests.