fix(restore): bound blob processing + negative-cache permanent decrypt failures (GH #501) - #529
Closed
harrymove-ctrl wants to merge 2 commits into
Closed
fix(restore): bound blob processing + negative-cache permanent decrypt failures (GH #501)#529harrymove-ctrl wants to merge 2 commits into
harrymove-ctrl wants to merge 2 commits into
Conversation
…ailures (GH #501) restore() discovered blob_ids purely by current on-chain ownership, so a Walrus Blob transferred into a victim's wallet by an attacker (carrying memwal_* metadata) got downloaded and SEAL-decrypt-attempted on every single future restore() call, forever, since a failed decrypt/validation was never recorded anywhere. Per Henry's explicit rejection of an uploader/relayer allowlist fix for this issue, this instead: - Negative-caches permanently-failed (owner, namespace, blob_id) attempts in a new restore_failed_blobs table, so a foreign blob that fails SEAL decrypt (classified via the existing DecryptOutcome::permanent_from_error) or UTF-8 validation is excluded from future restore() calls. Transient failures (SEAL timeouts, 429/503) are never cached, so an infra blip can't permanently blacklist a legitimate blob. - Clamps RestoreRequest.limit to 100, matching /api/ask's existing cap. - Adds an owner-scoped restore call-frequency guard (default 10/min) on top of the generic account rate limiter, to bound how often an attacker can force a fresh first-time-discovery cost by repeatedly transferring junk blob_ids into a victim's wallet. None of this inspects who uploaded, created, or relayed a blob -- it's keyed purely by (owner, blob_id) outcome and owner-level call frequency, so the self-host-then-migrate-to-managed-relayer path is untouched.
…ure as transient
sendSealFailure (sidecar) echoes the configured request timeout as
'(traceId=..., timeoutMs=...)' on every composed error, permanent or
not. The classifier's bare lower.contains("timeout") check matched
that config echo unconditionally, so it returned transient for every
real error from /seal/decrypt before the permanent-error branch was
ever reached -- including NoAccessError, the exact failure GH #501's
bounded-restore fix depends on catching to negative-cache a foreign,
undecryptable blob.
Match the actual timeout signal (TimeoutError / "aborted due to
timeout") instead of the bare substring, and add NoAccessError to the
permanent list. Both existing classifier tests used simplified
messages that never exercised the real wire format, so they passed
despite the bug; added tests pin the actual composed shape for both
the no-access and genuine-timeout cases.
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.
Summary
restore()discovers Walrus Blob objects by current on-chain ownership only, never by who created/uploaded them. A blob transferred into a victim's wallet by an attacker (carryingmemwal_*metadata) gets downloaded and SEAL-decrypt-attempted on every futurerestore()call, forever — a failed decrypt/validation was never recorded anywhere, so the same foreign blob keeps getting rediscovered and reprocessed indefinitely.Per Henry's explicit direction on this thread (an uploader/relayer allowlist would break the self-host-then-migrate-to-managed-relayer path and is expensive/fragile to implement), this fixes it with bounded processing + rate limiting instead:
restore_failed_blobstable. A blob that fails SEAL decrypt (classified via the existingDecryptOutcome::permanent_from_error) or UTF-8 validation for a given(owner, namespace, blob_id)is excluded from futurerestore()calls. Transient failures (timeouts, 429/503) are never cached, so an infra blip can't permanently blacklist a legitimate blob.limit:RestoreRequest.limitis now capped at 100, matching/api/ask's existing convention. It was previously unbounded on the Rust side (the only real ceiling was an incidental sidecar constant unrelated to this parameter).check_restore_call_rate_limit(default 10/min/owner), reusing the existing Redis sliding-window primitive inrate_limit.rs, on top of the generic account limiter — bounds how often an attacker can force a fresh first-time-discovery cost by repeatedly transferring junk blob_ids into a victim's wallet.permanent_from_error's timeout check matched the sidecar'stimeoutMs=config echo (present on every composed error), so it misclassified essentially all real decrypt failures — including the exactNoAccessErrorthis fix depends on — as transient. Tightened the timeout signal and addedNoAccessErrorto the permanent list, with tests pinning the actual wire format.None of this inspects, records, or restricts who uploaded/created/relayed a blob — it's keyed purely by
(owner, blob_id)outcome and owner-level call frequency. The self-host-then-migrate-to-managed-relayer path is untouched.Fixes WALM-299 / GH #501.
Test plan
cargo build— cleancargo test --bin memwal-server— 412 passed, 5 failed (pre-existing, unrelatedjobs::tests::*, fail only due to no local Postgres in this sandbox —PoolTimedOut), 31 ignoredNoAccessError/timeout classifier fix, and thelimitclamp all pass