Replace the openssl CLI with pure-Rust signing & verification - #13
Conversation
Signature verification shelled out to the `openssl` CLI, which made it
depend on a transient subprocess spawn succeeding. Under the parallel
test run on CI, spawning openssl intermittently failed ("failed to
launch openssl"), flaking the verification tests; it also forced every
end user to have the openssl CLI on PATH at runtime.
Replace the openssl invocations with a pure-Rust RSASSA-PKCS#1 v1.5 /
SHA-256 verifier (rsa + sha2). A shared helper in rocm-core backs both
the THeRock metadata and the model-recipe-index paths; test helpers now
generate and sign keys in-process too. A committed openssl-produced test
vector pins byte-compatibility with `openssl dgst -verify` without ever
launching it.
Release signing, CI signature verification, and the acceptance-test keygen shelled out to the openssl CLI. Move them to a pure-Rust `cargo xtask` (keygen/sign/verify) over shared rocm-core helpers, so the build, CI, and test paths no longer require openssl. Signing is deterministic PKCS#1 v1.5, byte-identical to `openssl dgst -sha256 -sign`, so existing keys and the openssl-based installers keep working unchanged. - rocm-core: add sign_rsa_pkcs1_sha256_signature and generate_rsa_signing_keypair next to the existing verifier; rand becomes a normal dependency. - xtask: new crate plus a `cargo xtask` alias. - scripts: package-linux-release.sh, package-windows-release.ps1, release_readiness.py, and the acceptance scripts call the xtask. - tests: replace the hardcoded interop vector with a pure-Rust round-trip plus a skippable openssl cross-check that asserts byte-for-byte parity; shared helpers let apps/rocm drop its rsa/sha2/rand dev-deps. The end-user installers (install.sh, install.ps1) intentionally keep openssl: they verify before extracting anything, and the pinned public key must be checked by a tool that is not the just-downloaded artifact.
There was a problem hiding this comment.
Pull request overview
This PR removes runtime/release-time dependence on the external openssl CLI for RSA PKCS#1 v1.5 / SHA-256 signing & verification by introducing a shared pure-Rust implementation in rocm-core, and wires repo tooling/scripts to use a new cargo xtask interface.
Changes:
- Added pure-Rust RSA PKCS#1 v1.5 + SHA-256 signing/verification + keygen helpers to
rocm-core, and switched runtime signature checks to use them. - Introduced an
xtaskcrate (with acargo xtaskalias) to providekeygen,sign, andverifycommands backed by the sharedrocm-corehelpers. - Updated release/acceptance scripts and
release_readiness.pyto usecargo xtaskinstead of shelling out toopensslfor signing/verification workflows.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
xtask/src/main.rs |
New repo task runner CLI for keygen/sign/verify using rocm-core helpers. |
xtask/Cargo.toml |
Defines the new xtask crate and its dependencies. |
.cargo/config.toml |
Adds cargo xtask alias to run the xtask crate in release mode. |
crates/rocm-core/src/lib.rs |
Adds pure-Rust RSA sign/verify/keygen helpers and replaces OpenSSL-based verification for recipe-index signatures; adds interop + round-trip tests. |
crates/rocm-core/Cargo.toml |
Adds rand, rsa, and sha2 dependencies needed for the crypto helpers. |
apps/rocm/src/therock.rs |
Replaces metadata signature verification via OpenSSL with the new pure-Rust verifier; updates test signing helpers. |
scripts/release_readiness.py |
Switches signature verification to cargo xtask verify (run from repo root). |
scripts/package-linux-release.sh |
Switches archive signing to cargo xtask sign. |
scripts/package-windows-release.ps1 |
Switches archive signing to cargo xtask sign. |
scripts/acceptance-install-upgrade-tui-uninstall.sh |
Uses cargo xtask keygen and builds xtask in release builds for acceptance flow. |
scripts/acceptance-install-upgrade-tui-uninstall.ps1 |
Uses cargo xtask keygen and builds xtask in release builds for acceptance flow. |
Cargo.toml |
Adds xtask as a workspace member; adds workspace deps (rand, rsa, sha2). |
Cargo.lock |
Locks new transitive dependencies introduced by rsa/sha2/rand and xtask. |
Comments suppressed due to low confidence (1)
scripts/package-windows-release.ps1:89
- The signing step invokes
cargo xtaskvia the unqualifiedcargocommand and without ensuring the working directory is the repo root. This can fail if the script is run from outside the repo (Cargo won’t find the.cargo/config.tomlalias), and it also ignores the already-resolved$cargoExepath.
if ([string]::IsNullOrWhiteSpace($PrivateKeyPath)) {
return
}
& cargo xtask sign --private-key $PrivateKeyPath --in $ArchivePath --out "$ArchivePath.sig"
if ($LASTEXITCODE -ne 0) {
Fail "failed to sign $ArchivePath"
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- xtask verify labels failures with the input file path instead of a static "artifact", so errors name which file failed to verify. - package-linux-release.sh checks for cargo before signing, matching the old openssl presence check. - package-windows-release.ps1 runs cargo xtask from the repo root so the cargo alias resolves regardless of the caller's working directory.
|
Also addressed the low-confidence note on |
…dows - xtask keygen writes the private key as owner-only (0o600) on Unix so it is never group/world-readable. - package-windows-release.ps1 signs via the already-resolved $cargoExe instead of unqualified cargo, matching the rest of the script.
The pure-Rust RSA path uses the strict RFC 7468 PEM reader, which rejected private keys written by Windows tooling (PowerShell Set-Content adds a trailing blank line to the already-terminated PEM, surfaced as "PEM error in pre-encapsulation boundary"). The openssl CLI tolerated this, so the Windows release acceptance lifecycle broke after the switch. Normalize PEM input before parsing: strip a UTF-8 BOM, accept any line-ending style, drop trailing whitespace, and remove blank lines. Add a regression test covering CRLF, trailing-whitespace, and BOM variants. Also reword the interop test skip message to cover all skip causes, not just a missing CLI.
verify_signature runs the subprocess with cwd=repo_root() so the cargo alias resolves, but passed possibly-relative archive/signature/public-key paths, which would then resolve against the repo root instead of the caller's CWD. Resolve them to absolute paths so verification is correct regardless of where the script is invoked.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
scripts/package-linux-release.sh:60
- When
ROCM_CLI_SIGNING_PRIVATE_KEY_PEMis used, the script writes the private key to disk with default permissions (subject to umask), which can leave it group/world-readable. Since this is a signing private key (even if temporary), restrict the temp directory and key file permissions (e.g., 0700/0600) before runningcargo xtask sign.
SIGNING_TMP_DIR="${OUTPUT_DIR}/.signing-tmp-$$"
rm -rf "${SIGNING_TMP_DIR}"
mkdir -p "${SIGNING_TMP_DIR}"
if [[ -n "${ROCM_CLI_SIGNING_PRIVATE_KEY_PATH:-}" ]]; then
| let private_pem = fs::read_to_string(&private_key) | ||
| .with_context(|| format!("failed to read {}", private_key.display()))?; | ||
| let payload = | ||
| fs::read(&input).with_context(|| format!("failed to read {}", input.display()))?; | ||
| let signature = sign_rsa_pkcs1_sha256_signature(&private_pem, &payload)?; |
Summary
Removes the dependency on the external
opensslCLI for signature verification (at runtime) and signing (at release time), replacing both with a pure-Rust implementation built onrsa+sha2.openssl dgst -verify. A sharedverify_rsa_pkcs1_sha256_signaturehelper inrocm-coredoes RSASSA-PKCS#1 v1.5 / SHA-256 verification directly.xtask— release signing, CI signature verification, and the acceptance-test keygen now usecargo xtask {keygen,sign,verify}over the samerocm-corehelpers, instead of theopensslCLI. Scripts updated:package-linux-release.sh,package-windows-release.ps1,release_readiness.py, and the acceptance scripts.Why
CI intermittently failed with
Error: failed to launch openssl for metadata signature verification(e.g. run 27604998719). The test generated a key, signed, and verified — all viaopenssl— and only the verify spawn failed, after the same binary had already launched twice in the same test.Root cause: a transient
fork/posix_spawnfailure (EAGAIN/ENOMEM) under fork pressure while the full test suite runs in parallel on a memory-constrained runner. Shelling out toopensslfor a small, well-specified crypto operation made every verification depend on a subprocess spawn succeeding — and required theopensslCLI to be present at runtime for end users.Key decisions
openssl dgst -sha256 -sign. Existing keys and already-published signatures keep verifying, and the openssl-based installers stay interoperable. A test cross-checks Rust output against theopensslCLI byte-for-byte (skipped automatically whenopensslis absent, so it can never reintroduce the flake).rocm-corehosts the crypto; thextaskis a thin CLI over it, so there's one audited implementation shared by product code, tooling, and tests.Scope boundaries (intentionally unchanged)
install.sh,install.ps1) keepopenssl: they verify the archive against the pinned public key before extracting anything, and that check must be performed by a tool that is not the just-downloaded artifact.Risk
Low. Crypto is a standard, well-specified scheme implemented by the mature
rsacrate; production verification is public-key only; signing is deterministic and proven byte-compatible with the previousopenssloutput.Test plan
cargo build/test --workspace --all-targets,cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --all --checkall pass.openssl↔Rust byte-parity cross-check.cargo xtask keygen → sign → verifyround-trip, with the resulting signature also verified byopenssl dgst -sha256 -verify(Verified OK) and rejected after tampering.python scripts/release_readiness.py --self-testpasses.