diff --git a/CHANGELOG.md b/CHANGELOG.md index 03010cec..6ec2b553 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,44 @@ All notable changes to AgentFlow are documented in this file. ## [Unreleased] +### 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 +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. 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 * **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 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/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..ec0ffe99 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). 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) +- **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 diff --git a/docs/specs/api-key-identity.md b/docs/specs/api-key-identity.md new file mode 100644 index 00000000..f8679f08 --- /dev/null +++ b/docs/specs/api-key-identity.md @@ -0,0 +1,62 @@ +# 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: 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>` + +### 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 + +### 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 — 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 +- **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..a540c8b0 --- /dev/null +++ b/docs/specs/api-key-rate-limiting.md @@ -0,0 +1,46 @@ +# 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. 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 +- **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 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 49ed09fc..6cc3660f 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. + 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 @@ -696,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", @@ -705,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/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) 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_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 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(