chore: add linux package and checksum signing - #236
Conversation
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.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:
📝 WalkthroughWalkthroughThe release workflow now creates draft releases, signs and verifies Linux packages and checksum manifests, replaces draft assets, and publishes verified releases. New scripts, a Fleet Intelligence public key, release instructions, and checksum/signature verification guidance support the process. ChangesRelease signing and verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GoReleaser
participant ReleaseWorkflow
participant NVSEC
participant Verifier
participant GitHubRelease
GoReleaser->>GitHubRelease: Create draft release
ReleaseWorkflow->>NVSEC: Submit DEB/RPM packages
NVSEC-->>ReleaseWorkflow: Return signed artifacts
ReleaseWorkflow->>Verifier: Verify packages and checksum manifest
Verifier-->>ReleaseWorkflow: Confirm Fleet Intelligence signature
ReleaseWorkflow->>GitHubRelease: Replace draft assets
ReleaseWorkflow->>GitHubRelease: Publish verified release
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1364b1349a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
scripts/verify-linux-package-signature.sh (1)
30-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared NVSEC/3S signing + key-fingerprint helpers. The Fleet Intelligence
EXPECTED_FINGERPRINTconstant and its extraction/compare logic, the requiredNVSEC_SSA_CLIENT_ID/NVSEC_SSA_CLIENT_SECRETguards, and the3s submitargs-array construction are each hand-duplicated across these scripts. A future key rotation or NVSEC CLI flag change would need to be applied consistently in multiple places, risking drift.
scripts/verify-linux-package-signature.sh#L30-L44: keep as the canonicalverify_key_fingerprint/EXPECTED_FINGERPRINTdefinition and move it into a small sourced helper (e.g.scripts/lib/fleet-intelligence-key.sh).scripts/sign-checksum-manifest.sh#L63-L70: source the shared fingerprint-check helper instead of re-implementing the samegpg --show-keys --with-colons+ compare logic.scripts/sign-linux-packages.sh#L46-L59: extract the3s submitargs-array construction (job_type/description/input_file/download/auth/scope/timeout/result_dir/result_filename/nspect_id) into a shared helper function taking the varying fields as parameters.scripts/sign-checksum-manifest.sh#L33-L46: reuse the same shared3s submithelper instead of re-declaring the identical args pattern.🤖 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/verify-linux-package-signature.sh` around lines 30 - 44, Extract the canonical EXPECTED_FINGERPRINT and verify_key_fingerprint logic from scripts/verify-linux-package-signature.sh into a sourced shared helper, then update scripts/sign-checksum-manifest.sh lines 63-70 to reuse it. Also extract the shared 3s submit args-array construction from scripts/sign-linux-packages.sh lines 46-59 into a parameterized helper, and update scripts/sign-linux-packages.sh lines 46-59 and scripts/sign-checksum-manifest.sh lines 33-46 to use it, preserving the existing varying job fields and NVSEC credential guards..goreleaser.yaml (1)
180-191: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerification instructions never show how to check the fingerprint.
The footer tells users to "confirm" the key fingerprint but the code block goes straight to
gpg --importwithout a command to actually inspect the downloaded key first. Without that step, users have no way to detect a substituted key before trusting it.♻️ Proposed fix
Confirm that `fleet-intelligence.pub.asc` has fingerprint `FE0C 8B74 CA66 357C 13BE 197D CCE3 C963 0871 99B3`, then authenticate the checksum manifest: ```bash + gpg --show-keys --with-colons fleet-intelligence.pub.asc | awk -F: '$1=="fpr"{print $10}' gpg --import fleet-intelligence.pub.asc gpg --verify checksums.txt.asc checksums.txt ```🤖 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 @.goreleaser.yaml around lines 180 - 191, Update the verification instructions in the release documentation to include a command that extracts and displays the downloaded key’s fingerprint before importing it. Keep the expected fingerprint visible for manual comparison, then retain the existing gpg --import and gpg --verify commands in their current order.docs/install-rpm.md (1)
74-95: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo command given to verify the key fingerprint before import.
Same gap as
docs/install-deb.mdand the.goreleaser.yamlfooter.🤖 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 `@docs/install-rpm.md` around lines 74 - 95, Add a command in the RPM installation instructions before gpg --import that verifies the downloaded public key fingerprint matches FE0C 8B74 CA66 357C 13BE 197D CCE3 C963 0871 99B3, consistent with docs/install-deb.md and the .goreleaser.yaml footer..github/workflows/release.yml (1)
58-70: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the
nvsecpackage version.
pip install nvsechas no version constraint, so the signing tool used for every release artifact can silently change behavior between runs. For a tool this security-critical, pin an exact version and bump deliberately.♻️ Proposed fix
- "$RUNNER_TEMP/nvsec-venv/bin/pip" install nvsec \ + "$RUNNER_TEMP/nvsec-venv/bin/pip" install "nvsec==<pinned-version>" \ --index-url "https://gitlab-master.nvidia.com/api/v4/projects/25923/packages/pypi/simple" \ --extra-index-url "https://urm.nvidia.com/artifactory/api/pypi/sw-cftt-pypi-local/simple"🤖 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 @.github/workflows/release.yml around lines 58 - 70, Update the nvsec installation command in the “Install nvsec” workflow step to require an exact, explicitly chosen package version instead of installing the unversioned package. Keep the existing index configuration and virtual-environment setup unchanged so future version changes are deliberate.docs/install-deb.md (1)
49-70: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo command given to verify the key fingerprint before import.
Same gap as the
.goreleaser.yamlfooter: users are told the expected fingerprint but the shown commandsgpg --importimmediately without a preceding check.🤖 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 `@docs/install-deb.md` around lines 49 - 70, Add a command before gpg --import in the installation instructions to verify the downloaded public key matches the documented fingerprint, using the existing fleet-intelligence.pub.asc file and expected fingerprint. Keep the subsequent signature and checksum verification commands 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 `@scripts/refresh-checksums.sh`:
- Around line 42-61: Update the checksum-processing loop around read so it also
processes a final checksum record when checksum_file lacks a trailing newline.
Preserve the existing validation, artifact verification, checksum generation,
and artifact_count behavior for both newline-terminated and unterminated
records.
In `@scripts/verify-linux-package-signature.sh`:
- Around line 107-128: Update the failure condition in verify_rpm to include BAD
alongside NOT OK, NOKEY, and NOTTRUSTED when matching rpm --checksig output,
ensuring digest or signature failures cause the script to exit nonzero.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 58-70: Update the nvsec installation command in the “Install
nvsec” workflow step to require an exact, explicitly chosen package version
instead of installing the unversioned package. Keep the existing index
configuration and virtual-environment setup unchanged so future version changes
are deliberate.
In @.goreleaser.yaml:
- Around line 180-191: Update the verification instructions in the release
documentation to include a command that extracts and displays the downloaded
key’s fingerprint before importing it. Keep the expected fingerprint visible for
manual comparison, then retain the existing gpg --import and gpg --verify
commands in their current order.
In `@docs/install-deb.md`:
- Around line 49-70: Add a command before gpg --import in the installation
instructions to verify the downloaded public key matches the documented
fingerprint, using the existing fleet-intelligence.pub.asc file and expected
fingerprint. Keep the subsequent signature and checksum verification commands
unchanged.
In `@docs/install-rpm.md`:
- Around line 74-95: Add a command in the RPM installation instructions before
gpg --import that verifies the downloaded public key fingerprint matches FE0C
8B74 CA66 357C 13BE 197D CCE3 C963 0871 99B3, consistent with
docs/install-deb.md and the .goreleaser.yaml footer.
In `@scripts/verify-linux-package-signature.sh`:
- Around line 30-44: Extract the canonical EXPECTED_FINGERPRINT and
verify_key_fingerprint logic from scripts/verify-linux-package-signature.sh into
a sourced shared helper, then update scripts/sign-checksum-manifest.sh lines
63-70 to reuse it. Also extract the shared 3s submit args-array construction
from scripts/sign-linux-packages.sh lines 46-59 into a parameterized helper, and
update scripts/sign-linux-packages.sh lines 46-59 and
scripts/sign-checksum-manifest.sh lines 33-46 to use it, preserving the existing
varying job fields and NVSEC credential guards.
🪄 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: b701100d-22d6-47ce-b031-69116c817ddd
📒 Files selected for processing (10)
.github/workflows/release.yml.goreleaser.yamldeployments/packages/keys/fleet-intelligence.pub.ascdocs/development.mddocs/install-deb.mddocs/install-rpm.mdscripts/refresh-checksums.shscripts/sign-checksum-manifest.shscripts/sign-linux-packages.shscripts/verify-linux-package-signature.sh
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
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)
.github/workflows/release.yml (1)
107-109: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin both
actions/github-scriptsteps to a commit SHA
.github/workflows/release.yml#L107-L109.github/workflows/release.yml#L279-L281
actions/github-script@v7is a mutable ref; pinning both release steps to a full commit SHA prevents the action code from changing under the same tag.🤖 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 @.github/workflows/release.yml around lines 107 - 109, Pin both actions/github-script steps in .github/workflows/release.yml at lines 107-109 and 279-281 to the same full commit SHA instead of the mutable `@v7` reference, preserving each step’s existing configuration.
🤖 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 @.github/workflows/release.yml:
- Around line 107-109: Pin both actions/github-script steps in
.github/workflows/release.yml at lines 107-109 and 279-281 to the same full
commit SHA instead of the mutable `@v7` reference, preserving each step’s existing
configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 33cf95f3-c134-4258-a636-e7aa37d70e19
📒 Files selected for processing (1)
.github/workflows/release.yml
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
| Confirm that the public key fingerprint is: | ||
|
|
||
| ```text | ||
| FE0C 8B74 CA66 357C 13BE 197D CCE3 C963 0871 99B3 |
There was a problem hiding this comment.
is this key an existing, already-published NVIDIA release key authoritative outside this repo? If so, better to link it here in md.
There was a problem hiding this comment.
The public key will be published in this repo and release artifact.
There was a problem hiding this comment.
if i understand correctly, the steps are: sign packages -> refresh checksums -> sign the manifest? this script re-hashes every artifact listed in checksums.txt and overwrites each entry unconditionally. It never compares against what GoReleaser computed. if i seed a wrong hash for an artifact that the signing step never touches, and the script will silently corrected it and exited 0
There was a problem hiding this comment.
So the goreleaser generated deb/rpm package is not signed, after it is signed, the checksum will change, so the pipeline has to sign the changed checksum.
There was a problem hiding this comment.
I agree with the chain: rewrites the deb/rpm and SHA-256 changes, so the manifest has to be regenerated before it's signed. I wasn't asking whether the refresh step should exist - the step in the workflow sounds about right. My point was about its scope. sign-linux-packages.sh only check deb/rpm, so those are the only files whose hashes can legitimately change. The tarballs and every other GoReleaser artifact must be identical. But refresh-checksums.sh re-hashes every line unconditionally, so it can't tell "expected change" from "unexpected change"
2 participants
Description
Checklist
Summary by CodeRabbit
New Features
checksums.txtis refreshed, and a detached checksum-manifest signature is generated and verified before publishing.Documentation