From 7cd4a37acb7437dd98b2bef179d8ce0e2389388e Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 03:32:09 -0400 Subject: [PATCH 1/6] docs(specs): state the rate-limit window and env key-identity requirements 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) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- docs/specs/api-key-identity.md | 41 +++++++++++++++++++++++++++ docs/specs/api-key-rate-limiting.md | 43 +++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 docs/specs/api-key-identity.md create mode 100644 docs/specs/api-key-rate-limiting.md diff --git a/docs/specs/api-key-identity.md b/docs/specs/api-key-identity.md new file mode 100644 index 00000000..c2261a09 --- /dev/null +++ b/docs/specs/api-key-identity.md @@ -0,0 +1,41 @@ +# Capability: api-key-identity + +A key's `key_id` names its rate-limit bucket (`kid:`), its usage rows +(`api_usage.key_id`) and the admin views addressed by id. An id that changes +while the key stays the same splits all three. + +## Requirement: an environment-configured key has a stable id +A key configured through `AGENTFLOW_API_KEYS` SHALL get a `key_id` derived from +the key itself, so the same key has the same id on every load, every restart and +every replica that runs with the same key-lookup pepper. + +### Scenario: same key, two managers +- **GIVEN** `AGENTFLOW_API_KEYS="k1:Support Agent"` and a fixed key-lookup pepper +- **WHEN** two separate `AuthManager` instances load their keys +- **THEN** both give the key the same `key_id`, of the form `default-support-agent-<8 lowercase hex>` + +### Scenario: a reload keeps the id and the bucket +- **GIVEN** a loaded environment-configured key +- **WHEN** the manager reloads +- **THEN** the key's `key_id` is unchanged, and so is its rate-limit bucket + +### Scenario: different keys under one name get different ids +- **GIVEN** `AGENTFLOW_API_KEYS="k1:bot,k2:bot"` +- **WHEN** the manager loads its keys +- **THEN** the two keys have different `key_id` values + +## Requirement: the id never exposes the key +The derived id SHALL come from the peppered key-lookup digest +(`compute_key_lookup`), never from the plaintext key or an unpeppered hash of +it: the id is written to logs, Redis key names, usage rows and admin responses, +and must not let a guessed key be confirmed offline. + +### Scenario: the suffix is the lookup digest's prefix +- **GIVEN** an environment key `k1` and key-lookup pepper `P` +- **WHEN** the manager loads it +- **THEN** the `key_id` suffix equals the first 8 characters of `compute_key_lookup("k1", P)` + +### Scenario: a different pepper gives a different id +- **GIVEN** the same environment key and two different key-lookup peppers +- **WHEN** one manager loads it under each pepper +- **THEN** the two `key_id` values differ diff --git a/docs/specs/api-key-rate-limiting.md b/docs/specs/api-key-rate-limiting.md new file mode 100644 index 00000000..1c766da8 --- /dev/null +++ b/docs/specs/api-key-rate-limiting.md @@ -0,0 +1,43 @@ +# Capability: api-key-rate-limiting + +Per-key request budgets enforced by `AuthManager` +(`src/agentflow_runtime/serving/api/auth/manager.py`). The budget shared across +replicas lives in Redis behind `RateLimiter`. The manager also keeps an +in-memory window per bucket: it is the whole of `is_rate_limited()`, and it is +the secondary check in `check_rate_limit()` when the Redis limiter answers +"allowed, full quota" while a Redis handle is live. + +## Requirement: a bucket is named by non-secret key identity +The rate-limit bucket of a key SHALL be named `kid:` when the key has an +id, and no bucket name SHALL contain a plaintext API key. + +### Scenario: key with an id +- **GIVEN** a key whose `key_id` is `acme-support-1a2b3c4d` +- **WHEN** a request authenticated by that key is rate-limited +- **THEN** its bucket is `kid:acme-support-1a2b3c4d` + +## Requirement: reloading the key store keeps live windows +Reloading the key configuration — `load()`, a SIGHUP reload, or the reload that +ends every key create, rotate and revoke — SHALL keep the in-memory rate-limit +window of every key that is still configured, and SHALL drop the window of a +key the reload removed. + +### Scenario: a full window survives a reload +- **GIVEN** a key with `rate_limit_rpm: 1` that has already made its one request in the current window +- **WHEN** the key store is reloaded and the same key makes another request inside that window +- **THEN** that request is rate-limited + +### Scenario: the secondary window survives a reload +- **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 +- **WHEN** the key store is reloaded and `check_rate_limit()` is called for the same key inside the window +- **THEN** the answer is not allowed, with zero remaining + +### Scenario: a removed key's window is dropped +- **GIVEN** two configured keys that both made a request in the current window +- **WHEN** one of them is removed from the key file and the store is reloaded +- **THEN** the in-memory windows hold the remaining key's bucket and not the removed key's + +### Scenario: no plaintext key becomes a window name +- **GIVEN** a plaintext key configured in the key file, which has made a request +- **WHEN** the store is reloaded +- **THEN** no in-memory window is named by that plaintext key, at any point of the reload From 418958930f8a8bb037b2c06244129d307ddb979d Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 04:09:27 -0400 Subject: [PATCH 2/6] docs: link the capability specs from the pages that own their subject 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) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- docs/README.md | 5 ++++- docs/architecture.md | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/README.md b/docs/README.md index 0b2e05ad..ac3c966f 100644 --- a/docs/README.md +++ b/docs/README.md @@ -154,6 +154,7 @@ the former mutable pre-Q1.2 report is preserved as the | Current engineering gates | [Engineering status](STATUS.md) | Dated acceptance/evidence records | | Lifecycle and non-goals | [Project closure](PROJECT_CLOSURE.md) | Audit and planning records | | Runtime design | [Architecture reference](architecture.md) | [Walkthrough](architecture/index.md) and ADRs | +| 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 | | API contract | [`openapi.json`](openapi.json) and running FastAPI schema | API guide/reference and SDKs | | Security policy | [`SECURITY.md`](../SECURITY.md) | Security audit and dated remediation evidence | | 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 `scripts/check_docs_root_placement.py`; update it only for an intentional stable entrypoint or current reference. - Put immutable measurements in `perf/` or `evidence/`, operational procedures - in `operations/` or `runbooks/`, and decisions in `decisions/`. + in `operations/` or `runbooks/`, decisions in `decisions/`, and the required + behaviour of a capability (requirements with scenarios) in `specs/`, one + file per capability. - Do not delete documentation. Move superseded or duplicate narrative to `archive/` with its original path, archive date, reason, and replacement. - Update every inbound link in the same commit as a move. Use `git mv` so file diff --git a/docs/architecture.md b/docs/architecture.md index cd666709..e08390a9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -134,9 +134,9 @@ See [Architecture Decision Records](decisions/) for detailed trade-off analysis. ## Security ### Implemented -- **API authentication**: API key via `X-API-Key` header (set `AGENTFLOW_API_KEYS` env var) +- **API authentication**: API key via `X-API-Key` header (set `AGENTFLOW_API_KEYS` env var). Requirements for a key's id: [API key identity](specs/api-key-identity.md) - **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`) -- **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) +- **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) - **Health/docs exempt**: `/v1/health`, `/docs`, `/metrics` don't require auth - **No secrets in code**: All credentials via environment variables - **Terraform state**: Encrypted S3 backend with DynamoDB locking From a681cb5dcd08683ef7a6c631d76f85716781b612 Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 04:09:27 -0400 Subject: [PATCH 3/6] docs(specs): a key-file entry without a persisted id needs a stable id 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) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- docs/specs/api-key-identity.md | 41 +++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/docs/specs/api-key-identity.md b/docs/specs/api-key-identity.md index c2261a09..f8679f08 100644 --- a/docs/specs/api-key-identity.md +++ b/docs/specs/api-key-identity.md @@ -4,12 +4,15 @@ A key's `key_id` names its rate-limit bucket (`kid:`), its usage rows (`api_usage.key_id`) and the admin views addressed by id. An id that changes while the key stays the same splits all three. -## Requirement: an environment-configured key has a stable id -A key configured through `AGENTFLOW_API_KEYS` SHALL get a `key_id` derived from -the key itself, so the same key has the same id on every load, every restart and -every replica that runs with the same key-lookup pepper. - -### Scenario: same key, two managers +## Requirement: a key without a persisted id has a stable id +A key whose configuration carries no `key_id` SHALL get a `key_id` derived from +the key's identity, so the same key has the same id on every load, every +restart and every replica that runs with the same key-lookup pepper. This +covers every key from `AGENTFLOW_API_KEYS`, and every key-file entry without a +`key_id` — including one in a key file the process cannot write the id back +to, as when the file is mounted read-only. + +### Scenario: same environment key, two managers - **GIVEN** `AGENTFLOW_API_KEYS="k1:Support Agent"` and a fixed key-lookup pepper - **WHEN** two separate `AuthManager` instances load their keys - **THEN** both give the key the same `key_id`, of the form `default-support-agent-<8 lowercase hex>` @@ -24,17 +27,35 @@ every replica that runs with the same key-lookup pepper. - **WHEN** the manager loads its keys - **THEN** the two keys have different `key_id` values +### Scenario: an id-less entry in a read-only key file +- **GIVEN** a key file the process cannot write, holding an entry that has a `key_lookup` and no `key_id` +- **WHEN** two separate `AuthManager` instances load it, and one of them reloads +- **THEN** the entry has the same `key_id` in both managers and after the reload + +### Scenario: a writable key file persists the derived id +- **GIVEN** a writable key file holding an entry that has a plaintext `key` and no `key_id` +- **WHEN** the manager loads it +- **THEN** the file on disk now carries the `key_id` the manager uses, and it is the id derived from that key + ## Requirement: the id never exposes the key -The derived id SHALL come from the peppered key-lookup digest -(`compute_key_lookup`), never from the plaintext key or an unpeppered hash of -it: the id is written to logs, Redis key names, usage rows and admin responses, -and must not let a guessed key be confirmed offline. +The derived id SHALL come from the peppered key-lookup digest — the entry's +stored `key_lookup`, or `compute_key_lookup` over its plaintext key — never +from the plaintext key or an unpeppered hash of it: the id is written to logs, +Redis key names, usage rows and admin responses, and must not let a guessed +key be confirmed offline. An entry that has neither a plaintext key nor a +`key_lookup` (a legacy hash-only entry) has nothing to derive from and keeps a +random id. ### Scenario: the suffix is the lookup digest's prefix - **GIVEN** an environment key `k1` and key-lookup pepper `P` - **WHEN** the manager loads it - **THEN** the `key_id` suffix equals the first 8 characters of `compute_key_lookup("k1", P)` +### Scenario: a stored key_lookup is used as it is +- **GIVEN** a key-file entry with `key_lookup: L` and no `key_id` +- **WHEN** the manager loads it +- **THEN** the `key_id` suffix equals the first 8 characters of `L` + ### Scenario: a different pepper gives a different id - **GIVEN** the same environment key and two different key-lookup peppers - **WHEN** one manager loads it under each pepper From 63022aad1da36c3e558cc378e47c9e2c3c0a8e9d Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 04:17:41 -0400 Subject: [PATCH 4/6] fix(auth): stop every key-store reload from emptying the rate-limit windows 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:), 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) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- CHANGELOG.md | 14 ++ .../serving/api/auth/manager.py | 10 +- tests/unit/test_auth_rate_window_reload.py | 150 ++++++++++++++++++ 3 files changed, 173 insertions(+), 1 deletion(-) create mode 100644 tests/unit/test_auth_rate_window_reload.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 03010cec..07974bce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,20 @@ All notable changes to AgentFlow are documented in this file. ## [Unreleased] +### Fixed — reloading the key store no longer resets rate-limit windows (T-46) + +`AuthManager.load()` carried the in-memory rate-limit windows over by the +plaintext key index (`keys_by_value`), but since audit S-6 every window is +named by its bucket (`kid:`), so no window ever matched: every reload +— SIGHUP, and the reload that ends every 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, which during a +Redis outage is the whole limit. Windows are now carried over by bucket name: +a still-configured key keeps its window, a removed key loses it, and no +plaintext key names a window at any point of the reload. Keys from +`AGENTFLOW_API_KEYS` still get a new random `key_id` on every load, so their +windows keep resetting until T-47. + ### Docs — the pre-push hedges outlived the push * **Several notes described work the owner "still has to do" that has since diff --git a/src/agentflow_runtime/serving/api/auth/manager.py b/src/agentflow_runtime/serving/api/auth/manager.py index 49ed09fc..e14c4c7f 100644 --- a/src/agentflow_runtime/serving/api/auth/manager.py +++ b/src/agentflow_runtime/serving/api/auth/manager.py @@ -328,9 +328,17 @@ def load(self) -> None: if item.previous_key_lookup is not None: self._previous_keys_by_lookup[item.previous_key_lookup] = item self._key_rotator.schedule_rotation_cleanup(item) + # Carry windows over by bucket name (`_rate_limit_key`), never by the + # plaintext `keys_by_value` index: a still-configured key keeps its + # window across the reload, a removed key's window is dropped. (T-46) + live_buckets = {self._rate_limit_key(item) for item in config.keys} self._rate_windows = defaultdict( list, - {key: self._rate_windows.get(key, []) for key in self.keys_by_value}, + { + bucket: window + for bucket, window in self._rate_windows.items() + if bucket in live_buckets + }, ) # H-C4: drop cached plaintext entries for hashes that no longer # exist after this reload (revoked/rotated keys). Without this the diff --git a/tests/unit/test_auth_rate_window_reload.py b/tests/unit/test_auth_rate_window_reload.py new file mode 100644 index 00000000..5bfe294e --- /dev/null +++ b/tests/unit/test_auth_rate_window_reload.py @@ -0,0 +1,150 @@ +"""Reloading the key store keeps the in-memory rate-limit windows +(docs/specs/api-key-rate-limiting.md, T-46). + +``AuthManager.load()`` runs on SIGHUP and at the end of every key create, +rotate and revoke. It used to rebuild ``_rate_windows`` from ``keys_by_value`` +-- the PLAINTEXT key index -- while every window is named by +``_rate_limit_key()`` (``kid:``), so each reload emptied every window +and handed every tenant a fresh budget. One test per spec scenario, against +both readers of the window: ``is_rate_limited()`` and the secondary check in +``check_rate_limit()``. + +Keys live in a tmp key file with an explicit ``key_id``: environment keys get +a random key_id on every load (until T-47), so they cannot pin a bucket here. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from agentflow_runtime.serving.api.auth.manager import AuthManager + +ALPHA_PLAIN = "plain-secret-alpha" +BETA_PLAIN = "plain-secret-beta" +ALPHA_ID = "acme-support-1a2b3c4d" +BETA_ID = "acme-billing-5e6f7a8b" + + +class _FrozenClock: + def __init__(self, now: float = 1_000.0) -> None: + self.now = now + + def __call__(self) -> float: + return self.now + + +class _FullRemainingLimiter: + """Redis limiter that answers "allowed, full quota" while its `_redis` + handle is live -- the condition for `check_rate_limit`'s in-memory + secondary window.""" + + def __init__(self) -> None: + self._redis = object() + + async def check(self, key: str, rpm: int) -> tuple[bool, int, int]: + return True, rpm, 0 + + +def _entry(key_id: str, plain: str, name: str) -> str: + return ( + f" - key_id: {key_id}\n" + f" key: {plain}\n" + f" name: {name}\n" + " tenant: acme\n" + " rate_limit_rpm: 1\n" + " created_at: '2026-04-10'\n" + ) + + +def _write_keys(path: Path, *entries: str) -> None: + path.write_text("keys:\n" + "".join(entries), encoding="utf-8") + + +def _manager(tmp_path: Path, *entries: str, **overrides: object) -> AuthManager: + api_keys_path = tmp_path / "api_keys.yaml" + _write_keys(api_keys_path, *entries) + params: dict[str, object] = { + "api_keys_path": api_keys_path, + "db_path": tmp_path / "usage.duckdb", + "time_source": _FrozenClock(), + } + params.update(overrides) + manager = AuthManager(**params) # type: ignore[arg-type] + manager.load() + return manager + + +def test_a_key_with_an_id_is_rate_limited_in_its_kid_bucket(tmp_path: Path) -> None: + manager = _manager(tmp_path, _entry(ALPHA_ID, ALPHA_PLAIN, "Support")) + + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is False + + assert set(manager._rate_windows) == {f"kid:{ALPHA_ID}"} + + +def test_a_full_window_survives_a_reload(tmp_path: Path) -> None: + manager = _manager(tmp_path, _entry(ALPHA_ID, ALPHA_PLAIN, "Support")) + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is False + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is True + + manager.load() + + # Same frozen instant: still inside the window the first request opened. + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is True + + +@pytest.mark.asyncio +async def test_the_secondary_window_survives_a_reload(tmp_path: Path) -> None: + manager = _manager( + tmp_path, + _entry(ALPHA_ID, ALPHA_PLAIN, "Support"), + rate_limiter=_FullRemainingLimiter(), + ) + allowed, remaining, _ = await manager.check_rate_limit(manager.keys_by_value[ALPHA_PLAIN]) + assert (allowed, remaining) == (True, 0) + + manager.load() + + allowed, remaining, _ = await manager.check_rate_limit(manager.keys_by_value[ALPHA_PLAIN]) + assert (allowed, remaining) == (False, 0) + + +def test_a_removed_keys_window_is_dropped(tmp_path: Path) -> None: + alpha = _entry(ALPHA_ID, ALPHA_PLAIN, "Support") + beta = _entry(BETA_ID, BETA_PLAIN, "Billing") + manager = _manager(tmp_path, alpha, beta) + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is False + assert manager.is_rate_limited(manager.keys_by_value[BETA_PLAIN]) is False + + _write_keys(tmp_path / "api_keys.yaml", alpha) + manager.load() + + assert f"kid:{ALPHA_ID}" in manager._rate_windows + assert f"kid:{BETA_ID}" not in manager._rate_windows + + +def test_no_plaintext_key_becomes_a_window_name( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + manager = _manager(tmp_path, _entry(ALPHA_ID, ALPHA_PLAIN, "Support")) + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is False + + # load() rebuilds the windows and then sweeps them; the sweep is the one + # point inside the reload where the rebuilt dict is observable before it + # is trimmed, so snapshot the names there as well as after the reload. + names_seen: list[set[str]] = [] + sweep = manager._sweep_expired_windows + + def recording_sweep() -> None: + names_seen.append(set(manager._rate_windows)) + sweep() + + monkeypatch.setattr(manager, "_sweep_expired_windows", recording_sweep) + manager.load() + names_seen.append(set(manager._rate_windows)) + + assert len(names_seen) == 2 + for names in names_seen: + assert not any(ALPHA_PLAIN in name for name in names), names From 1582ed1f60ba2bd7a504d41112c466ce5b6163fb Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 06:09:52 -0400 Subject: [PATCH 5/6] test(property): expect the reader's cents, not Python's rounding of them 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) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- tests/property/test_tenant_isolation_properties.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/property/test_tenant_isolation_properties.py b/tests/property/test_tenant_isolation_properties.py index 41d9286d..251d214c 100644 --- a/tests/property/test_tenant_isolation_properties.py +++ b/tests/property/test_tenant_isolation_properties.py @@ -23,7 +23,7 @@ from collections.abc import Iterator import pytest -from hypothesis import assume, given, settings +from hypothesis import assume, example, given, settings from hypothesis import strategies as st from agentflow_runtime.serving.semantic_layer.catalog import DataCatalog @@ -170,6 +170,7 @@ def test_the_tenant_column_never_reaches_the_payload( @settings(max_examples=40) @given(tenant=_TENANT_IDS, order_id=_ENTITY_IDS, amount=st.floats(1.0, 10_000.0, width=32)) +@example(tenant="0", order_id="ORD-0", amount=8.125) def test_an_aggregate_sums_only_the_readers_rows( engine: QueryEngine, tenant: str, order_id: str, amount: float ) -> None: @@ -184,7 +185,9 @@ def test_an_aggregate_sums_only_the_readers_rows( metric = engine.get_metric("revenue", window="24h", tenant_id=scoped_tenant) - assert metric["value"] == pytest.approx(round(amount, 2), abs=0.01) + # total_amount is DECIMAL(10,2), which DuckDB fills half away from zero while Python's round() + # is half-even on the binary value: the reader's own amount to the cent, a leaked row adds >= 1.0. + assert metric["value"] == pytest.approx(amount, abs=0.006) @given(hostile=_HOSTILE_TENANT_IDS) From 6231b731f778e8296256e72b052e8a2512cbec89 Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 06:09:52 -0400 Subject: [PATCH 6/6] fix(auth): give a key configured without a key_id the same id on every 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 -- --<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) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- CHANGELOG.md | 52 +++- docs/architecture.md | 2 +- docs/specs/api-key-rate-limiting.md | 5 +- .../serving/api/auth/key_rotation.py | 49 ++- .../serving/api/auth/manager.py | 10 +- tests/unit/test_auth_key_identity.py | 280 ++++++++++++++++++ tests/unit/test_key_rotation_mutation.py | 106 ++++++- 7 files changed, 480 insertions(+), 24 deletions(-) create mode 100644 tests/unit/test_auth_key_identity.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 07974bce..6ec2b553 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,28 @@ All notable changes to AgentFlow are documented in this file. ## [Unreleased] -### Fixed — reloading the key store no longer resets rate-limit windows (T-46) +### Fixed — a key configured without a key_id keeps the same id on every 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 on +every `AuthManager.load()`. A key-file entry kept its id only when the id could +be written back, and `docker-compose.prod.yml` mounts the key file read-only. +The id names the key's Redis bucket (`kid:`), its `api_usage` rows and +the admin views keyed by id, so 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 the key's usage history. Such a key now +gets `--`: the stored +`key_lookup`, else the peppered `compute_key_lookup` of its plaintext, never +the plaintext or an unpeppered hash of it. The same key gets the same id on +every load, restart and replica that shares the pepper. 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. 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. An id +already taken lengthens the digest prefix. A legacy hash-only entry, with +neither a plaintext key nor a `key_lookup`, still gets a random id. + +### Fixed — reloading the key store no longer resets rate-limit windows `AuthManager.load()` carried the in-memory rate-limit windows over by the plaintext key index (`keys_by_value`), but since audit S-6 every window is @@ -14,9 +35,12 @@ emptied them all. `is_rate_limited()` and the in-memory secondary check in `check_rate_limit()` then handed every tenant a fresh budget, which during a Redis outage is the whole limit. Windows are now carried over by bucket name: a still-configured key keeps its window, a removed key loses it, and no -plaintext key names a window at any point of the reload. Keys from -`AGENTFLOW_API_KEYS` still get a new random `key_id` on every load, so their -windows keep resetting until T-47. +plaintext key names a window at any point of the reload. A window survives +only while its key keeps its `key_id`, which a key configured without one — +from `AGENTFLOW_API_KEYS`, or in a key file the process cannot write — now does +too (entry above), with one exception: a legacy hash-only entry, with neither a +plaintext key nor a `key_lookup`, in a key file the process cannot write still +gets a new id, and so an emptied window, on every reload. ### Docs — the pre-push hedges outlived the push @@ -605,16 +629,16 @@ applies `--ignore` only for `safety_id` values whose waiver scope matches that bucket. Expired waivers stop suppressing findings. A waiver whose scope is not scanned, or a duplicate `safety_id`, fails closed. -### Documentation — root records archived (2026-09-02–2026-09-04) - -Twenty-one immutable tracked Markdown records moved from the repository root -to `docs/evidence/records/` with unchanged filenames and SHA-256 digests. -Living citations now use those paths. After all eight documentation-cleanup -items closed, `plan_26_08_2026.md` moved to `docs/archive/plans/` with its -closure evidence intact. The retired local 2026-04-17 benchmark baseline moved -from the standalone `docs/benchmark-baseline-archive/` directory into the -performance archive. - +### Documentation — root records archived (2026-09-02–2026-09-04) + +Twenty-one immutable tracked Markdown records moved from the repository root +to `docs/evidence/records/` with unchanged filenames and SHA-256 digests. +Living citations now use those paths. After all eight documentation-cleanup +items closed, `plan_26_08_2026.md` moved to `docs/archive/plans/` with its +closure evidence intact. The retired local 2026-04-17 benchmark baseline moved +from the standalone `docs/benchmark-baseline-archive/` directory into the +performance archive. + ### Security — nltk 3.10.0 -> 3.10.3 in uv.lock (Dependabot GHSA-m4rf-3fr8-xwx3, GHSA-6hwm-xvph-95vm) - `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. diff --git a/docs/architecture.md b/docs/architecture.md index e08390a9..ec0ffe99 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -134,7 +134,7 @@ See [Architecture Decision Records](decisions/) for detailed trade-off analysis. ## Security ### Implemented -- **API authentication**: API key via `X-API-Key` header (set `AGENTFLOW_API_KEYS` env var). Requirements for a key's id: [API key identity](specs/api-key-identity.md) +- **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) - **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`) - **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) - **Health/docs exempt**: `/v1/health`, `/docs`, `/metrics` don't require auth diff --git a/docs/specs/api-key-rate-limiting.md b/docs/specs/api-key-rate-limiting.md index 1c766da8..a540c8b0 100644 --- a/docs/specs/api-key-rate-limiting.md +++ b/docs/specs/api-key-rate-limiting.md @@ -20,7 +20,10 @@ id, and no bucket name SHALL contain a plaintext API key. Reloading the key configuration — `load()`, a SIGHUP reload, or the reload that ends every key create, rotate and revoke — SHALL keep the in-memory rate-limit window of every key that is still configured, and SHALL drop the window of a -key the reload removed. +key the reload removed. A key whose id changes on every load, a legacy +hash-only entry in a key file the process cannot write (see +[API key identity](api-key-identity.md)), gets a new bucket on every reload, so +its window starts empty. ### Scenario: a full window survives a reload - **GIVEN** a key with `rate_limit_rpm: 1` that has already made its one request in the current window diff --git a/src/agentflow_runtime/serving/api/auth/key_rotation.py b/src/agentflow_runtime/serving/api/auth/key_rotation.py index 19e855b8..3016dbd6 100644 --- a/src/agentflow_runtime/serving/api/auth/key_rotation.py +++ b/src/agentflow_runtime/serving/api/auth/key_rotation.py @@ -4,6 +4,7 @@ import re import secrets import threading +from collections.abc import Collection from datetime import UTC, datetime, timedelta try: @@ -22,6 +23,42 @@ is_permission_denied, ) +# A derived key_id ends in this many characters of the key's lookup digest: the +# same shape as the 8 hex characters `generate_key_id` draws at random. +KEY_ID_DIGEST_CHARS = 8 + + +def key_id_slug(value: str, fallback: str) -> str: + return "-".join(re.findall(r"[a-z0-9]+", value.lower())) or fallback + + +def derived_key_id(item: TenantKey, existing_ids: Collection[str]) -> str | None: + """A stable key_id for an entry configured without one, or None. + + The suffix is a prefix of the entry's peppered lookup digest -- its stored + `key_lookup`, else `compute_key_lookup` over its plaintext key -- so the + same key gets the same id on every load, restart and replica that shares + the pepper. Never the plaintext or an unpeppered hash of it: the id lands in + logs, Redis key names, usage rows and admin responses, and must not let a + guessed key be confirmed offline (audit FB-07). + + An id already taken lengthens the prefix, so the later entry of a clashing + pair still gets the same id on every load. None when there is nothing to + derive from (a legacy hash-only entry) or the whole digest is taken. + """ + if item.key_lookup is not None: + lookup = item.key_lookup + elif item.key is not None: + lookup = compute_key_lookup(item.key) + else: + return None + stem = f"{key_id_slug(item.tenant, 'tenant')}-{key_id_slug(item.name, 'agent')}-" + for size in range(KEY_ID_DIGEST_CHARS, len(lookup) + 1): + candidate = stem + lookup[:size] + if candidate not in existing_ids: + return candidate + return None + class KeyRotator: def __init__(self, manager: AuthManager) -> None: @@ -274,8 +311,8 @@ def generate_key_id( name: str, existing_ids: set[str] | None = None, ) -> str: - tenant_slug = re.sub(r"[^a-z0-9]+", "-", tenant.lower()).strip("-") or "tenant" - name_slug = re.sub(r"[^a-z0-9]+", "-", name.lower()).strip("-") or "agent" + tenant_slug = key_id_slug(tenant, "tenant") + name_slug = key_id_slug(name, "agent") seen_ids = set(existing_ids or ()) while True: candidate = f"{tenant_slug}-{name_slug}-{secrets.token_hex(4)}" @@ -288,7 +325,13 @@ def ensure_key_ids(self, config: ApiKeysConfig) -> bool: for index, item in enumerate(config.keys): if item.key_id is not None: continue - key_id = self.generate_key_id(item.tenant, item.name, existing_ids) + # Derived, not drawn: an id that never reaches the file (a read-only + # key store, or AGENTFLOW_API_KEYS) must come out the same on the next + # load and in every replica. Only a legacy hash-only entry, with + # nothing to derive from, still gets a random one. + key_id = derived_key_id(item, existing_ids) or self.generate_key_id( + item.tenant, item.name, existing_ids + ) existing_ids.add(key_id) config.keys[index] = item.model_copy(update={"key_id": key_id}) changed = True diff --git a/src/agentflow_runtime/serving/api/auth/manager.py b/src/agentflow_runtime/serving/api/auth/manager.py index e14c4c7f..6cc3660f 100644 --- a/src/agentflow_runtime/serving/api/auth/manager.py +++ b/src/agentflow_runtime/serving/api/auth/manager.py @@ -330,7 +330,7 @@ def load(self) -> None: self._key_rotator.schedule_rotation_cleanup(item) # Carry windows over by bucket name (`_rate_limit_key`), never by the # plaintext `keys_by_value` index: a still-configured key keeps its - # window across the reload, a removed key's window is dropped. (T-46) + # window across the reload, a removed key's window is dropped. live_buckets = {self._rate_limit_key(item) for item in config.keys} self._rate_windows = defaultdict( list, @@ -704,7 +704,6 @@ def _legacy_env_keys(self) -> list[TenantKey]: key, name = pair, "unnamed" items.append( TenantKey( - key_id=self._key_rotator.generate_key_id("default", name.strip(), set()), key=key.strip(), name=name.strip(), tenant="default", @@ -713,7 +712,12 @@ def _legacy_env_keys(self) -> list[TenantKey]: created_at=datetime.now(UTC).date(), ) ) - return items + # The same derivation as an id-less key-file entry: from the key's + # lookup digest, so the key keeps its id -- and its rate-limit bucket -- + # across reloads, restarts and replicas. + config = ApiKeysConfig(keys=items) + self._key_rotator.ensure_key_ids(config) + return config.keys def _rate_limit_key(self, tenant_key: TenantKey) -> str: # This string becomes a Redis sorted-set key NAME (``rate_limiter`` calls diff --git a/tests/unit/test_auth_key_identity.py b/tests/unit/test_auth_key_identity.py new file mode 100644 index 00000000..439ec451 --- /dev/null +++ b/tests/unit/test_auth_key_identity.py @@ -0,0 +1,280 @@ +"""Requirement tests for docs/specs/api-key-identity.md, one per scenario. + +A key configured without a `key_id` -- every key from `AGENTFLOW_API_KEYS`, and +a key-file entry without one -- gets an id derived from its peppered key-lookup +digest, so the same key keeps the same id (and so the same rate-limit bucket +and usage rows) on every load, restart and replica, even when the key file is +mounted read-only and the id can never be written back. +""" + +from __future__ import annotations + +import os +import re +import shutil +import stat +from pathlib import Path + +import pytest +import yaml + +from agentflow_runtime.serving.api.auth import manager as manager_module +from agentflow_runtime.serving.api.auth.manager import AuthManager +from agentflow_runtime.serving.api.security import compute_key_lookup + +PEPPER = "api-key-identity-test-pepper" +REPO_ROOT = Path(__file__).resolve().parents[2] +STORED_LOOKUP = "5e1f0c3a9b7d24866d3f2e1c0b9a8f7e6d5c4b3a29180716f5e4d3c2b1a09f8e" +HASHED_ENTRY_YAML = ( + "keys:\n" + ' - key_hash: "$2b$04$storedhashstoredhashstoredhashstoredhashstoredhashst"\n' + f' key_lookup: "{STORED_LOOKUP}"\n' + ' name: "Support Agent"\n' + ' tenant: "acme"\n' + " rate_limit_rpm: 60\n" + ' created_at: "2026-04-10"\n' +) + + +@pytest.fixture(autouse=True) +def _pinned_pepper(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("AGENTFLOW_KEY_LOOKUP_PEPPER", PEPPER) + monkeypatch.delenv("AGENTFLOW_PROFILE", raising=False) + monkeypatch.delenv("AGENTFLOW_API_KEYS", raising=False) + monkeypatch.delenv("REDIS_URL", raising=False) + + +def _env_manager(tmp_path: Path, db_name: str = "usage.duckdb") -> AuthManager: + manager = AuthManager(api_keys_path=None, db_path=tmp_path / db_name) + manager.load() + return manager + + +def _file_manager(path: Path, tmp_path: Path, db_name: str = "usage.duckdb") -> AuthManager: + manager = AuthManager(api_keys_path=path, db_path=tmp_path / db_name) + manager.load() + return manager + + +def _key_file(tmp_path: Path, content: str) -> Path: + path = tmp_path / "config" / "api_keys.yaml" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8", newline="\n") + return path + + +def test_same_environment_key_two_managers(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:Support Agent") + + first = _env_manager(tmp_path, "first.duckdb") + second = _env_manager(tmp_path, "second.duckdb") + + key_id = first.keys_by_value["k1"].key_id + assert key_id is not None + assert re.fullmatch(r"default-support-agent-[0-9a-f]{8}", key_id) + assert second.keys_by_value["k1"].key_id == key_id + + +def test_a_reload_keeps_the_id_and_the_bucket( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Environment keys carry the default rpm; one request fills a 1-rpm window. + monkeypatch.setattr(manager_module, "DEFAULT_RATE_LIMIT_RPM", 1) + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:bot") + manager = _env_manager(tmp_path) + key = manager.keys_by_value["k1"] + bucket = manager._rate_limit_key(key) + assert manager.is_rate_limited(key) is False + assert manager.is_rate_limited(key) is True + + manager.load() + + reloaded = manager.keys_by_value["k1"] + assert reloaded.key_id == key.key_id + assert manager._rate_limit_key(reloaded) == bucket + # The practical effect: the full window survived the reload. + assert manager.is_rate_limited(reloaded) is True + + +def test_different_keys_under_one_name_get_different_ids( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:bot,k2:bot") + + manager = _env_manager(tmp_path) + + first = manager.keys_by_value["k1"].key_id + second = manager.keys_by_value["k2"].key_id + assert first is not None + assert second is not None + assert first != second + + +def test_an_idless_entry_in_a_read_only_key_file(tmp_path: Path) -> None: + if hasattr(os, "geteuid") and os.geteuid() == 0: + pytest.skip("root writes a read-only file regardless of its mode") + path = _key_file(tmp_path, HASHED_ENTRY_YAML) + on_disk = path.read_bytes() + os.chmod(path, stat.S_IREAD) + try: + first = _file_manager(path, tmp_path, "first.duckdb") + second = _file_manager(path, tmp_path, "second.duckdb") + [first_key] = first._loaded_keys + [second_key] = second._loaded_keys + assert first_key.key_id is not None + assert second_key.key_id == first_key.key_id + + first.load() + + [reloaded] = first._loaded_keys + assert reloaded.key_id == first_key.key_id + # The id was never written back: it is the derivation, not the file, + # that keeps it stable. + assert path.read_bytes() == on_disk + finally: + os.chmod(path, stat.S_IREAD | stat.S_IWRITE) + + +def test_a_writable_key_file_persists_the_derived_id(tmp_path: Path) -> None: + path = _key_file( + tmp_path, + "keys:\n" + ' - key: "plain-file-key"\n' + ' name: "Report Bot"\n' + ' tenant: "Acme"\n' + ' created_at: "2026-04-10"\n', + ) + + manager = _file_manager(path, tmp_path) + + [key] = manager._loaded_keys + expected = f"acme-report-bot-{compute_key_lookup('plain-file-key', PEPPER)[:8]}" + assert key.key_id == expected + [stored] = yaml.safe_load(path.read_text(encoding="utf-8"))["keys"] + assert stored["key_id"] == expected + + +def test_the_suffix_is_the_lookup_digests_prefix( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:bot") + + manager = _env_manager(tmp_path) + + key_id = manager.keys_by_value["k1"].key_id + assert key_id == f"default-bot-{compute_key_lookup('k1', PEPPER)[:8]}" + + +def test_a_stored_key_lookup_is_used_as_it_is(tmp_path: Path) -> None: + path = _key_file(tmp_path, HASHED_ENTRY_YAML) + + manager = _file_manager(path, tmp_path) + + [key] = manager._loaded_keys + assert key.key_id == f"acme-support-agent-{STORED_LOOKUP[:8]}" + + +def test_a_different_pepper_gives_a_different_id( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:bot") + first = _env_manager(tmp_path, "first.duckdb") + monkeypatch.setenv("AGENTFLOW_KEY_LOOKUP_PEPPER", PEPPER + "-rotated") + second = _env_manager(tmp_path, "second.duckdb") + + assert first.keys_by_value["k1"].key_id != second.keys_by_value["k1"].key_id + + +def test_a_stored_key_lookup_keeps_its_id_under_another_pepper( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Two copies of one file: a writable file gets the id written back, and the + # second load must derive it again rather than read it. + first_path = _key_file(tmp_path, HASHED_ENTRY_YAML) + second_path = tmp_path / "copy" / "api_keys.yaml" + second_path.parent.mkdir() + second_path.write_text(HASHED_ENTRY_YAML, encoding="utf-8", newline="\n") + + first = _file_manager(first_path, tmp_path, "first.duckdb") + monkeypatch.setenv("AGENTFLOW_KEY_LOOKUP_PEPPER", PEPPER + "-rotated") + second = _file_manager(second_path, tmp_path, "second.duckdb") + + [first_key] = first._loaded_keys + [second_key] = second._loaded_keys + assert first_key.key_id == f"acme-support-agent-{STORED_LOOKUP[:8]}" + assert second_key.key_id == first_key.key_id + + +def test_a_writable_key_file_keeps_its_written_id_under_another_pepper( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # The pepper changes only an id that was never written back: the first load + # persists the derived id, and a later load reads it from the file. + path = _key_file( + tmp_path, + "keys:\n" + ' - key: "plain-file-key"\n' + ' name: "Report Bot"\n' + ' tenant: "Acme"\n' + ' created_at: "2026-04-10"\n', + ) + first = _file_manager(path, tmp_path, "first.duckdb") + [written] = yaml.safe_load(path.read_text(encoding="utf-8"))["keys"] + monkeypatch.setenv("AGENTFLOW_KEY_LOOKUP_PEPPER", PEPPER + "-rotated") + second = _file_manager(path, tmp_path, "second.duckdb") + + [first_key] = first._loaded_keys + [second_key] = second._loaded_keys + expected = f"acme-report-bot-{compute_key_lookup('plain-file-key', PEPPER)[:8]}" + assert first_key.key_id == expected + assert written["key_id"] == expected + assert second_key.key_id == expected + + +# --------------------------------------------------------------------------- # +# False-reject control: the change derives ids, it rejects nothing that loaded. +# --------------------------------------------------------------------------- # + + +def test_the_shipped_key_file_still_loads_with_ids_from_its_stored_lookups( + tmp_path: Path, +) -> None: + # config/api_keys.yaml is the file docker-compose.prod.yml mounts + # read-only: two hashed entries with a key_lookup and no key_id. + path = tmp_path / "config" / "api_keys.yaml" + path.parent.mkdir(parents=True) + shutil.copyfile(REPO_ROOT / "config" / "api_keys.yaml", path) + + manager = _file_manager(path, tmp_path) + + by_name = {item.name: item for item in manager._loaded_keys} + assert set(by_name) == {"Support Agent", "Ops Agent"} + for name, slug in (("Support Agent", "support-agent"), ("Ops Agent", "ops-agent")): + item = by_name[name] + assert item.key_lookup is not None + assert item.key_id == f"default-{slug}-{item.key_lookup[:8]}" + assert manager._keys_by_lookup[item.key_lookup] is item + + +def test_a_persisted_id_and_a_hash_only_entry_still_load(tmp_path: Path) -> None: + path = _key_file( + tmp_path, + "keys:\n" + ' - key_id: "acme-kept-id"\n' + ' key: "kept-plain-key"\n' + ' name: "Kept"\n' + ' tenant: "acme"\n' + ' created_at: "2026-04-10"\n' + ' - key_hash: "$2b$04$legacyhashlegacyhashlegacyhashlegacyhashlegacyhash"\n' + ' name: "Legacy"\n' + ' tenant: "acme"\n' + ' created_at: "2026-04-10"\n', + ) + + manager = _file_manager(path, tmp_path) + + kept, legacy = manager._loaded_keys + assert kept.key_id == "acme-kept-id" + # Nothing to derive from: the legacy entry keeps generate_key_id's shape. + assert legacy.key_id is not None + assert re.fullmatch(r"acme-legacy-[0-9a-f]{8}", legacy.key_id) diff --git a/tests/unit/test_key_rotation_mutation.py b/tests/unit/test_key_rotation_mutation.py index 9497dd36..512a543f 100644 --- a/tests/unit/test_key_rotation_mutation.py +++ b/tests/unit/test_key_rotation_mutation.py @@ -787,8 +787,8 @@ def test_assigns_ids_to_idless_entries_accumulating_existing( manager._key_rotator, "generate_key_id", _rec_gen_id(id_calls, ["gen-1", "gen-2"]) ) keyed = _tk(key_id="keep-id", key_hash="h0") - first = _tk(key_id=None, key_hash="h1", tenant="t1", name="n1") - second = _tk(key_id=None, key_hash="h2", tenant="t2", name="n2") + first = _tk(key_id=None, key=None, key_hash="h1", tenant="t1", name="n1") + second = _tk(key_id=None, key=None, key_hash="h2", tenant="t2", name="n2") config = ApiKeysConfig(keys=[keyed, first, second]) changed = manager._key_rotator.ensure_key_ids(config) @@ -817,6 +817,108 @@ def _boom(*_a: object, **_k: object) -> str: assert manager._key_rotator.ensure_key_ids(config) is False assert called["n"] == 0 # no id generated when every entry has one + def test_derives_ids_for_entries_it_can_and_keeps_them_unique( + self, tmp_path: object, monkeypatch: pytest.MonkeyPatch + ) -> None: + manager = _build_manager(tmp_path, monkeypatch) + id_calls: list = [] + monkeypatch.setattr( + manager._key_rotator, "generate_key_id", _rec_gen_id(id_calls, ["gen-1"]) + ) + # The persisted id comes AFTER the id-less entries and still takes the + # 8-char id first; the same key configured twice lengthens again. + derivable = _tk(key_id=None, key=None, key_hash="h1", key_lookup=_DIGEST) + again = _tk(key_id=None, key=None, key_hash="h2", key_lookup=_DIGEST) + legacy = _tk(key_id=None, key=None, key_hash="h3", tenant="t3", name="n3") + persisted = _tk(key_id=f"acme-n-{_DIGEST[:8]}", key_hash="h0") + config = ApiKeysConfig(keys=[derivable, again, legacy, persisted]) + + assert manager._key_rotator.ensure_key_ids(config) is True + + assert [item.key_id for item in config.keys] == [ + f"acme-n-{_DIGEST[:9]}", + f"acme-n-{_DIGEST[:10]}", + "gen-1", + f"acme-n-{_DIGEST[:8]}", + ] + # Only the hash-only entry drew a random id, against every id taken so far. + assert id_calls == [ + ( + "t3", + "n3", + {f"acme-n-{_DIGEST[:8]}", f"acme-n-{_DIGEST[:9]}", f"acme-n-{_DIGEST[:10]}"}, + ) + ] + + +# --------------------------------------------------------------------------- # +# derived_key_id / key_id_slug (module-level; no manager needed) +# --------------------------------------------------------------------------- # + +_DIGEST = "0123456789abcdef" * 4 + + +class _ProbedIds(set): + # Records every membership test, so the prefix search order is observable. + def __init__(self, *args: object) -> None: + super().__init__(*args) + self.probes: list = [] + + def __contains__(self, value: object) -> bool: + self.probes.append(value) + return super().__contains__(value) + + +def _no_lookup(value: str) -> str: + raise AssertionError(f"compute_key_lookup must not be called (got {value!r})") + + +class TestDerivedKeyId: + def test_stored_lookup_is_used_as_it_is(self, monkeypatch: pytest.MonkeyPatch) -> None: + # A stored key_lookup wins even when a plaintext key is present too. + monkeypatch.setattr(kr, "compute_key_lookup", _no_lookup) + item = _tk(key="plain-key", key_lookup=_DIGEST) + assert kr.derived_key_id(item, set()) == f"acme-n-{_DIGEST[:8]}" + + def test_plaintext_key_goes_through_the_peppered_lookup( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + calls: list = [] + monkeypatch.setattr(kr, "compute_key_lookup", lambda value: calls.append(value) or _DIGEST) + item = _tk(key="plain-key", key_lookup=None) + assert kr.derived_key_id(item, set()) == f"acme-n-{_DIGEST[:8]}" + assert calls == ["plain-key"] + + def test_hash_only_entry_has_nothing_to_derive_from( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr(kr, "compute_key_lookup", _no_lookup) + item = _tk(key=None, key_hash="h", key_lookup=None) + assert kr.derived_key_id(item, set()) is None + + def test_slugs_follow_generate_key_id_rules(self) -> None: + item = _tk(key_lookup=_DIGEST, tenant=" Acme Corp!", name="Support Agent 2 ") + assert kr.derived_key_id(item, set()) == f"acme-corp-support-agent-2-{_DIGEST[:8]}" + empty = _tk(key_lookup=_DIGEST, tenant="!!!", name="***") + assert kr.derived_key_id(empty, set()) == f"tenant-agent-{_DIGEST[:8]}" + + def test_a_taken_id_lengthens_the_prefix(self) -> None: + item = _tk(key_lookup=_DIGEST) + taken = {f"acme-n-{_DIGEST[:8]}", f"acme-n-{_DIGEST[:9]}"} + assert kr.derived_key_id(item, taken) == f"acme-n-{_DIGEST[:10]}" + + def test_prefixes_are_tried_once_each_up_to_the_whole_digest(self) -> None: + item = _tk(key_lookup="0123456789") + candidates = ["acme-n-01234567", "acme-n-012345678", "acme-n-0123456789"] + # Only the whole digest is free: it is the last candidate. + free_at_end = _ProbedIds(candidates[:2]) + assert kr.derived_key_id(item, free_at_end) == candidates[2] + assert free_at_end.probes == candidates + # Every prefix taken: nothing left to derive, and no prefix probed twice. + exhausted = _ProbedIds(candidates) + assert kr.derived_key_id(item, exhausted) is None + assert exhausted.probes == candidates + class TestFindKeyIndex: def test_returns_matching_index(