Skip to content

feat(store): expire, cap and rate-limit the anonymous registration path - #28

Merged
stormer78 merged 1 commit into
mainfrom
sec-4045/register-quotas
Sep 12, 2026
Merged

feat(store): expire, cap and rate-limit the anonymous registration path#28
stormer78 merged 1 commit into
mainfrom
sec-4045/register-quotas

Conversation

@stormer78

Copy link
Copy Markdown
Contributor

The rest of PG-2. push/register takes no credentials, so the handle registry
was a map any unauthenticated caller could grow without bound — and every
mutation reserialised the whole map under the write lock, so the cost per
anonymous request was O(n) in the number of handles already registered.

Four changes, deliberately in this order of importance.

1. Unprovisioned handles expire — the root-cause fix

A freshly registered handle is inert: it wakes nobody until its controller
VTA provisions a trigger onto it. So a handle whose allowlist is still empty long
after it was issued is junk, and the flood PoC produced nothing but junk.

HandleRecord gains created_at, and a tokio sweeper drops handles that are
still unprovisioned after GATEWAY_UNPROVISIONED_TTL_SECS (default 24 h).
Anonymous growth becomes bounded churn rather than a monotonic leak, which is
what makes the caps below a backstop rather than the whole defence.

created_at is #[serde(default)] so existing snapshots load. Those records
read as created_at = 0 and, if unprovisioned, are swept on the first pass —
which is the intended outcome rather than a quirk: a handle in an old snapshot
that was never provisioned is exactly what the sweep is for. One that was
provisioned has a non-empty allowlist and survives untouched.

A provisioned handle is never swept, at any age. Devices do not re-register on
a schedule, and expiring a live wake binding would break the thing the gateway
exists to do.

2. Caps

  • GATEWAY_MAX_HANDLES (default 100k) — total, refused as gateway at capacity.
  • GATEWAY_MAX_HANDLES_PER_TOKEN (default 4) — live handles sharing one device
    token or Web Push endpoint, so one device (or one stolen token) cannot occupy
    the registry on its own.

The per-token count is an index maintained under the same lock as the map,
not a scan. A scan would be O(n) per registration, i.e. most expensive during
exactly the flood it is meant to stop. Handles and index live in one State
struct behind the single RwLock so they cannot drift apart.

The index keys on a truncated SHA-256 of the token rather than the token itself.
That is not about collision resistance (128 bits is far beyond what a counting
index needs) — it avoids keeping a second copy of a bearer credential in another
map, and in the keys of one, where it is easy to log by accident.

Web Push is keyed on the endpoint, since the endpoint is the destination and
the keys only encrypt; re-registering the same subscription with fresh keys does
not buy a new slot.

A snapshot that loads over the cap is kept, not truncated, with a warning:
dropping live devices' handles because a limit was lowered would be worse than
being temporarily over it. New registrations are refused until the sweep drains
it.

3. Rate limits, in two layers

Two layers because the DIDComm transport never passes through axum
middleware
, and DIDComm is the preferred transport. A tower layer alone
would leave the main path unlimited.

  • HTTP, per peer IPtower_governor on POST /trust-tasks, answering 429
    before the body is read. This needs
    into_make_service_with_connect_info::<SocketAddr>(), which is why main.rs
    changes how it serves. PeerIpKeyExtractor is used; behind a trusted reverse
    proxy SmartIpKeyExtractor would read X-Forwarded-For, which is only sound
    when that proxy is the sole ingress — noted in a comment rather than switched,
    because this repo does not know its own deployment.
  • In the dispatch coreregister draws on one global budget (it is
    anonymous, so there is no caller to key on), and provision/wake on a budget
    keyed by the authenticated caller DID, so one misbehaving VTA or trigger is
    throttled without affecting anyone else. This layer covers both transports.

The global register budget is an explicit trade, and worth stating plainly: a
flood from anywhere consumes the same bucket, so a sustained attack can crowd out
legitimate registrations. It bounds the work and the registry growth, which is
what PG-2 is about; the per-IP layer is what separates a well-behaved client from
one noisy source. Closing this properly needs a controller identity to key on,
which is PG-9.

tower_governor is taken with default-features = false, features = ["axum"]:
its default feature set enables tonic and drags a gRPC stack into a crate that
speaks neither.

The keyed limiter is itself a map keyed by caller-chosen input — a growth
vector of exactly the kind this PR exists to close. Limits::shrink drops
fully-replenished buckets (indistinguishable from absent ones, so there is no
policy effect) and runs on the same 60 s timer as the sweep.

4. Debounced persistence

persist_locked ran on every insert, provision and remove. Mutations now set a
dirty flag and a background flusher writes at most once per
GATEWAY_SNAPSHOT_FLUSH_MS (default 1000), keeping the temp-file + fsync +
rename sequence. Drop flushes, so a clean shutdown — and a test that drops and
reopens — loses nothing, and the flusher takes a final write on cancellation.

Tests

store and limits unit tests plus four integration tests:

  • register_flood_is_refused_after_the_burst — the register-flood.sh PoC: 200
    registrations, only the burst accepted, and gateway_register_total stops at
    the burst. The counter is the assertion that matters: it proves the refusals
    happened before anything was stored.
  • sweep_drops_only_stale_unprovisioned_handles — nothing swept at TTL−1, both
    unprovisioned handles swept at TTL, the provisioned one untouched;
    sweep_loop_runs_on_its_interval drives the real task under
    #[tokio::test(start_paused = true)].
  • eleventh_registration_is_refused_at_capacity with max_handles = 10, and
    repeated_registration_of_one_token_is_capped.
  • inserts_are_debounced_into_few_snapshot_writes — 1,000 inserts cause zero
    writes on their own, one flush writes exactly once, a second flush writes
    nothing, and the single write holds all 1,000 handles. There is a write counter
    on the store so this asserts the debounce rather than trusting a timer.
  • legacy_snapshot_without_created_at_loads — a hand-written pre-created_at
    snapshot loads, its unprovisioned record sweeps, its provisioned one survives.
  • wake_budget_is_per_caller_did — a noisy trigger is capped at its burst while a
    different trigger DID is unaffected.

Existing tests use Limits::permissive(), so a rate limit tripping mid-test
cannot turn into a confusing unrelated failure.

The per-IP HTTP layer is applied in main.rs rather than inside api::router,
deliberately: PeerIpKeyExtractor requires ConnectInfo, which oneshot does
not provide, so putting it in router() would break every integration test for
reasons unrelated to what they test. The behaviour that needs a regression test —
a flood being refused and the counter not climbing — is the core limiter's, and
that is tested through the router.

Operator-visible changes

All new, all optional, all with working defaults:

Variable Default Effect
GATEWAY_UNPROVISIONED_TTL_SECS 86400 sweep never-provisioned handles
GATEWAY_MAX_HANDLES 100000 total registry cap
GATEWAY_MAX_HANDLES_PER_TOKEN 4 per-destination cap
GATEWAY_SNAPSHOT_FLUSH_MS 1000 minimum gap between snapshot writes
GATEWAY_REGISTER_PER_SEC / _BURST 5 / 20 global register budget
GATEWAY_PER_DID_PER_SEC / _BURST 20 / 60 per-caller-DID budget
GATEWAY_HTTP_PER_SEC / _BURST 10 / 40 per-peer-IP HTTP budget

Two behaviour changes to be aware of before rolling out:

  • A client that registers more than 4 handles for one device token will now be
    refused.
    That should not happen in normal use (a device registers once per
    install), but a client that re-registers on every service-worker spin-up
    without discarding the old handle would hit it. Raise
    GATEWAY_MAX_HANDLES_PER_TOKEN if so.
  • Handles never provisioned within 24 h disappear. A device that registers
    and only later conveys its handle to its VTA must do so inside the TTL.

The snapshot format is unchanged apart from the added created_at, which older
builds ignore, so this rolls back cleanly.

Verification

cargo fmt --all --check, cargo clippy --all-targets -- -D warnings,
cargo test --all-targets (47 unit + 16 integration, all passing) and
cargo deny check (advisories/bans/licenses/sources ok — the two new crates
introduce no new licence).

cargo deny emits one pre-existing warning, advisory-not-detected for
RUSTSEC-2025-0134 — the deny.toml ignore no longer matches anything in the
tree. It is a warning, exits 0, and is identical on origin/main (verified
against an untouched worktree at ae9a09b).

Not in this PR

  • PG-9, a controller-DID policy. Needs an owner decision on whether did:key
    controller VTAs are legitimate in production. It is also what would let the
    anonymous register budget become per-controller instead of global — the one real
    weakness in the limits above.
  • PG-N1, a provision replay guard. Blocked on the VTA setting expiresAt on
    provision documents; without it there is no window to bound a replay against.

@stormer78

Copy link
Copy Markdown
Contributor Author

Merge order. All four of #26, #27, #28 and #29 merge cleanly into main as it stands, but they conflict with each other, so the second one merged will need a rebase. Suggested order, and I will do each rebase as the one before it lands:

  1. security(resolver)!: refuse did:web/did:webvh resolution to non-public hosts #29 (resolver bump) — clean against the other three, so it can go any time.
  2. fix(store): write the handle snapshot and the key files owner-only #26 (snapshot and key-file permissions) — smallest of the remaining three.
  3. feat(store): expire, cap and rate-limit the anonymous registration path #28 (expiry, caps, rate limiting) — overlaps fix(store): write the handle snapshot and the key files owner-only #26 in Cargo.toml, README.md, src/main.rs and src/store.rs.
  4. feat(api): move metrics off the public listener; bound provision and errors #27 (metrics listener and bounds) — overlaps both in README.md, src/main.rs and tests/api.rs.

This PR is third in that order. Its store.rs and main.rs changes are the ones that need care against #26.

@stormer78

Copy link
Copy Markdown
Contributor Author

Rebased onto main (c0d6f91, post-#26). Four files conflicted — Cargo.toml, README.md, src/main.rs, src/store.rs — and every conflict was additive, so both sides are kept throughout. Worth calling out what was not in the conflict markers, because git merged it cleanly into something that would not have compiled:

Inside the markers

  • Cargo.toml / README.md / src/main.rsfix(store): write the handle snapshot and the key files owner-only #26's tempfile rationale, GATEWAY_STRICT_KEY_PERMS docs and secretfile import alongside this branch's governor/tower_governor, registry-bounds and rate-limit docs, and limits::Limits import.
  • src/store.rs module docs — this branch's "Why this file is more than a HashMap" section plus fix(store): write the handle snapshot and the key files owner-only #26's "The snapshot is a secret file" paragraph, which is still accurate.
  • Store::open was the one that mattered. fix(store): write the handle snapshot and the key files owner-only #26 put secretfile::tighten_to_owner_only(&path) at exactly the line this branch replaced with a delegation to the new open_with_limits. Taking either side alone would have silently dropped the permission-tightening. The call now lives at the top of open_with_limits, so both entry points tighten, and it still runs before the snapshot is read.
  • persist_locked is gone rather than merged: the debounced flusher replaces it and it had no callers left.

Outside the markers — three things git resolved "cleanly" into a broken tree:

  1. Two fn write_snapshot definitions. Both sides added one at different offsets. Kept fix(store): write the handle snapshot and the key files owner-only #26's, whose doc explains the predictable-temp-path symlink vulnerability it closes; dropped the terser duplicate.
  2. Two fn snapshot_is_owner_only tests. Kept the one that flushes, and folded the dropped one's "a rewrite stays 0600" assertion into it.
  3. Three tests calling insert() without handling its new Result-D warnings failure — two of which also asserted on snapshot contents before any flush, which the new debounce makes wrong. Fixed both problems: predictable_temp_path_symlink_is_not_followed now flushes before reading the snapshot off disk.

Verification (macOS arm64): cargo fmt --all --check clean, cargo clippy --all-targets -- -D warnings clean, cargo test 71 passed / 0 failed. The security tests that prove #26's properties survived this rebase all pass: open_tightens_a_world_readable_snapshot, predictable_temp_path_symlink_is_not_followed, snapshot_is_owner_only.

Next in the posted order: #27 rebases onto this once it merges.

push/register needs no credentials, so the handle registry was a map an
unauthenticated caller could grow without bound, and every mutation
reserialised the whole map under the write lock. Four changes, in order of how
much they matter.

Expiry is the root-cause fix. A handle is inert until its VTA provisions a
trigger, so HandleRecord gains created_at (#[serde(default)], so existing
snapshots load) and a tokio sweeper drops handles still unprovisioned after
GATEWAY_UNPROVISIONED_TTL_SECS, default 24h. Anonymous growth becomes bounded
churn. A provisioned handle is never swept, however old.

Caps: GATEWAY_MAX_HANDLES in total, and GATEWAY_MAX_HANDLES_PER_TOKEN live
handles per device token or Web Push endpoint, so one token cannot occupy the
registry. The per-token count is an index maintained under the same lock as the
map, not a scan, so the check stays O(1) under the flood it exists to stop; it
keys on a truncated SHA-256 rather than the token, to avoid a second copy of a
bearer credential in another map's keys.

Rate limits in two layers, because DIDComm bypasses HTTP middleware and is the
preferred transport. A tower_governor layer limits POST /trust-tasks per peer
IP, which needs into_make_service_with_connect_info; and the transport-agnostic
dispatch core limits register against a global budget and provision/wake
against a budget keyed by the authenticated DID. The keyed buckets are
reclaimed on a timer, since they are themselves keyed by caller-chosen input.

Persistence is debounced: mutations set a dirty flag and a background flusher
writes at most once per GATEWAY_SNAPSHOT_FLUSH_MS, keeping temp-file + fsync +
rename. Drop flushes, so a clean shutdown is durable.

Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
@stormer78
stormer78 force-pushed the sec-4045/register-quotas branch from cfd791f to 67cf60a Compare September 12, 2026 15:42
@stormer78

Copy link
Copy Markdown
Contributor Author

Rebased again — #27 merged while the previous rebase was in flight, so this is now on main @ 1714cf6. Three files conflicted: README.md, src/main.rs, tests/api.rs.

README.md and src/main.rs were additive. main.rs now runs, in order: the management/metrics listener (#27), the three maintenance timers, the per-IP governor, and a single let app carrying both GovernorLayer and TraceLayer. into_make_service_with_connect_info::<SocketAddr>() is still what serves it, which PeerIpKeyExtractor requires.

tests/api.rs needed more than a splice. Git interleaved four pairs of unrelated test functions. Rather than hand-merge those hunks, I rebuilt the file as main's version plus this branch's additions, then checked it structurally: 42 top-level items (main's 35 + this branch's 7), no duplicate names, nothing dropped from either side. The one genuine edit was state(), which this branch replaces with a state_with(store, limits) helper — AppState gained a limits field, so main's version of that helper no longer compiles.

One real semantic conflict, which the tests caught. #27 moved GET /metrics off the public router onto the management one. Two tests here — register_flood_is_refused_after_the_burst and eleventh_registration_is_refused_at_capacity — assert on gateway_register_total by scraping the router they just flooded, and so failed with a 404 against the public router. Both now build the public and management routers over one shared AppState, the way #27's own tests do: the public router takes the flood, the management router is scraped for what it recorded. The assertions are unchanged — they were right, they were just pointed at the wrong listener.

Verification (macOS arm64): cargo fmt --all --check clean, cargo clippy --all-targets -- -D warnings clean, cargo test 75 passed / 0 failed (55 unit + 20 integration).

@stormer78
stormer78 merged commit 01f4c72 into main Sep 12, 2026
3 checks passed
@stormer78
stormer78 deleted the sec-4045/register-quotas branch September 12, 2026 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant