fix(auth): keep rate-limit windows across reloads, and give id-less keys a stable id - #256
Merged
Merged
Conversation
…ments Two auth defects surfaced while classifying the 96 mutants that keep serving/api/auth/manager.py two points above its mutation threshold: - load() carries in-memory rate-limit windows over by plaintext key, but since audit S-6 every window is named kid:/kh:, so every reload -- SIGHUP or the one that ends each key create, rotate and revoke -- empties them. - AGENTFLOW_API_KEYS entries get a random key_id on every load, so their Redis bucket, usage rows and admin views change per reload, restart and replica. The requirements land first, on their own, so the fixes that follow are reviewed against a statement that predates them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS
7cd4a37 added docs/specs/ with two capability files and no inbound link, and the docs-orphan gate (tests/unit/test_docs_orphans.py) failed the full unit run on them. The security section of architecture.md now points its API authentication and rate-limiting bullets at their requirements, and the docs index names specs/ as the home of required behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS
…d too The key-identity requirement covered only AGENTFLOW_API_KEYS. A key-file entry without a key_id gets a random id from ensure_key_ids on every load, and load() writes it back only when the file is writable. The shipped production compose mounts ./config read-only and config/api_keys.yaml carries no key_id, so both of its keys draw a new id -- and with it a new rate-limit bucket -- on every load, restart and replica. The requirement now covers every key without a persisted id, derives the id from the stored key_lookup when there is one, and leaves a legacy hash-only entry random. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS
…indows AuthManager.load() carried the in-memory rate-limit windows over by the plaintext key index (keys_by_value). Since audit S-6 every window is named by its bucket (kid:<key_id>), so no window ever matched: SIGHUP, and the reload that ends each key create, rotate and revoke, emptied them all. is_rate_limited() and the in-memory secondary check in check_rate_limit() then handed every tenant a fresh budget -- during a Redis outage, the whole limit. load() now keeps the windows whose bucket belongs to a key this reload still configures, and drops the rest. The new tests take each scenario of docs/specs/api-key-rate-limiting.md against both readers of the window; four of the five failed before the fix. Environment keys still draw a random key_id on every load, so their windows keep resetting; the next commit derives that id from the key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS
test_an_aggregate_sums_only_the_readers_rows compared the revenue metric with round(amount, 2). But orders_v2.total_amount is DECIMAL(10,2): DuckDB casts a double into it half away from zero, while Python's round() rounds the exact binary value half to even. Hypothesis drew amount=8.125. The store held 8.13, round() said 8.12, and the 0.01 tolerance missed by float noise. Once the example database saved that case, the failure replayed on every run and turned a full local gate red. The test now asserts that the metric is the reader's own amount to the cent (abs 0.006). That holds under either rounding mode and stays far below the 1.0 minimum a leaked row would add. amount=8.125 is pinned with @example. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS
…y load A key whose configuration carries no key_id -- every key from AGENTFLOW_API_KEYS, and a key-file entry without one -- drew a random id from generate_key_id on every load. A key-file entry kept its id only when load() could write it back, and docker-compose.prod.yml mounts the key file read-only, over a config/api_keys.yaml whose two entries carry no key_id. The id names the key's Redis bucket, its api_usage rows and the admin views keyed by id: replicas sharing Redis each kept their own bucket for the same key (N replicas, N times its rpm), and every restart and reload started a fresh bucket and split its usage history. ensure_key_ids now derives the id -- <tenant>-<name>-<8 hex of the entry's peppered lookup digest>, from its stored key_lookup or compute_key_lookup over its plaintext -- and _legacy_env_keys runs environment keys through it. A clash lengthens the digest prefix, so ids stay unique and stable. A new pepper changes only an id derived from plaintext and never written back -- every environment key, and a plaintext entry of a key file the process cannot write; an entry with a stored key_lookup, and a derived id already written to a writable key file, keep theirs. A legacy hash-only entry has nothing to derive from and keeps a random id; generate_key_id and create_key are unchanged. tests/unit/test_auth_key_identity.py takes each scenario of docs/specs/api-key-identity.md (nine of its twelve tests fail against the code before this change), with false-reject controls that load the shipped key file and a persisted id beside a hash-only entry. The new key_rotation helpers are killed from test_key_rotation_mutation.py, the file the mutation gate runs; its one test that pinned a random id for entries with a plaintext key now builds hash-only entries, the case that keeps one. The CHANGELOG's ten LF-only lines are now CRLF like the rest of the file. docs/architecture.md and the reload requirement of docs/specs/api-key-rate-limiting.md now name the one key a reload still cannot keep: a legacy hash-only entry in a key file the process cannot write, whose random id -- and so its bucket and window -- changes on every load. docs/specs/api-key-identity.md already specified that id. Reviewed by opus-cli, the writer's own engine: glm-5.3 was rate-limited until 2026-09-14 and codex until 2026-10-05. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS
DORA Metrics
|
brownjuly2003-code
deleted the
fix/auth-rate-windows-and-key-identity
branch
September 11, 2026 01:40
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
2 participants
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
This PR fixes two auth defects. Both turned up while classifying the mutants that survive in
manager.py. Each fix comes with a capability spec and one test per scenario.63022aa).AuthManager.load()carried the in-memory windows over by plaintext key. Since audit S-6, though, windows are named by bucket (kid:<key_id>), so none matched.key_iddrew a random id on every load (6231b73).AGENTFLOW_API_KEYSkey, and every key-file entry without an id. That includes both entries of the shippedconfig/api_keys.yaml, whichdocker-compose.prod.ymlmounts read-only, so the id could never be written back.api_usagerows. N replicas sharing Redis gave one key N buckets, so N × its rpm, and every restart started a fresh bucket.<tenant>-<name>-<8 hex of its key-lookup digest>. The digest comes from the storedkey_lookup, or else from the pepperedcompute_key_lookup. It is never taken from the plaintext.1582ed1). This is a separate, test-only commit.test_an_aggregate_sums_only_the_readers_rowsexpected Python's half-evenround(amount, 2). Buttotal_amountisDECIMAL(10,2), which DuckDB fills half away from zero.amount=8.125: 8.13 stored, 8.12 expected. Once that example was saved, every local full gate failed.@example.The specs are
docs/specs/api-key-rate-limiting.mdanddocs/specs/api-key-identity.md. Both are linked fromdocs/architecture.mdanddocs/README.md.Test plan
tests/unit/test_auth_rate_window_reload.py: 5 tests, one per rate-limiting scenario.tests/unit/test_auth_key_identity.py: 12 tests. 9 of them fail against63022aa, before the fix; all 12 pass here.gate --fullwas GREEN before each commit: lint, format, mypy, contracts and the full unit suite.Review note
glm-5.3 reviewed the rate-limit fix. opus-cli reviewed the key-id fix; it is the same engine as the writer, because glm-5.3 was rate-limited until 2026-09-14 and codex until 2026-10-05. That review took five passes. The first three found only wording defects:
key_lookupkeeps its id under a new pepper, and so does a writable key file, which keeps the id written to it on the first load. Both fixed, each with a test.docs/architecture.mdand in the reload requirement of the rate-limit spec.MyFlow stopped the loop after the third pass as not converging, because each pass found a new wording edge. The fix for the last edge was applied as a recorded closing decision, and pass 4 confirmed it. Pass 5 reviewed the property-test fix and found no defect. Maximum severity fell from 3 to 1 across the five passes.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS