blacklight: verified-streaming downloads anchored in Sigstore#1
Merged
Conversation
A download tool where a transparency-logged signature covers a BLAKE3 Merkle root that authorizes chunk-granular, abort-on-first-bad-byte verification during transfer from untrusted networks and mirrors. - publish: builds a 16 KiB-group outboard Merkle tree (bao-tree), writes a signed manifest, keyless-signs it via Sigstore (Fulcio + Rekor), emitting an offline-verifiable v0.3 bundle - fetch: verifies the bundle offline (signature, cert chain, Rekor inclusion proof, pinned identity/issuer) before downloading, then streams the artifact verifying every 16 KiB group as it arrives; aborts on the first tampered group (exit 3), deletes the partial - hand-rolled forward-only verifier on blake3::hazmat, unit-tested for round-trip equivalence with bao-tree's outboard format - attack demo: tampering MITM proxy + metrics vs curl|sha256sum - research paper (paper/PAPER.md) with verified citations, design doc, README with mermaid architecture diagrams - CI: fmt/clippy/test matrix, offline attack demo, and a signed round-trip job using GitHub Actions ambient OIDC against Sigstore - 13 tests passing (9 unit + 4 integration); gitleaks-scanned clean Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds the ChangesBlacklight Verified Streaming Downloads
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Publisher
participant Sigstore
participant Origin
participant blacklight as blacklight fetch
participant Verifier as GroupPlan
Publisher->>Publisher: build outboard tree + manifest
Publisher->>Sigstore: sign manifest bytes
Sigstore-->>Publisher: bundle JSON
Publisher->>Origin: upload manifest, outboard, bundle
blacklight->>Origin: load manifest bytes
blacklight->>Sigstore: verify_manifest
Sigstore-->>blacklight: verified identity and issuer
blacklight->>Origin: load outboard bytes
blacklight->>Verifier: GroupPlan::new(root, size, outboard)
loop streamed groups
blacklight->>Origin: GET artifact bytes
Origin-->>blacklight: 16 KiB group
blacklight->>Verifier: check_group(index, data)
Verifier-->>blacklight: match or tamper error
end
blacklight->>blacklight: flush, sync, rename output
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (2)
src/sigstore.rs (1)
54-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAmbient-detection errors are silently converted into a browser flow.
The
_ =>arm catches bothOk(None)andErr(_). A genuine failure indetect_ambient()(e.g. a misconfigured CI OIDC provider) will be discarded and fall through to the interactive browser flow, which hangs in a headless CI runner instead of surfacing the real error. Consider matchingErr(e)explicitly and logging it before falling back.♻️ Surface the ambient error
let token: IdentityToken = match IdentityToken::detect_ambient().await { Ok(Some(t)) => { eprintln!(" using ambient CI OIDC identity"); t } + Err(e) => { + eprintln!(" ambient OIDC detection failed ({e}); opening browser …"); + get_identity_token(context.config().oidc_url.as_deref()) + .await + .context("could not obtain an OIDC identity token")? + } _ => { eprintln!(" opening browser for OIDC sign-in …"); get_identity_token(context.config().oidc_url.as_deref()) .await .context("could not obtain an OIDC identity token")? } };🤖 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 `@src/sigstore.rs` around lines 54 - 65, The ambient OIDC detection in the `IdentityToken::detect_ambient` match is hiding real failures by treating `Err(_)` the same as `Ok(None)`. Update the match in this token acquisition flow to handle `Err(e)` explicitly, log or surface that error before any fallback, and only open the browser for the true no-ambient case (`Ok(None)`), so the `get_identity_token` path is not used to mask detection failures.demo/evil_proxy.py (1)
37-37: 🩺 Stability & Availability | 🔵 TrivialConsider a request timeout.
urlopen(upstream)has no timeout; a stalled/hanging origin would block the handler thread indefinitely. Low-impact for a demo tool, but a cheap robustness improvement.♻️ Add a timeout
- with urllib.request.urlopen(upstream) as resp: + with urllib.request.urlopen(upstream, timeout=30) as resp:🤖 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 `@demo/evil_proxy.py` at line 37, Add a timeout to the upstream fetch in evil_proxy.py so a stalled origin cannot block the handler thread indefinitely. Update the urlopen call inside the request handling path to pass a reasonable timeout, and keep the behavior otherwise unchanged in the same handler logic that uses urllib.request.urlopen(upstream).
🤖 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 @.github/workflows/ci.yml:
- Line 23: The workflow uses actions/checkout@v4 without disabling credential
persistence, leaving GITHUB_TOKEN available in git config for the rest of each
job. Update every checkout step in the CI workflow to add a with block on
actions/checkout@v4 that sets persist-credentials to false, including the
checkout used by signed-roundtrip and the other two checkout invocations.
- Around line 1-61: Add a restrictive default permissions block for the workflow
so the `test` and `attack-demo` jobs do not inherit broader `GITHUB_TOKEN`
scope. Update the top-level workflow permissions in CI to read-only access, and
ensure the existing jobs (`test`, `attack-demo`) continue to work without any
extra write permissions.
In `@demo/run_demo.sh`:
- Around line 66-74: SCENARIO 2 in run_demo.sh only reports a bug instead of
enforcing the security check. After the "$BL fetch" call and BL_EXIT capture,
make the script fail if the exit code is not 3 or if "$WORK/tampered.out"
exists, so the tampering demo actually asserts the expected integrity violation.
Keep the current set +e/set -e pattern around fetch, but add explicit failure
handling in the scenario block using the existing BL_EXIT and tampered.out
checks.
In `@src/fetch.rs`:
- Around line 251-262: `resolve_artifact_url` can return a local filesystem
path, but `stream_verified_inner` still hands that value to `reqwest::get`,
which only works for URLs. Update the artifact streaming flow to either detect
local paths before the request and read/stream them from disk, or have
`resolve_artifact_url`/`stream_verified_inner` reject non-URL sources with a
clear error message. Use the existing `resolve_artifact_url` and
`stream_verified_inner` symbols to keep the fix localized.
- Around line 136-141: The mirror fetch path currently uses the bare
reqwest::get helper, which can block indefinitely on slow or hostile servers.
Update the fetch logic in the relevant mirror download flow, especially the code
in stream_verified_inner() and load(), to use a shared reqwest::Client instead
of reqwest::get. Configure that client with connect_timeout and read_timeout,
and thread it through the existing fetch helper so all mirror requests inherit
the timeout behavior.
In `@src/manifest.rs`:
- Around line 26-27: The `Manifest::from_bytes` parsing path must enforce the
basename-only contract for `Manifest.name`, since `fetch()` later uses
`PathBuf::from(&manifest.name)` as the default output target. Update
`Manifest::from_bytes` to reject any `name` containing path separators or path
traversal components like `.` and `..`, and ensure the validation keeps `name`
aligned with its documented meaning before the manifest is accepted.
In `@tests/tamper.rs`:
- Around line 67-72: The tampering guard in tamper logic uses `if let` chaining,
which is newer than the stated MSRV. Update the conditional in `tamper.rs` to
use older-compatible nested `if let`/`if` checks around the `tamper` tuple,
`path` comparison, and `off < body.len()` test, or alternatively bump the
documented MSRV to match the language feature if that is the intended direction.
---
Nitpick comments:
In `@demo/evil_proxy.py`:
- Line 37: Add a timeout to the upstream fetch in evil_proxy.py so a stalled
origin cannot block the handler thread indefinitely. Update the urlopen call
inside the request handling path to pass a reasonable timeout, and keep the
behavior otherwise unchanged in the same handler logic that uses
urllib.request.urlopen(upstream).
In `@src/sigstore.rs`:
- Around line 54-65: The ambient OIDC detection in the
`IdentityToken::detect_ambient` match is hiding real failures by treating
`Err(_)` the same as `Ok(None)`. Update the match in this token acquisition flow
to handle `Err(e)` explicitly, log or surface that error before any fallback,
and only open the browser for the true no-ambient case (`Ok(None)`), so the
`get_identity_token` path is not used to mask detection failures.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fccce62-01bc-4c83-914b-9eab21da5d4f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/ci.yml.gitignoreCargo.tomlLICENSE-APACHELICENSE-MITREADME.mddemo/evil_proxy.pydemo/run_demo.shdocs/DESIGN.mdpaper/PAPER.mdsrc/fetch.rssrc/main.rssrc/manifest.rssrc/publish.rssrc/sigstore.rssrc/verify.rstests/tamper.rs
Security / correctness: - manifest: reject path-traversal / non-basename `name` values before a hostile manifest can steer `fetch`'s default output path (adds tests) - fetch: stream local-path artifacts from disk instead of handing a filesystem path to reqwest::get (which only speaks http) - fetch: use a shared reqwest::Client with connect_timeout + read_timeout so a slow/hostile mirror cannot block indefinitely - sigstore: surface ambient-OIDC detection errors instead of masking them as "no identity" and silently falling back to a browser flow CI hardening: - add top-level read-only `permissions`; signed-roundtrip keeps id-token - set persist-credentials: false on all checkout steps Demo / misc: - run_demo.sh: SCENARIO 2 now asserts (exit 3 + no output file), not just reports - evil_proxy.py: add a 30s upstream urlopen timeout - tests: replace the let-chain with a match guard to stay within MSRV 15 tests pass (11 unit + 4 integration); fmt + clippy -D warnings clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.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)
demo/evil_proxy.py (1)
36-78: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBroad
exceptafter headers are already sent can corrupt the response.
send_response/end_headers()run at lines 39-48, before streaming starts. If anything fails during the streaming loop that isn't aBrokenPipeError/ConnectionResetError(e.g. upstream read timeout, an unexpected write error), the outerexceptat Line 77 callsself.send_error(502, ...), which writes another status line/headers onto a socket that has already sent headers and possibly partial body — producing a malformed response or a secondary exception that masks the real failure. Since this demo is exercised in CI, this can turn a real upstream/network hiccup into a confusing flaky failure instead of a clear log message.🩹 Proposed fix: track whether headers were sent
def do_GET(self): upstream = ARGS.origin.rstrip("/") + self.path tamper = self.path == ARGS.target + headers_sent = False try: with urllib.request.urlopen(upstream, timeout=30) as resp: body_len = resp.headers.get("Content-Length") self.send_response(resp.status) # Force plain streaming: known length, no transfer-encoding games. for k, v in resp.headers.items(): if k.lower() in ("transfer-encoding", "connection", "content-length"): continue self.send_header(k, v) if body_len is not None: self.send_header("Content-Length", body_len) self.send_header("Connection", "close") self.end_headers() + headers_sent = True if tamper: ... except Exception as e: # noqa: BLE001 - demo tool, surface anything - self.send_error(502, f"upstream error: {e}") + if headers_sent: + sys.stderr.write(f"[evil_proxy] upstream error mid-stream: {e}\n") + else: + self.send_error(502, f"upstream error: {e}")🤖 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 `@demo/evil_proxy.py` around lines 36 - 78, The streaming path in evil_proxy’s request handler can emit a second HTTP response after headers have already been sent. Update the try/except around the urllib.request.urlopen + streaming loop so that once send_response/end_headers has run, later failures in the body stream do not call self.send_error; instead, log the error and close the connection (or only send_error before headers are committed). Use the existing request-handler logic around self.send_response, end_headers, and the streaming loop to distinguish pre-header and post-header failures.
🤖 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 `@demo/evil_proxy.py`:
- Around line 36-78: The streaming path in evil_proxy’s request handler can emit
a second HTTP response after headers have already been sent. Update the
try/except around the urllib.request.urlopen + streaming loop so that once
send_response/end_headers has run, later failures in the body stream do not call
self.send_error; instead, log the error and close the connection (or only
send_error before headers are committed). Use the existing request-handler logic
around self.send_response, end_headers, and the streaming loop to distinguish
pre-header and post-header failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02011038-9b85-4327-923c-4a8d495fa21e
📒 Files selected for processing (7)
.github/workflows/ci.ymldemo/evil_proxy.pydemo/run_demo.shsrc/fetch.rssrc/manifest.rssrc/sigstore.rstests/tamper.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- demo/run_demo.sh
- src/manifest.rs
- src/fetch.rs
- tests/tamper.rs
- .github/workflows/ci.yml
- src/sigstore.rs
The except block wrapped the whole streaming loop, so an upstream read failure mid-body would call send_error() and emit a second status line into an already-started 200 response. Track whether headers were sent; after that point, log the error and drop the connection instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
blacklight — a download tool where a Sigstore-transparency-logged signature covers a BLAKE3 Merkle root that authorizes chunk-granular, abort-on-first-bad-byte verification during transfer from untrusted networks/mirrors.
publish— hashes the file, builds a 16 KiB-chunk-group outboard Merkle tree (.obao, ~0.4% overhead) via bao-tree, writes a small JSON manifest, and keyless-signs the manifest bytes via Sigstore (OIDC → Fulcio → Rekor), emitting an offline-verifiable v0.3 bundle.fetch— verifies the bundle offline (signature, Fulcio cert chain, Rekor inclusion proof, pinned--expect-identity/--expect-issuer) before downloading a single artifact byte, then streams the artifact verifying every 16 KiB group the instant it arrives. First tampered group → abort (exit 3), partial file deleted, byte offset reported.Proven behavior
curl | sha256summust read all 32 MiB before its hash can mismatch. Reproduce withbash demo/run_demo.sh 32 16000000.-D warnings,--lockedbuild/test.Contents
src/— the CLI (~1.4k LOC): manifest format, publish, streaming verifier (verify.rs, hand-rolled forward-only fail-fast walk onblake3::hazmat, unit-tested for equivalence with bao-tree's outboard), fetch, Sigstore wrapper.demo/— tampering MITM proxy + end-to-end attack demo with metrics.paper/PAPER.md— research write-up with verified citations;docs/DESIGN.md— threat model, trust chain, formats; README with mermaid architecture diagrams..github/workflows/ci.yml— test matrix (ubuntu/macos), offline attack demo, and a signed round-trip job that exercises the full Sigstore path using the workflow's ambient OIDC identity.Notes for review
signed-roundtripCI job runs the sign→Rekor→verify path against live Sigstore staging for the first time in CI (it cannot run locally without a browser); if anything in this PR needs iteration, expect it there.🤖 Generated with Claude Code
Summary by CodeRabbit
publishandfetchfor verified streaming downloads with keyless signing and offline integrity checks.