fix: enforce the per-request-type event mask, not always mask.get - #250
Open
jonaswre wants to merge 2 commits into
Open
fix: enforce the per-request-type event mask, not always mask.get#250jonaswre wants to merge 2 commits into
jonaswre wants to merge 2 commits into
Conversation
`EventSender::request` is generic over the four request types but reads a
single hardcoded field for all of them:
match self.mask.get { // for get, get_many, push AND observe
`mask.push`, `mask.get_many` and `mask.observe` are never read anywhere in
the crate -- the only mask reads are `connected`, `get` and `throttle`.
The consequence is that `RequestMode::Disabled` never takes effect for
push. `RequestMode::None` -- what `BlobsProtocol::new(&store, None)`
installs via `EventMask::DEFAULT` -- means "allow, don't notify", so the
`Disabled` arm returning `ProgressError::Permission` is unreachable for
push under every configuration I could construct.
That contradicts the documented contract in three places: the `EventMask`
docs ("push requests are disabled by default, as they can write to the
local store"), `DEFAULT` and `ALL_READONLY` both setting
`push: RequestMode::Disabled`, and `ALL_READONLY` explicitly declining to
ship a push-enabled constant because it "would risk misuse".
So a peer that knows a node's `EndpointId` can push blobs of its choosing
into the store, which the node then serves to anyone presenting the hash.
Bounding it honestly: content is BLAKE3-verified against the hash the
pusher names, so existing blobs cannot be forged, substituted or
corrupted. The impact is unauthorized write and content injection -- the
node becomes a content host for attacker-chosen data under the operator's
identity.
Thread the applicable `RequestMode` into `request` as a parameter,
supplied by four typed wrappers next to the mask itself so the selection
lives with the data it reads. Observe maps through a new
`From<ObserveMode> for RequestMode`; it has no `Disabled` variant, so its
behaviour is unchanged beyond reading the right field.
Note the existing push tests were passing *because* of this:
`event_handler` used `ALL_READONLY`, which disables push, and
`two_nodes_push_blobs_fs`/`_mem` asserted the data arrived anyway. They
now use an explicitly push-enabled mask -- the conscious act the
`ALL_READONLY` doc asks for. Anyone relying on push under the default
mask will see the same change.
New tests push the same blob to a denying node and to a permitting one.
The pusher cannot observe a refusal (`execute_push_sink` stops its
receive stream and returns `Stats::default()`, and streams are handled in
detached tasks), so the assertion is that the blob does not land. The
permitting node is the control: it proves the push path works in that
setup, so the deny assertion cannot pass vacuously. Both were confirmed
to fail on unpatched main, for the right reason.
96 lib tests pass, clippy and fmt clean.
Reading the right mask field un-gates every config that had only ever set `mask.get` and relied on the old shared dispatch. Two shipped examples do exactly that: - `examples/limit.rs::limit_by_hash` is `get: Intercept, ..DEFAULT` and only answers `GetRequestReceived`. get_many and observe used to be intercepted through the same field and refused; on this branch they read `DEFAULT`'s `None` and are served unconditionally, so a peer reads any blob by asking as a `GetManyRequest` and probes bitfields with `ObserveRequest`. Answer all three request types with the same allowlist. - `examples/random_store.rs` builds on `ALL_READONLY` and gates pushes on `--allow-push` inside the `PushRequestReceived` arm. `mask.push` is `Disabled` there, so the mask now rejects before the handler runs and the flag silently does nothing. Hand push to the handler that decides. My claim that observe's behaviour was unchanged was wrong in both directions. Observe used to read `mask.get`, so `get: Disabled` did disable it, and nothing on this branch could — an observe response streams which byte ranges of a hash the node holds, so add `ObserveMode::Disabled`. And `get: InterceptLog` gave observe an `Active` tracker, while `Intercept -> Intercept` yields `Disabled`: an observe request transfers no blobs, so completion is the only update it can emit, and the plain variants dropped it. Map through the `*Log` variants. The new test was weaker than it read. `allow_count_rx.changed()` does not synchronize the denying node at all — nothing observes it, and a disabled request type emits no event — so the assertion was one early sample that would pass on a broken mask. `has` is also false for a partial import, so a rejection arriving mid-transfer would leave attacker-chosen bytes in the store and still pass. Assert `BlobStatus::NotFound`, repeatedly, to a deadline. Co-Authored-By: Claude Opus 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
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.
Description
EventSender::requestis generic over the four request types but reads a singlehardcoded field for all of them (
src/provider/events.rs):mask.push,mask.get_manyandmask.observeare never read anywhere in thecrate — the only mask reads are
connected,getandthrottle. All four entrypoints in
provider.rshave byte-identical bodies calling the same generic.The consequence is that
RequestMode::Disablednever takes effect for push.RequestMode::None— whatBlobsProtocol::new(&store, None)installs viaEventMask::DEFAULT— means "allow, don't notify", so theDisabledarmreturning
ProgressError::Permissionis unreachable for push under everyconfiguration I could construct.
That contradicts the documented contract in three places: the
EventMaskdocs("push requests are disabled by default, as they can write to the local store"),
DEFAULTandALL_READONLYboth settingpush: RequestMode::Disabled, andALL_READONLYexplicitly declining to ship a push-enabled constant because it"would risk misuse".
Impact. A peer that knows a node's
EndpointId— a public identifierdistributed in tickets and gossip — can dial the blobs ALPN and push blobs of its
choosing into the store, which the node then serves to anyone presenting the
hash. Bounding it honestly: content is BLAKE3-verified against the hash the
pusher names, so existing blobs cannot be forged, substituted or corrupted. The
impact is unauthorized write and content injection — the node becomes a content
host for attacker-chosen data under the operator's IP and identity.
Fix. Thread the applicable
RequestModeintorequestas a parameter,supplied by four typed wrappers next to the mask itself. Observe maps through a
new
From<ObserveMode> for RequestMode.Second commit: closing the gaps the first one opens
Reading the right field un-gates every config that had only ever set
mask.getand relied on the shared dispatch. Two of your own examples do exactly that,
so the first commit alone would ship a hole in the crate's own allowlist demo:
examples/limit.rs::limit_by_hashisget: Intercept, ..DEFAULTand onlyanswers
GetRequestReceived. Today get_many and observe are interceptedthrough the same field and refused (the example never answers them, the
dropped oneshot fails
rx.await??). After the first commit they readDEFAULT'sNoneand are served unconditionally — a peer reads any blob byasking as a
GetManyRequest, and probes bitfields withObserveRequest. Theexample now answers all three with the same allowlist.
examples/random_store.rsbuilds onALL_READONLYand gates pushes on--allow-pushinside thePushRequestReceivedarm.mask.pushisDisabledthere, so after the first commit the mask rejects before the handler ever
runs and the flag silently does nothing.
I was wrong about observe — the original text here claimed its behaviour was
unchanged because
ObserveModehas noDisabledvariant. It is wrong in bothdirections:
mask.get, soget: Disableddid disable observe.After the change nothing can, and an observe response streams the local
bitfield for a hash — exactly which byte ranges the node holds. Added
ObserveMode::Disabled.get: InterceptLoggave observe anActivetracker;Intercept -> Interceptyields
Disabled, so completion updates stop being delivered. An observerequest transfers no blobs, so completion is the only update it can ever emit,
and the plain variants drop it. The conversion now maps through the
*Logvariants.
The test in the first commit was also weaker than it read.
allow_count_rx.changed()does not synchronize the denying node at all —nothing observes it, and a disabled request type emits no event — so the
assertion was a single early sample that would pass on a broken mask.
hasisadditionally false for a partial import, so a rejection arriving mid-transfer
would leave attacker-chosen bytes in the store and still pass. It now asserts
BlobStatus::NotFound, repeatedly, to a deadline.Breaking Changes
Behavioural, not API: push now actually requires a mask that enables it.
Your own tests were passing because of the bug —
event_handlerinsrc/tests.rsusedALL_READONLY(which setspush: Disabled) andtwo_nodes_push_blobs_fs/_memasserted the pushed data arrived anyway. This PRswitches them to an explicitly push-enabled mask, which is the conscious act the
ALL_READONLYdoc asks for. Any downstream relying on push underDEFAULTorALL_READONLYwill see the same change, and I think that is the point of thefix rather than a regression — but it is worth a release note.
Two smaller behavioural changes come with the second commit and are worth the
same note:
ObserveModegains aDisabledvariant (additive, but the enum ismatched exhaustively inside the crate), and
ObserveMode::Notify/Interceptnow deliver the completion update they used to get from
mask.get's*Logsetting.
Notes & open questions
which imported iroh-blobs v0.103.0. Traced to
feat: Provider events refactor (#142)(2025-09-11) — it looks like a copy-paste slip while DRYing the four per-type
gates into one generic, not a deliberate choice.
before recognising it was inherited from upstream rather than introduced by us.
Details are in fix: three access-control bugs found by a security review, plus a relay teardown bug holon-technologies/iroh#24. Happy to help with coordination.
get_manyhas the same shape: an operator settingget_many: Disabledalongside
get: Nonecurrently gets get-many silently allowed. Same fix coversit.
observe a refusal at all.
execute_push_sinkcallsrecv.stop(0)and returnsOk(Stats::default())unconditionally, and the provider handles each stream ina detached task, so a small push reports success while the receiver drops
every byte. That makes the newly-enforced default silent data loss for anyone
who was relying on
BlobsProtocol::new(&store, None)accepting pushes. Wortha follow-up on the remote API's error contract; say the word and I'll open one.
main, for the rightreason. 96 lib tests pass;
cargo clippy --all-targets --all-featuresandcargo fmtclean on both commits.Change checklist
🤖 Generated with Claude Code