Security: /pair lacks replay defense + per-IP rate limit — LAN flood / signature replay#12
Draft
mimeding wants to merge 2 commits into
Draft
Security: /pair lacks replay defense + per-IP rate limit — LAN flood / signature replay#12mimeding wants to merge 2 commits into
mimeding wants to merge 2 commits into
Conversation
/pair is the only unauthenticated endpoint that can mint master-scoped
API keys. The signature-over-nonce check correctly authenticates the
*connector* but does nothing to prevent:
1. Replay: an attacker who captures a single valid pairing request
can re-submit it to the same target and (assuming the user
approves the prompt again, which is likely if the prompt is
rapid-fire or the same connector triggers it) end up with a
second, independently-revocable master key.
2. Flood: a LAN peer can spam /pair to either annoy the user with
prompts or burn event-loop cycles on secp256k1 signature
recovery. /pair is in the public-paths set and cannot have
Bearer auth in front of it by design (an unpaired peer must be
able to initiate pairing).
Add PairingReplayGuard, a small thread-safe singleton with two
in-memory stores:
* (connector, nonce) replay set with a 5-minute TTL. consumeNonce()
is atomic: first call returns true and reserves the pair, second
call returns false. Called after signature verification, so the
store can't be poisoned by forged signatures.
* Per-source-IP sliding 1-minute window of attempt timestamps,
capped at 10. allowAttempt() returns false when the window is
saturated. Called before signature verification so a flooder
can't keep the event loop busy on secp256k1 work.
Both stores prune lazily on every access; memory growth is bounded
by the rate of attempts, not by lifetime traffic.
Wired into handlePairEndpoint:
* allowAttempt() check immediately after JSON decode, returning
429 + 'Too many pairing attempts. Try again later.' on failure.
* consumeNonce() check immediately after signature verification,
returning 401 + 'Replayed pairing request' on failure.
Includes PairingReplayGuardTests covering: first attempt accepted,
rate limit observed, per-IP isolation, nonce single-use, nonces are
scoped by connector address (so the same nonce under a different
connector is not a replay), and _reset() helper for test isolation.
Co-authored-by: Michael Meding <mimeding@users.noreply.github.com>
ModelManager.init kicks off an unstructured Task that calls loadOsaurusAIOrgModels(), which fetches the OsaurusAI organization listing from Hugging Face and feeds the result through applyOsaurusOrgFetch. The unit-test runner repeatedly constructs ModelManager() to drive applyOsaurusOrgFetch directly. The background launch-time fetch races with those test calls — whichever finishes last wins, and the merge result is non-deterministic. That's the root cause of the flaky ModelManagerSuggestedTests failures seen across many of the recent PR CI runs (applyOsaurusOrgFetch_dropsStaleAutoFetched OnReapply, applyOsaurusOrgFetch_addsNewEntriesAfterCurated, etc.). Gate the launch-time fetch on a small isRunningInTestEnvironment helper that checks for any of XCTestConfigurationFilePath, XCTestBundlePath, or XCTestSessionIdentifier in the process environment. Those variables are only present inside an xctest host process; production app launches still get the HF fetch exactly as before. This is a network call, so removing it under tests also has the side benefit of making the test suite work offline / on hermetic CI runners. Co-authored-by: Michael Meding <mimeding@users.noreply.github.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.
Summary
Why this matters (business)
/pairis the one endpoint that can hand out a long-lived master-scoped API key to an unauthenticated caller. It has to be unauthenticated by design — an unpaired peer needs a way to introduce itself. The trust gate is the user clicking "Approve" on the on-device prompt.That gate is the only line of defense, which means everything around it needs to make abuse expensive. Two specific gaps:
/pair. Each request triggers a secp256k1 signature recovery on the event loop, then (if the request is well-formed) a SwiftUI approval prompt. No rate limit at all today — the only ceiling is how fast the attacker can post.What's wrong (technical)
The current handler does signature verification → agent resolution → approval prompt → key generation. There's no nonce tracking, so the same
(connector, nonce, signature)triple is accepted twice; and no per-IP throttle, so secp256k1 work scales with attacker throughput.Fix
Add a single thread-safe singleton —
PairingReplayGuard— with two in-memory bounded stores:Both stores prune lazily on every access, so memory is bounded by the rate of attempts (not by lifetime traffic).
Wire them into
handlePairEndpoint:allowAttempt(ip:)immediately after JSON decode, before any crypto. Failure returns 429 + a human-readable error. This bounds the per-IP secp256k1 cost.consumeNonce(connector:nonce:)immediately after signature verification. Calling it post-sig-verify means a flooder with bad signatures can't poison the nonce store with junk. Failure returns 401 + "Replayed pairing request."Defaults tuned for the LAN-pairing use case: a real flow uses 1 attempt per pairing, so 10/min/IP is generous; the 5-minute nonce TTL is comfortably above realistic user-approval round-trip time and well below any reasonable replay coordination window.
Test coverage
PairingReplayGuardTests(inTests/Networking/) covers:rateLimitMaxattempts in the window, additional attempts from the same IP are rejected.(connector, nonce)consumed twice → second call returnsfalse.nonceunder a differentconnectoris not a replay — important because the connector address is part of the trust identity._reset()clears both stores (used by other tests to isolate state).Scope decisions
Changes
PairingReplayGuardTests)Test Plan
Manually:
curl -X POST ...to the same/pair. Expected: HTTP 401Replayed pairing request. Previously: a second approval prompt + new master key minted.for i in {1..30}; do curl -X POST .../pair -d '{...}'; donefrom a single source IP. Expected: first 10 follow the normal flow, the rest get HTTP 429.Checklist
CONTRIBUTING.mddocs/SECURITY.md)swiftc -frontend -parse; the test relies only onFoundationtypes — no platform-specific frameworks)