feat(sec): cosign keyless signing + verify-install + unsafe-unverified contract (genie-supply-chain-signing)#1363
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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. Comment |
![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: 124fb4d165
ℹ️ 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".
| if (options.bundleDir) return options.bundleDir; | ||
| if (options.tarball) return dirname(resolve(options.tarball)); | ||
| return resolve(genieRoot); |
There was a problem hiding this comment.
The --tarball option is documented as targeting a specific artifact, but this code discards the filename (dirname(resolve(options.tarball))) and then discoverSignatureBundle verifies the first *.tgz it finds in that directory. If a directory contains multiple release bundles, genie sec verify-install --tarball <path> can return success for a different tarball than the one the operator asked to check, which creates a false verification result in exactly the scenario this command is meant to secure.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the genie sec verify-install command and establishes a supply chain signing contract using Cosign keyless signing and SLSA provenance. It introduces centralized validation logic for the --unsafe-unverified flag, a local verification script, and a detailed key rotation runbook. Feedback focuses on resolving non-deterministic file discovery in directories with multiple artifacts, ensuring shell script portability across different operating systems (specifically macOS), and tightening the regex for incident identifiers.
| for (const tarballName of candidates) { | ||
| const tarball = join(bundleDir, tarballName); | ||
| const signature = `${tarball}.sig`; | ||
| const certificate = `${tarball}.cert`; | ||
| if (!deps.existsSync(signature)) continue; | ||
| if (!deps.existsSync(certificate)) continue; | ||
| const provenancePath = join(bundleDir, 'provenance.intoto.jsonl'); | ||
| const provenance = deps.existsSync(provenancePath) ? provenancePath : null; | ||
| return { tarball, signature, certificate, provenance }; | ||
| } |
There was a problem hiding this comment.
The discoverSignatureBundle function returns the first .tgz file it finds that has matching .sig and .cert siblings. If a directory contains multiple release tarballs (e.g., from multiple downloads), this behavior is non-deterministic and might verify a different version than the one intended by the operator. Consider throwing an error or requiring an explicit selection if multiple candidates are found.
| ;; | ||
| --local) | ||
| [ -n "${2:-}" ] || usage | ||
| tarball="$(readlink -f "$2")" |
There was a problem hiding this comment.
The readlink -f command is not POSIX-compliant and is unavailable on macOS by default (unless coreutils is installed). Since this script is intended for local operator use, it should use a more portable method to resolve absolute paths.
| tarball="$(readlink -f "$2")" | |
| tarball="$(cd "$(dirname "$2")" && pwd)/$(basename "$2")" |
| --pattern 'provenance.intoto.jsonl' \ | ||
| || { echo "error: release assets missing — exit 5" >&2; exit 5; } | ||
| ) | ||
| tarball="$(ls "${workdir}"/*.tgz 2>/dev/null | head -1)" |
There was a problem hiding this comment.
Parsing the output of ls is generally discouraged as it can fail with filenames containing special characters (like newlines). Additionally, using head -1 makes the selection non-deterministic if multiple .tgz files are present in the temporary directory. It is safer to use a glob expansion into an array.
| tarball="$(ls "${workdir}"/*.tgz 2>/dev/null | head -1)" | |
| local tgz_files=("${workdir}"/*.tgz) | |
| tarball="${tgz_files[0]}" |
| * faithful form: UPPER-snake prefix (at least one letter, trailing underscore | ||
| * before the date), YYYY_MM_DD, and an optional alphanumeric `_extra` tail. | ||
| */ | ||
| export const INCIDENT_ID_REGEX = /^[A-Z][A-Z0-9_]*_[0-9]{4}_[0-9]{2}_[0-9]{2}(_[A-Za-z0-9_]+)?$/; |
There was a problem hiding this comment.
The regex [A-Z][A-Z0-9_]* allows for multiple consecutive underscores or a trailing underscore before the date, which might deviate from the strict UPPER_SNAKE requirement mentioned in the documentation. While the current regex is functional, a stricter pattern like [A-Z]+(_[A-Z0-9]+)* would better enforce the snake_case convention.
| export const INCIDENT_ID_REGEX = /^[A-Z][A-Z0-9_]*_[0-9]{4}_[0-9]{2}_[0-9]{2}(_[A-Za-z0-9_]+)?$/; | |
| export const INCIDENT_ID_REGEX = /^[A-Z]+(_[A-Z0-9]+)*_[0-9]{4}_[0-9]{2}_[0-9]{2}(_[A-Za-z0-9_]+)?$/; |
|
|
||
| ```bash | ||
| # 1. Stand up a disposable fixture directory under /tmp. No production repos. | ||
| export DRILL=$(mktemp -d -t genie-signing-drill-XXXXXX) |
There was a problem hiding this comment.
The mktemp -t flag behavior varies between GNU and BSD (macOS) implementations. On macOS, -t requires a prefix argument, whereas on Linux it is often optional or behaves differently. For a runbook intended to be executed by operators on various platforms, using a more compatible mktemp invocation is recommended.
| export DRILL=$(mktemp -d -t genie-signing-drill-XXXXXX) | |
| export DRILL=$(mktemp -d 2>/dev/null || mktemp -d -t 'genie-signing-drill') |
namastex888
force-pushed
the
supply-chain-signing
branch
from
124fb4d to
5780aa5
Compare
1 participant
Summary
Lands the supply-chain signing stack from the canisterworm umbrella (#1360). Two groups:
G1 — Release signing CI pipeline
.github/workflows/release.yml— cosign KEYLESS signing (OIDC via GitHub Actions) + SLSA Level 3 provenance. Signs before the GitHub Release is created, so a broken signature or failed self-verify guarantees no release assets are published. In-band tamper-detection self-test gates every release..github/cosign.pub— explicit NO-PINNED-KEY sentinel (not a PEM). There is no long-lived fallback key anywhere (no repo secret, no HSM, no ceremony). Any tool loading this expecting a PEM must fail closed..github/ISSUE_TEMPLATE/signing-key-fingerprint.md— redirects operator pinned-key questions toward the real verification contract:certificate-identity-regexp+certificate-oidc-issuer.scripts/verify-release.sh(+bun run verify:releasealias) — operator-facing script that pinscert-identity-regexp,cert-oidc-issuer, and provenancesource-uri. Never reads a key fingerprint.G2 — verify-install subcommand + --unsafe-unverified contract
src/sec/unsafe-verify.ts— single source of truth for the--unsafe-unverified <INCIDENT_ID>escape hatch:INCIDENT_ID_REGEX,TYPED_ACK_PREFIX,LEGITIMATE_CONTEXTS,validateUnsafeUnverified. Council-mandated (M2 → HIGH) to prevent divergent implementations eroding friction.src/term-commands/sec.ts—genie sec verify-installsubcommand: runscosign verify-blob+slsa-verifier verify-artifact, pins signer identity regexp + OIDC issuer, supports--offline,--json,--tarball,--bundle-dir. Public exit-code contract:VERIFIED(0)/SIGNATURE_INVALID(2)/SIGNER_IDENTITY_MISMATCH(3)/PROVENANCE_INVALID(4)/NO_SIGNATURE_MATERIAL(5)/MISSING_BINARY(127). Treats thecosign.pubsentinel as exit 5 (no signature material) — no false positives.docs/security/key-rotation.md— operator runbook making clear there is no key to rotate: only cert-identity or OIDC-issuer changes matter.Tests
bun test src/sec/unsafe-verify.test.ts— 35 pass / 0 failbun test src/term-commands/sec.test.ts— 18 pass / 0 fail (includes sentinel → exit-5 guard)bun run typecheck— cleanbun run lint— 670 files, no fixes neededReview dossier:
.genie/wishes/genie-supply-chain-signing/REVIEW.md.Integration follow-up (deliberately out of scope)
sec-remediatelanded in #1361 with a stub validator for its--unsafe-unverifiedflag. A follow-up integration PR will wireremediate/restore/rollbacktovalidateUnsafeUnverifiedfromsrc/sec/unsafe-verify.ts. That's whyscripts/sec-scan.cjsandscripts/sec-remediate.cjsare intentionally untouched in this PR — single-abstraction focus.This PR unblocks the last wish in the umbrella:
sec-incident-runbook.Test plan
bun test src/sec/unsafe-verify.test.tspasses on CIbun test src/term-commands/sec.test.tspasses on CIbun run typecheck+bun run lintclean on CIrelease.ymlproduces signed tarball + provenance andgenie sec verify-installreturns exit 0🤖 Generated with Claude Code