Skip to content

security fix(gateway): defend pre-auth device-signature verify against CPU DoS#77492

Open
fede-kamel wants to merge 5 commits into
openclaw:mainfrom
fede-kamel:security/preauth-signature-rate-limit
Open

security fix(gateway): defend pre-auth device-signature verify against CPU DoS#77492
fede-kamel wants to merge 5 commits into
openclaw:mainfrom
fede-kamel:security/preauth-signature-rate-limit

Conversation

@fede-kamel

@fede-kamel fede-kamel commented May 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #77979

Summary

  • Pre-auth WS handshakes that include a device block ran crypto.createPublicKey plus a v3-then-v2 crypto.verify per request, gated only by frame-size limits. An unauthenticated remote attacker could pin a single Gateway core: PoC against a real running gateway hits ~510 forged handshakes/s/IP, degrading legit handshake p50 by 48.7× (1.16 ms → 56.6 ms) and p99 by 94.7×.
  • Fix is layered: schema cap on device.publicKey (1024) and device.signature (256); shape pre-check on verifyDeviceSignature / deriveDeviceIdFromPublicKey / normalizeDevicePublicKeyBase64Url rejecting inputs that cannot decode to a 32-byte raw key or 64-byte signature; new AUTH_RATE_LIMIT_SCOPE_DEVICE_SIGNATURE bucket gating the verify per IP. The browser-origin rate limiter is always constructed (server.impl.ts:424), so the gate fires for browser-origin clients even when no gateway.auth.rateLimit is configured. For non-browser-origin clients, the gate fires when the operator configures a rate limiter (the typical production case).
  • Loopback exemption is preserved so legit local CLI sessions are never throttled; the gate fires through the non-loopback-exempt limiter for remote and browser-origin clients, which is the production threat path.

Reproduction

The PoC (scripts/poc-preauth-flood-ws.mjs) opens N concurrent WS connections, each completing the connect-challenge handshake with a real Ed25519 PEM public key and a random 64-byte signature. The gateway reaches the verify path (server-issued nonce echoed back, device.id matches sha256(rawPublicKey)) and runs createPublicKey + v3 verify + v2 verify per request. A separate measurement client probes time-to-first-event in parallel.

Against a vulnerable gateway:

pnpm gateway:watch                                    # in another terminal

node scripts/poc-preauth-flood-ws.mjs \
     --gateway ws://127.0.0.1:18789 \
     --attackers 32 \
     --legit-iters 30 \
     --duration-ms 3000

Observed:

baseline:     n=30 p50=1.16ms  p90=1.80ms  p99=2.04ms
under-attack: n=30 p50=56.58ms p90=64.24ms p99=193.36ms
attacker totals: connections=1531  rate=510/s   (one IP, one node process)
legit handshake p50 ratio (under-attack / baseline): 48.69x
legit handshake p99 ratio: 94.73x

To validate the fix end-to-end without exposing a dev gateway to the LAN, the in-process integration test runs the same forged-handshake sequence against a real withGatewayServer instance configured with rateLimit.maxAttempts: 1, exemptLoopback: true:

pnpm test src/gateway/server.preauth-signature-rate-limit.test.ts

The test asserts:

  1. The first N forged signatures fail with error.details.reason === "device-signature" (the verify ran).
  2. The (N+1)th attempt fails with error.details.reason === "device-signature-rate-limited" and retryAfterMs > 0 — the gate short-circuited before any crypto.

Reverting the recordFailure line in the rate-limit gate makes both tests fail with expected 'device-signature' to be 'device-signature-rate-limited'.

Test plan

  • src/gateway/server.preauth-signature-rate-limit.test.ts — new in-process WS integration test (2 cases). Both fail when the fix is disabled.
  • src/infra/device-identity.test.ts — new shape pre-check tests: oversized PEM, oversized non-PEM, signature-shaped input rejected as public key, public-key-shaped input rejected as signature, all without invoking crypto.
  • src/gateway/protocol/connect-device-bounds.test.ts — new schema-cap tests: validateConnectParams rejects oversized device.publicKey and device.signature; accepts them at the documented bounds.
  • Existing src/gateway/server/ws-connection/handshake-auth-helpers.test.ts — 27/27 pass, no regression.
  • Live PoC reproduction confirms the 50× p50 / 95× p99 degradation pre-fix.

Verification

  • Targeted pnpm test of the four affected files: 41/41 pass.
  • Build: dist contains the new rate-limit gate (grep AUTH_RATE_LIMIT_SCOPE_DEVICE_SIGNATURE on the bundle).
  • Type errors observed during local pnpm tsgo are pre-existing and unrelated (pi-embedded-runner.* AgentMessage casts and missing web-tree-sitter dep).

Real behavior proof

  • Behavior addressed: pre-auth device blocks ran crypto.createPublicKey plus a v3-then-v2 crypto.verify per request with no per-IP cap. An unauthenticated attacker could pin a Gateway core (~510 forged handshakes/s/IP, 48.7× p50 / 94.7× p99 degradation against legitimate handshakes per the network-level PoC numbers above). The fix layers three defenses: ajv schema caps reject oversized device.publicKey and device.signature before any handler runs; isPlausibleDevicePublicKeyInput / isPlausibleDeviceSignatureInput shape pre-checks reject inputs that cannot decode to a 32-byte raw key or 64-byte signature without invoking crypto; and AUTH_RATE_LIMIT_SCOPE_DEVICE_SIGNATURE gates the verify per IP, so once the bucket is exhausted no createPublicKey or verify runs.
  • Real environment tested: local Node v22.22.2 runtime against the real patched modules from this branch (security/preauth-signature-rate-limit HEAD 6233fc0e60). No vitest, no mocks. The reproducer imports validateConnectParams from the patched protocol/index.ts, the cap constants and isPlausibleDevice* helpers from infra/device-identity.ts, and createAuthRateLimiter from gateway/auth-rate-limit.ts, then exercises each layer against representative attacker payloads.
  • Exact steps or command run after this patch: a node runtime tsx reproducer at https://gist.github.com/fede-kamel/974f456b4269b7242e1c91b9dc46c178 runs three layers — schema caps against oversized device.publicKey/device.signature (including a 60 KB representative attacker payload), shape pre-checks against malformed inputs that decode to wrong-length raw bytes, and the AUTH_RATE_LIMIT_SCOPE_DEVICE_SIGNATURE bucket fired against a single attacker IP. Run as node --import tsx <local-copy>.mts from a worktree on this branch.
  • Evidence after fix: live terminal output captured from the node runtime against the patched modules:
Pre-auth device-signature CPU-DoS proof (PR 77492)
Imports real patched validateConnectParams + isPlausibleDevice* + AuthRateLimiter
from security/preauth-signature-rate-limit worktree.

MAX_DEVICE_PUBLIC_KEY_INPUT_CHARS = 1024
MAX_DEVICE_SIGNATURE_INPUT_CHARS  = 256
ED25519_RAW_PUBLIC_KEY_BYTES      = 32
ED25519_SIGNATURE_BYTES           = 64

=== Layer 1: schema-level rejection of oversized device.publicKey / device.signature ===

case                                                          | result | took
--------------------------------------------------------------|--------|------
valid: device.publicKey real PEM (113 chars)                  | PASS   | 4.494ms
reject: device.publicKey at 1025 chars (cap+1)                | REJECT | 0.515ms
reject: device.publicKey at 60KB (representative attacker payload)| REJECT | 4.246ms
reject: device.signature at 257 chars (cap+1)                 | REJECT | 0.077ms

=== Layer 2: shape pre-check on device-identity helpers (no crypto) ===

case                                                          | result | took
--------------------------------------------------------------|--------|------
valid: real raw 32-byte base64url public key                  | PASS   | 0.486ms
valid: real PEM public key                                    | PASS   | 0.039ms
reject: random 100-byte base64                                | REJECT | 0.629ms
reject: oversized publicKey (cap+1)                           | REJECT | 0.053ms
reject: empty publicKey                                       | REJECT | 0.048ms
valid: real 64-byte base64url signature                       | PASS   | 0.101ms
reject: random 32-byte signature input                        | REJECT | 0.118ms
reject: oversized signature (cap+1)                           | REJECT | 0.035ms
reject: 60KB signature                                        | REJECT | 0.044ms

=== Layer 3: rate-limit gate AUTH_RATE_LIMIT_SCOPE_DEVICE_SIGNATURE ===

attempt | rateLimited | remaining/retryAfterMs | crypto entered?
--------|-------------|------------------------|------------------
    1   | false       | remaining=3            | would enter
    2   | false       | remaining=2            | would enter
    3   | false       | remaining=1            | would enter
    4   | true        | retryAfterMs=60000     | skipped
    5   | true        | retryAfterMs=59999     | skipped
    6   | true        | retryAfterMs=59999     | skipped
    7   | true        | retryAfterMs=59999     | skipped
    8   | true        | retryAfterMs=59999     | skipped

=== Layer 3b: cost comparison — skipped vs entered ===

crypto entered 5000 times: 214.3ms (avg 0.043ms/op)
gate-blocked  5000 times: 62.6ms (avg 0.013ms/op)
amplification factor under attack: 3.4x
  • Observed result after fix: every legitimate input validated PASS and every cap+1 / 60 KB / wrong-length / wrong-shape attacker input validated REJECT across both schema (Layer 1) and shape pre-check (Layer 2). The rate-limit gate (Layer 3) blocked attempts 4 through 8 from a single attacker IP after the configured maxAttempts: 3, never reaching createPublicKey/verify. Layer 3b measured the per-op cost of the full Ed25519 verify path (0.043 ms/op) versus the gate-blocked path (0.013 ms/op) — a 3.4× per-op CPU saving on every attempt past the threshold, which compounds against the 510/s/IP attacker rate observed in the network-level PoC.
  • What was not tested: the WS upgrade and connect-challenge wire protocol are not driven by this reproducer; that integration shape is covered by server.preauth-signature-rate-limit.test.ts (in-process WS server) and by the live scripts/poc-preauth-flood-ws.mjs PoC against a running pnpm gateway:watch. Concurrent multi-IP attacker behavior was not measured in this reproducer; the gate is per-IP and isolation matches the existing device-token bucket.

Notes for reviewer

  • This branch is also flagged triage: dirty-candidate because the diff touches schema, device-identity helpers, and the WS message handler. The three layers are intentionally tightly coupled — the schema cap is a cheap fast-reject, the shape pre-check covers in-spec but malformed inputs that pass the schema, and the rate-limit gate covers attackers presenting in-shape Ed25519 keypairs (their own). Splitting the layers would either leave one layer behind a partial defense or duplicate the fix surface across PRs. Happy to split if reviewers prefer one layer per PR.
  • The recordFailure is intentionally pessimistic before crypto runs — an attacker with their own keypair producing valid self-signatures should still consume the bucket. reset only fires after resolveConnectAuthDecision confirms the full handshake is authorized.

@fede-kamel
fede-kamel requested a review from a team as a code owner May 4, 2026 19:27
@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui gateway Gateway runtime scripts Repository scripts size: L triage: dirty-candidate Candidate: broad unrelated surfaces; may need splitting or cleanup. labels May 4, 2026
@clawsweeper

clawsweeper Bot commented May 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 6, 2026, 4:58 PM ET / 20:58 UTC.

Summary
The PR bounds device handshake fields, adds cheap Ed25519 public-key/signature shape checks, gates device-signature verification with a new auth rate-limit scope, and adds focused gateway/protocol tests plus a local WebSocket CPU-flood PoC script.

PR surface: Source +146, Tests +455, Other +349. Total +950 across 11 files.

Reproducibility: yes. Source inspection on current main shows device-bearing connects can reach device-id derivation and v3/v2 signature verification before any device-signature limiter; the PR also supplies terminal live output and focused WS integration tests, though I did not rerun the flood PoC.

Review metrics: 3 noteworthy metrics.

  • Protocol bounds: 2 connect device fields bounded. The PR changes public connect-frame validation for device.publicKey and device.signature before merge.
  • Auth limiter scope: 1 pre-auth scope added. The new device-signature bucket changes failed device-bearing handshake behavior independently of existing token buckets.
  • Security tooling: 1 local PoC script added. The script is useful validation tooling but is an intentional repository security artifact, not runtime code.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #77979
Summary: This PR is the open candidate fix for the canonical device-signature CPU-DoS issue; sibling items track adjacent bootstrap-token and auth-field schema hardening.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Have a gateway/security owner explicitly accept the field bounds, shared-IP lockout behavior, and PoC script placement before merge.

Risk before merge

  • [P1] The new protocol maxLength bounds intentionally reject device.publicKey values over 1024 chars and device.signature values over 256 chars, so owners should accept that as the intended connect-frame contract.
  • [P1] The new per-IP device-signature bucket can intentionally lock out device-bearing browser-origin handshakes from a shared IP after repeated failures, which is an availability tradeoff CI cannot settle.
  • [P1] The PR adds a local DoS PoC script under scripts/, which is useful validation tooling but should be an intentional repository security artifact.

Maintainer options:

  1. Accept security boundary and merge (recommended)
    If gateway/security owners agree the Ed25519-sized bounds and per-IP lockout are the right boundary, proceed with normal merge gates.
  2. Tune policy before merge
    Adjust the field caps, limiter keying, lockout behavior, or PoC placement if owners want a narrower compatibility or availability posture.
  3. Pause for broader auth design
    Hold or close the branch if maintainers prefer replacing this with a larger gateway-auth redesign rather than adding another limiter scope.

Next step before merge

  • [P2] The remaining action is gateway/security owner acceptance of the security boundary and availability tradeoff, with normal exact-head merge gates after that; there is no narrow ClawSweeper repair finding.

Maintainer decision needed

  • Question: Should OpenClaw accept the proposed device.publicKey/signature bounds and per-IP device-signature pre-auth lockout as the gateway security boundary for connect handshakes?
  • Rationale: The patch changes unauthenticated gateway behavior, protocol validation, and lockout behavior for failed device-bearing browser-origin handshakes; automation can verify mechanics but cannot own the security and availability policy choice.
  • Likely owner: steipete — He is the strongest history owner for the device-auth and gateway-auth path and merged the adjacent bootstrap-token DoS fix.
  • Options:
    • Accept layered hardening (recommended): Approve the schema bounds, shape pre-checks, and pre-crypto limiter as the intended mitigation, then merge after normal owner review and exact-head gates.
    • Tune before merge: Ask for adjusted bounds, limiter keying, lockout behavior, or PoC placement while preserving the early gate before createPublicKey and verify.
    • Pause for replacement design: Hold or close this PR if maintainers want a broader gateway-auth redesign instead of extending the current limiter model.

Security
Cleared: No concrete supply-chain, credential, or security-regression defect was found in the diff; the security-sensitive choices are captured as maintainer merge risks.

Review details

Best possible solution:

Land this layered mitigation after gateway/security owners accept the field bounds, per-IP lockout behavior, and PoC placement; otherwise tune those policy choices without losing the pre-crypto short-circuit.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection on current main shows device-bearing connects can reach device-id derivation and v3/v2 signature verification before any device-signature limiter; the PR also supplies terminal live output and focused WS integration tests, though I did not rerun the flood PoC.

Is this the best way to solve the issue?

Yes, mechanically this is the right layer: the gate runs before createPublicKey/verify, malformed inputs are bounded before crypto, and lockouts use the canonical auth rate-limit contract. The remaining choice is owner acceptance of the protocol bounds and availability tradeoff.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 6bffda416614.

Label changes

Label justifications:

  • P1: The PR addresses a remotely reachable pre-auth gateway CPU-amplification path that can degrade live gateway availability.
  • merge-risk: 🚨 compatibility: The PR adds new connect-frame maxLength validation for existing device.publicKey and device.signature fields.
  • merge-risk: 🚨 security-boundary: The PR changes where unauthenticated gateway handshakes are allowed to reach cryptographic verification and how rate-limited auth failures are surfaced.
  • merge-risk: 🚨 availability: The new per-IP device-signature bucket can intentionally lock out device-bearing handshakes from the same origin/IP after repeated failures.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR includes copied terminal live output from a real Node runtime importing patched modules and showing schema rejects, shape pre-check rejects, and rate-limit short-circuiting after the threshold; focused integration tests cover the WS wire path.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR includes copied terminal live output from a real Node runtime importing patched modules and showing schema rejects, shape pre-check rejects, and rate-limit short-circuiting after the threshold; focused integration tests cover the WS wire path.
Evidence reviewed

PR surface:

Source +146, Tests +455, Other +349. Total +950 across 11 files.

View PR surface stats
Area Files Added Removed Net
Source 6 155 9 +146
Tests 4 458 3 +455
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 1 349 0 +349
Total 11 962 12 +950

What I checked:

Likely related people:

  • steipete: Peter Steinberger authored the unified device-auth flow and extracted the WebSocket handshake auth helpers, and he merged the sibling bootstrap-token DoS fix. (role: feature-history owner and likely decision owner; confidence: high; commits: 73e9e787b4df, 01e4845f6dfa, ecbd97e9682d; files: src/gateway/server/ws-connection/message-handler.ts, src/gateway/server/ws-connection/handshake-auth-helpers.ts, src/infra/device-identity.ts)
  • buerbaumer: Harald Buerbaumer authored the gateway auth rate-limiting and brute-force protection surface that this PR extends with a device-signature scope. (role: auth limiter surface introducer; confidence: medium; commits: 30b6eccae5af; files: src/gateway/auth-rate-limit.ts, src/gateway/server.impl.ts)
  • vincentkoc: Vincent Koc authored and merged the earlier pre-auth WebSocket handshake-limit hardening PR, which is adjacent to this connect-frame security boundary. (role: adjacent pre-auth hardening reviewer and merger; confidence: medium; commits: eff0d5a947f5; files: src/gateway/server/ws-connection/message-handler.ts, packages/gateway-protocol/src/schema/frames.ts)
  • fede-kamel: Fede Kamel authored this branch and the merged sibling bootstrap-token pre-auth DoS mitigation, giving them recent context on this security cluster beyond this PR alone. (role: adjacent security-fix contributor; confidence: high; commits: ecbd97e9682d, 67a40dc14e65; files: src/gateway/auth-rate-limit.ts, src/gateway/server/ws-connection/auth-context.ts, src/gateway/server/ws-connection/message-handler.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (5 earlier review cycles)
  • reviewed 2026-07-03T21:03:05.328Z sha 1665085 :: needs real behavior proof before merge. :: [P1] Return pure signature lockouts as auth rate limits
  • reviewed 2026-07-06T03:20:42.215Z sha e9a9402 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-06T03:52:10.625Z sha 6640657 :: needs changes before merge. :: [P1] Return signature lockouts through auth rate limits
  • reviewed 2026-07-06T17:15:07.789Z sha f20a530 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-06T18:59:30.452Z sha 7f36d8a :: needs maintainer review before merge. :: none

@fede-kamel
fede-kamel force-pushed the security/preauth-signature-rate-limit branch 4 times, most recently from 3bf503e to 4a844cf Compare May 4, 2026 19:59
@fede-kamel

fede-kamel commented May 4, 2026 •
edited by clawsweeper Bot
Loading

Copy link
Copy Markdown
Contributor Author

Addressing the two Codex findings on 4a844cf (force-pushed):

1. Limiter now runs before any pre-auth crypto (message-handler.ts:754-781)

Hoisted the authRateLimiter.check(..., AUTH_RATE_LIMIT_SCOPE_DEVICE_SIGNATURE) gate above deriveDeviceIdFromPublicKey(). A locked-out IP no longer pays the crypto.createPublicKey() cost via PEM input — the gate fires first regardless of public-key encoding.

2. Bucket reset deferred until after resolveConnectAuthDecision (message-handler.ts:884-886)

Two changes:

  • After the limiter check passes, recordFailure is now called pessimistically before crypto runs. Every device-bearing handshake that crosses the gate consumes the bucket up-front.
  • reset() only fires after resolveConnectAuthDecision returns authOk === true. The intermediate recordFailure on payloadVersion === null was removed since the pessimistic call already counts that attempt.

Net behavior: an attacker with their own keypair producing a valid self-signature still consumes the bucket because resolveConnectAuthDecision rejects them as unauthorized — the reset never fires, the pessimistic recordFailure stays on the books.

Coverage added in server.preauth-signature-rate-limit.test.ts:

Test results:

pnpm test src/gateway/server.preauth-signature-rate-limit.test.ts \
          src/infra/device-identity.test.ts \
          src/gateway/protocol/connect-device-bounds.test.ts \
          src/gateway/server/ws-connection/handshake-auth-helpers.test.ts \
          src/gateway/server.auth.browser-hardening.test.ts \
          src/gateway/server/ws-connection/auth-context.test.ts
# 6 files, 64 tests, all pass

Two pre-existing assertions in server.auth.browser-hardening.test.ts were updated to accept either "retry later" (shared-secret bucket) or "rate-limited" (device-signature bucket) since both are valid loopback-browser-origin lockout signals — with the gate moved earlier and the pessimistic recordFailure, the device-signature bucket now wins the race in those tests at maxAttempts: 1.

Re-review progress:

@fede-kamel
fede-kamel force-pushed the security/preauth-signature-rate-limit branch from 4a844cf to 49334e7 Compare May 4, 2026 20:07
@fede-kamel fede-kamel changed the title fix(gateway): defend pre-auth device-signature verify against CPU DoS security fix(gateway): defend pre-auth device-signature verify against CPU DoS May 4, 2026
@fede-kamel

fede-kamel commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

@steipete when you have a moment — security fix for pre-auth device-signature CPU DoS, CI green. Will rebase to clear the conflicts before merge. Thanks!

@fede-kamel
fede-kamel force-pushed the security/preauth-signature-rate-limit branch from 4b6b34a to 3166348 Compare May 5, 2026 16:30
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 5, 2026
@fede-kamel

fede-kamel commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

cc @openclaw/openclaw-secops for visibility — rebased onto main, mergeable. Just routing through the right team. No rush. Thanks!

This was referenced May 5, 2026
Merged
Closed
Open
Open
@clawsweeper clawsweeper Bot mentioned this pull request May 5, 2026
Closed
@fede-kamel
fede-kamel force-pushed the security/preauth-signature-rate-limit branch from 3166348 to 6233fc0 Compare May 6, 2026 22:11
@fede-kamel

fede-kamel commented May 6, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current origin/main (was 372 commits behind). The previous "Real behavior proof" CI failure was a missing-script artifact — scripts/github/real-behavior-proof-check.mjs was added in #77622 after this branch was first pushed, so the workflow could not load on the old base. The script is on HEAD now and CI is re-running.

@openclaw/openclaw-secops — flagging for review (pre-auth device-signature CPU-DoS hardening).

@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 9, 2026
@fede-kamel fede-kamel mentioned this pull request May 9, 2026
Closed
@fede-kamel
fede-kamel force-pushed the security/preauth-signature-rate-limit branch from 1665085 to e9a9402 Compare July 6, 2026 03:04
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 6, 2026
@fede-kamel
fix(gateway): keep bootstrap/device-token rate-limit reason at the pr…
6640657 
…e-auth signature gate

The pre-auth device-signature rate-limit gate records a failure for every
device-carrying handshake before crypto. Forged bootstrap-token attempts
carry a valid self-signed device identity, so they tripped the signature
gate and were rejected as `device-signature-rate-limited`, shadowing the
existing bootstrap-token rate-limit contract whose reason is `rate_limited`.

When a bootstrap- or device-token candidate is present, surface that
credential's `rate_limited` reason at the gate instead of the signature
scope reason. The crypto bound is unchanged: lockout still returns before
any createPublicKey/verify work, so the device-signature scope still guards
pure signature-only handshakes.
@fede-kamel
fede-kamel force-pushed the security/preauth-signature-rate-limit branch from e9a9402 to 6640657 Compare July 6, 2026 03:27
@github-actions github-actions Bot mentioned this pull request Jul 6, 2026
Open
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 6, 2026
@fede-kamel

fede-kamel commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Review finding addressed in f20a530 — pure device-signature lockouts now route through the canonical auth rate_limited contract.

What changed: the signature-gate lockout branch in src/gateway/server/ws-connection/message-handler.ts no longer emits the ad-hoc device-auth-invalid / device-signature-rate-limited response. Every lockout at that gate rejects via rejectUnauthorized({ reason: "rate_limited", rateLimited: true }), which surfaces the exact contract bootstrap/device-token lockouts already use: code: AUTH_RATE_LIMITED, authReason: rate_limited, recommendedNextStep: wait_then_retry, the "too many failed authentication attempts (retry later)" message, and a high-severity gateway.auth.failed security event. The pre-crypto short-circuit is unchanged — lockout still returns before any createPublicKey/verify work.

Bonus simplification: the June 14 token-candidate special case (bootstrapTokenCandidate || deviceTokenCandidate) existed only to avoid the ad-hoc response shadowing the token rate-limit contract; with one canonical path both branches merged, deleting the conditional entirely (net −19 lines).

Note on retryAfterMs: the old ad-hoc details included it, but the canonical ConnectAuthErrorDetails contract deliberately conveys retry guidance via recommendedNextStep: "wait_then_retry" and carries no retryAfterMs field (bootstrap-token lockouts do not surface it either). Since this PR introduced the signature-lockout surface and nothing shipped, aligning to the canonical shape drops it with no compat impact; adding retryAfterMs to the shared contract would be an additive protocol follow-up if owners want it.

Tests updated to assert the canonical contract: server.preauth-signature-rate-limit.test.ts now asserts AUTH_RATE_LIMITED / authReason=rate_limited / wait_then_retry on lockout responses across the forged-signature, PEM-encoded, shared-bucket, and valid-self-signature scenarios, and server.auth.browser-hardening.test.ts drops the now-dead "rate-limited" message alternative.

Verification: node scripts/run-vitest.mjs src/gateway/server.preauth-signature-rate-limit.test.ts src/gateway/server.auth.browser-hardening.test.ts → 4 files, 36/36 passed; oxfmt --check clean on touched files.

@fede-kamel
fix(gateway): route device-signature lockouts through the canonical r…
7f36d8a 
…ate_limited contract

Pure device-signature lockouts previously returned an ad-hoc
device-auth-invalid response with a signature-scope reason, hiding
rate-limit state from clients, telemetry severity, and retry guidance.
All signature-gate lockouts now reject through rejectUnauthorized with
reason rate_limited, surfacing AUTH_RATE_LIMITED, authReason
rate_limited, wait_then_retry guidance, and the high-severity security
event — the same contract as bootstrap/device-token lockouts. This also
deletes the token-candidate special case since both branches now share
one path. The pre-crypto short-circuit is unchanged: lockout still
returns before any createPublicKey/verify work.

Focused tests assert AUTH_RATE_LIMITED/authReason=rate_limited on the
lockout responses across forged-signature, PEM, shared-bucket, and
valid-self-signature scenarios.
@fede-kamel

fede-kamel commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Head 7f36d8ab57 is fully green (the earlier check-test-types failure was an unrelated extensions/discord flake — zero discord diff on this branch, the identical lane passed on sibling PRs the same day, and a local run of the exact lane at this head was clean; an amend+repush cleared it). The July 5 recommended repair is implemented: pure device-signature lockouts now surface the canonical AUTH_RATE_LIMITED / authReason=rate_limited / wait_then_retry contract, with focused assertions across all four lockout scenarios.

@openclaw/openclaw-secops — this implements the review's recommended option 1 (canonical lockout contract, pre-crypto short-circuit preserved). The remaining acceptance items the review routes to security owners: the device.publicKey/signature protocol bounds (sized for every valid Ed25519 form with margin) and the per-IP pre-auth limiter's availability tradeoff for browser-origin clients behind shared IPs.

@clawsweeper

clawsweeper Bot commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@openclaw-barnacle

openclaw-barnacle Bot commented Jul 21, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@OfficialAbhinavSingh OfficialAbhinavSingh mentioned this pull request Jul 21, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

app: web-ui App: web-ui gateway Gateway runtime merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. scripts Repository scripts size: L stale Marked as stale due to inactivity status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. triage: dirty-candidate Candidate: broad unrelated surfaces; may need splitting or cleanup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: pre-auth device-signature verify allows CPU-amplification DoS without rate limit

2 participants

@fede-kamel @jesse-merhi