Pin release signing key and verify by default by volen-silo · Pull Request #95 · ROCm/rocm-cli

volen-silo · 2026-07-09T13:17:01Z

Summary

Turns on signed-release trust for the release channel: the production release signing public key is pinned in the installers, and signature verification is now on by default for install.sh release / install.ps1 release — downloaded archives are verified against the pinned trust root before install.

What's in here

Pinned key + default-on (install.sh, install.ps1): the release public key is embedded as a constant; verification is mandatory by default on the release channel. Env overrides (ROCM_CLI_SIGNING_PUBLIC_KEY_PATH/PEM) remain as escape hatches. Multi-key support is present for future rotation.
Build-time guard (scripts/release_readiness.py): CI verifies built archives against the configured signing public key, so a private/public key mismatch fails at release time, not on users' machines.
Consistency check (scripts/verify_pinned_keys.py): asserts the pinned constants match the canonical docs/keys/ file and the CI signing key. Runs in CI (PRs + release/nightly), embedding-agnostic (shell/PowerShell/Rust).
Canonical key (docs/keys/rocm-cli-release-current-public.pem).
Dormant scaffolding: metadata pinning (therock.rs, empty sentinel) and the production-trust CI gate (ROCM_CLI_REQUIRE_PRODUCTION_TRUST, unset) are wired but inert until their inputs exist — no behavior change from them.

Safety / rollout

No stable latest release exists yet (only pre-releases + a draft), so releases/latest/download has nothing to serve — enabling default-on cannot break existing users.
The pinned public key was confirmed to match the private key in the CI secret.
Metadata + recipe-index pinning are intentionally deferred (they need hosted artifacts); do not set ROCM_CLI_REQUIRE_PRODUCTION_TRUST yet — it requires those inputs.

Validation

Pinned key is a valid RSA-2048 key and matches both installer constants.
Sign → readiness-verify chain proven end to end; wrong key rejected.
verify_pinned_keys.py + release_readiness.py self-tests, metadata unit tests, clippy, fmt, shell/YAML checks all green.

Suggested pre-merge check

Cut a tag (or use the v0.1.0-rc.1 draft) so CI signs with the current key and confirm the readiness step reports "signature verified" — the final on-CI proof.

🤖 Generated with Claude Code

volen-silo · 2026-07-14T09:07:51Z

🔴 Automated review · pr-review-watcher · 1029b8c

Summary

Pins the production release signing public key into install.sh/install.ps1 and turns signature verification on by default for the release channel, with env/param escape hatches, current+next multi-key rotation, and CI gates (verify_pinned_keys.py, release_readiness.py) meant to keep the pinned key and the CI signing key in lockstep. Verdict: Needs work — the installer/Rust trust logic is correct and fails closed, but the CI consistency gate that couples the pinned constant to the CI signing key has a soundness bug that defeats its purpose during key rotation. Verified: RSA-2048 key is valid and byte-identical across all three embeddings + docs/keys/; both self-tests and release_readiness.py --self-test pass; installer multi-key verify loops fail closed (empty/mismatched key → fail, no silent bypass), escape-hatch precedence correct in shell/PS/Rust; the two blocking findings below were reproduced with runnable PoCs. Blocking: 2 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

scripts/verify_pinned_keys.py:146 (check_pinned_keys) — whole-file containment is an unsound equality proxy. The check does canonical_body in base64_only(source_file) over the entire source file, not against the specific pinned constant. So a wrong/tampered PINNED_RELEASE_PUBLIC_KEY_CURRENT passes as long as the canonical body appears anywhere in the file. Reproduced: with CURRENT set to a wrong key and the real canonical body present in the NEXT slot (a plausible mid-rotation state — exactly the multi-key flow this PR adds) or in a stale comment, check_pinned_keys() returns success. This is the only gate that ties the installer-pinned constant to the CI signing key (release_readiness.py verifies archives against the CI key, not the pinned constant), so its docstring claim of being a "reliable equality proxy" / "single source of truth" is the load-bearing property — and it's violated. Fix: extract each named pinned constant's own PEM body from its source (parse the specific PINNED_..._CURRENT/_NEXT assignment / Rust const) and compare that body for equality, instead of substring-scanning the whole file.

.github/workflows/ci.yml:68-79 — the PR-time consistency check is skippable for docs/keys/**-only changes. The new verify_pinned_keys.py step is gated on needs.changes.outputs.heavy == 'true', but the heavy paths-filter does not include docs/keys/**. A PR that edits only docs/keys/rocm-cli-release-current-public.pem (rotating the canonical key without touching an installer) never sets heavy=true, so the consistency check is skipped at review time for precisely the divergence it exists to catch. It compounds with the finding above. (nightly.yml/release.yml run the check unconditionally, so this is a PR-gate hole, not a release bypass — but it should still be closed.) Fix: add docs/keys/** to the heavy filter (and/or run verify_pinned_keys.py unconditionally in ci.yml).

Non-blocking

.github/workflows/nightly.yml / release.yml (env-block comments) and the PR body reference plans/release-production-trust-rollout.md, which does not exist in the repo — dead reference.
scripts/release_readiness.py:488,1020 — resolve_release_public_key() uses NamedTemporaryFile(delete=False) and never unlinks on the real main() path (only the self-test cleans up); minor temp-file leak on CI runners.
install.ps1:183 (Confirm-ArchiveSignature) — $key/$SignaturePath/$ArchivePath passed unquoted to native openssl; a path containing a space breaks the call (fails closed, not a bypass). Pre-existing pattern the PR extended to the new loop var; quote all three.
actions/checkout@v6 / actions-rust-lang/setup-rust-toolchain@v1 use moving tags (AGENTS.md §6 wants full-SHA pins). Confirmed pre-existing — this PR adds no uses: lines — so out of PR scope, noted for future hardening.

rominf

Please check the automated review. Also, there are efforts to get rid of the mix of programming languages in the repository. Could you please convert it to cargo xtask to keep everything as rusty as possible?

Pin the production release signing public key in install.sh and install.ps1 and require signature verification by default on the release channel, so downloaded archives are checked against the pinned trust root before install. Guard the key end to end: release_readiness.py verifies built archives against the configured signing public key (a key mismatch fails at build time, not on users' machines), and verify_pinned_keys.py asserts the pinned constants match the canonical docs/keys file and the CI signing key. Both run in CI. Metadata pinning and the production-trust CI gate are wired but dormant (empty sentinel / unset variable) until their inputs exist. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>

+        fn new(tag: &str) -> Result<Self> {
+            let base =
+                std::env::temp_dir().join(format!("pinned-key-{tag}-{}", std::process::id()));
+            let _ = std::fs::remove_dir_all(&base);


+            let base =
+                std::env::temp_dir().join(format!("pinned-key-{tag}-{}", std::process::id()));
+            let _ = std::fs::remove_dir_all(&base);
+            std::fs::create_dir_all(&base)?;


Replace the Python pinned-key consistency check with a Rust `cargo xtask verify-pinned-keys` command, keeping the repo all-Rust. The port fixes an unsound equality proxy: the old check used whole-file substring containment, so a wrong pinned constant passed whenever the canonical key body appeared anywhere else in the file (e.g. the NEXT rotation slot). The command now extracts each named pinned constant's own PEM body (shell, PowerShell, and Rust embeddings) and compares it for equality against the canonical docs/keys file, plus the CI signing key cross-check. Close the CI gate hole by adding docs/keys/** to the heavy paths filter so a canonical-key-only change still runs the check, and repoint every CI call site to the xtask command. Teach `cargo xtask verify` to read the public key from ROCM_CLI_SIGNING_PUBLIC_KEY_PEM when no --public-key is given, so release_readiness.py no longer materializes a temp key file. Also quote the openssl path arguments in install.ps1 and drop a stale doc reference from the workflow comments. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>

github-advanced-security AI found potential problems Jul 9, 2026

View reviewed changes

volen-silo force-pushed the production-keys branch 3 times, most recently from b5293cb to f71bb1a Compare July 9, 2026 13:36

volen-silo requested review from fredespi and juhovainio July 9, 2026 13:41

volen-silo force-pushed the production-keys branch 2 times, most recently from fe8292c to 1029b8c Compare July 9, 2026 14:01

rominf requested changes Jul 14, 2026

View reviewed changes

volen-silo force-pushed the production-keys branch from 1029b8c to 4132eb8 Compare July 15, 2026 11:16

github-advanced-security AI found potential problems Jul 15, 2026

View reviewed changes

volen-silo force-pushed the production-keys branch from 4132eb8 to 02a5383 Compare July 15, 2026 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Pin release signing key and verify by default#95

Pin release signing key and verify by default#95
volen-silo wants to merge 2 commits into
mainfrom
production-keys

volen-silo commented Jul 9, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

volen-silo commented Jul 14, 2026

Uh oh!

rominf left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

volen-silo commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What's in here

Safety / rollout

Validation

Suggested pre-merge check

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

volen-silo commented Jul 14, 2026

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

volen-silo commented Jul 9, 2026 •

edited

Loading