Skip to content

Commit 233eeb6

Browse files
Merge pull request #256 from brownjuly2003-code/fix/auth-rate-windows-and-key-identity
fix(auth): keep rate-limit windows across reloads, and give id-less keys a stable id
2 parents 127be28 + 6231b73 commit 233eeb6

11 files changed

Lines changed: 762 additions & 23 deletions

‎CHANGELOG.md‎

Lines changed: 48 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,44 @@ All notable changes to AgentFlow are documented in this file.
44

55
## [Unreleased]
66

7+
### Fixed — a key configured without a key_id keeps the same id on every load
8+
9+
A key whose configuration carries no `key_id` — every key from
10+
`AGENTFLOW_API_KEYS`, and a key-file entry without one — drew a random id on
11+
every `AuthManager.load()`. A key-file entry kept its id only when the id could
12+
be written back, and `docker-compose.prod.yml` mounts the key file read-only.
13+
The id names the key's Redis bucket (`kid:<key_id>`), its `api_usage` rows and
14+
the admin views keyed by id, so replicas sharing Redis each kept their own
15+
bucket for the same key — N replicas, N times its rpm — and every restart and
16+
reload started a fresh bucket and split the key's usage history. Such a key now
17+
gets `<tenant>-<name>-<first 8 hex of its key-lookup digest>`: the stored
18+
`key_lookup`, else the peppered `compute_key_lookup` of its plaintext, never
19+
the plaintext or an unpeppered hash of it. The same key gets the same id on
20+
every load, restart and replica that shares the pepper. Changing the pepper
21+
changes only an id derived from a plaintext key and never written back: every
22+
environment key, and a plaintext key-file entry in a file the process cannot
23+
write. A writable key file keeps the id written to it on the first load, and an
24+
entry with a stored `key_lookup` keeps the id derived from that digest. An id
25+
already taken lengthens the digest prefix. A legacy hash-only entry, with
26+
neither a plaintext key nor a `key_lookup`, still gets a random id.
27+
28+
### Fixed — reloading the key store no longer resets rate-limit windows
29+
30+
`AuthManager.load()` carried the in-memory rate-limit windows over by the
31+
plaintext key index (`keys_by_value`), but since audit S-6 every window is
32+
named by its bucket (`kid:<key_id>`), so no window ever matched: every reload
33+
— SIGHUP, and the reload that ends every key create, rotate and revoke —
34+
emptied them all. `is_rate_limited()` and the in-memory secondary check in
35+
`check_rate_limit()` then handed every tenant a fresh budget, which during a
36+
Redis outage is the whole limit. Windows are now carried over by bucket name:
37+
a still-configured key keeps its window, a removed key loses it, and no
38+
plaintext key names a window at any point of the reload. A window survives
39+
only while its key keeps its `key_id`, which a key configured without one —
40+
from `AGENTFLOW_API_KEYS`, or in a key file the process cannot write — now does
41+
too (entry above), with one exception: a legacy hash-only entry, with neither a
42+
plaintext key nor a `key_lookup`, in a key file the process cannot write still
43+
gets a new id, and so an emptied window, on every reload.
44+
745
### Docs — the pre-push hedges outlived the push
846

947
* **Several notes described work the owner "still has to do" that has since
@@ -591,16 +629,16 @@ applies `--ignore` only for `safety_id` values whose waiver scope matches that
591629
bucket. Expired waivers stop suppressing findings. A waiver whose scope is not
592630
scanned, or a duplicate `safety_id`, fails closed.
593631

594-
### Documentation — root records archived (2026-09-02–2026-09-04)
595-
596-
Twenty-one immutable tracked Markdown records moved from the repository root
597-
to `docs/evidence/records/` with unchanged filenames and SHA-256 digests.
598-
Living citations now use those paths. After all eight documentation-cleanup
599-
items closed, `plan_26_08_2026.md` moved to `docs/archive/plans/` with its
600-
closure evidence intact. The retired local 2026-04-17 benchmark baseline moved
601-
from the standalone `docs/benchmark-baseline-archive/` directory into the
602-
performance archive.
603-
632+
### Documentation — root records archived (2026-09-02–2026-09-04)
633+
634+
Twenty-one immutable tracked Markdown records moved from the repository root
635+
to `docs/evidence/records/` with unchanged filenames and SHA-256 digests.
636+
Living citations now use those paths. After all eight documentation-cleanup
637+
items closed, `plan_26_08_2026.md` moved to `docs/archive/plans/` with its
638+
closure evidence intact. The retired local 2026-04-17 benchmark baseline moved
639+
from the standalone `docs/benchmark-baseline-archive/` directory into the
640+
performance archive.
641+
604642
### Security — nltk 3.10.0 -> 3.10.3 in uv.lock (Dependabot GHSA-m4rf-3fr8-xwx3, GHSA-6hwm-xvph-95vm)
605643

606644
- `uv lock --upgrade-package nltk` only; nltk is a transitive dependency of `llama-index-core` and is not part of the `cloud`/`postgres` export, so `requirements-docker.lock` is unchanged. Closes the critical (JVM argument injection in the Stanford wrappers) and high (uncontrolled `dot` search path) advisories GitHub reported on the default branch on 2026-09-01.

‎docs/README.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@ the former mutable pre-Q1.2 report is preserved as the
154154
| Current engineering gates | [Engineering status](STATUS.md) | Dated acceptance/evidence records |
155155
| Lifecycle and non-goals | [Project closure](PROJECT_CLOSURE.md) | Audit and planning records |
156156
| Runtime design | [Architecture reference](architecture.md) | [Walkthrough](architecture/index.md) and ADRs |
157+
| Required behaviour | One file per capability in `specs/`: [API key rate limiting](specs/api-key-rate-limiting.md), [API key identity](specs/api-key-identity.md) | The tests that exercise each scenario |
157158
| API contract | [`openapi.json`](openapi.json) and running FastAPI schema | API guide/reference and SDKs |
158159
| Security policy | [`SECURITY.md`](../SECURITY.md) | Security audit and dated remediation evidence |
159160
| Release history | [Changelog](../CHANGELOG.md) | [Archived narrative](archive/release-history-v1-v2.md) |
@@ -166,7 +167,9 @@ the former mutable pre-Q1.2 report is preserved as the
166167
`scripts/check_docs_root_placement.py`; update it only for an intentional
167168
stable entrypoint or current reference.
168169
- Put immutable measurements in `perf/` or `evidence/`, operational procedures
169-
in `operations/` or `runbooks/`, and decisions in `decisions/`.
170+
in `operations/` or `runbooks/`, decisions in `decisions/`, and the required
171+
behaviour of a capability (requirements with scenarios) in `specs/`, one
172+
file per capability.
170173
- Do not delete documentation. Move superseded or duplicate narrative to
171174
`archive/` with its original path, archive date, reason, and replacement.
172175
- Update every inbound link in the same commit as a move. Use `git mv` so file

‎docs/architecture.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,9 +134,9 @@ See [Architecture Decision Records](decisions/) for detailed trade-off analysis.
134134
## Security
135135

136136
### Implemented
137-
- **API authentication**: API key via `X-API-Key` header (set `AGENTFLOW_API_KEYS` env var)
137+
- **API authentication**: API key via `X-API-Key` header (set `AGENTFLOW_API_KEYS` env var). A key configured without a `key_id` (every environment key, and a key-file entry without one) gets an id derived from its key-lookup digest, stable across reloads, restarts and replicas (except a legacy hash-only entry, with neither a plaintext key nor a `key_lookup`: it has nothing to derive from and keeps a random id, which in a key file the process cannot write changes on every load); changing the pepper changes only an id derived from a plaintext key and never written back (every environment key, and a plaintext key-file entry in a file the process cannot write), while a writable key file keeps the id written to it on the first load and an entry with a stored `key_lookup` keeps the id derived from that digest. Requirements for a key's id: [API key identity](specs/api-key-identity.md)
138138
- **Transport gate (audit P2-3)**: `AGENTFLOW_PROFILE=production` refuses to boot over plaintext transport to an external ClickHouse/Redis/PostgreSQL (loopback exempt; deliberate exceptions named in `AGENTFLOW_INSECURE_TRANSPORT_OK`), and refuses a wildcard CORS origin outside demo mode. The ClickHouse client supports HTTPS with hostname verification and a private-CA bundle (`CLICKHOUSE_SECURE`, `CLICKHOUSE_CA_CERT`)
139-
- **Rate limiting**: Per-key sliding window with Redis backing when available and in-memory fallback for local/test, configurable via `AGENTFLOW_RATE_LIMIT_RPM` (default: 120/min)
139+
- **Rate limiting**: Per-key sliding window with Redis backing when available and in-memory fallback for local/test, configurable via `AGENTFLOW_RATE_LIMIT_RPM` (default: 120/min). Requirements: [API key rate limiting](specs/api-key-rate-limiting.md)
140140
- **Health/docs exempt**: `/v1/health`, `/docs`, `/metrics` don't require auth
141141
- **No secrets in code**: All credentials via environment variables
142142
- **Terraform state**: Encrypted S3 backend with DynamoDB locking

‎docs/specs/api-key-identity.md‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
# Capability: api-key-identity
2+
3+
A key's `key_id` names its rate-limit bucket (`kid:<key_id>`), its usage rows
4+
(`api_usage.key_id`) and the admin views addressed by id. An id that changes
5+
while the key stays the same splits all three.
6+
7+
## Requirement: a key without a persisted id has a stable id
8+
A key whose configuration carries no `key_id` SHALL get a `key_id` derived from
9+
the key's identity, so the same key has the same id on every load, every
10+
restart and every replica that runs with the same key-lookup pepper. This
11+
covers every key from `AGENTFLOW_API_KEYS`, and every key-file entry without a
12+
`key_id` — including one in a key file the process cannot write the id back
13+
to, as when the file is mounted read-only.
14+
15+
### Scenario: same environment key, two managers
16+
- **GIVEN** `AGENTFLOW_API_KEYS="k1:Support Agent"` and a fixed key-lookup pepper
17+
- **WHEN** two separate `AuthManager` instances load their keys
18+
- **THEN** both give the key the same `key_id`, of the form `default-support-agent-<8 lowercase hex>`
19+
20+
### Scenario: a reload keeps the id and the bucket
21+
- **GIVEN** a loaded environment-configured key
22+
- **WHEN** the manager reloads
23+
- **THEN** the key's `key_id` is unchanged, and so is its rate-limit bucket
24+
25+
### Scenario: different keys under one name get different ids
26+
- **GIVEN** `AGENTFLOW_API_KEYS="k1:bot,k2:bot"`
27+
- **WHEN** the manager loads its keys
28+
- **THEN** the two keys have different `key_id` values
29+
30+
### Scenario: an id-less entry in a read-only key file
31+
- **GIVEN** a key file the process cannot write, holding an entry that has a `key_lookup` and no `key_id`
32+
- **WHEN** two separate `AuthManager` instances load it, and one of them reloads
33+
- **THEN** the entry has the same `key_id` in both managers and after the reload
34+
35+
### Scenario: a writable key file persists the derived id
36+
- **GIVEN** a writable key file holding an entry that has a plaintext `key` and no `key_id`
37+
- **WHEN** the manager loads it
38+
- **THEN** the file on disk now carries the `key_id` the manager uses, and it is the id derived from that key
39+
40+
## Requirement: the id never exposes the key
41+
The derived id SHALL come from the peppered key-lookup digest — the entry's
42+
stored `key_lookup`, or `compute_key_lookup` over its plaintext key — never
43+
from the plaintext key or an unpeppered hash of it: the id is written to logs,
44+
Redis key names, usage rows and admin responses, and must not let a guessed
45+
key be confirmed offline. An entry that has neither a plaintext key nor a
46+
`key_lookup` (a legacy hash-only entry) has nothing to derive from and keeps a
47+
random id.
48+
49+
### Scenario: the suffix is the lookup digest's prefix
50+
- **GIVEN** an environment key `k1` and key-lookup pepper `P`
51+
- **WHEN** the manager loads it
52+
- **THEN** the `key_id` suffix equals the first 8 characters of `compute_key_lookup("k1", P)`
53+
54+
### Scenario: a stored key_lookup is used as it is
55+
- **GIVEN** a key-file entry with `key_lookup: L` and no `key_id`
56+
- **WHEN** the manager loads it
57+
- **THEN** the `key_id` suffix equals the first 8 characters of `L`
58+
59+
### Scenario: a different pepper gives a different id
60+
- **GIVEN** the same environment key and two different key-lookup peppers
61+
- **WHEN** one manager loads it under each pepper
62+
- **THEN** the two `key_id` values differ
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
# Capability: api-key-rate-limiting
2+
3+
Per-key request budgets enforced by `AuthManager`
4+
(`src/agentflow_runtime/serving/api/auth/manager.py`). The budget shared across
5+
replicas lives in Redis behind `RateLimiter`. The manager also keeps an
6+
in-memory window per bucket: it is the whole of `is_rate_limited()`, and it is
7+
the secondary check in `check_rate_limit()` when the Redis limiter answers
8+
"allowed, full quota" while a Redis handle is live.
9+
10+
## Requirement: a bucket is named by non-secret key identity
11+
The rate-limit bucket of a key SHALL be named `kid:<key_id>` when the key has an
12+
id, and no bucket name SHALL contain a plaintext API key.
13+
14+
### Scenario: key with an id
15+
- **GIVEN** a key whose `key_id` is `acme-support-1a2b3c4d`
16+
- **WHEN** a request authenticated by that key is rate-limited
17+
- **THEN** its bucket is `kid:acme-support-1a2b3c4d`
18+
19+
## Requirement: reloading the key store keeps live windows
20+
Reloading the key configuration — `load()`, a SIGHUP reload, or the reload that
21+
ends every key create, rotate and revoke — SHALL keep the in-memory rate-limit
22+
window of every key that is still configured, and SHALL drop the window of a
23+
key the reload removed. A key whose id changes on every load, a legacy
24+
hash-only entry in a key file the process cannot write (see
25+
[API key identity](api-key-identity.md)), gets a new bucket on every reload, so
26+
its window starts empty.
27+
28+
### Scenario: a full window survives a reload
29+
- **GIVEN** a key with `rate_limit_rpm: 1` that has already made its one request in the current window
30+
- **WHEN** the key store is reloaded and the same key makes another request inside that window
31+
- **THEN** that request is rate-limited
32+
33+
### Scenario: the secondary window survives a reload
34+
- **GIVEN** a Redis limiter that answers "allowed, full quota" with a live handle, and a key with `rate_limit_rpm: 1` that has made its one request
35+
- **WHEN** the key store is reloaded and `check_rate_limit()` is called for the same key inside the window
36+
- **THEN** the answer is not allowed, with zero remaining
37+
38+
### Scenario: a removed key's window is dropped
39+
- **GIVEN** two configured keys that both made a request in the current window
40+
- **WHEN** one of them is removed from the key file and the store is reloaded
41+
- **THEN** the in-memory windows hold the remaining key's bucket and not the removed key's
42+
43+
### Scenario: no plaintext key becomes a window name
44+
- **GIVEN** a plaintext key configured in the key file, which has made a request
45+
- **WHEN** the store is reloaded
46+
- **THEN** no in-memory window is named by that plaintext key, at any point of the reload

‎src/agentflow_runtime/serving/api/auth/key_rotation.py‎

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import re
55
import secrets
66
import threading
7+
from collections.abc import Collection
78
from datetime import UTC, datetime, timedelta
89

910
try:
@@ -22,6 +23,42 @@
2223
is_permission_denied,
2324
)
2425

26+
# A derived key_id ends in this many characters of the key's lookup digest: the
27+
# same shape as the 8 hex characters `generate_key_id` draws at random.
28+
KEY_ID_DIGEST_CHARS = 8
29+
30+
31+
def key_id_slug(value: str, fallback: str) -> str:
32+
return "-".join(re.findall(r"[a-z0-9]+", value.lower())) or fallback
33+
34+
35+
def derived_key_id(item: TenantKey, existing_ids: Collection[str]) -> str | None:
36+
"""A stable key_id for an entry configured without one, or None.
37+
38+
The suffix is a prefix of the entry's peppered lookup digest -- its stored
39+
`key_lookup`, else `compute_key_lookup` over its plaintext key -- so the
40+
same key gets the same id on every load, restart and replica that shares
41+
the pepper. Never the plaintext or an unpeppered hash of it: the id lands in
42+
logs, Redis key names, usage rows and admin responses, and must not let a
43+
guessed key be confirmed offline (audit FB-07).
44+
45+
An id already taken lengthens the prefix, so the later entry of a clashing
46+
pair still gets the same id on every load. None when there is nothing to
47+
derive from (a legacy hash-only entry) or the whole digest is taken.
48+
"""
49+
if item.key_lookup is not None:
50+
lookup = item.key_lookup
51+
elif item.key is not None:
52+
lookup = compute_key_lookup(item.key)
53+
else:
54+
return None
55+
stem = f"{key_id_slug(item.tenant, 'tenant')}-{key_id_slug(item.name, 'agent')}-"
56+
for size in range(KEY_ID_DIGEST_CHARS, len(lookup) + 1):
57+
candidate = stem + lookup[:size]
58+
if candidate not in existing_ids:
59+
return candidate
60+
return None
61+
2562

2663
class KeyRotator:
2764
def __init__(self, manager: AuthManager) -> None:
@@ -274,8 +311,8 @@ def generate_key_id(
274311
name: str,
275312
existing_ids: set[str] | None = None,
276313
) -> str:
277-
tenant_slug = re.sub(r"[^a-z0-9]+", "-", tenant.lower()).strip("-") or "tenant"
278-
name_slug = re.sub(r"[^a-z0-9]+", "-", name.lower()).strip("-") or "agent"
314+
tenant_slug = key_id_slug(tenant, "tenant")
315+
name_slug = key_id_slug(name, "agent")
279316
seen_ids = set(existing_ids or ())
280317
while True:
281318
candidate = f"{tenant_slug}-{name_slug}-{secrets.token_hex(4)}"
@@ -288,7 +325,13 @@ def ensure_key_ids(self, config: ApiKeysConfig) -> bool:
288325
for index, item in enumerate(config.keys):
289326
if item.key_id is not None:
290327
continue
291-
key_id = self.generate_key_id(item.tenant, item.name, existing_ids)
328+
# Derived, not drawn: an id that never reaches the file (a read-only
329+
# key store, or AGENTFLOW_API_KEYS) must come out the same on the next
330+
# load and in every replica. Only a legacy hash-only entry, with
331+
# nothing to derive from, still gets a random one.
332+
key_id = derived_key_id(item, existing_ids) or self.generate_key_id(
333+
item.tenant, item.name, existing_ids
334+
)
292335
existing_ids.add(key_id)
293336
config.keys[index] = item.model_copy(update={"key_id": key_id})
294337
changed = True

‎src/agentflow_runtime/serving/api/auth/manager.py‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -328,9 +328,17 @@ def load(self) -> None:
328328
if item.previous_key_lookup is not None:
329329
self._previous_keys_by_lookup[item.previous_key_lookup] = item
330330
self._key_rotator.schedule_rotation_cleanup(item)
331+
# Carry windows over by bucket name (`_rate_limit_key`), never by the
332+
# plaintext `keys_by_value` index: a still-configured key keeps its
333+
# window across the reload, a removed key's window is dropped.
334+
live_buckets = {self._rate_limit_key(item) for item in config.keys}
331335
self._rate_windows = defaultdict(
332336
list,
333-
{key: self._rate_windows.get(key, []) for key in self.keys_by_value},
337+
{
338+
bucket: window
339+
for bucket, window in self._rate_windows.items()
340+
if bucket in live_buckets
341+
},
334342
)
335343
# H-C4: drop cached plaintext entries for hashes that no longer
336344
# exist after this reload (revoked/rotated keys). Without this the
@@ -696,7 +704,6 @@ def _legacy_env_keys(self) -> list[TenantKey]:
696704
key, name = pair, "unnamed"
697705
items.append(
698706
TenantKey(
699-
key_id=self._key_rotator.generate_key_id("default", name.strip(), set()),
700707
key=key.strip(),
701708
name=name.strip(),
702709
tenant="default",
@@ -705,7 +712,12 @@ def _legacy_env_keys(self) -> list[TenantKey]:
705712
created_at=datetime.now(UTC).date(),
706713
)
707714
)
708-
return items
715+
# The same derivation as an id-less key-file entry: from the key's
716+
# lookup digest, so the key keeps its id -- and its rate-limit bucket --
717+
# across reloads, restarts and replicas.
718+
config = ApiKeysConfig(keys=items)
719+
self._key_rotator.ensure_key_ids(config)
720+
return config.keys
709721

710722
def _rate_limit_key(self, tenant_key: TenantKey) -> str:
711723
# This string becomes a Redis sorted-set key NAME (``rate_limiter`` calls

0 commit comments

Comments
 (0)