publish: add --provenance (npm provenance via Sigstore keyless signing) - #30522
publish: add --provenance (npm provenance via Sigstore keyless signing)#30522robobun wants to merge 1 commit into
Conversation
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
All my earlier nits have been addressed and I didn't find anything new, but this is a ~1.3k-line cryptographic/supply-chain feature (new bun_sigstore crate, p256/ecdsa deps, Fulcio/Rekor protocol, OIDC token handling) that deserves a human pass on the protocol shape and dependency additions.
Extended reasoning...
Overview
This PR implements bun publish --provenance (npm provenance via Sigstore keyless signing), closing #15601. It introduces a new src/sigstore/ crate (~1000 LOC) that speaks the Fulcio/Rekor protocol directly over bun_http, generates SLSA in-toto statements from CI env vars, builds DSSE envelopes signed with an ephemeral P-256 key, and embeds the resulting Sigstore bundle in the registry PUT body. It also wires --provenance / --no-provenance / --provenance-file through CLI parsing and PublishConfig, adds docs, adds ~460 lines of integration tests against a mock Fulcio/Rekor/registry, and includes incidental as _ → as u16/i32 fixes in node_fs.rs / win_watcher.rs / options_jsc.rs exposed by linking serde_json into bun_runtime. New transitive deps land in Cargo.lock (p256, ecdsa, elliptic-curve, pkcs8, der, sec1, sha2, rfc6979, getrandom 0.2, etc.) and the x64 baseline-verify allowlists gain entries for sha2's SHA-NI path and a memchr count_raw symbol.
Security risks
This is squarely security-sensitive: it handles OIDC identity tokens (read from ACTIONS_ID_TOKEN_REQUEST_TOKEN / SIGSTORE_ID_TOKEN and sent as Bearer auth to Fulcio), generates ephemeral ECDSA keypairs via rand_core::OsRng, signs DSSE PAE encodings, parses PEM certificates by hand, hand-builds the canonical-JSON envelope hash for Rekor, and constructs the bundle that registries will verify for supply-chain provenance. The --provenance-file path deliberately skips full sigstore.verify() and only checks subject name/digest, deferring chain/tlog verification to the registry — a documented divergence from npm. The http_json helper Box::leaks request buffers (bounded to ≤3 calls per publish, so not a practical leak, but worth a glance). None of this looks wrong to me, but it's exactly the class of code where a second human pair of eyes on the wire-format fidelity vs. sigstore-js and the dependency surface is appropriate.
Level of scrutiny
High. This is a new user-facing feature implementing a cryptographic attestation protocol from scratch (rather than via sigstore-rs, for reasons documented in the PR description and Cargo.toml), pulling in ~20 new transitive crates, and producing artifacts that downstream consumers will trust for supply-chain integrity. The author has been very responsive — every nit I raised across four review rounds was either fixed or convincingly rebutted (the extract_jwt_subject behavior matches current sigstore-js post-#1485, not the older issuer-switch). But correctness of the bundle/tlog wire shapes and the decision to hand-roll vs. depend on sigstore-rs are design calls a maintainer should sign off on.
Other factors
Test coverage is solid for the happy path and preflight errors (mock OIDC/Fulcio/Rekor/registry server, asserts on request shapes and bundle contents). The musl build failure flagged by robobun was against an earlier commit (5708e5d) and predates the baseline-allowlist additions in 4c24826; CI status on HEAD (2f0a2fa) should be confirmed. The incidental as _ fixes are strictly more robust and low-risk.
…0527) ## WebKit changes (88b2f7a2 → 5488984d) Single commit on top of the previous pin: ### `module-loader: don't double-fire moduleRegistryModuleSettled after inline sync replay` (oven-sh/WebKit#225, rebased #217) **File:** `Source/JavaScriptCore/runtime/JSMicrotask.cpp` **What:** Adds a `modulePromise->status() != Pending` early-return guard to `moduleRegistryModuleSettled`, symmetric with the guard already present in `moduleRegistryFetchSettled`. Gated under `#if USE(BUN_JSC_ADDITIONS)`. **Why:** `require()` of an ESM whose graph contained a diamond dependency through a barrel deadlocked (release) / aborted on `ASSERTION FAILED: m_status == Status::Fetching` (debug). `hostLoadImportedModule`'s synchronous-replay branch (taken when `require(esm)` is draining the synchronous module queue) calls `fetchComplete` + fulfills `modulePromise` inline. If a `ModuleRegistryFetchSettled` reaction had already run on the *normal* microtask queue for the same entry before sync mode was entered, it left a stale `ModuleRegistryModuleSettled` reaction queued there. When the normal queue later drained, that reaction re-entered `fetchComplete` on an already-`Fetched` entry. No changes to `JSType.h`. No WebCore code-generator changes. --- **Verification:** - ✅ `test/regression/issue/30493.test.ts` fails on current `main` (assertion crash, empty stdout) - ✅ Same test passes on `bun run build:local` with the patched WebKit - ✅ Same test passes on `bun bd` with the prebuilt preview tarball - ✅ Full bun CI green against `autobuild-preview-pr-225-2b6b1c39` (build #53556 — 67 pass, 3 pre-existing main flakes also red on #30522) Fixes #30493 Fixes #30281 Closes #30283 (the dependency-free 6-file repro in this PR covers the same root cause without needing a react+MUI install) --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
2f0a2fa to
6ed659b
Compare
There was a problem hiding this comment.
No further issues from me — all prior comments are addressed. Deferring to a human given this is a hand-rolled Sigstore signing implementation with new crypto deps (p256/ecdsa/pkcs8), and the musl build failures in #53767 still need a look.
Extended reasoning...
Overview
This PR adds bun publish --provenance by introducing a new src/sigstore/ crate (~1000 LOC) that implements Sigstore keyless signing end-to-end: OIDC token fetch, ephemeral ECDSA P-256 keypair generation, Fulcio cert issuance, DSSE envelope construction, Rekor tlog upload, and Sigstore bundle assembly. It wires this into publish_command.rs, adds CLI flags, reads publishConfig.provenance from package.json, honors NPM_CONFIG_PROVENANCE, and ships docs + an integration test that mocks Fulcio/Rekor/OIDC/registry. It also adds ~20 transitive crates to Cargo.lock (p256, ecdsa, elliptic-curve, der, spki, sec1, rfc6979, sha2, etc.) and includes a side-fix for as _ inference ambiguity in node_fs.rs / win_watcher.rs / options_jsc.rs triggered by linking serde_json.
Security risks
This is supply-chain security code by definition. It hand-implements a cryptographic attestation protocol (rather than using sigstore-rs, for documented reasons), handles OIDC bearer tokens, signs with ephemeral keys, and constructs the bundle that the npm registry uses to display provenance badges. The implementation is closely modeled on sigstore-js / libnpmpublish and is well-tested against mocks, but the wire-format correctness against real Fulcio/Rekor and the npm registry's verifier hasn't been demonstrated in CI. The new RustCrypto deps (p256, pkcs8, getrandom 0.2) are reputable but expand the dependency surface.
Level of scrutiny
High. New crate, new crypto deps, security-sensitive protocol implementation, and a documented design decision (bypass sigstore-rs, speak the protocol directly over bun_http) that a maintainer should ratify. This is well outside the "simple/mechanical" bar for auto-approval.
Other factors
I've reviewed this across several rounds; every inline comment I raised (dead parameter, .sigstore key stem, comment accuracy, mutual-exclusion check, lowercase npm_config_provenance, publishConfig.provenance wiring, == Some(true) vs .is_some(), docs gaps, error-prefix wording) has been addressed and resolved — the diff now looks clean to me. However, Build #53767 shows the aarch64-musl and x64-musl builds failing on this branch, which should be resolved before merge. The Windows fs-promises-writeFile-async-iterator failures appear unrelated (pre-existing flake referenced in the Cargo.toml profile comment).
|
Status at 51979c4, rebased on All review threads resolved. 8/8 tests pass locally on debug, release, and under the exact CI LSAN env; merge-base canary fails 8/8. CI (build 63709):
This rebase (104 commits): one What this PR does: implements How verified: 8 integration tests against in-process mock Fulcio/Rekor/OIDC/registry assert on the full bundle shape (DSSE envelope, SLSA v1 predicate, tlog entry, UTF-8 round-trip of the em-dash character in Rekor's signed note, |
59f7cc5 to
53e24b1
Compare
53e24b1 to
49773a4
Compare
|
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:
WalkthroughThis PR implements ChangesSigstore Provenance Publishing Feature
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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
🤖 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 `@src/runtime/cli/publish_command.rs`:
- Around line 914-916: Add a typed env-var accessor for NPM_CONFIG_PROVENANCE in
src/bun_core/env_var.rs using the existing macro pattern (new!() or
platform_specific_new!() to handle "NPM_CONFIG_PROVENANCE" and
"npm_config_provenance" variants), give it a clear identifier like
NPM_CONFIG_PROVENANCE and ensure it exposes a .get() method; then update the
call site in publish_command.rs (replace the bun_core::getenv_z(...)
.or_else(...) block) to use the new accessor's .get() to retrieve the value
(preserving the same behavior for missing values).
In `@src/sigstore/Cargo.toml`:
- Around line 40-42: Update the rand_core dependency entry in Cargo.toml to pin
to a patched release: change the version specifier for rand_core from "0.6" to
"0.6.2" (keeping default-features = false and features = ["getrandom"]) so the
resolver will exclude vulnerable 0.6.0/0.6.1 releases; leave the p256 and pkcs8
entries unchanged.
In `@src/sigstore/lib.rs`:
- Around line 210-211: Replace the std::fs::read(...) call with
bun_sys::File-based I/O: open the provenance path using bun_sys::File (instead
of converting path bytes with String::from_utf8_lossy), read its contents into a
Vec<u8> (e.g., read_to_end) and assign to bytes, and map errors to
SigstoreError::Usage as before; when converting the incoming path bytes to a
platform-native path, use platform-appropriate OsString/OsStr conversion
(OsStrExt::from_bytes on Unix, wide-string conversion on Windows) so you never
rely on from_utf8_lossy and ensure the error mapping still includes the
underlying error.
In `@test/cli/install/bun-publish-provenance.test.ts`:
- Around line 481-490: The test currently asserts packProc.exitCode directly
which hides subprocess stderr; capture the stderr from the Bun.spawn'd process
(packProc) after awaiting packProc.exited (e.g., read packProc.stderr as text
into a variable like stderr) and immediately before
expect(packProc.exitCode).toBe(0) add a guarded failure check: if
(packProc.exitCode !== 0) { expect(stderr).toBe(""); } so that failing runs
surface the full stderr output for diagnostics while keeping the final expect on
packProc.exitCode; update references around the Bun.spawn call that creates
packProc and the tarballPath assertion accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44fde984-5499-48ea-8ad2-47c87a008423
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.tomldocs/pm/cli/publish.mdxdocs/snippets/cli/publish.mdxscripts/verify-baseline-static/allowlist-x64-windows.txtscripts/verify-baseline-static/allowlist-x64.txtsrc/crash_handler/lib.rssrc/errno/lib.rssrc/install/PackageManager/CommandLineArguments.rssrc/install/PackageManager/PackageManagerOptions.rssrc/perf/tracy.rssrc/runtime/Cargo.tomlsrc/runtime/cli/Arguments.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/dns_jsc/options_jsc.rssrc/runtime/jsc_hooks.rssrc/runtime/node/node_fs.rssrc/runtime/node/win_watcher.rssrc/runtime/webview/ChromeProcess.rssrc/sigstore/Cargo.tomlsrc/sigstore/lib.rssrc/sigstore/provenance.rssrc/spawn/process.rssrc/spawn_sys/spawn_process.rstest/cli/install/bun-publish-provenance.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/runtime/cli/publish_command.rs`:
- Around line 1091-1095: Move cheap provenance validations to run before the
dry-run early return: create a small preflight function (e.g.,
validate_provenance_preflight or split out a no-network branch) that checks the
conflicting flags (`--provenance` vs `--provenance-file`), ensures `--access
public` when required, validates CI support, and verifies the
`--provenance-file` input format; call this new validation from publish() before
the dry_run check. Keep the existing maybe_generate_provenance(ctx) (the
networked Sigstore bundle generation) after the dry-run return so the heavy,
networked behavior is not executed during dry-run. Ensure publish() uses
validate_provenance_preflight for quick checks and only invokes
maybe_generate_provenance for real generation when not dry-run.
In `@src/sigstore/lib.rs`:
- Around line 208-267: verify_bundle currently only parses dsseEnvelope.payload
and compares subjects without authenticating the bundle; update verify_bundle to
perform full provenance verification before returning LoadedBundle by (1)
validating the DSSE envelope signature (call a new or existing helper like
verify_dsse_envelope or the sigstore verifier on the parsed bundle), (2)
validating the signer certificate chain / Fulcio certificate (use a helper
validate_fulcio_chain or sigstore cert chain verifier) and (3) verifying the
Rekor entry/inclusion proof if present (verify_rekor_entry or equivalent). Only
after these checks succeed should verify_bundle return Ok(LoadedBundle { .. });
on any verification failure return a SigstoreError::Usage with a clear message.
Ensure you reference and use the parsed bundle, dsseEnvelope, and subject
variables already in verify_bundle.
- Around line 414-424: The current logic in the identity extraction returns an
error when a non-empty c.email exists but c.email_verified != Some(true);
instead, follow the docstring by falling back to c.sub in that case: change the
branch around c.email / c.email_verified so that if c.email is Some(non-empty)
and c.email_verified == Some(true) you return Ok(email), otherwise continue to
check c.sub (i.e., do not return Err(SigstoreError::Identity(...)) immediately)
and only return Err(SigstoreError::Identity("JWT subject not found".into())) if
c.sub is missing or empty.
In `@test/cli/install/bun-publish-provenance.test.ts`:
- Around line 2-15: Add a 5-minute default timeout by importing
setDefaultTimeout from "bun:test" and calling beforeAll(() =>
setDefaultTimeout(1000 * 60 * 5)); near the top of the file (alongside the
existing beforeAll/afterAll and the VerdaccioRegistry setup) so long-running
operations in this install CLI test (registry, Bun.serve mocks, tarball packing
and subprocesses) won't hit the default timeout.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c6ec1c9c-41dd-465b-8cd1-5d68cb2327dd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
src/install/PackageManager/CommandLineArguments.rssrc/runtime/cli/publish_command.rssrc/sigstore/Cargo.tomlsrc/sigstore/lib.rstest/cli/install/bun-publish-provenance.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // ────────────────────────────────────────────────────────────────────────── | ||
| // Public entry point | ||
| // ────────────────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// A serialized Sigstore bundle plus the bits the caller needs to report | ||
| /// on / attach to the publish body. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Public-good Sigstore + npm defaults, matching sigstore-js and | ||
| /// `libnpmpublish/lib/provenance.js`. Overridable for testing against a | ||
| /// mock Fulcio/Rekor (e.g. a `Bun.serve` fixture) via the `BUN_SIGSTORE_*` | ||
| /// env vars. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Sigstore bundle media type for the v0.2 wire format (uses | ||
| /// `x509CertificateChain` for verification material; npm accepts v0.2+). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Sign an in-toto statement and produce a Sigstore bundle. | ||
| /// | ||
| /// `payload` is the JSON-encoded in-toto statement (e.g. from | ||
| /// [`provenance::generate`]). The payload is *not* re-serialized — it is | ||
| /// embedded as-is in the DSSE envelope, so the caller controls the exact | ||
| /// byte representation the tlog records. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Result of [`verify_bundle`] — a pre-built bundle ready to be | ||
| /// attached to the publish body. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `--provenance-file` path: verify an externally-generated Sigstore bundle's | ||
| /// DSSE-envelope subject matches the package being published (name + sha512) | ||
| /// and return it for attachment. Ported from `libnpmpublish` `verifyProvenance` | ||
| /// — npm additionally runs `sigstore.verify()` over the bundle (chain + tlog); | ||
| /// we do the subject match only, leaving full verification to the registry. | ||
| /// | ||
| /// The caller reads the file (via `bun_sys::File`, not `std::fs`) and passes | ||
| /// the bytes — keeps this crate I/O-free apart from the HTTP calls in [`attest`]. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // ────────────────────────────────────────────────────────────────────────── | ||
| // OIDC identity — sigstore-js `CIContextProvider` | ||
| // ────────────────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Lossy UTF-8 for env/wire bytes that land in JSON or error text. npm | ||
| /// reads the same values through Node's `process.env`, itself a lossy | ||
| /// UTF-8 decode, so U+FFFD replacement matches its behavior. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Which CI provider supplies the OIDC token. Drives the SLSA predicate | ||
| /// shape and the preflight error messages (matching npm's wording). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Detect the provider from the environment, mirroring npm's | ||
| /// `ci-info` checks used in `libnpmpublish/lib/provenance.js`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Preflight checks ported from `libnpmpublish` `ensureProvenanceGeneration` | ||
| /// — surfaces a precise error *before* we start talking to Fulcio. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // GitHub Actions: GET $ACTIONS_ID_TOKEN_REQUEST_URL&audience=sigstore with | ||
| // `Authorization: Bearer $ACTIONS_ID_TOKEN_REQUEST_TOKEN`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `@actions/core` does `encodeURIComponent(audience)`; the value is | ||
| // user-settable via `BUN_SIGSTORE_OIDC_AUDIENCE`, so percent-encode it. | ||
| // `bun_core::strings::percent_encode_write` is a path-segment encoder | ||
| // (doesn't escape `& = + space`), so match `encodeURIComponent` directly. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Extract the subject to sign as proof-of-possession — sigstore-js | ||
| /// `oidc.extractJWTSubject`: `email` (if verified) else `sub`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `header.payload[.signature]` — the payload is the second segment; a | ||
| // token with only one `.` yields everything after it (unsigned JWT). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `Value`, not `bool`: Dex (the reference self-hosted Sigstore OIDC | ||
| // backend), Azure AD v1, and some Auth0 configs emit this as the | ||
| // *string* `"true"`. `Option<bool>` would fail the whole struct | ||
| // deserialize on that — sigstore-rs / sigstore-python both carry a | ||
| // string-or-bool shim for this field; sigstore-js (untyped | ||
| // `JSON.parse`) just sees `"true" === true` → false and falls | ||
| // through to `sub`. We match that behavior. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // sigstore-js: `claims.email_verified === true ? claims.email : claims.sub` | ||
| // — fall through to `sub` when email is present but unverified (or the | ||
| // claim is a non-`true` boolean / string / anything else). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // ────────────────────────────────────────────────────────────────────────── | ||
| // Fulcio — sigstore-js `CAClient` / `external/fulcio.ts` | ||
| // ────────────────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // ────────────────────────────────────────────────────────────────────────── | ||
| // Rekor — sigstore-js `toProposedIntotoEntry` + `TLogClient` | ||
| // ────────────────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Build the Rekor `intoto` v0.0.2 proposed entry, POST it, and convert the | ||
| /// response into the serialized `tlogEntries[0]` object the bundle carries. | ||
| /// | ||
| /// We use the legacy `intoto` kind (not `dsse`) — npm still submits `intoto` | ||
| /// by default via sigstore-js's `entryType: 'intoto'`, and the npm registry | ||
| /// accepts either, so matching keeps the bundle byte-for-byte diffable | ||
| /// against `npm publish --provenance` output. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Rekor's canonical DSSE-envelope hash — sigstore-js `calculateDSSEHash`: | ||
| // JSON-canonicalize `{payloadType,payload:b64,signatures:[{sig:b64,publicKey:<PEM>}]}` | ||
| // (keyid omitted when empty) and SHA-256 it. With no optional fields the | ||
| // key-sorted form is fixed, so a hand-written template suffices; keep it | ||
| // in sync if a `keyid` is ever threaded through here. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `inclusionProof` is optional in the bundle format (older Rekor | ||
| // deployments omit it), but when Rekor *does* send one it must be | ||
| // well-formed: a partial hash chain or a dropped `rootHash` would | ||
| // produce a corrupt proof that fails verification. So: absent → None; | ||
| // present-but-malformed → hard error. `.filter(!is_null)` treats | ||
| // explicit `"inclusionProof": null` as absent (sigstore-js parity — | ||
| // `?.inclusionProof ? … : undefined`; public-good Rekor uses Go | ||
| // `omitempty` and never emits it, but private Rekor via | ||
| // `BUN_SIGSTORE_REKOR_URL` might). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `Option<Vec<_>>` collect — any bad element fails the | ||
| // whole proof rather than silently shrinking the chain. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
Implements --provenance / --no-provenance / --provenance-file for
bun publish, matching npm's libnpmpublish wire format: OIDC token from
CI, ephemeral P-256 key, Fulcio signing cert, DSSE envelope, Rekor
intoto entry, Sigstore bundle v0.2 attached as
_attachments["{name}-{version}.sigstore"] on the registry PUT. Honors
publishConfig.provenance and NPM_CONFIG_PROVENANCE. New bun_sigstore
crate over bun_http; SLSA predicates for GitHub Actions and GitLab CI.
Closes #15601
Closes #18611
ff64d9d to
7726110
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/sigstore/lib.rs:942-950— The localfn hex(bytes: &[u8]) -> Stringreimplementsbun_core::fmt::bytes_to_hex_lower_string/bun_core::fmt::hex_lower(bytes).to_string()— the same helper this PR already uses atpublish_command.rs(bun_fmt::hex_lower(&integrity).to_string()).bun_sigstorealready depends onbun_core, so no new dep is needed; suggest droppinghex()and calling the in-tree helper. (hex_decodehas no arbitrary-length equivalent inbun_core::fmt, so keeping that one is fine.)Extended reasoning...
What the issue is
src/sigstore/lib.rsdefines a local helper in the "Misc small helpers" section:fn hex(bytes: &[u8]) -> String { const HEX: &[u8; 16] = b"0123456789abcdef"; let mut s = String::with_capacity(bytes.len() * 2); for &b in bytes { s.push(HEX[(b >> 4) as usize] as char); s.push(HEX[(b & 0xF) as usize] as char); } s }
This is byte-for-byte equivalent to two existing helpers in
bun_core::fmt(src/bun_core/fmt.rs):bytes_to_hex_lower_string(input: &[u8]) -> Stringat fmt.rs:2585 — allocating lowercase hex encode, exactly whathex()does.hex_lower(bytes: &[u8]) -> HexBytes<'_, true>at fmt.rs:2622 — theDisplayadapter, sohex_lower(bytes).to_string()yields the sameString.
Why this fits REVIEW.md
REVIEW.md "Code style & idioms reviewers enforce" says: "In runtime native code, grep for the in-tree helper before hand-writing anything. File I/O, paths, strings, hashing, formatting … Being the only file touching a raw primitive is a red flag." Hex encoding is exactly the formatting-primitive category that rule covers.
Two facts make this concrete rather than hypothetical:
- The dep is already wired.
src/sigstore/Cargo.tomllistsbun_core.workspace = true, andlib.rsalready imports from it (use bun_core::{MutableString, Output, ZStr, strings};). No new dependency is needed — justbun_core::fmt::hex_lower(...)(or adduse bun_core::fmt as bun_fmt;to match the aliaspublish_command.rsuses). - This same PR already uses the in-tree helper for the identical purpose. In
src/runtime/cli/publish_command.rs(added in this PR), the SHA-512 tarball digest is hex-encoded vialet sha512_hex = bun_fmt::hex_lower(&integrity).to_string();— same input shape (a hash digest), same output (lowercase hexString). So the author demonstrably knows the helper exists and that it's fit for exactly this job; the localhex()inlib.rsis just an accidental duplicate.
Step-by-step equivalence proof
Take the two
hex()call sites inrekor_create_intoto_entry:let payload_hash_hex = hex(&sha256(payload)); // ... let envelope_hash_hex = hex(&sha256(canon.as_bytes()));
For
sha256(payload)=[0xc0, 0xff, 0xee, ...](32 bytes):- Local
hex(): iterates each byte, pushesHEX[b>>4]thenHEX[b&0xF]fromb"0123456789abcdef"→"c0ffee..."(64 lowercase hex chars). bun_core::fmt::bytes_to_hex_lower_string()(fmt.rs:2585): allocatesvec[0u8; 64], callsbytes_to_hex_lowerwhich writes the same nibble table into it, thenString::from_utf8_unchecked→"c0ffee...".bun_core::fmt::hex_lower(bytes).to_string(): theHexBytes<'_, true>Displayimpl formats each byte as two lowercase hex chars →"c0ffee...".
All three produce identical output for every possible
&[u8]input. Replacing the two call sites with either in-tree helper is a pure no-op at the wire-format level — the Rekorhash.value/payloadHash.valuefields carry the same string.What about
hex_decode?The neighboring
fn hex_decode(s: &str) -> Option<Vec<u8>>(used for Rekor'slogID/rootHash/hashes) does not have an arbitrary-length equivalent inbun_core::fmt— the closest,parse_hex*, targets fixed-width integer types. So keeping the localhex_decodeis correct; onlyhex()(the encoder) is a duplicate.Impact
Zero functional impact. Both implementations produce identical lowercase hex; the Rekor entry, bundle JSON, and test assertions are unchanged. This is purely a house-convention nit under the "grep for the in-tree helper before hand-writing" rule — filed as nit, non-blocking.
How to fix
Replace the two call sites and drop the local helper:
let payload_hash_hex = bun_core::fmt::hex_lower(&sha256(payload)).to_string(); // ... let envelope_hash_hex = bun_core::fmt::hex_lower(&sha256(canon.as_bytes())).to_string();
(or
bun_core::fmt::bytes_to_hex_lower_string(&sha256(...))— either matches whatpublish_command.rsalready does in this PR.) Then deletefn hex(...).
| const env = { | ||
| ...bunEnv, | ||
| // Pretend we're in GitHub Actions with id-token: write. | ||
| GITHUB_ACTIONS: "true", | ||
| ACTIONS_ID_TOKEN_REQUEST_URL: `${base}/gha-oidc`, | ||
| ACTIONS_ID_TOKEN_REQUEST_TOKEN: "gha-req-tok", | ||
| GITHUB_REPOSITORY: "oven-sh/bun", | ||
| GITHUB_SERVER_URL: "https://github.com", | ||
| GITHUB_WORKFLOW_REF: "oven-sh/bun/.github/workflows/release.yml@refs/heads/main", | ||
| GITHUB_REF: "refs/heads/main", | ||
| GITHUB_SHA: "deadbeef", | ||
| GITHUB_EVENT_NAME: "push", | ||
| GITHUB_REPOSITORY_ID: "1", | ||
| GITHUB_REPOSITORY_OWNER_ID: "2", | ||
| GITHUB_RUN_ID: "99", | ||
| GITHUB_RUN_ATTEMPT: "1", | ||
| RUNNER_ENVIRONMENT: "github-hosted", | ||
| // Point sigstore at our mock. | ||
| BUN_SIGSTORE_FULCIO_URL: base, | ||
| BUN_SIGSTORE_REKOR_URL: base, | ||
| CI: "1", |
There was a problem hiding this comment.
🟡 The GHA happy-path test spreads bunEnv (which spreads process.env) but never unsets SIGSTORE_ID_TOKEN — fetch_identity_token() checks that var first and returns it directly, so an ambient value (e.g. running the suite inside a GitLab job with id_tokens, or a stray cosign export) short-circuits the mocked /gha-oidc fetch and fails expect(oidcUrl).not.toBeNull() / the fakeJwt() assertion. The sibling tests already unset GITHUB_ACTIONS/GITLAB_CI for hermeticity; add SIGSTORE_ID_TOKEN: undefined, to this env object. Bun's own CI is BuildKite so this won't fire there — narrow nit.
Extended reasoning...
What the issue is
The GitHub Actions happy-path test ("attaches a sigstore bundle built against mock Fulcio/Rekor") builds its subprocess environment as:
const env = {
...bunEnv,
GITHUB_ACTIONS: "true",
ACTIONS_ID_TOKEN_REQUEST_URL: `${base}/gha-oidc`,
ACTIONS_ID_TOKEN_REQUEST_TOKEN: "gha-req-tok",
GITHUB_REPOSITORY: "oven-sh/bun",
// ... GITHUB_* predicate vars, BUN_SIGSTORE_* endpoint overrides ...
};bunEnv (test/harness.ts) spreads ...process.env and does not scrub SIGSTORE_ID_TOKEN. The env object above overrides GITHUB_ACTIONS, ACTIONS_ID_TOKEN_REQUEST_URL, ACTIONS_ID_TOKEN_REQUEST_TOKEN, and the GITHUB_*/BUN_SIGSTORE_* vars — but never sets SIGSTORE_ID_TOKEN: undefined. So if the ambient environment carries a non-empty SIGSTORE_ID_TOKEN, it flows through to the child process.
Why that changes behavior
fetch_identity_token() in src/sigstore/lib.rs checks SIGSTORE_ID_TOKEN before the GitHub Actions ACTIONS_ID_TOKEN_REQUEST_URL fetch:
fn fetch_identity_token(audience: &str) -> Result<String, SigstoreError> {
// cosign-compatible env override — also how GitLab supplies its token.
if let Some(tok) = env(bun_core::zstr!("SIGSTORE_ID_TOKEN")) {
return Ok(utf8_lossy(tok).into_owned());
}
// ... only falls through to ACTIONS_ID_TOKEN_REQUEST_URL fetch here ...The env() helper only filters empty strings, so any non-empty ambient value short-circuits the function. ensure_provenance_generation() (the preflight, which the test does control via GITHUB_ACTIONS: "true") is a separate function and doesn't gate on SIGSTORE_ID_TOKEN in the GitHub arm, so the preflight passes and the short-circuit only happens later at token-fetch time.
Step-by-step proof
Suppose the suite is run in an environment where SIGSTORE_ID_TOKEN=eyJ... is set — e.g. a contributor running bun bd test inside a GitLab CI job that has id_tokens: { SIGSTORE_ID_TOKEN: { aud: sigstore } } configured, or a shell with a leftover export SIGSTORE_ID_TOKEN=... from cosign:
bunEnvspreadsprocess.env, carryingSIGSTORE_ID_TOKENintoenv.- The child
bun publish --provenance --access publicrunsensure_provenance_generation()→ seesGITHUB_ACTIONS=trueandACTIONS_ID_TOKEN_REQUEST_URLset → returnsOk(GithubActions). Preflight passes. attest()callsfetch_identity_token("sigstore")→env("SIGSTORE_ID_TOKEN")isSome(<ambient>)→ returns the ambient token immediately. The mock/gha-oidchandler is never hit, sooidcUrlstaysnull.- The Fulcio POST proceeds with
credentials.oidcIdentityToken = <ambient token>, notfakeJwt(). expect(oidcUrl).not.toBeNull()fails;expect(fulcioReq.credentials.oidcIdentityToken).toBe(fakeJwt())also fails.
Why nothing catches this
REVIEW.md's "Tests must be hermetic" and "Copy harness conventions exactly — spread bunEnv when modifying it ({...bunEnv, KEY: undefined})" both apply. The other tests in this same file demonstrate the intended pattern — GITHUB_ACTIONS: undefined, GITLAB_CI: undefined appear in the "errors outside of supported CI", "NPM_CONFIG_PROVENANCE", and "publishConfig.provenance" tests. SIGSTORE_ID_TOKEN was missed because it's read in fetch_identity_token(), not in the ensure_provenance_generation() preflight the author was reasoning about when picking which vars to override. grep SIGSTORE_ID_TOKEN test/ returns nothing.
Impact
Narrow. Bun's own CI is BuildKite (harness.ts isBuildKite), which doesn't set SIGSTORE_ID_TOKEN, so this won't fire in the project's CI. It only affects contributors running the suite in an environment where SIGSTORE_ID_TOKEN happens to be set — plausible (GitLab CI with id_tokens, or a cosign env export) but uncommon. Filed as nit, non-blocking.
How to fix
One line — add SIGSTORE_ID_TOKEN: undefined, to the env object (and optionally to the other GHA-mode tests for consistency):
const env = {
...bunEnv,
SIGSTORE_ID_TOKEN: undefined,
GITHUB_ACTIONS: "true",
ACTIONS_ID_TOKEN_REQUEST_URL: `${base}/gha-oidc`,
// ...
};| describe("--provenance", () => { | ||
| test("attaches a sigstore bundle built against mock Fulcio/Rekor (GitHub Actions)", async () => { |
There was a problem hiding this comment.
🟡 The GitLab CI provenance path has zero test coverage — every test in this file either sets GITHUB_ACTIONS: "true" or explicitly unsets both providers, so CiProvider::GitlabCi, the SIGSTORE_ID_TOKEN early-return in fetch_identity_token(), the GitLab preflight-error arm, and the entire gitlab_statement() SLSA-v0.2 predicate builder (~75 CI_* env-var reads, a completely different in-toto/SLSA schema) are never executed. Per REVIEW.md "Cover the variant matrix", the GHA end-to-end test could be duplicated with a GitLab env fixture (GITLAB_CI: "true", SIGSTORE_ID_TOKEN: fakeJwt(), a handful of CI_* vars) asserting stmt._type === "https://in-toto.io/Statement/v0.1" / predicateType === "https://slsa.dev/provenance/v0.2" and the invocation.parameters / materials shape, plus a GITLAB_CI-without-SIGSTORE_ID_TOKEN preflight-error test. Nit — coverage gap only; the more-common GHA path is well-tested.
Extended reasoning...
What the gap is
GitLab CI is documented as one of exactly two first-class --provenance providers — in docs/pm/cli/publish.mdx:129, docs/snippets/cli/publish.mdx:92, and the CLI --provenance help string in CommandLineArguments.rs — and this PR ships ~150 lines of GitLab-specific code:
CiProvider::GitlabCiand itsdetect()branch (src/sigstore/lib.rs);- the
SIGSTORE_ID_TOKENearly-return infetch_identity_token()(lib.rs:314-316) — the only token path GitLab uses (no OIDC HTTP fetch); - the GitLab preflight-error arm in
ensure_provenance_generation()(lib.rs:296-304); - the entire
gitlab_statement()predicate builder (src/sigstore/provenance.rs:132-280) — a completely different schema from GitHub's (in-toto v0.1 / SLSA v0.2 withinvocation.configSource/parameters/environment/materials, vs GitHub's in-toto v1 / SLSA v1 withbuildDefinition/runDetails), plus theci_params!macro reading ~75 distinctzstr!("CI_*")env-var literals.
None of it is exercised by bun-publish-provenance.test.ts.
Step-by-step proof
Walking the 8 tests in the file:
- "attaches a sigstore bundle … (GitHub Actions)" — sets
GITHUB_ACTIONS: "true". GitHub path. - "errors outside of supported CI" — sets
GITHUB_ACTIONS: undefined, GITLAB_CI: undefined.CiProvider::detect()returnsNone. - "errors in GitHub Actions without id-token permission" — sets
GITHUB_ACTIONS: "true". GitHub preflight error. - "requires --access public" — sets
GITHUB_ACTIONS: "true". Fails beforeensure_provenance_generation(). - "NPM_CONFIG_PROVENANCE=true …" — sets
GITHUB_ACTIONS: undefined, GITLAB_CI: undefined. Unsupported-CI path. - "publishConfig.provenance: true …" — sets
GITHUB_ACTIONS: undefined, GITLAB_CI: undefined. Unsupported-CI path. - "--provenance-file: …" — bare
bunEnv. Skips CI detection entirely (--provenance-fileshort-circuits beforeensure_provenance_generation()). - "--provenance and --provenance-file are mutually exclusive" — bare
bunEnv. Fails on mutual-exclusion / bundle validation before CI detection.
Grepping the file: GITLAB_CI appears only as GITLAB_CI: undefined; SIGSTORE_ID_TOKEN never appears at all. So no test ever reaches CiProvider::GitlabCi, and gitlab_statement() is never called. A typo in any of the ~75 zstr!("CI_*") string literals, or a shape mismatch in the json!({...}) predicate (e.g. invocation.configSource.digest.sha1 vs sha256), would ship silently.
Why this fits the repo's review bar
REVIEW.md is explicit on both counts:
- "Cover the variant matrix, not just the repro. Every sibling entry point receiving the same fix (CLI flag AND JS API), both states of every flag …" — GitHub Actions and GitLab CI are the two arms of the
CiProviderenum, and only one arm is tested. - "Every behavioral change ships an automated test in the same PR" —
gitlab_statement()is ~150 lines of new behavior producing a wire format consumed by external verifiers (npm registry,sigstore verify), with no test.
The shared Fulcio/Rekor/bundle-assembly plumbing is exercised via the GHA test, so this is scoped to the GitLab-specific bits: provider detection, the SIGSTORE_ID_TOKEN token path, the preflight error, and the SLSA-v0.2 predicate shape.
Impact
Coverage gap only — no known incorrect behavior. The GitHub Actions path (the far more common real-world use) is thoroughly tested end-to-end, and gitlab_statement() is a straightforward port of libnpmpublish/lib/provenance.js. But if any of the ~75 hard-coded CI_* literals is misspelled, or if the SLSA-v0.2 nesting is wrong, GitLab users would get either a rejected publish or a bundle that fails downstream verification, and CI would not catch it.
How to fix
Duplicate the GHA end-to-end test with a GitLab env fixture — the same Bun.serve mock works, since SIGSTORE_ID_TOKEN short-circuits the OIDC HTTP fetch:
const env = {
...bunEnv,
GITLAB_CI: "true",
SIGSTORE_ID_TOKEN: fakeJwt(), // early-return in fetch_identity_token()
CI: "true",
CI_PROJECT_URL: "https://gitlab.com/oven-sh/bun",
CI_COMMIT_SHA: "deadbeef",
CI_JOB_NAME: "publish",
CI_JOB_URL: "https://gitlab.com/oven-sh/bun/-/jobs/99",
CI_JOB_ID: "99",
CI_PIPELINE_ID: "1",
CI_RUNNER_ID: "42",
CI_CONFIG_PATH: ".gitlab-ci.yml",
CI_PROJECT_PATH: "oven-sh/bun",
BUN_SIGSTORE_FULCIO_URL: base,
BUN_SIGSTORE_REKOR_URL: base,
};
// …
expect(stmt._type).toBe("https://in-toto.io/Statement/v0.1");
expect(stmt.predicateType).toBe("https://slsa.dev/provenance/v0.2");
expect(stmt.predicate.buildType).toBe("https://github.com/npm/cli/gitlab/v0alpha1");
expect(stmt.predicate.invocation.configSource.entryPoint).toBe("publish");
expect(stmt.predicate.invocation.parameters.CI_PROJECT_PATH).toBe("oven-sh/bun");
expect(stmt.predicate.invocation.parameters.CI_PAGES_URL).toBeUndefined(); // unset → omitted
expect(stmt.predicate.materials[0].digest.sha1).toBe("deadbeef");Plus a preflight-error test: GITLAB_CI: "true" without SIGSTORE_ID_TOKEN → expect(err).toContain('"SIGSTORE_ID_TOKEN"').
Filed as nit — the GHA path is well-covered and this is a variant-matrix coverage gap, not a demonstrated bug.
| oidcUrl = req.url; | ||
| expect(url.searchParams.get("audience")).toBe("sigstore"); | ||
| expect(req.headers.get("authorization")).toBe("Bearer gha-req-tok"); | ||
| return Response.json({ value: fakeJwt() }); | ||
| } |
There was a problem hiding this comment.
🟡 The two expect() calls inside the Bun.serve fetch handler (audience + Authorization header) violate REVIEW.md's "never throw inside event callbacks": if either fails, Bun.serve swallows the throw into a 500 and the test surfaces expect(out + err).toContain("Signed provenance statement") / expect(err).not.toContain("error:") instead of the real mismatch. Suggest capturing url.searchParams.get("audience") and req.headers.get("authorization") into locals (as fulcioReq/rekorReq/putBody already are) and asserting on them after await publish(...). Diagnostics-only nit — the test still fails on regression.
Extended reasoning...
What the issue is
The GHA happy-path test ("attaches a sigstore bundle built against mock Fulcio/Rekor") calls expect() directly inside the mock server's fetch handler:
using server = Bun.serve({
port: 0,
async fetch(req) {
const url = new URL(req.url);
if (url.pathname === "/gha-oidc") {
oidcUrl = req.url;
expect(url.searchParams.get("audience")).toBe("sigstore");
expect(req.headers.get("authorization")).toBe("Bearer gha-req-tok");
return Response.json({ value: fakeJwt() });
}
...REVIEW.md's "Tests reviewers reject" section calls out both halves of this pattern: "Wire EVERY failure event … to reject the awaited promise — never throw inside event callbacks" and "Hunt vacuous patterns: … expects inside catch blocks or callbacks that may never fire". A Bun.serve fetch handler is exactly such a callback — the throw is caught by Bun.serve's error handling and converted into an HTTP 500 response rather than propagating to the test runner.
Step-by-step proof
Suppose a regression causes bun publish to send ?audience=sigstore-dev (or drops the Bearer prefix on the Authorization header):
- The child
bun publish --provenance --access publiccallsfetch_identity_token("sigstore")→http_json(GET, "http://localhost:PORT/gha-oidc?audience=sigstore-dev", …). - The mock
fetchhandler runs;oidcUrl = req.urlcaptures the URL, thenexpect(url.searchParams.get("audience")).toBe("sigstore")throws aJestAssertionError. Bun.servecatches the throw and responds with 500 Internal Server Error (the default error handler); the assertion error message goes into the response body, not to the test runner.- Back in the child,
http_jsonseesstatus >= 400and returnsSigstoreError::Http { who: "GitHub Actions OIDC", detail: "HTTP 500: …" }. SigstoreError::print()writeserror: failed to generate provenance: GitHub Actions OIDC: HTTP 500: …to the child's stderr;Global::crash()exits nonzero.- Back in the test,
await publish(...)returns{ out, err, exitCode: 1 }. The first post-publish assertion isexpect(out + err).toContain("Signed provenance statement")— that is what fails, with a diff showing the child's stderr rather thanExpected: "sigstore" / Received: "sigstore-dev".
So the test still fails on regression (this is not a false-pass), but the failure message points at "missing 'Signed provenance statement' in output" rather than the actual audience/authorization mismatch — degraded diagnostics only.
Why nothing catches this
The "may never fire" concern is partially covered — expect(oidcUrl).not.toBeNull() after publish() confirms the endpoint was hit at all — but the two in-handler expect()s themselves would silently not run if the OIDC fetch were skipped, and their thrown errors never reach the test runner when they do run.
The same test already uses the correct capture-then-assert pattern for the other three endpoints: fulcioReq = await req.json(), rekorReq = await req.json(), and putBody = await req.json() are all captured inside the handler and asserted on after await publish(...) returns. The two OIDC checks are the only assertions inside the handler, so they're inconsistent with the file's own established convention.
Impact
Test-diagnostics only. The test correctly fails when bun publish sends the wrong audience or Authorization header; it just fails on the wrong assertion with an obscured message. Filed as nit (non-blocking) — this doesn't cause incorrect behavior, a false pass, or flakiness.
How to fix
Match the sibling captures — replace the two in-handler expect()s with local captures and assert after publish():
let oidcAudience: string | null = null;
let oidcAuth: string | null = null;
...
if (url.pathname === "/gha-oidc") {
oidcUrl = req.url;
oidcAudience = url.searchParams.get("audience");
oidcAuth = req.headers.get("authorization");
return Response.json({ value: fakeJwt() });
}
...
// ── OIDC flow ──
expect(oidcUrl).not.toBeNull();
expect(oidcAudience).toBe("sigstore");
expect(oidcAuth).toBe("Bearer gha-req-tok");| fn hex(bytes: &[u8]) -> String { | ||
| const HEX: &[u8; 16] = b"0123456789abcdef"; | ||
| let mut s = String::with_capacity(bytes.len() * 2); | ||
| for &b in bytes { | ||
| s.push(HEX[(b >> 4) as usize] as char); | ||
| s.push(HEX[(b & 0xF) as usize] as char); | ||
| } | ||
| s | ||
| } |
There was a problem hiding this comment.
🟡 This hand-rolled fn hex() duplicates bun_core::fmt::hex_lower (src/bun_core/fmt.rs:2622), which this same PR already uses in publish_command.rs (bun_fmt::hex_lower(&integrity).to_string()) for the identical digest→lowercase-hex purpose. bun_sigstore already depends on bun_core, so the two call sites in rekor_create_intoto_entry can be bun_core::fmt::hex_lower(&sha256(...)).to_string() and this local helper deleted. Nit — no functional difference; per REVIEW.md "grep for the in-tree helper before hand-writing anything … formatting".
Extended reasoning...
What the issue is
src/sigstore/lib.rs:834-842 defines a private fn hex(bytes: &[u8]) -> String that hand-writes a lowercase hex encoder via a nybble table:
fn hex(bytes: &[u8]) -> String {
const HEX: &[u8; 16] = b"0123456789abcdef";
let mut s = String::with_capacity(bytes.len() * 2);
for &b in bytes {
s.push(HEX[(b >> 4) as usize] as char);
s.push(HEX[(b & 0xF) as usize] as char);
}
s
}But bun_core::fmt::hex_lower already exists at src/bun_core/fmt.rs:2622 as a Display adapter (HexBytes<'_, true>) that produces byte-identical output. bun_sigstore's own Cargo.toml (added in this PR) declares bun_core.workspace = true, so the helper is already in scope with no new dependency edge.
Why this fits the repo's review bar
REVIEW.md's "Code style & idioms reviewers enforce" section says: "In runtime native code, grep for the in-tree helper before hand-writing anything. File I/O, paths, strings, hashing, formatting, validation, spawning, timers — use the most specific existing helper: … bun_core strings/fmt/Output … Being the only file touching a raw primitive is a red flag." Lowercase hex encoding of a byte slice is squarely in "formatting", and bun_core::fmt is the named module.
The strongest signal that this is a duplicate rather than a deliberate choice: this same PR already uses the in-tree helper for the identical purpose. In src/runtime/cli/publish_command.rs (added by this PR), the SHA-512 tarball digest is formatted with:
let sha512_hex = bun_fmt::hex_lower(&integrity).to_string();— same operation (hash digest → lowercase hex string), same output shape. So the PR's own code establishes the convention; the sigstore crate just didn't follow it.
Step-by-step proof of equivalence
hex(&[0xde, 0xad, 0xbe, 0xef])allocates aString::with_capacity(8), then for each byte pushesHEX[hi_nybble]andHEX[lo_nybble]fromb"0123456789abcdef"→"deadbeef".bun_core::fmt::hex_lower(&[0xde, 0xad, 0xbe, 0xef])returnsHexBytes<'_, true>(&[0xde, ...]); itsDisplayimpl walks the slice writing two lowercase hex chars per byte..to_string()allocates and returns"deadbeef".- Both produce lowercase, no separators, no
0xprefix — the exact wire format Rekor'sspec.content.hash.valueandspec.content.payloadHash.valuefields expect (verified by the test'sexpect(rekorReq.spec.content.payloadHash.algorithm).toBe("sha256")assertions, and bypublish_command.rsalready usinghex_lowerfor the sha512 subject digest that Rekor also validates).
So the substitution is byte-for-byte equivalent on the wire.
Where it's called
Two call sites, both in rekor_create_intoto_entry():
let payload_hash_hex = hex(&sha256(payload));let envelope_hash_hex = hex(&sha256(canon.as_bytes()));
Both take a [u8; 32] SHA-256 output.
Impact
None functionally — the hand-rolled encoder is correct. This is purely a code-duplication / convention nit in new code, filed as nit (non-blocking).
How to fix
// delete fn hex() at lines 834-842, then:
let payload_hash_hex = bun_core::fmt::hex_lower(&sha256(payload)).to_string();
// ...
let envelope_hash_hex = bun_core::fmt::hex_lower(&sha256(canon.as_bytes())).to_string();(or add use bun_core::fmt as bun_fmt; at the top to match publish_command.rs's spelling).
1 participant
Fixes #15601
Fixes #18611
What
Adds
bun publish --provenance/--no-provenance/--provenance-file=<path>, matchingnpm publish --provenance.When run inside GitHub Actions (with
id-token: write) or GitLab CI (withSIGSTORE_ID_TOKEN),bun publish --provenance --access publicwill:https://fulcio.sigstore.dev)intotoentry to Rekor (https://rekor.sigstore.dev)_attachments["{name}-{version}.sigstore"]NPM_CONFIG_PROVENANCE=trueis honored (whatactions/setup-nodesets).Why not
sigstore-rs?The
sigstorecrate (https://github.com/sigstore/sigstore-rs) was evaluated:hashedrekordsigning (bundlemessageSignature), not DSSE attestation (bundledsseEnvelope) — which is what npm provenance requires.IdentityTokenhard-requires anemailclaim, which GitHub Actions OIDC tokens don't carry.sign/fulcio/rekorfeatures pullreqwest+tokio+aws-lc-rs, duplicating bun's own HTTP (bun_http) and TLS (BoringSSL) stacks.So the new
src/sigstore/crate uses the same RustCrypto primitivessigstore-rsdoes (p256,pkcs8) for the ephemeral key + signature, but speaks the Fulcio/Rekor protocol directly overbun_http::AsyncHTTP::send_sync()and assembles the bundle JSON withserde_json. The wire format is matched againstsigstore-js(what npm actually uses).SLSA predicate
Matches
libnpmpublish/lib/provenance.jsexactly — same env var set, same build-type URIs, same statement shape:buildDefinition/runDetails)invocation/materials)Side fix:
as _inference vs serde_jsonLinking
serde_jsonintobun_runtimeexposes itsimpl PartialEq<Value> for i32/u16/…blanket. That makes existingerr.errno == E::EEXIST as _patterns innode_fs.rs,win_watcher.rs, andoptions_jsc.rsfailE0282—_now has two candidate RHS types (u16andserde_json::Value). Those sites are fixed to use explicitas u16/as i32, which is strictly more robust regardless of this change.Testing
test/cli/install/bun-publish-provenance.test.tsstands up a singleBun.servethat mocks the GitHub OIDC token endpoint, Fulcio/api/v2/signingCert, Rekor/api/v1/log/entries, and the npm registry PUT. Asserts on:_attachments["*.sigstore"]contains a valid bundle with the rightmediaType,dsseEnvelope, SLSA v1 predicate,tlogEntriesdecoded from the Rekor mockid-token: write, missing--access public,NPM_CONFIG_PROVENANCE+--no-provenanceoverride,--provenance-filesubject mismatchAll 8 tests pass with the change, all 8 fail on system bun (
--provenanceis unrecognized).Override endpoints (for testing / private sigstore)
BUN_SIGSTORE_FULCIO_URL(defaulthttps://fulcio.sigstore.dev)BUN_SIGSTORE_REKOR_URL(defaulthttps://rekor.sigstore.dev)BUN_SIGSTORE_TLOG_BASE_URL(defaulthttps://search.sigstore.dev/)BUN_SIGSTORE_OIDC_AUDIENCE(defaultsigstore)Rebase notes (onto b9ef885)
The branch was squashed to one commit before rebasing 1329 commits of main; conflicts and follow-on fixes:
publish_command.rs: main changedpub(crate) fn publishto private andwrite!(..).ok()tolet _ = write!(..); took main's style, kept the provenance code. Main also removedintegrityfromContext, so the subject digest is now computed fromctx.tarball_bytesinmaybe_generate_provenance(verified by the--provenance-filetest, which packs a real tarball and matches its digest).bun_http::AsyncHTTP:init_syncno longer takes the response buffer (it moved tosend_sync(&mut buf)),send_syncreturns owned metadata, andstatus_codeis a method;http_jsonupdated. Because the metadata is now owned and dropped, theclone_metadataLSAN suppression this PR previously added is gone (verified: the suite passes under the CI LSAN env without it), sotest/leaksan.suppis untouched.str::find/contains/splitn/lines,chunks_exactwith a constant) converted tobun_core::strings/as_chunks.pack_command.rs:managerbecamectx.managerin the surrounding code.stderrForInstallwas removed from the harness (bunEnvnow suppresses the warning it filtered); the helper reads stdout/stderr/exit concurrently.Cargo.lockregenerated from main's; the added package set is unchanged.src/compressed to single lines or removed (comment-cop).Known follow-ups (out of scope for this PR)
bun publishdoes not honorHTTPS_PROXY/HTTP_PROXY/NO_PROXY: all four pre-existingAsyncHTTP::init_synccall sites inpublish_command.rs(registry GET, PUT, OTP retry, web-login poll) passNoneforhttp_proxy, and the three new Sigstore calls inbun_sigstore::http_jsonfollow that pattern. Threadingenv_loader::get_http_proxy_forthrough all seven (asinstall'sNetworkTaskalready does) is a self-contained follow-up with its own test surface; fixing only the sigstore subset here would not make the self-hosted-runner-behind-proxy scenario work end-to-end.