feat(node): blind recipient identities at rest and gate B1 by repo readability#40
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughRemoves cleartext recipient DID storage from the encrypted blob pipeline. A DB migration (v5) drops the ChangesRecipient identity removal from encrypted blob pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gitlawb-node/src/api/repos.rs (1)
327-340:⚠️ Potential issue | 🔴 CriticalGate
service=git-receive-packinfo/refswith the same visibility/RepoNotFound behavior asgit-upload-pack
crates/gitlawb-node/src/server.rsmountsGET /{owner}/{repo}/info/refswithauth::optional_signature, so unauthenticated callers can hit this handler.- In
crates/gitlawb-node/src/api/repos.rs(git_info_refs), thevisibility_check(...)->Decision::Denypath that maps toAppError::RepoNotFound(...)runs only forservice == "git-upload-pack".- For
service == "git-receive-pack", the handler skipsvisibility_check, still acquires the repo (acquire_fresh) and returnssmart_http::info_refs(...), which can leak private-repo existence and advertised refs.Add the same read-visibility gate for
service == "git-receive-pack"and keep mapping deny/unauthorized toRepoNotFoundto avoid an existence oracle and ref disclosure.🤖 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 `@crates/gitlawb-node/src/api/repos.rs` around lines 327 - 340, The handler currently only enforces read visibility for service == "git-upload-pack"; update the branch handling of service == "git-receive-pack" in git_info_refs (where service, record, auth, and name are in scope) to call state.db.list_visibility_rules(&record.id).await?, compute caller the same way (auth.as_ref().map(|e| e.0 .0.as_str())), and run visibility_check(&rules, record.is_public, &record.owner_did, caller, "/") and if it returns Decision::Deny return Err(AppError::RepoNotFound(format!("{owner}/{name}"))), mirroring the existing git-upload-pack logic so unauthenticated/unauthorized callers do not observe private repo existence or refs.Source: Learnings
🧹 Nitpick comments (1)
crates/gitlawb-node/src/sync.rs (1)
310-324: 💤 Low valuePinned envelope with mismatched CID is not unpinned.
When
pin_git_objectreturns a CID that differs from the expected one, the code logs a warning and skips recording, but the incorrectly-pinned content remains in IPFS storage. This is a minor storage leak rather than a correctness issue (the DB row is not created, so the next sync will re-attempt), but over many syncs with a misbehaving IPFS node it could accumulate garbage.Consider unpinning the mismatched CID before continuing, or document this as an accepted trade-off.
🤖 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 `@crates/gitlawb-node/src/sync.rs` around lines 310 - 324, The mismatched CID path currently logs and continues but leaves the wrong content pinned in IPFS; update the match arm handling Ok(cid) if cid != blob.cid to unpin the unexpected CID before continuing (call the appropriate unpin helper in crate::ipfs_pin, e.g., crate::ipfs_pin::unpin_git_object(ipfs_api, &cid) or use ipfs_api directly), then log any unpin error and continue; keep the existing call to db.record_encrypted_blob only in the successful CID-equals branch and ensure references to pin_git_object, blob.oid, blob.cid, db.record_encrypted_blob, ipfs_api, and envelope are used to locate where to add the unpin/cleanup call.
🤖 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 `@crates/gitlawb-core/src/identity.rs`:
- Around line 55-59: The method seed_bytes() exposes raw 32-byte private seed as
[u8; 32]; change it to follow the existing to_seed() safety pattern by returning
a zeroizing/secret wrapper instead of a plain array (e.g., use the same type
returned by to_seed() or Zeroizing<[u8;32]>), and update call sites that use
seed_bytes() (e.g., where x25519_secret_from_seed and XSecret::from are invoked)
to accept the secret wrapper or to extract the inner bytes temporarily in a
zeroized scope; keep signing_key and to_seed() as the canonical source of seed
material and remove plain [u8;32] returns to ensure automatic zeroization.
In `@crates/gitlawb-node/src/api/encrypted.rs`:
- Around line 40-42: Update the endpoint doc comment for GET
/api/v1/repos/{owner}/{repo}/encrypted-blob/{oid} (function get_encrypted_blob)
to reflect current authorization: state that the handler returns raw envelope
bytes when the repository is readable by the caller (repo read permission),
rather than claiming recipient-based access; keep any note about additional
envelope-level checks if still applicable but remove the outdated "recipient"
wording so clients and future maintainers see the correct contract.
- Around line 75-90: The replicate_encrypted_blobs handler currently calls
state.db.list_all_encrypted_blobs(&record.id) without verifying read access;
before listing blobs, perform the repo-readability gate using the app's
authorization helper (e.g. call a method like
state.ensure_repo_readable(&record) or state.auth.ensure_repo_readable(&owner,
&repo, &record.id)), return an authorization error if the caller cannot read the
repo, and only then call state.db.list_all_encrypted_blobs(&record.id); ensure
the check happens in replicate_encrypted_blobs immediately after get_repo(...)
and before constructing blobs so private repo metadata is never exposed to
unauthorized callers.
In `@crates/gitlawb-node/src/encrypted_pin.rs`:
- Around line 71-74: The loop silently skips when
crate::git::store::read_object(repo_path, oid) returns Err or Ok(None); change
those silent continues to log the failure before continuing: capture the Err
detail and log it (including repo_path and oid) and also log a warning when
Ok(None) (missing blob) so missing encrypted blobs are diagnosable; apply the
same change to the similar branch later (the second read_object check at lines
~82-85) so both read_object failure paths emit contextual logs rather than
silently continuing.
- Around line 61-65: The current code treats errors from
db.encrypted_blob_recipients_tag(repo_id, oid).await as cache-misses and
proceeds to reseal/pin, causing external writes during DB failures; change the
logic to match on the Result explicitly: handle Ok(Some(stored_tag)) -> compare
to tag and continue if equal, Ok(None) -> proceed with reseal, and Err(e) ->
propagate or return the error (do not continue to reseal/pin). Use
encrypted_blob_recipients_tag, repo_id, oid and tag in the match and propagate
the DB error via the function's error return (or use the ? operator) instead of
swallowing it.
In `@crates/gitlawb-node/src/git/visibility_pack.rs`:
- Around line 21-24: The current loop in visibility_pack.rs (the refname check
inside the for (refname, _oid) in refs) only keeps refs under refs/heads/* and
refs/tags/*, which mismatches the broader object enumeration used elsewhere
(smart_http.rs uses rev-list --objects --all and ipfs_pin.rs filters all repo
objects), so update visibility_pack.rs to use the same ref/object enumerator as
the pack/pin paths: remove the restrictive
starts_with("refs/heads/")/starts_with("refs/tags/") checks and instead iterate
the full ref namespace (e.g., all refs under refs/* or use the same
rev-list-based enumeration method), so the withheld set includes any object
reachable from all refs just like blob_paths, smart_http.rs, and ipfs_pin.rs do.
Ensure you reference the same mechanism (function or iterator used for rev-list
traversal) so both packing/pinning and withholding share identical ref/object
enumeration.
- Around line 30-31: The code currently continues on a non-zero `listing.status`
which silently skips a failing `git ls-tree` and can leak data; in the function
in visibility_pack.rs where `listing` is handled (the block checking `if
!listing.status.success()`), replace the `continue` with an error return that
fails closed (e.g., return an Err with context indicating the ref and the
`listing.status` and `listing.stderr`/output). Ensure you propagate a failure up
so callers abort fetch/replication instead of treating missing blobs as public.
In `@crates/gitlawb-node/src/ipfs_pin.rs`:
- Around line 75-86: The cat function currently issues
reqwest::Client::new().post(...).send().await? with no timeout; update cat to
use a bounded request timeout (e.g., via
reqwest::Client::builder().timeout(Duration::from_secs(<N>)).build() and then
.post(...).send().await?) or wrap the send/bytes await in
tokio::time::timeout(Duration::from_secs(<N>), ...).await and convert timeout
errors to anyhow errors; reference the cat function and ipfs_api/cid parameters
when making the change and pick a configurable constant or env-driven value for
the timeout so production worst-case envelope/read latency can be tuned.
---
Outside diff comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 327-340: The handler currently only enforces read visibility for
service == "git-upload-pack"; update the branch handling of service ==
"git-receive-pack" in git_info_refs (where service, record, auth, and name are
in scope) to call state.db.list_visibility_rules(&record.id).await?, compute
caller the same way (auth.as_ref().map(|e| e.0 .0.as_str())), and run
visibility_check(&rules, record.is_public, &record.owner_did, caller, "/") and
if it returns Decision::Deny return
Err(AppError::RepoNotFound(format!("{owner}/{name}"))), mirroring the existing
git-upload-pack logic so unauthenticated/unauthorized callers do not observe
private repo existence or refs.
---
Nitpick comments:
In `@crates/gitlawb-node/src/sync.rs`:
- Around line 310-324: The mismatched CID path currently logs and continues but
leaves the wrong content pinned in IPFS; update the match arm handling Ok(cid)
if cid != blob.cid to unpin the unexpected CID before continuing (call the
appropriate unpin helper in crate::ipfs_pin, e.g.,
crate::ipfs_pin::unpin_git_object(ipfs_api, &cid) or use ipfs_api directly),
then log any unpin error and continue; keep the existing call to
db.record_encrypted_blob only in the successful CID-equals branch and ensure
references to pin_git_object, blob.oid, blob.cid, db.record_encrypted_blob,
ipfs_api, and envelope are used to locate where to add the unpin/cleanup call.
🪄 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 Plus
Run ID: 774f892c-6ba4-481c-acdb-4008c3174443
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.gitignorecrates/gitlawb-core/Cargo.tomlcrates/gitlawb-core/src/encrypt.rscrates/gitlawb-core/src/identity.rscrates/gitlawb-core/src/lib.rscrates/gitlawb-node/Cargo.tomlcrates/gitlawb-node/src/api/encrypted.rscrates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/api/visibility.rscrates/gitlawb-node/src/arweave.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/encrypted_pin.rscrates/gitlawb-node/src/git/mod.rscrates/gitlawb-node/src/git/smart_http.rscrates/gitlawb-node/src/git/visibility_pack.rscrates/gitlawb-node/src/ipfs_pin.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/pinata.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/sync.rscrates/gitlawb-node/src/visibility.rscrates/gl/src/clone.rscrates/gl/src/main.rs
|
hello bro please fix conflicts! |
f5864fc to
eab9e77
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/git/visibility_pack.rs (1)
94-124: 💤 Low valueConsider caching
blob_pathsresult to avoid duplicate traversal.
withheld_blob_oids(called on line 100) internally invokesblob_paths, and then line 111 callsblob_pathsagain. For large repositories, this duplicates thegit ls-treework across all refs.A refactor could extract
blob_pathsonce and pass the result to both the withholding check and the recipient computation, halving the subprocess overhead.🤖 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 `@crates/gitlawb-node/src/git/visibility_pack.rs` around lines 94 - 124, The function withheld_blob_recipients calls blob_paths twice - once indirectly within withheld_blob_oids and again explicitly in the for loop starting around line 111. For large repositories, this duplicates expensive git ls-tree operations. Refactor the code to call blob_paths once at the beginning and pass the result to both withheld_blob_oids and the recipient computation loop, either by modifying withheld_blob_oids to accept the blob paths as a parameter or by restructuring the logic to compute blob_paths once and reuse it throughout the function.
🤖 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.
Nitpick comments:
In `@crates/gitlawb-node/src/git/visibility_pack.rs`:
- Around line 94-124: The function withheld_blob_recipients calls blob_paths
twice - once indirectly within withheld_blob_oids and again explicitly in the
for loop starting around line 111. For large repositories, this duplicates
expensive git ls-tree operations. Refactor the code to call blob_paths once at
the beginning and pass the result to both withheld_blob_oids and the recipient
computation loop, either by modifying withheld_blob_oids to accept the blob
paths as a parameter or by restructuring the logic to compute blob_paths once
and reuse it throughout the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fc40e51d-90bc-4958-a4f8-5d1953837585
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
crates/gitlawb-core/Cargo.tomlcrates/gitlawb-core/src/encrypt.rscrates/gitlawb-core/src/identity.rscrates/gitlawb-core/src/lib.rscrates/gitlawb-node/Cargo.tomlcrates/gitlawb-node/src/api/encrypted.rscrates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/arweave.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/encrypted_pin.rscrates/gitlawb-node/src/git/visibility_pack.rscrates/gitlawb-node/src/ipfs_pin.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/pinata.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/sync.rscrates/gitlawb-node/src/visibility.rscrates/gl/src/clone.rs
✅ Files skipped from review due to trivial changes (1)
- crates/gitlawb-node/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- crates/gitlawb-core/Cargo.toml
- crates/gitlawb-core/src/identity.rs
- crates/gitlawb-node/src/api/mod.rs
- crates/gitlawb-node/Cargo.toml
- crates/gitlawb-node/src/server.rs
- crates/gitlawb-node/src/ipfs_pin.rs
- crates/gitlawb-node/src/pinata.rs
- crates/gitlawb-node/src/arweave.rs
- crates/gitlawb-node/src/visibility.rs
- crates/gitlawb-node/src/encrypted_pin.rs
- crates/gitlawb-core/src/lib.rs
- crates/gitlawb-node/src/db/mod.rs
- crates/gitlawb-node/src/api/encrypted.rs
- crates/gitlawb-node/src/api/repos.rs
- crates/gitlawb-node/src/sync.rs
eab9e77 to
d0a059b
Compare
|
hello bro @beardthelion please fix rebase and fix conflicts |
…adability
The origin no longer stores recipient DIDs. Migration v5 replaces the
encrypted_blobs.recipients column with an opaque, node-keyed recipients_tag
used only to detect a recipient-set change for re-seal. B1 discovery and fetch
are now gated by the same repo-readability check the git read path uses, not by
per-recipient matching; decryption is gated by the envelope crypto, so a
non-recipient who can read the repo sees a blob's {oid, cid} but cannot open it.
encrypt_and_pin keys the tag from the node seed and returns {oid, cid}; the
Arweave manifest tuple drops the now-unused recipient vec.
A DB compromise no longer reveals the reader set; recovering it would require
brute-forcing candidate DID sets against the keyed tag with the node key.
…rror
Address review on the at-rest blinding change:
- The encrypted-blobs/replicate listing returned {oid, cid} with no visibility
check, so a non-readable repo's blob index was reachable by an unauthenticated
caller who guessed {owner}/{repo}. Gate it by the same repo-readability check
discovery and fetch use. For the intended case (a public repo with withheld
subtrees) the public root keeps this open to peers; only fully non-readable
repos are withheld, which is the desired behavior.
- encrypt_and_pin treated a recipients_tag DB read error as a cache miss and
resealed, causing avoidable IPFS writes during a partial outage; skip and retry
on the next push instead.
- Correct the get_encrypted_blob doc comment to describe repo-readability access.
d0a059b to
6b7bf15
Compare
|
Rebased onto main and conflicts resolved. The branch is down to 3 commits (tip 6b7bf15) and fast-forwards cleanly on the current main, no conflicts remaining. Ready for review/merge. |
main grew a third seed_bytes() caller (repos.rs, from #40) after this branch was cut. Route it through to_seed() too: node_seed is moved into a long-lived tokio::spawn pin task, so the Zeroizing wrapper scrubs the copy when that task ends. encrypt_and_pin takes &[u8; 32]; the wrapper deref-coerces, so the call site is unchanged.
Resolves a migration version collision in db/mod.rs: both this branch and #40 (now on main) added a version-5 migration. Kept main's released v5 (encrypted_blobs_blind_recipients) and renumbered this branch's agent_retirement migration to v6, appended after it. The two are independent (agents vs encrypted_blobs); only the version number collided. schema_migrations keys on version, and the strictly-increasing invariant test passes (1..6).
Reconciles encrypted_pin.rs with #40 (blind recipient identities, now on main). The encrypt_and_pin body auto-merged cleanly: #67's fail-closed resolve_all_recipients gate now sits on top of #40's opaque recipients_tag structure (tag unchanged -> skip; tag changed -> require full recipient resolution before sealing). Two spots needed manual resolution: - kept #67's resolve_all_recipients helper (the merged body calls it) and took #40's encrypt_and_pin doc (the merged fn returns (oid, cid) and never returns recipient identities). - merged both test groups under one generic set<S: AsRef<str>> helper so the #47 fail-closed tests and the recipients_tag tests coexist.
* fix(core): route seed access through the zeroizing wrapper (#41) Remove Keypair::seed_bytes(), which returned a bare [u8; 32] copy of the Ed25519 private seed. Because [u8; 32] is Copy with no Drop, each caller's copy was released without being scrubbed, leaving secret key material in freed memory. The zeroizing accessor to_seed() -> Zeroizing<[u8; 32]> already existed beside it; this makes it the sole seed accessor and migrates the two in-crate callers (envelope open path and a crypto test) to it via deref coercion. No behavior change to envelope encryption: the derived X25519 secret is byte-identical, covered by the existing round-trip tests. Adds an identity test pinning to_seed() as the only seed accessor. * test(core): pin to_seed's Zeroizing return type, drop duplicate round-trip The to_seed_is_sole_seed_accessor test was named for a compile-time property its body couldn't assert, and its from_seed -> DID check duplicated from_seed_round_trip. Replace it with a type-annotated binding that fails to compile if to_seed ever returns a bare [u8; 32] again, plus the determinism check, which is the only behavior it uniquely covered. * fix(node): migrate node_seed pin caller to to_seed() main grew a third seed_bytes() caller (repos.rs, from #40) after this branch was cut. Route it through to_seed() too: node_seed is moved into a long-lived tokio::spawn pin task, so the Zeroizing wrapper scrubs the copy when that task ends. encrypt_and_pin takes &[u8; 32]; the wrapper deref-coerces, so the call site is unchanged.
Completes recipient-set blinding for encrypted replication: the reader set of a withheld blob is no longer stored or published anywhere in cleartext, only in the encrypted envelope key-wraps.
This stacks on #36 (encrypted replication B1/B2/B3, which already blinds the envelope, the peer-replication wire, and the Arweave manifest). Since the upstream repo only has
main, the diff here carries that stack as prerequisites; the new work in this PR is the at-rest change.What changed
encrypted_blobs.recipientscolumn (a cleartext DID list) is replaced (migration v5) with an opaque, node-keyedrecipients_tag(HMAC-SHA256 over the sorted DID set). The tag is used only to detect a recipient-set change so an unchanged blob is not re-sealed; it is never the DID list./encrypted-blobs) and fetch (/encrypted-blob/{oid}) are now authorized by repo readability (the samevisibility_checkthe git read path uses) rather than per-recipient matching. Decryption is gated by the envelope crypto: only a real recipient can open an envelope.encrypt_and_pinkeys the tag from the node seed and returns{oid, cid}; the Arweave manifest tuple drops the now-unused recipient vec; the mirror path stores no recipient data.Why
An origin that enforces per-recipient authorization must be able to test whether a caller is a recipient, which means it can always recover the reader set. Moving authorization onto repo readability plus envelope crypto takes the origin out of that loop, so a DB compromise no longer reveals who can read a blob.
Accepted trade-offs
{oid, cid}). The OID is already in the tree they clone and the envelope bytes are already public on IPFS, so no new secret is exposed.Tests
recipients_tagis covered by unit tests (order-insensitive, set-sensitive, node-keyed). The handler authorization change is wiring around the existing, testedvisibility_check.Summary by CodeRabbit