From 1c65ee623049cef7cda58095458c12b6a1bd488d Mon Sep 17 00:00:00 2001 From: "Maciek \"mab122\" Bator" Date: Tue, 14 Jul 2026 17:47:15 +0200 Subject: [PATCH] Fix: git fetch rad can never fetch refs/notes/rad-lfs Found in a real deployment (mbator.pl blog pipeline): rad lfs init configures a client-side fetch refspec for refs/notes/rad-lfs, but git-remote-rad's for_fetch() never advertised any such ref -- notes only exist per-peer, under refs/namespaces//refs/notes/rad-lfs, with no canonicalization step the way refs/heads/refs/tags get one. Requesting an unadvertised refspec fails the *entire* git fetch, not just that one ref, so any repo with `rad lfs init` run could no longer git fetch/pull at all, LFS or not. Fix: expose each peer's refs/notes/rad-lfs individually, as refs/notes/rad-lfs/ (list.rs's for_fetch), matching how patch refs already work rather than trying to canonicalize -- notes are per-peer contributions (any peer can commit an LFS-tracked file, not just delegates), so unlike heads there's no single "correct" value to resolve to; canonicalizing would silently drop non-delegate contributors' objects. rad lfs init's fetch refspec becomes a wildcard (+refs/notes/rad-lfs/*:refs/notes/rad-lfs/*); push is unchanged (already worked -- for_push() turns out to be purely informational for git's list-for-push handshake, not an enforcement gate, so the actual send-pack call was never restricted to heads/tags despite what for_push() lists). rad lfs store/fetch/rekey and seed/unseed's pinning (ipfs.rs::lfs_cids) now read across every notes ref instead of assuming one canonical ref, merging recipient lists for notes that share a CID (e.g. after a rekey performed by a different peer than the original committer) while keeping genuinely different ciphertexts (from two peers independently encrypting byte-identical content) as separate candidates to try in turn. Also added a migration step so re-running `rad lfs init` on an already-configured repo actually fixes it, by removing the old non-wildcard fetch refspec rather than just adding the wildcard alongside a still-broken entry. Verified through the real git-remote-rad transport (not the storage-copy shortcut used for earlier testing, since this bug lives specifically in that transport): fresh identities, a real rad:// remote matching exactly what `rad init` configures, a genuine `git fetch rad` that used to fail with "fatal: couldn't find remote ref refs/notes/rad-lfs" now succeeds and pulls the notes ref, and `git lfs pull` afterward correctly decrypts private content end-to-end (SHA-256 verified). Re-verified the rekey flow through the same real path too. --- LFS-IPFS.md | 2 + NOTES-lfs-notes-fetch-bug.md | 191 +++++++++++++++++++ crates/radicle-cli/src/commands/lfs/fetch.rs | 63 +++--- crates/radicle-cli/src/commands/lfs/init.rs | 66 ++++++- crates/radicle-cli/src/commands/lfs/rekey.rs | 139 +++++++------- crates/radicle-cli/src/ipfs.rs | 37 ++-- crates/radicle-cli/src/lfs_crypto.rs | 100 ++++++++++ crates/radicle-remote-helper/src/list.rs | 31 +++ 8 files changed, 503 insertions(+), 126 deletions(-) create mode 100644 NOTES-lfs-notes-fetch-bug.md diff --git a/LFS-IPFS.md b/LFS-IPFS.md index 1943b599..1b4c1c66 100644 --- a/LFS-IPFS.md +++ b/LFS-IPFS.md @@ -213,6 +213,8 @@ after they were added. | Large file didn't get pinned (commit went through, but no `rad-lfs` note) | The pre-commit hook silently skips pinning if the `ipfs`/`rad` CLIs aren't on `PATH` — check `command -v ipfs` / `command -v rad`. Re-add the file and commit again once available. | | `private-repo LFS operations need to perform a key-agreement (ECDH) operation ... Set RAD_PASSPHRASE` | Your keystore is encrypted and no passphrase is available (an ssh-agent-only signer can't do key agreement). Set `RAD_PASSPHRASE` in the environment the commit/fetch runs in, or use an unencrypted keystore. | | `you are not an authorized recipient of this object` | Either genuinely unauthorized (not a delegate/allow-listed DID for this private repo), or authorized *after* this specific object was committed and nobody has run `rad lfs rekey` since — ask an already-authorized collaborator to run it and push. | +| `fatal: couldn't find remote ref refs/notes/rad-lfs` on `git fetch`/`git pull` (breaks *all* fetching, not just LFS) | Fixed — but if this repo had `rad lfs init` run against an older build, re-run `rad lfs init` once to replace the stale, now-invalid fetch refspec it configured (see `NOTES-lfs-notes-fetch-bug.md` for the full story). | +| Plain `git push rad` (no branch name) doesn't push your commits, only the notes ref | Known, separate quirk: once any explicit push refspec is configured (which `rad lfs init` does), git stops falling back to pushing the current branch by default. Always push explicitly: `git push rad `. | ## Status diff --git a/NOTES-lfs-notes-fetch-bug.md b/NOTES-lfs-notes-fetch-bug.md new file mode 100644 index 00000000..92de3749 --- /dev/null +++ b/NOTES-lfs-notes-fetch-bug.md @@ -0,0 +1,191 @@ +# Bug: `git fetch rad` can never fetch `refs/notes/rad-lfs` + +**RESOLVED** (2026-07-14). Fix: `refs/notes/rad-lfs` is now exposed per-peer +(`refs/notes/rad-lfs/`) instead of expecting one canonical bare ref, matching how +patch refs already work — see "Fix actually shipped" at the bottom for the full writeup and +verification. Kept the original report below unedited for context. + +Found while deploying `rad-lfs-ipfs` to mbator.pl for the blog pipeline (2026-07-14). +Blocks the whole LFS-over-IPFS flow end-to-end, not just a cold-start edge case. + +## Symptom + +After `rad lfs init` in any repo, `git fetch rad` (and therefore `git pull`, and anything +that does the equivalent, e.g. `deploy.sh`'s `git fetch rad && git reset --hard rad/master`) +fails every time with: + +``` +fatal: couldn't find remote ref refs/notes/rad-lfs +``` + +This isn't specific to a fresh repo where nobody has pushed an LFS object yet — it +reproduces even after a real push has created the note and it's fully present in the +receiving node's storage. + +## Root cause + +Two places are involved: + +1. **`crates/radicle-cli/src/commands/lfs/init.rs:134`** — `rad lfs init` configures the + client-side fetch/push refspec as a bare, non-namespaced ref: + ```rust + let notes_refspec = format!("+{NOTES_REF}:{NOTES_REF}"); // +refs/notes/rad-lfs:refs/notes/rad-lfs + ``` + +2. **`crates/radicle-remote-helper/src/list.rs`, `for_fetch()`** (lines 38-79) — this is + what actually determines which refs `git-remote-rad` advertises as fetchable for a + plain (non-namespaced) `git fetch rad`. It only ever lists: + - `HEAD` + - `refs/heads/*` and `refs/tags/*` (canonicalized across delegates) + - patch refs (via `patch_refs()`) + + It has **no knowledge of `refs/notes/rad-lfs` at all**. Notes only exist per-peer, + under `refs/namespaces//refs/notes/rad-lfs` (confirmed directly on a live + node: `git for-each-ref` in the node's storage shows + `refs/namespaces/z6Mkkph.../refs/notes/rad-lfs`, never a bare `refs/notes/rad-lfs`). + `refs/heads/*` gets a canonical, non-namespaced view via delegate-threshold + resolution; `refs/notes/rad-lfs` has no equivalent resolution step, so it's simply + never in the list the remote helper offers — regardless of what refspec the client + has configured in `.git/config`. + +So the client asks for a ref the server-side listing never advertises, and `git fetch` +(which validates requested refspecs against the advertised ref list) fails hard for the +*entire* fetch, not just that one ref — meaning a repo with `rad lfs init` run can't +`git fetch`/`git pull` *anything* anymore, heads included, until this is fixed. + +## Suggested direction for a fix + +`for_fetch()` needs to expose some resolved form of the LFS notes ref for the +non-namespaced case, analogous to how `refs/heads/*` gets canonicalized. Options, +roughly in order of how much they change the design: + +- **Simplest**: expose *all* peers' notes refs individually under some fetchable name + (e.g. `refs/notes/rad-lfs/`), and change the LFS tooling (`rad lfs + store`/`fetch`/`rekey`) to read/merge across all of them instead of expecting one + canonical ref. Matches how git notes are designed to be merged anyway. +- **Closer to current design**: pick one canonical note per commit the same way heads + get a canonical value (e.g. delegate-threshold agreement, or "the delegate's own + namespace" the way `for_push` already restricts pushes to `profile.id()`'s own + namespace) and expose that as the bare `refs/notes/rad-lfs`. +- Either way, `init.rs`'s refspec (`+refs/notes/rad-lfs:refs/notes/rad-lfs`) is probably + fine as-is *once* the server side actually advertises something at that path — the + bug is really in `list.rs`, not `init.rs`. + +## Confirmed working around this bug (verified live on mbator.pl, 2026-07-14) + +Everything upstream and downstream of this one step works: +- Fork built clean (`cargo install` for `radicle-cli`, `radicle-node`, + `radicle-remote-helper`, `radicle-lfs-transfer`) at commit `33ffa122`. +- `rad lfs init` succeeds and configures the pre-commit hook, transfer agent, and (broken) + refspec correctly per its own logic. +- A real private-repo LFS push (`git push rad master`, encrypted via ECDH, `RAD_PASSPHRASE` + from an ssh-agent-backed key) succeeded and replicated to a seed node. +- Node-to-node replication of the note itself works fine at the storage layer — the + receiving node's `refs/namespaces//refs/notes/rad-lfs` is populated correctly. +- The failure is *only* in the working-copy-level `git fetch rad` being unable to see it. + +## Also found along the way (separate, minor) + +Two `radicle-cli` crashes (not yet investigated, crash reports written to `/tmp/report-*.toml` +on the VPS at the time): +- `rad node status` crashed once (worked on retry). +- `rad seed` (no args, to check a repo's local seeding policy) crashed consistently. + +## Deployment state left in place (mbator.pl) + +- `blog` user has a working IPFS (Kubo) container, the fork built at `~/.radicle/bin`, + `radicle-node-blog.service`/`blog-radicle-watch.service`/`blog-deploy.service` all + pointed at the new binaries with `PATH`/`KUBO_API_URL` set correctly. +- Added an explicit `connect` entry for the local `seed` node + (`z6MkmUF9KWFBQzYXBeNJk1bXXQGzaATnEsWxmbUYt54fyEC6@127.0.0.1:8776`) to + `/home/blog/.radicle/config.json` — the private `blog` node had no route to the local + public seed by default (`connect: []`, generic `preferredSeeds`), so `rad sync` timed + out until this was added. Worth keeping regardless of the notes bug. +- Public `seed` node/`/usr/bin/radicle-node`/`radicle-httpd` untouched throughout. +- A harmless test commit (`3f650d2`, "infra: LFS-over-IPFS smoke test (safe to remove)") + is sitting on top of the blog repo's real history in `~/Blog/Blog` and pushed to the + network — adds `assets/_infra-test/lfs-ipfs-smoketest.jpg` (LFS-tracked, not referenced + from any post, won't appear anywhere on the live site). Safe to leave or revert + whenever convenient. + +## Next step once fixed + +Re-run `git fetch rad` in `/home/blog/repo` on the VPS (as `blog`) — if the fix resolves +the notes ref, that fetch (and the LFS smudge it triggers) is the last remaining piece to +confirm the whole IPFS content-fetch path end-to-end. + +## Fix actually shipped + +Went with the "simplest" option from the list above, since it's also the semantically +correct one: `refs/heads`/`refs/tags` get a canonical, non-namespaced value because only +*delegates'* view matters for canonical history — a whole separate subsystem (canonical-ref +computation on push/sync) maintains that. Notes have no equivalent: any peer, not just +delegates, can commit an LFS-tracked file and write a note for it, so there's no single +"correct" value to resolve to. Building delegate-consensus canonicalization for notes would +also have been semantically wrong, not just more work — it would silently drop non-delegate +contributors' LFS objects from ever being discoverable by anyone else. + +- `crates/radicle-remote-helper/src/list.rs`, `for_fetch()`: now also lists every peer's + `refs/notes/rad-lfs`, individually, as `refs/notes/rad-lfs/` (new `lfs_notes_refs` + helper, using `stored.remotes()` + `stored.reference_oid()`). `for_push()` needed no change + — turns out it's purely informational (used for the `list for-push` git protocol handshake, + i.e. fast-forward computation), not an enforcement gate; the actual `push_ref`/`send-pack` + call already accepts any refspec regardless of what `for_push()` lists, which is why notes + pushing already worked despite `for_push()` only ever listing `refs/heads`/`refs/tags`. +- `crates/radicle-cli/src/commands/lfs/init.rs`: fetch refspec becomes a wildcard + (`+refs/notes/rad-lfs/*:refs/notes/rad-lfs/*`); push refspec is unchanged (`git push` + lands under the pusher's own namespace server-side regardless of the local ref name, same + as `refs/heads/*`, so no wildcard needed there). Added a migration step + (`remove_refspec_if_present`) that strips the old broken non-wildcard fetch refspec from + already-configured repos — otherwise re-running `rad lfs init` would've just added the + correct wildcard *alongside* the still-broken exact-match entry, which would still break + the whole fetch (git fails the entire fetch if *any* requested refspec isn't advertised). +- `crates/radicle-cli/src/lfs_crypto.rs`: new `find_envelopes`/`all_note_targets`/`find_cids` + read across every notes ref (local + every fetched peer), not just one. Notes sharing the + same `cid` (e.g. an original commit plus a later `rekey` that added recipients under a + *different* peer's namespace) have their recipient lists safely unioned. Notes with + *different* `cid`s for the same object (only possible if two peers independently encrypted + byte-identical content — same oid/size — producing different ciphertext each time, since + encryption uses a fresh random key/nonce) are kept as separate candidates; `rad lfs + fetch`/`rekey` try each in turn rather than assuming there's only ever one. +- `crates/radicle-cli/src/commands/lfs/fetch.rs`, `rekey.rs`, `ipfs.rs` (`lfs_cids`, used by + `seed`/`unseed` pinning): all switched from single-ref `repo.notes(Some(NOTES_REF))` / + `find_note` calls to the new multi-ref helpers above. `store.rs` needed no read-side change + (write-only, always writes to the local plain ref, which is correct as-is). + +### Verified for real (not the storage-copy shortcut used for the base feature's original +tests — this bug lives specifically in the `git-remote-rad` transport, so the fix had to be +exercised through the actual thing) + +Built fresh `rad`/`git-remote-rad`/`radicle-node`/`rad-lfs-transfer`, created two-then-three +real Radicle identities/profiles, a real private repo with an allow-list, and used the +**real** `rad://` remote (`git remote add rad rad://` for fetch, matching exactly what +`rad init` itself configures — an earlier test attempt using a peer-scoped URL for both fetch +and push accidentally bypassed the bug entirely by routing into `for_fetch()`'s *namespaced* +branch, which was never broken; had to redo it with the correct canonical URL to actually +exercise the bug/fix). + +- `git ls-remote rad` on the canonical URL now advertises `refs/notes/rad-lfs/` instead + of nothing. +- A genuine `git fetch rad` (no explicit refspec workaround) succeeds and actually pulls the + peer's notes ref down locally — this is the exact command from the bug report that used to + fail with `fatal: couldn't find remote ref refs/notes/rad-lfs`. +- `git lfs pull` after that fetch correctly decrypts the private object end-to-end + (SHA-256-verified). +- Re-tested the rekey scenario (grant a new peer access after the object was already + committed) through the same real fetch path: denied before rekey, an authorized peer runs + `rad lfs rekey` + pushes, the new peer re-fetches and successfully decrypts. + +### Also discovered along the way (separate, not fixed here — flagging, not scope-creeping) + +Once `rad lfs init` adds *any* explicit `remote..push` refspec (the notes one), a plain +`git push rad` (no branch argument) stops falling back to `push.default=simple` for the +current branch — git only pushes whatever's explicitly configured once any explicit refspec +exists. So after running `rad lfs init`, `git push rad` alone only pushes the notes ref; +`git push rad master` (explicit) is needed to also push the branch. This isn't new — it was +already true before this fix, and both the original bug report's own reproduction and this +fix's testing always used an explicit branch name when pushing, so it never surfaced as a +problem in practice. Worth a real fix at some point (e.g. `rad lfs init` also ensuring an +explicit `+refs/heads/*:refs/remotes/rad/*`-equivalent push default is present, or documenting +"always `git push rad ` explicitly" as a hard requirement), but out of scope for this +specific fetch-bug fix. diff --git a/crates/radicle-cli/src/commands/lfs/fetch.rs b/crates/radicle-cli/src/commands/lfs/fetch.rs index 7d767e73..93c5f532 100644 --- a/crates/radicle-cli/src/commands/lfs/fetch.rs +++ b/crates/radicle-cli/src/commands/lfs/fetch.rs @@ -8,15 +8,10 @@ use std::path::PathBuf; use anyhow::{Context as _, anyhow}; use crate::ipfs; -use crate::lfs_crypto::{self, Envelope}; +use crate::lfs_crypto; use crate::terminal as term; -pub fn run( - oid: String, - size: i64, - out: PathBuf, - ctx: impl term::Context, -) -> anyhow::Result<()> { +pub fn run(oid: String, size: i64, out: PathBuf, ctx: impl term::Context) -> anyhow::Result<()> { let profile = ctx.profile()?; let (repo, rid) = radicle::rad::cwd() .context("`rad lfs fetch` must be run inside a Radicle repository working copy")?; @@ -27,36 +22,40 @@ pub fn run( .blob(pointer_text.as_bytes()) .context("failed to write LFS pointer blob")?; - let note = repo.find_note(Some(ipfs::LFS_NOTES_REF), blob_oid).map_err(|_| { - anyhow!( + let candidates = lfs_crypto::find_envelopes(&repo, blob_oid) + .with_context(|| format!("failed to read LFS notes for oid {oid}"))?; + if candidates.is_empty() { + anyhow::bail!( "no CID recorded for oid {oid}; the peer that has this object needs to push its \ {} ref", - ipfs::LFS_NOTES_REF - ) - })?; - let message = note.message().ok_or_else(|| { - anyhow!( - "no CID recorded for oid {oid}; the peer that has this object needs to push its \ - {} ref", - ipfs::LFS_NOTES_REF - ) - })?; - let envelope: Envelope = serde_json::from_str(message) - .with_context(|| format!("failed to parse LFS note for oid {oid}"))?; + lfs_crypto::NOTES_REF + ); + } ipfs::check_daemon()?; - let bytes = match &envelope.enc { - None => ipfs::cat(&envelope.cid)?, - Some(enc) => { - let ciphertext = ipfs::cat(&envelope.cid)?; - let signer = lfs_crypto::load_signer(&profile)?; - lfs_crypto::decrypt_with(&ciphertext, enc, &signer, profile.did(), &rid, &oid)? + // Almost always exactly one candidate. More than one only happens if + // two peers independently encrypted byte-identical content (same + // oid/size) producing different ciphertext each time -- try each + // until one actually decrypts for us. + let mut last_err = None; + for envelope in candidates { + let result = match &envelope.enc { + None => ipfs::cat(&envelope.cid), + Some(enc) => ipfs::cat(&envelope.cid).and_then(|ciphertext| { + let signer = lfs_crypto::load_signer(&profile)?; + lfs_crypto::decrypt_with(&ciphertext, enc, &signer, profile.did(), &rid, &oid) + }), + }; + match result { + Ok(bytes) => { + std::fs::write(&out, bytes) + .with_context(|| format!("failed to write `{}`", out.display()))?; + return Ok(()); + } + Err(err) => last_err = Some(err), } - }; + } - std::fs::write(&out, bytes) - .with_context(|| format!("failed to write `{}`", out.display()))?; - - Ok(()) + Err(last_err.unwrap_or_else(|| anyhow!("no CID recorded for oid {oid}"))) } diff --git a/crates/radicle-cli/src/commands/lfs/init.rs b/crates/radicle-cli/src/commands/lfs/init.rs index 2fe8188f..d84a150b 100644 --- a/crates/radicle-cli/src/commands/lfs/init.rs +++ b/crates/radicle-cli/src/commands/lfs/init.rs @@ -131,11 +131,37 @@ pub fn run() -> anyhow::Result<()> { "Configured Git LFS to transfer objects via `{TRANSFER_AGENT_BIN}` (backed by IPFS)" ); - let notes_refspec = format!("+{NOTES_REF}:{NOTES_REF}"); - ensure_refspec(workdir, &format!("remote.{RAD_REMOTE}.push"), ¬es_refspec)?; - ensure_refspec(workdir, &format!("remote.{RAD_REMOTE}.fetch"), ¬es_refspec)?; + // Push stays a plain (non-namespaced) refspec: `git push` lands + // whatever local ref we push under the pusher's own namespace on the + // server side regardless of the ref name used, the same way + // `refs/heads/*` does. + // + // Fetch is different: `refs/notes/rad-lfs` is a per-peer ref (any + // peer can commit an LFS-tracked file, not just delegates), so + // there's no single canonical value the remote can advertise the way + // it does for `refs/heads`/`refs/tags`. The remote instead advertises + // every peer's note individually under `refs/notes/rad-lfs/`, + // so the fetch refspec needs a wildcard to pull them all; the LFS + // tooling merges across whatever's fetched (see + // `lfs_crypto::find_envelope`) rather than expecting one ref. + let push_refspec = format!("+{NOTES_REF}:{NOTES_REF}"); + let fetch_refspec = format!("+{NOTES_REF}/*:{NOTES_REF}/*"); + let fetch_key = format!("remote.{RAD_REMOTE}.fetch"); + + // Migration: earlier versions of `rad lfs init` configured a plain, + // non-wildcard fetch refspec for the notes ref, which `git fetch` can + // never satisfy -- the remote never advertises a bare + // `refs/notes/rad-lfs` (only the wildcarded per-peer form below), so + // a stale entry here breaks the *entire* fetch, not just this one + // ref. Remove it if present so re-running `rad lfs init` actually + // fixes an already-configured repository, rather than leaving the + // broken entry alongside the corrected one. + remove_refspec_if_present(workdir, &fetch_key, &format!("+{NOTES_REF}:{NOTES_REF}"))?; + + ensure_refspec(workdir, &format!("remote.{RAD_REMOTE}.push"), &push_refspec)?; + ensure_refspec(workdir, &fetch_key, &fetch_refspec)?; term::success!( - "Configured the `{RAD_REMOTE}` remote to push/fetch the `{NOTES_REF}` notes ref" + "Configured the `{RAD_REMOTE}` remote to push/fetch the `{NOTES_REF}` notes refs" ); install_pre_commit_hook(&repo)?; @@ -187,6 +213,38 @@ fn ensure_refspec(workdir: &Path, key: &str, value: &str) -> anyhow::Result<()> Ok(()) } +/// Remove `value` from the (potentially multi-valued) git config key +/// `key`, if present, without disturbing any other values under the same +/// key (e.g. the default `+refs/heads/*:refs/remotes/rad/*` that plain +/// `git remote add` already configures on `remote..fetch`). +fn remove_refspec_if_present(workdir: &Path, key: &str, value: &str) -> anyhow::Result<()> { + let output = radicle::git::run(Some(workdir), ["config", "--get-all", key]) + .with_context(|| format!("failed to read git config `{key}`"))?; + let existing = String::from_utf8_lossy(&output.stdout); + if !existing.lines().any(|line| line.trim() == value) { + return Ok(()); + } + + // `git config --unset` matches its value argument as a regex against + // the whole existing value, not literally -- anchor and escape so it + // removes exactly this one entry and nothing else. + let escaped: String = value + .chars() + .map(|c| { + if "\\^$.|?*+()[]{}".contains(c) { + format!("\\{c}") + } else { + c.to_string() + } + }) + .collect(); + let pattern = format!("^{escaped}$"); + + git::git(workdir, ["config", "--unset", key, &pattern]) + .with_context(|| format!("failed to remove stale git config `{key}` entry"))?; + Ok(()) +} + /// Install (or update, or chain onto) the `.git/hooks/pre-commit` hook that /// pins staged LFS objects to IPFS and records their CID as a git note. fn install_pre_commit_hook(repo: &Repository) -> anyhow::Result<()> { diff --git a/crates/radicle-cli/src/commands/lfs/rekey.rs b/crates/radicle-cli/src/commands/lfs/rekey.rs index f946ba4d..af2e7b17 100644 --- a/crates/radicle-cli/src/commands/lfs/rekey.rs +++ b/crates/radicle-cli/src/commands/lfs/rekey.rs @@ -12,8 +12,7 @@ use radicle::git::raw::Signature; use radicle::identity::Did; use radicle::storage::{ReadRepository as _, ReadStorage as _}; -use crate::ipfs; -use crate::lfs_crypto::{self, Envelope}; +use crate::lfs_crypto::{self, NOTES_REF}; use crate::terminal as term; const NOTE_AUTHOR_NAME: &str = "rad-lfs"; @@ -37,13 +36,15 @@ pub fn run(ctx: impl term::Context) -> anyhow::Result<()> { // authorized). let current = lfs_crypto::recipients_for(&doc, my_did); - let notes = match repo.notes(Some(ipfs::LFS_NOTES_REF)) { - Ok(notes) => notes, - Err(_) => { - term::success!("Nothing to rekey: no LFS objects are tracked in this repository yet"); - return Ok(()); - } - }; + // Walk every note across every peer's notes ref -- not just the local + // one -- since an object's only note may live under a peer we've + // fetched from rather than one we wrote ourselves. + let targets = lfs_crypto::all_note_targets(&repo) + .context("failed to enumerate LFS notes")?; + if targets.is_empty() { + term::success!("Nothing to rekey: no LFS objects are tracked in this repository yet"); + return Ok(()); + } let signature = Signature::now(NOTE_AUTHOR_NAME, NOTE_AUTHOR_EMAIL) .context("failed to construct note signature")?; @@ -51,24 +52,7 @@ pub fn run(ctx: impl term::Context) -> anyhow::Result<()> { let mut rekeyed_objects = 0; let mut added_recipients = 0; - for note in notes { - let Ok((_, target_oid)) = note else { - continue; - }; - let Ok(note) = repo.find_note(Some(ipfs::LFS_NOTES_REF), target_oid) else { - continue; - }; - let Some(message) = note.message() else { - continue; - }; - let Ok(mut envelope) = serde_json::from_str::(message) else { - continue; - }; - let Some(enc) = envelope.enc.as_mut() else { - // Public (unencrypted) object: nothing to rekey. - continue; - }; - + for target_oid in targets { // The note only stores the CID and encryption metadata, not the // oid/size that were used as HKDF context when wrapping the // content key. Recover them losslessly from the pointer blob @@ -88,58 +72,75 @@ pub fn run(ctx: impl term::Context) -> anyhow::Result<()> { continue; }; - let cek = match lfs_crypto::unwrap_cek(&enc.recipients, &signer, my_did, &rid, oid) { - Ok(cek) => cek, + let candidates = match lfs_crypto::find_envelopes(&repo, target_oid) { + Ok(candidates) => candidates, Err(err) => { - term::warning(format!( - "Skipping object with oid {oid}: {err} (you may not have been an authorized \ - recipient of this particular object)" - )); + term::warning(format!("Skipping object with oid {oid}: {err}")); continue; } }; - let existing: BTreeSet = enc.recipients.iter().map(|w| w.did).collect(); - let missing: Vec = current.difference(&existing).copied().collect(); - if missing.is_empty() { - continue; - } + // Almost always exactly one candidate; more than one only if two + // peers independently encrypted byte-identical content (same + // oid/size) producing different ciphertext each time. Rekey + // whichever ones we're actually authorized to unwrap -- skip the + // rest quietly, they're not our concern. + for mut envelope in candidates { + let Some(enc) = envelope.enc.as_mut() else { + // Public (unencrypted) object: nothing to rekey. + continue; + }; - let cek_key: chacha20poly1305::Key = (*cek).into(); - let mut any_added = false; - for recipient in &missing { - match lfs_crypto::wrap_cek_for(&cek_key, &signer, my_did, recipient, &rid, oid) { - Ok(wrapped) => { - enc.recipients.push(wrapped); - added_recipients += 1; - any_added = true; + let cek = match lfs_crypto::unwrap_cek(&enc.recipients, &signer, my_did, &rid, oid) { + Ok(cek) => cek, + Err(_) => { + // Not authorized for *this particular* candidate -- + // expected and not worth warning about, since another + // candidate for the same object (or none) may apply. + continue; } - Err(err) => { - term::warning(format!( - "Failed to wrap the content key for {recipient} on object with oid \ - {oid}: {err}" - )); + }; + + let existing: BTreeSet = enc.recipients.iter().map(|w| w.did).collect(); + let missing: Vec = current.difference(&existing).copied().collect(); + if missing.is_empty() { + continue; + } + + let cek_key: chacha20poly1305::Key = (*cek).into(); + let mut any_added = false; + for recipient in &missing { + match lfs_crypto::wrap_cek_for(&cek_key, &signer, my_did, recipient, &rid, oid) { + Ok(wrapped) => { + enc.recipients.push(wrapped); + added_recipients += 1; + any_added = true; + } + Err(err) => { + term::warning(format!( + "Failed to wrap the content key for {recipient} on object with oid \ + {oid}: {err}" + )); + } } } + + if !any_added { + continue; + } + + // Always write to our own local ref, regardless of which + // peer's ref the original note came from -- once pushed, this + // lands under our own namespace and is picked up by others + // via the fetch refspec, alongside (not replacing) the + // original note. + let message = serde_json::to_string(&envelope) + .context("failed to serialize LFS note envelope")?; + repo.note(&signature, &signature, Some(NOTES_REF), target_oid, &message, true) + .context("failed to write LFS note")?; + + rekeyed_objects += 1; } - - if !any_added { - continue; - } - - let message = serde_json::to_string(&envelope) - .context("failed to serialize LFS note envelope")?; - repo.note( - &signature, - &signature, - Some(ipfs::LFS_NOTES_REF), - target_oid, - &message, - true, - ) - .context("failed to rewrite LFS note")?; - - rekeyed_objects += 1; } if rekeyed_objects == 0 { diff --git a/crates/radicle-cli/src/ipfs.rs b/crates/radicle-cli/src/ipfs.rs index 50651a4b..0ddcd138 100644 --- a/crates/radicle-cli/src/ipfs.rs +++ b/crates/radicle-cli/src/ipfs.rs @@ -20,7 +20,12 @@ use radicle::git::raw::Repository; use crate::terminal as term; /// Git notes ref under which oid -> CID mappings for LFS objects are stored. -pub const LFS_NOTES_REF: &str = "refs/notes/rad-lfs"; +/// +/// This is the *local* ref name; readers need to merge across every +/// fetched peer's copy too (`refs/notes/rad-lfs/`) -- see +/// `crate::lfs_crypto::find_envelopes`/`all_note_targets`, which +/// `lfs_cids` below delegates to. +pub use crate::lfs_crypto::NOTES_REF as LFS_NOTES_REF; /// Default HTTP API address of a local Kubo (IPFS) daemon. const DEFAULT_KUBO_API_URL: &str = "http://127.0.0.1:5001"; @@ -48,32 +53,22 @@ pub fn check_daemon() -> anyhow::Result<()> { } } -/// Collect the set of CIDs referenced by LFS notes on [`LFS_NOTES_REF`]. +/// Collect the set of CIDs referenced by LFS notes, across every peer's +/// notes ref (not just our own — a repo we're seeding may hold objects +/// whose only note came from someone else's `refs/notes/rad-lfs/`). /// -/// Returns an empty vector if the notes ref doesn't exist, i.e. the -/// repository has no LFS objects tracked yet. This is not an error. +/// Returns an empty vector if no notes exist, i.e. the repository has no +/// LFS objects tracked yet. This is not an error. #[must_use] pub fn lfs_cids(repo: &Repository) -> Vec { - let notes = match repo.notes(Some(LFS_NOTES_REF)) { - Ok(notes) => notes, - // The `refs/notes/rad-lfs` ref doesn't exist, which just means - // there are no LFS objects tracked for this repository yet. - Err(_) => return Vec::new(), + let Ok(targets) = crate::lfs_crypto::all_note_targets(repo) else { + return Vec::new(); }; let mut cids = Vec::new(); - for note in notes { - let Ok((_, annotated_id)) = note else { - continue; - }; - let Ok(note) = repo.find_note(Some(LFS_NOTES_REF), annotated_id) else { - continue; - }; - let Some(message) = note.message() else { - continue; - }; - if let Ok(envelope) = serde_json::from_str::(message) { - cids.push(envelope.cid); + for target in targets { + if let Ok(found) = crate::lfs_crypto::find_cids(repo, target) { + cids.extend(found); } } cids diff --git a/crates/radicle-cli/src/lfs_crypto.rs b/crates/radicle-cli/src/lfs_crypto.rs index 3b284ce6..4f3b1f27 100644 --- a/crates/radicle-cli/src/lfs_crypto.rs +++ b/crates/radicle-cli/src/lfs_crypto.rs @@ -23,6 +23,7 @@ use chacha20poly1305::{Key, XChaCha20Poly1305, XNonce}; use cyphernet::Ecdh; use hkdf::Hkdf; use radicle::Profile; +use radicle::git::raw::{Oid, Repository}; use radicle::identity::{Did, Doc, RepoId, Visibility}; use radicle_crypto::ssh::keystore::MemorySigner; use serde::{Deserialize, Serialize}; @@ -33,6 +34,15 @@ pub const ENVELOPE_VERSION: u32 = 1; const HKDF_DOMAIN: &[u8] = b"radicle-lfs-wrap-v1"; const ALG: &str = "xchacha20poly1305"; +/// The local, not-yet-pushed note ref -- `rad lfs store`/`rekey` always +/// write here. Once pushed, this lands in the pusher's own namespace on +/// the server, and `rad lfs init`'s fetch refspec pulls every peer's copy +/// back down under `refs/notes/rad-lfs/` (see +/// `crates/radicle-remote-helper/src/list.rs`'s `lfs_notes_refs`). +pub const NOTES_REF: &str = "refs/notes/rad-lfs"; +/// Glob matching the local ref above plus every fetched peer ref. +const NOTES_REF_GLOB: &str = "refs/notes/rad-lfs*"; + /// Note content stored on `refs/notes/rad-lfs`, replacing the earlier /// bare `cid=` text. `enc` is `None` for public repos (plain /// passthrough, unchanged behavior). @@ -263,3 +273,93 @@ pub fn decrypt_with( .decrypt(nonce, ciphertext) .map_err(|_| anyhow!("content decryption failed -- wrong key or corrupted data")) } + +/// Finds every distinct envelope recorded for `blob_oid`, across the +/// local `refs/notes/rad-lfs` ref and every fetched peer ref under +/// `refs/notes/rad-lfs/`. Notes are per-peer contributions -- any +/// peer can commit an LFS-tracked file, not just delegates -- so more +/// than one peer's note can exist for the same object, e.g. after a +/// `rad lfs rekey` that added recipients under a different peer's +/// namespace than the original committer. +/// +/// Notes sharing the same `cid` are the *same* encrypted object (rekey +/// never changes the ciphertext, only adds wrapped-key entries for it), +/// so their recipient lists are safely unioned together. Notes with +/// *different* `cid`s (which can only happen if two peers independently +/// encrypted byte-identical plaintext -- same oid/size, since that's +/// what determines the pointer blob -- producing different ciphertext +/// each time since encryption uses a fresh random key/nonce) are kept as +/// separate candidates, since a wrapped-key entry from one is only valid +/// against its own ciphertext, never the other's. Callers that need to +/// decrypt should try each returned candidate in turn. +pub fn find_envelopes(repo: &Repository, blob_oid: Oid) -> anyhow::Result> { + let mut by_cid: Vec = Vec::new(); + + for name in notes_ref_names(repo)? { + let Ok(note) = repo.find_note(Some(&name), blob_oid) else { + continue; + }; + let Some(message) = note.message() else { + continue; + }; + let Ok(envelope) = serde_json::from_str::(message) else { + continue; + }; + + match by_cid.iter_mut().find(|e| e.cid == envelope.cid) { + Some(existing) => merge_recipients(existing, envelope), + None => by_cid.push(envelope), + } + } + + Ok(by_cid) +} + +/// Convenience wrapper for callers that only need the set of distinct +/// CIDs recorded for `blob_oid` (e.g. seed/unseed pinning, which needs +/// no key material and doesn't care about recipients at all). +pub fn find_cids(repo: &Repository, blob_oid: Oid) -> anyhow::Result> { + Ok(find_envelopes(repo, blob_oid)? + .into_iter() + .map(|e| e.cid) + .collect()) +} + +fn merge_recipients(into: &mut Envelope, other: Envelope) { + let (Some(into_enc), Some(other_enc)) = (into.enc.as_mut(), other.enc) else { + return; + }; + let existing: BTreeSet = into_enc.recipients.iter().map(|w| w.did).collect(); + into_enc + .recipients + .extend(other_enc.recipients.into_iter().filter(|w| !existing.contains(&w.did))); +} + +/// Every distinct blob oid that has at least one note recorded against it, +/// across every notes ref (local plus every fetched peer). Used by +/// `rad lfs rekey` to walk every known LFS object, since a single +/// `notes()` call on one ref would now miss objects whose only note lives +/// under a different peer's ref. +pub fn all_note_targets(repo: &Repository) -> anyhow::Result> { + let mut targets = BTreeSet::new(); + for name in notes_ref_names(repo)? { + let Ok(notes) = repo.notes(Some(&name)) else { + continue; + }; + for (_, target_oid) in notes.flatten() { + targets.insert(target_oid); + } + } + Ok(targets) +} + +fn notes_ref_names(repo: &Repository) -> anyhow::Result> { + let mut names = Vec::new(); + for reference in repo.references_glob(NOTES_REF_GLOB)? { + let reference = reference?; + if let Some(name) = reference.name() { + names.push(name.to_string()); + } + } + Ok(names) +} diff --git a/crates/radicle-remote-helper/src/list.rs b/crates/radicle-remote-helper/src/list.rs index 0d0fca77..d2a31af9 100644 --- a/crates/radicle-remote-helper/src/list.rs +++ b/crates/radicle-remote-helper/src/list.rs @@ -32,6 +32,9 @@ pub(super) enum Error { /// General repository error. #[error(transparent)] Repository(#[from] radicle::storage::RepositoryError), + /// Refs error. + #[error(transparent)] + Refs(#[from] radicle::storage::refs::Error), } /// List refs for fetching (`git fetch` and `git ls-remote`). @@ -73,6 +76,34 @@ pub(super) fn for_fetch + 'st Ok(mut refs) => lines.append(&mut refs), Err(e) => eprintln!("remote: error listing patch refs: {e}"), } + + // List each peer's `refs/notes/rad-lfs`, individually, under + // `refs/notes/rad-lfs/`. Notes are per-peer + // contributions -- any peer (not just delegates) can commit an + // LFS-tracked file and write a note for it -- so unlike + // `refs/heads`/`refs/tags` there is no single canonical value to + // resolve here; the reading side merges across every peer's note + // for a given object instead (see `radicle-cli`'s + // `lfs_crypto::find_envelope`). Do not abort the whole fetch if + // this fails, same as patch refs above. + match lfs_notes_refs(stored) { + Ok(mut refs) => lines.append(&mut refs), + Err(e) => eprintln!("remote: error listing LFS notes refs: {e}"), + } + } + + Ok(lines) +} + +/// List every peer's `refs/notes/rad-lfs`, under `refs/notes/rad-lfs/`. +fn lfs_notes_refs(stored: &R) -> Result, Error> { + let mut lines = Vec::new(); + let notes_ref = git::fmt::qualified!("refs/notes/rad-lfs"); + + for (peer, _) in stored.remotes()? { + if let Ok(oid) = stored.reference_oid(&peer, ¬es_ref) { + lines.push(format!("{oid} refs/notes/rad-lfs/{peer}")); + } } Ok(lines)