diff --git a/CHANGELOG.md b/CHANGELOG.md index 03010cec..0480dd3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,105 @@ All notable changes to AgentFlow are documented in this file. ## [Unreleased] +### Quality — the auth-manager mutation threshold ratchets to 0.90 + +* **`serving/api/auth/manager.py` now gates at 0.90.** CI run 34542418689 on + 77825f6 scored it 98.4% (553 killed of 562). The nine survivors are the + named residue in `tests/unit/test_auth_manager_mutation.py`. The old + comment above its `ModuleTarget` in `scripts/mutation_report.py` called + the structured-logging and Redis-URL survivors equivalents. T-48's + behaviour tests killed all of them except the upper-cased Redis URL + default, which is one of the nine. The comment now gives the measured + score and names the nine equivalent survivors, pointing to the test + file's docstring for their reasons. 0.90 is the bar the other serving + modules hold, and it sits 8.4 points under the measured 98.4%, so the + nine named equivalents cannot fail the gate while a real loss of killed + mutants still does. + +### Fixed — mutation_local no longer scores a missing pytest temp directory as a kill + +* **`scripts/mutation_local.py` recorded every mutant that reached a + `tmp_path` test as killed.** `measure_module()` handed pytest a `--basetemp` + under a scratch directory it never created, and pytest `mkdir`s that path + without parents, so the first `tmp_path` fixture failed at setup and pytest + exited 1. Under `-x` that false kill covered every mutant that survived the + tests before it. `run_mutant()` now creates the `--basetemp` parent before it + starts pytest, so a mutant that reaches a passing `tmp_path` test is scored + survived, and a real failure is still a kill. + +### Quality — the auth-manager mutation lane pins what its survivors changed + +* **`serving/api/auth/manager.py` sat two points above its 0.80 threshold.** + CI run 34464697021 on 6231b73 scored it 82.1% (430 killed of 524). Most of + the 94 survivors changed something a behaviour test can see, including the + logging and Redis-URL mutants that `scripts/mutation_report.py` calls + equivalents: a log event is the operator's interface, and the Redis URL + decides which server holds the rate-limit budget. + `tests/unit/test_auth_manager_mutation.py` now pins the key-store + writability probe, read-only file included; the construction wiring (the + injected store and audit publisher, usage rows flowing through the real + `UsageWriter`, the default store following `db_path`, the security config + path at construction and on `load()`, the grace-period fallback and its + warning, the Redis server the limiter targets); the `load()` log events + `api_keys_loaded`, `api_key_store_write_skipped_readonly` and + `hashed_key_count_exceeds_guidance` (strictly above the soft limit, + unindexed entries only); what `load()` writes back, and the key file it + leaves byte-for-byte alone; that a match's slot comes from the material that + matched, never from the stored entry; that `load()` carries rate-limit + windows over by bucket; scan order in `authenticate()`; when + `check_rate_limit()` trusts Redis; and `AGENTFLOW_API_KEYS` parsing. +* **Two simplifications remove mutants no test could kill.** The writability + probe opens the key file in binary append mode: the text layer and its + `encoding="utf-8"` did nothing for an open-and-close probe, and three + mutants changed only them. `_legacy_env_keys()` stops passing + `allowed_entity_types` at `TenantKey`'s own default (`None`); + `rate_limit_rpm=DEFAULT_RATE_LIMIT_RPM` stays because `TenantKey`'s Field + default is evaluated at import, while a monkeypatch of the module global + is a load-time read. +* **The residue is named, not suppressed.** Nine equivalent mutants stay + alive, each on a line that also carries killable mutants, so a line-level + `# pragma: no mutate` would silence those too; the test file's docstring + lists them with their reasons. The threshold is unchanged here: it is set + from the CI measurement of this tree. + +### 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 +690,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/pyproject.toml b/pyproject.toml index 02e673ad..1939d271 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -338,13 +338,9 @@ asyncio_mode = "auto" # starting with `src.`, which (not duckdb) was the real blocker. (The gate runner # keeps mutmut workspaces free of relative pytest --basetemp overrides because # under py3.11 they break coverage->mutant mapping for file-I/O targets like -# key_rotation; see scripts/mutation_report.py.) manager.py runs at an honest 0.80 -# threshold (not 0.90): it is a -# large stateful auth class whose residual survivors are equivalent mutants -# (structured-logging args, model_copy updates equal to their defaults, redis-url -# strings masked by the `_redis = None` override, the env-only-dead write path) -- -# every behaviour-reachable mutant, including every auth-bypass and throttle -# off-by-one, is killed. See scripts/mutation_report.py. +# key_rotation; see scripts/mutation_report.py.) manager.py gates at 0.90 like +# the other serving modules; scripts/mutation_report.py gives its measured +# score and names its nine equivalent survivors. paths_to_mutate = [ "src/agentflow_runtime/serving/api/auth/manager.py", "src/agentflow_runtime/serving/api/auth/key_rotation.py", diff --git a/scripts/mutation_local.py b/scripts/mutation_local.py index 42704f34..13fc361b 100644 --- a/scripts/mutation_local.py +++ b/scripts/mutation_local.py @@ -495,8 +495,12 @@ def run_mutant( Each mutant gets its own `--basetemp`: pytest wipes and recreates that directory at startup, so concurrent runs sharing one abort each other and - exit 2. + exit 2. pytest creates `--basetemp` itself with a non-recursive mkdir, so + the parent has to exist before pytest starts -- otherwise the first + `tmp_path` fixture errors at setup, pytest exits 1, and the mutant is + scored killed. """ + basetemp.parent.mkdir(parents=True, exist_ok=True) command = [ python, "-m", diff --git a/scripts/mutation_report.py b/scripts/mutation_report.py index 3d2ecd94..8c1e3990 100644 --- a/scripts/mutation_report.py +++ b/scripts/mutation_report.py @@ -76,23 +76,22 @@ class ModuleTarget: threshold=0.90, tests=("tests/unit/test_nl_queries_mutation.py",), ), - # manager.py runs at 0.80, not the 0.90 the pure-function guards (sql_guard, - # sql_builder, ...) hold. It is a ~400-line stateful auth class whose - # surviving mutants are dominated by EQUIVALENTS that no behaviour-level test - # can kill: structured-logging arguments (the auth logger event names / kwargs), - # `model_copy(update=...)` dicts whose mutated field equals its default - # ("matched_slot" already defaults to "current"; "key"==api_key on a plaintext - # match), the redis-url strings masked by the `_redis = None` override under the - # duckdb-free harness, and the config-file write path that is dead under the - # env-only test. Every BEHAVIOUR-reachable mutant is killed -- crucially every - # auth bypass (the verify_api_key argument-swap mutants on the indexed / legacy - # / previous-key paths and in _matches_key_material) and every rate-limit / - # failed-auth throttle off-by-one. Local mutmut (py3.10) scores 405/483 = 83.9%; - # 0.80 leaves headroom for equivalent-mutant noise while still enforcing a real - # floor (the do-nothing baseline was 76.5%). key_rotation is the next target and - # stays declared-only until it gets its own duckdb-free test. + # manager.py scored 553/562 killed (98.4%) on CI run 34542418689 against + # commit 77825f6. The nine survivors are the named residue in the module + # docstring of tests/unit/test_auth_manager_mutation.py, grouped: + # __init__ 55 and 93 -- _key_store_readonly_skip_logged flag and the + # upper-cased Redis URL default; + # _load_config 3 and 5 -- encoding="utf-8" case / None; + # _sweep_expired_windows 10 and 20 -- pop(key, None) race guard; + # authenticate 10 and 11 -- model_copy of a just-proven-equal key; + # load 1 -- skipped_readonly_write truthiness. The docstring has the + # one-line reason for each. Every behaviour-reachable mutant is killed. + # The threshold is 0.90 because that is the bar the other serving modules + # hold; it sits 8.4 points under the measured 98.4%, so the nine named + # equivalents cannot fail the gate while a real loss of killed mutants + # still does. Path("serving/api/auth/manager.py"): ModuleTarget( - threshold=0.80, + threshold=0.90, tests=("tests/unit/test_auth_manager_mutation.py",), ), # key_rotation runs at 0.90. Its residual survivors (local mutmut: 21 of 365) 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..c27f9b8b 100644 --- a/src/agentflow_runtime/serving/api/auth/manager.py +++ b/src/agentflow_runtime/serving/api/auth/manager.py @@ -133,7 +133,8 @@ def probe_key_store_writable(path: Path | str | None) -> bool: def _file_is_writable(path: Path) -> bool: try: - with path.open("a", encoding="utf-8"): + # Binary: an open-and-close probe has no text to encode. + with path.open("ab"): return True except OSError as exc: if is_permission_denied(exc): @@ -328,9 +329,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,16 +705,19 @@ 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", rate_limit_rpm=DEFAULT_RATE_LIMIT_RPM, - allowed_entity_types=None, 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_manager_memory_bounds.py b/tests/unit/test_auth_manager_memory_bounds.py index ca3543dc..f811c746 100644 --- a/tests/unit/test_auth_manager_memory_bounds.py +++ b/tests/unit/test_auth_manager_memory_bounds.py @@ -60,12 +60,12 @@ def test_load_sweeps_expired_rate_windows(self, manager: AuthManager) -> None: manager._rate_windows["fresh-key"] = [now - 5] manager.load() # triggers _sweep_expired_windows under config lock assert "stale-key" not in manager._rate_windows - # 'fresh-key' is also removed because load() rebuilds _rate_windows - # from keys_by_value (none configured in this fixture) — that - # rebuild is documented behaviour from session 17 and is independent - # of the H-C4 sweep, but the assertion below pins it so a future - # refactor that reuses the old `defaultdict` cannot reintroduce - # the unbounded-growth path silently. + # 'fresh-key' is also removed: load() keeps only the windows whose + # bucket belongs to a still-configured key, and no configured key + # names 'fresh-key' (the fixture configures none). That carry-over is + # independent of the H-C4 sweep, but the assertion below pins it so a + # future refactor that keeps every window cannot reintroduce the + # unbounded-growth path silently. assert "fresh-key" not in manager._rate_windows diff --git a/tests/unit/test_auth_manager_mutation.py b/tests/unit/test_auth_manager_mutation.py index c4e987ef..68549ab5 100644 --- a/tests/unit/test_auth_manager_mutation.py +++ b/tests/unit/test_auth_manager_mutation.py @@ -36,13 +36,39 @@ on ``find_spec("serving")`` -- NOT ``import src``, which stays importable via the editable install even inside the workspace (cont.21 duckdb-crash root cause). Under ordinary pytest no stub is installed and the real modules load. + +4. **Named residue.** The mutants below stay alive on purpose. Each is + equivalent, and each sits on a line that also carries killable mutants, so a + line-level ``# pragma: no mutate`` would silence those too. + + * ``authenticate`` plaintext path, ``update={"key": api_key, ...}`` -> + ``"XXkeyXX"`` / ``"KEY"``: ``compare_digest`` has just proved + ``item.key == api_key``, so the copy carries the same key either way. + * ``load`` ``skipped_readonly_write = False`` -> ``None``, and ``__init__`` + ``_key_store_readonly_skip_logged = False`` -> ``None``: both flags are + read only for truthiness. + * ``_sweep_expired_windows`` ``pop(key, None)`` -> ``pop(key)``, in both + loops: the key comes from a snapshot of the same dict, so the default + matters only if a concurrent sweep removed it in between -- a race guard + no deterministic test can reproduce. + * ``_load_config`` ``read_text(encoding="utf-8")`` -> ``"UTF-8"`` (codec + names are case-insensitive) and -> ``encoding=None`` (the locale encoding + is UTF-8 on the Linux CI host; the non-ASCII key-file test kills it on a + cp1252 Windows host). + * ``__init__`` default ``"redis://localhost:6379"`` -> upper case: + ``urlparse`` lower-cases the scheme and the host. """ from __future__ import annotations +import importlib +import os +import stat import sys import types -from datetime import date +from datetime import UTC, date, datetime, tzinfo +from pathlib import Path +from typing import Any, Self def _in_mutation_workspace() -> bool: @@ -101,6 +127,7 @@ def _connect(*_args: object, **_kwargs: object) -> object: from agentflow_runtime.serving.api.auth import manager as manager_module import pytest +import yaml AuthManager = manager_module.AuthManager TenantKey = manager_module.TenantKey @@ -1172,3 +1199,664 @@ def test_close_usage_writer_forwards_an_explicit_timeout(self) -> None: m._usage_writer = writer # type: ignore[assignment] m.close_usage_writer(0.5) assert writer.close_calls == [0.5] + + +# --------------------------------------------------------------------------- # +# Doubles and key-file helpers for the wiring, log-contract and load tests. +# --------------------------------------------------------------------------- # + +LogRecord = tuple[str, str, dict[str, object]] + + +class _RecordingLogger: + """Stands in for the auth package logger. manager.py imports that logger + inside its functions from ``agentflow_runtime.serving.api.auth`` -- the + installed package, even in the mutation workspace -- so it is swapped on + that module, which is the same object in both environments. A log event is + the operator's interface: its name and fields are the contract pinned here.""" + + def __init__(self) -> None: + self.records: list[LogRecord] = [] + + def info(self, event: str, **fields: object) -> None: + self.records.append(("info", event, fields)) + + def warning(self, event: str, **fields: object) -> None: + self.records.append(("warning", event, fields)) + + def named(self, event: str) -> list[LogRecord]: + return [record for record in self.records if record[1] == event] + + +def _record_auth_logs(monkeypatch: pytest.MonkeyPatch) -> _RecordingLogger: + recorder = _RecordingLogger() + auth_package = importlib.import_module("agentflow_runtime.serving.api.auth") + monkeypatch.setattr(auth_package, "logger", recorder) + return recorder + + +class _RecordingStore: + """The one store method the UsageWriter calls.""" + + def __init__(self) -> None: + self.batches: list[list[Any]] = [] + + def record_api_usage_batch(self, rows: list[Any]) -> None: + self.batches.append(list(rows)) + + +class _RecordingPublisher: + def __init__(self) -> None: + self.payloads: list[dict] = [] + + def publish(self, payload: dict) -> None: + self.payloads.append(payload) + + +class _ScriptedLimiter: + """A limiter that gives a fixed verdict and records the bucket it was asked + about. ``with_redis=False`` leaves out the ``_redis`` attribute entirely.""" + + def __init__(self, verdict: tuple[bool, int, int], *, with_redis: bool = True) -> None: + self.verdict = verdict + self.asked: list[tuple[str, int]] = [] + if with_redis: + self._redis = object() + + async def check(self, key: str, rpm: int) -> tuple[bool, int, int]: + self.asked.append((key, rpm)) + return self.verdict + + +def _entry(**overrides: object) -> dict[str, object]: + base: dict[str, object] = {"name": "support", "tenant": "acme", "created_at": date(2026, 1, 1)} + base.update(overrides) + return base + + +def _write_key_file(path: Path, entries: list[dict[str, object]]) -> Path: + path.write_text(yaml.safe_dump({"keys": entries}, sort_keys=False), encoding="utf-8") + return path + + +def _skip_if_root() -> None: + geteuid = getattr(os, "geteuid", None) + if geteuid is not None and geteuid() == 0: + pytest.skip("root writes a read-only file regardless of its mode") + + +def _make_read_only(path: Path) -> None: + os.chmod(path, stat.S_IREAD) + + +def _make_writable(path: Path) -> None: + # Restored before tmp_path cleanup: Windows refuses to delete a read-only file. + os.chmod(path, stat.S_IREAD | stat.S_IWRITE) + + +def _redis_target(m: AuthManager) -> tuple[object, object]: + # redis-py's from_url does not connect, so no server is needed to read this. + kwargs = m.rate_limiter._redis.connection_pool.connection_kwargs # type: ignore[union-attr] + return kwargs["host"], kwargs["port"] + + +# --------------------------------------------------------------------------- # +# Key-store writability probe. +# --------------------------------------------------------------------------- # + + +class TestKeyStoreWritableProbe: + def test_no_path_is_not_writable(self) -> None: + assert manager_module.probe_key_store_writable(None) is False + + def test_existing_writable_file_probes_writable_and_is_left_unchanged( + self, tmp_path: Path + ) -> None: + keys_file = tmp_path / "api_keys.yaml" + keys_file.write_bytes(b"keys: []\n") + assert manager_module.probe_key_store_writable(keys_file) is True + assert keys_file.read_bytes() == b"keys: []\n" + + def test_existing_read_only_file_probes_not_writable(self, tmp_path: Path) -> None: + # Opening for reading would succeed; only an attempt to open for writing + # tells a read-only mount from a writable one. + _skip_if_root() + keys_file = tmp_path / "api_keys.yaml" + keys_file.write_bytes(b"keys: []\n") + _make_read_only(keys_file) + try: + assert manager_module.probe_key_store_writable(keys_file) is False + # The file check answers on its own; it does not lean on the probe's + # outer handler to turn a permission error into a verdict. + assert manager_module._file_is_writable(keys_file) is False + finally: + _make_writable(keys_file) + + +# --------------------------------------------------------------------------- # +# __init__ wiring: store, usage writer, security policy, grace period, Redis. +# --------------------------------------------------------------------------- # + + +class TestConstructionWiring: + def test_injected_store_is_the_store_the_manager_uses(self) -> None: + store = _RecordingStore() + m = _build_manager(store=store) + assert m.store is store + + def test_default_store_resolves_usage_db_from_the_managers_db_path( + self, tmp_path: Path + ) -> None: + m = _build_manager(db_path=tmp_path / "usage.duckdb") + store = m.store + assert store._usage_db_path == tmp_path / "usage.duckdb" # type: ignore[attr-defined] + # The provider reads the manager's attribute at call time, so a manager + # whose db_path moves takes its usage database with it. + m.db_path = tmp_path / "moved.duckdb" + assert store._usage_db_path == tmp_path / "moved.duckdb" # type: ignore[attr-defined] + + def test_submitted_usage_reaches_the_injected_store_and_publisher(self) -> None: + from agentflow_runtime.serving.control_plane.store import UsageRow + + store = _RecordingStore() + publisher = _RecordingPublisher() + m = _build_manager(store=store, audit_publisher=publisher) + tenant_key = _key(key_id="kid-1", name="support", tenant="acme", matched_slot="previous") + try: + assert m.submit_usage(tenant_key, "/v1/entity/order/1") is True + assert m.flush_usage(timeout=5.0) is True + finally: + m.close_usage_writer() + expected = UsageRow( + tenant="acme", + key_name="support", + endpoint="/v1/entity/order/1", + key_id="kid-1", + key_slot="previous", + ) + assert store.batches == [[expected]] + assert publisher.payloads == [ + { + "event_type": "api_usage", + "tenant": "acme", + "key_name": "support", + "endpoint": "/v1/entity/order/1", + "key_id": "kid-1", + "key_slot": "previous", + } + ] + + def test_security_config_path_is_honoured_at_construction_and_by_load( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + monkeypatch.delenv("AGENTFLOW_API_KEYS", raising=False) + security_file = tmp_path / "security.yaml" + security_file.write_text( + "security:\n max_failed_auth_per_ip_per_hour: 3\n", encoding="utf-8" + ) + # The default path's policy must differ, or reading it instead would pass. + assert manager_module.load_security_policy(None).max_failed_auth_per_ip_per_hour != 3 + m = _build_manager(security_config_path=security_file) + assert [m.record_failed_auth("10.0.0.1") for _ in range(4)] == [False, False, False, True] + m.load() + assert [m.record_failed_auth("10.0.0.2") for _ in range(4)] == [False, False, False, True] + + def test_key_store_writable_probes_lazily_before_any_load(self, tmp_path: Path) -> None: + keys_file = tmp_path / "api_keys.yaml" + keys_file.write_bytes(b"keys: []\n") + m = _build_manager(api_keys_path=keys_file) + assert m.key_store_writable is True + + def test_unset_rotation_grace_period_uses_the_default_without_a_warning( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("AGENTFLOW_ROTATION_GRACE_PERIOD_SECONDS", raising=False) + logs = _record_auth_logs(monkeypatch) + m = _build_manager() + assert m.rotation_grace_period_seconds == DEFAULT_ROTATION_GRACE_PERIOD_SECONDS + assert logs.named("invalid_rotation_grace_period_seconds") == [] + + def test_invalid_rotation_grace_period_logs_one_warning_with_the_raw_value( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("AGENTFLOW_ROTATION_GRACE_PERIOD_SECONDS", "ten-minutes") + logs = _record_auth_logs(monkeypatch) + _build_manager() + assert logs.records == [ + ( + "warning", + "invalid_rotation_grace_period_seconds", + {"value": "ten-minutes", "fallback": DEFAULT_ROTATION_GRACE_PERIOD_SECONDS}, + ) + ] + + def test_redis_url_argument_targets_that_server(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("REDIS_URL", raising=False) + m = _build_manager(redis_url="redis://cache.internal:6380/0") + assert _redis_target(m) == ("cache.internal", 6380) + + def test_redis_url_env_is_used_without_an_argument( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("REDIS_URL", "redis://env-cache.internal:6381/0") + m = _build_manager() + assert _redis_target(m) == ("env-cache.internal", 6381) + + def test_redis_url_argument_wins_over_the_env(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("REDIS_URL", "redis://env-cache.internal:6381/0") + m = _build_manager(redis_url="redis://cache.internal:6380/0") + assert _redis_target(m) == ("cache.internal", 6380) + + +# --------------------------------------------------------------------------- # +# load(): the log contract. +# --------------------------------------------------------------------------- # + + +class TestLoadLogContract: + def test_env_only_load_logs_env_only_and_the_configured_count( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:alpha,k2:beta") + logs = _record_auth_logs(monkeypatch) + m = _build_manager() + m.load() + assert logs.named("api_keys_loaded") == [ + ("info", "api_keys_loaded", {"path": "env_only", "keys": 2}) + ] + # Environment keys have nothing to write back, so nothing was skipped. + assert logs.named("api_key_store_write_skipped_readonly") == [] + + def test_key_file_load_logs_the_file_path( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", [_entry(key_id="kid-a", key="plain-a")] + ) + logs = _record_auth_logs(monkeypatch) + m = _build_manager(api_keys_path=keys_file) + m.load() + assert logs.named("api_keys_loaded") == [ + ("info", "api_keys_loaded", {"path": str(keys_file), "keys": 1}) + ] + + def test_unwritable_changed_config_warns_once_across_loads( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + _skip_if_root() + # No key_id -> load() derives one and wants to write it back. + keys_file = _write_key_file(tmp_path / "api_keys.yaml", [_entry(key="plain-a")]) + before = keys_file.read_bytes() + logs = _record_auth_logs(monkeypatch) + _make_read_only(keys_file) + try: + m = _build_manager(api_keys_path=keys_file) + m.load() + m.load() + finally: + _make_writable(keys_file) + assert logs.named("api_key_store_write_skipped_readonly") == [ + ("warning", "api_key_store_write_skipped_readonly", {"path": str(keys_file)}) + ] + assert keys_file.read_bytes() == before + assert m.key_store_writable is False + + def test_write_skip_warning_names_env_only_without_a_key_file( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + logs = _record_auth_logs(monkeypatch) + m = _build_manager() + m._warn_key_store_write_skipped() + assert logs.records == [ + ("warning", "api_key_store_write_skipped_readonly", {"path": "env_only"}) + ] + + +def _hashed_entries(count: int, *, indexed: bool) -> list[dict[str, object]]: + entries = [] + for index in range(count): + entry = _entry(key_id=f"kid-{index}", name=f"agent-{index}", key_hash=f"hash-{index}") + if indexed: + entry["key_lookup"] = f"lookup-{index}" + entries.append(entry) + return entries + + +class TestHashedKeyGuidance: + """``hashed_key_count_exceeds_guidance`` is documented in + docs/runbooks/auth-401-spike.md with its ``hashed_keys`` and ``soft_limit`` + fields. Only entries without a ``key_lookup`` pay the O(n) verify scan, so + only they count, and the warning fires strictly above the soft limit.""" + + def _load_logs( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, entries: list[dict[str, object]] + ) -> list[LogRecord]: + keys_file = _write_key_file(tmp_path / "api_keys.yaml", entries) + logs = _record_auth_logs(monkeypatch) + _build_manager(api_keys_path=keys_file).load() + return logs.named("hashed_key_count_exceeds_guidance") + + def test_exactly_the_soft_limit_is_silent( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + limit = manager_module.HASHED_KEY_SOFT_LIMIT + assert self._load_logs(monkeypatch, tmp_path, _hashed_entries(limit, indexed=False)) == [] + + def test_one_unindexed_key_over_the_soft_limit_warns_with_the_count( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + limit = manager_module.HASHED_KEY_SOFT_LIMIT + entries = _hashed_entries(limit + 1, indexed=False) + assert self._load_logs(monkeypatch, tmp_path, entries) == [ + ( + "warning", + "hashed_key_count_exceeds_guidance", + { + "hashed_keys": limit + 1, + "soft_limit": limit, + "reason": "cold_cache_bcrypt_latency", + "guidance": "docs/runbooks/auth-401-spike.md", + }, + ) + ] + + def test_indexed_keys_do_not_count( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + limit = manager_module.HASHED_KEY_SOFT_LIMIT + entries = _hashed_entries(limit + 5, indexed=True) + assert self._load_logs(monkeypatch, tmp_path, entries) == [] + + +# --------------------------------------------------------------------------- # +# load(): what it writes back, and what it leaves alone. +# --------------------------------------------------------------------------- # + + +class TestLoadWriteBack: + def test_writable_key_file_gains_the_key_ids_it_lacked(self, tmp_path: Path) -> None: + keys_file = _write_key_file(tmp_path / "api_keys.yaml", [_entry(key="plain-alpha")]) + m = _build_manager(api_keys_path=keys_file) + m.load() + [loaded] = m._loaded_keys + assert loaded.key_id is not None + assert loaded.key_id.startswith("acme-support-") + [stored] = yaml.safe_load(keys_file.read_text(encoding="utf-8"))["keys"] + assert stored["key_id"] == loaded.key_id + + def test_key_file_that_needs_no_change_is_left_byte_for_byte(self, tmp_path: Path) -> None: + raw = ( + b"# hand-maintained -- keep the comments\n" + b"keys:\n" + b" - key_id: kid-alpha # pinned id\n" + b" key: plain-alpha\n" + b" name: support\n" + b" tenant: acme\n" + b" created_at: 2026-01-01\n" + ) + keys_file = tmp_path / "api_keys.yaml" + keys_file.write_bytes(raw) + m = _build_manager(api_keys_path=keys_file) + m.load() + assert m.configured_key_count == 1 + assert keys_file.read_bytes() == raw + + def test_key_file_is_read_as_utf8(self, tmp_path: Path) -> None: + keys_file = tmp_path / "api_keys.yaml" + keys_file.write_bytes( + "keys:\n" + " - key_id: kid-alpha\n" + " key: plain-alpha\n" + " name: Zoë Müller\n" + " tenant: acme\n" + " created_at: 2026-01-01\n".encode() + ) + m = _build_manager(api_keys_path=keys_file) + m.load() + assert [key.name for key in m._loaded_keys] == ["Zoë Müller"] + + +# --------------------------------------------------------------------------- # +# The slot a match reports is decided by the material that matched. +# --------------------------------------------------------------------------- # + + +class TestSlotComesFromTheMatchedMaterial: + """``matched_slot`` is excluded from serialisation but accepted on input, so + a key-file entry can say ``matched_slot: previous``. Usage rows carry the + slot and rotation decisions read it, so a match on current material must + say "current" whatever the stored entry says.""" + + def test_load_labels_the_plaintext_index_current(self, tmp_path: Path) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", + [_entry(key_id="kid-a", key="plain-a", matched_slot="previous")], + ) + m = _build_manager(api_keys_path=keys_file) + m.load() + assert m._loaded_keys[0].matched_slot == "previous" # what the file says + assert m.keys_by_value["plain-a"].matched_slot == "current" + + def test_load_labels_a_runtime_cached_hashed_entry_current(self, tmp_path: Path) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", + [_entry(key_id="kid-h", key_hash="hash-h", key_lookup="lk-h", matched_slot="previous")], + ) + m = _build_manager(api_keys_path=keys_file) + m._runtime_plaintext_by_hash = {"hash-h": "runtime-plain"} + m.load() + assert m.keys_by_value["runtime-plain"].matched_slot == "current" + + def test_plaintext_match_is_current(self) -> None: + m = _build_manager() + m.keys_by_value = {"plain-a": _key(key="plain-a", matched_slot="previous")} + out = m.authenticate("plain-a") + assert out is not None + assert out.matched_slot == "current" + + def test_indexed_match_is_current(self, monkeypatch: pytest.MonkeyPatch) -> None: + m = _build_manager() + m._keys_by_lookup = { + "lk": _key(key=None, key_hash="idx-hash", key_lookup="lk", matched_slot="previous") + } + monkeypatch.setattr(manager_module, "compute_key_lookup", lambda value: "lk") + monkeypatch.setattr( + manager_module, "verify_api_key", lambda value, h: (value, h) == ("k", "idx-hash") + ) + out = m.authenticate("k") + assert out is not None + assert out.matched_slot == "current" + + def test_legacy_hashed_match_is_current(self, monkeypatch: pytest.MonkeyPatch) -> None: + m = _build_manager() + m._hashed_keys = [_key(key=None, key_hash="legacy-hash", matched_slot="previous")] + monkeypatch.setattr(manager_module, "compute_key_lookup", lambda value: "no-hit") + monkeypatch.setattr( + manager_module, "verify_api_key", lambda value, h: (value, h) == ("k", "legacy-hash") + ) + out = m.authenticate("k") + assert out is not None + assert out.matched_slot == "current" + + +# --------------------------------------------------------------------------- # +# load(): rate-limit windows are carried over by bucket name. +# --------------------------------------------------------------------------- # + + +class TestLoadCarriesRateWindowsByBucket: + def test_still_configured_key_keeps_its_full_window_across_load(self, tmp_path: Path) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", [_entry(key_id="kid-a", key="plain-a", rate_limit_rpm=2)] + ) + m = _build_manager(api_keys_path=keys_file) + m.load() + tenant_key = m.authenticate("plain-a") + assert tenant_key is not None + assert [m.is_rate_limited(tenant_key) for _ in range(2)] == [False, False] + m.load() + assert m.is_rate_limited(tenant_key) is True + + def test_removed_key_loses_its_window(self, tmp_path: Path) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", + [_entry(key_id="kid-a", key="plain-a"), _entry(key_id="kid-b", key="plain-b")], + ) + m = _build_manager(api_keys_path=keys_file) + m.load() + m._rate_windows["kid:kid-a"] = [1_000.0] + m._rate_windows["kid:kid-b"] = [1_000.0] + _write_key_file(keys_file, [_entry(key_id="kid-a", key="plain-a")]) + m.load() + assert dict(m._rate_windows) == {"kid:kid-a": [1_000.0]} + + def test_no_window_is_named_by_a_plaintext_key(self, tmp_path: Path) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", [_entry(key_id="kid-a", key="plain-a")] + ) + m = _build_manager(api_keys_path=keys_file) + m._rate_windows["plain-a"] = [1_000.0] + m._rate_windows["kid:kid-a"] = [1_000.0] + m.load() + assert dict(m._rate_windows) == {"kid:kid-a": [1_000.0]} + + +# --------------------------------------------------------------------------- # +# authenticate(): a skipped entry never ends a scan. +# --------------------------------------------------------------------------- # + + +class TestAuthenticateScanOrder: + def test_plaintext_scan_steps_over_an_entry_without_a_runtime_key(self) -> None: + m = _build_manager() + m.keys_by_value = { + "hash-only": _key(key=None, key_hash="h"), + "plain-a": _key(key="plain-a", tenant="acme"), + } + out = m.authenticate("plain-a") + assert out is not None + assert out.tenant == "acme" + + def _rotating(self, index: int, **previous: object) -> TenantKey: + entry = _key(key=None, key_hash=f"cur-{index}", tenant=f"t{index}") + return entry.model_copy(update={"previous_key_hash": f"prev-{index}", **previous}) + + def test_legacy_previous_scan_steps_over_an_inactive_entry( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + m = _build_manager() + inactive, match = self._rotating(0), self._rotating(1) + m._loaded_keys = [inactive, match] + monkeypatch.setattr(manager_module, "compute_key_lookup", lambda value: "no-hit") + monkeypatch.setattr(m._key_rotator, "is_previous_key_active", lambda item: item is match) + monkeypatch.setattr( + manager_module, "verify_api_key", lambda value, h: (value, h) == ("old", "prev-1") + ) + out = m.authenticate("old") + assert out is not None + assert (out.tenant, out.matched_slot) == ("t1", "previous") + + def test_legacy_previous_scan_steps_over_an_indexed_entry( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + m = _build_manager() + m._loaded_keys = [self._rotating(0, previous_key_lookup="lk-0"), self._rotating(1)] + monkeypatch.setattr(manager_module, "compute_key_lookup", lambda value: "no-hit") + monkeypatch.setattr(m._key_rotator, "is_previous_key_active", lambda item: True) + monkeypatch.setattr( + manager_module, "verify_api_key", lambda value, h: (value, h) == ("old", "prev-1") + ) + out = m.authenticate("old") + assert out is not None + assert (out.tenant, out.matched_slot) == ("t1", "previous") + + +# --------------------------------------------------------------------------- # +# check_rate_limit(): what the Redis limiter is asked, and when it is trusted. +# --------------------------------------------------------------------------- # + + +class TestCheckRateLimitDecisions: + @pytest.mark.asyncio + async def test_limiter_is_asked_about_the_key_id_bucket(self) -> None: + limiter = _ScriptedLimiter((True, 4, 77)) + m = _build_manager(rate_limiter=limiter) + await m.check_rate_limit(_key(key_id="kid-7", rate_limit_rpm=5)) + assert limiter.asked == [("kid:kid-7", 5)] + + @pytest.mark.asyncio + async def test_partial_quota_answer_passes_through_verbatim(self) -> None: + m = _build_manager(rate_limiter=_ScriptedLimiter((True, 4, 77))) + assert await m.check_rate_limit(_key(rate_limit_rpm=5)) == (True, 4, 77) + + @pytest.mark.asyncio + async def test_refusal_passes_through_verbatim(self) -> None: + m = _build_manager(rate_limiter=_ScriptedLimiter((False, 0, 77))) + assert await m.check_rate_limit(_key(rate_limit_rpm=5)) == (False, 0, 77) + + @pytest.mark.asyncio + async def test_limiter_without_a_redis_handle_passes_through(self) -> None: + m = _build_manager(rate_limiter=_ScriptedLimiter((True, 5, 77), with_redis=False)) + assert await m.check_rate_limit(_key(rate_limit_rpm=5)) == (True, 5, 77) + + @pytest.mark.asyncio + async def test_secondary_window_is_per_bucket(self) -> None: + m = _build_manager(rate_limiter=_FullRemainingLimiter()) + first = await m.check_rate_limit(_key(key_id="kid-a", rate_limit_rpm=1)) + second = await m.check_rate_limit(_key(key_id="kid-b", rate_limit_rpm=1)) + assert (first, second) == ((True, 0, 1_060), (True, 0, 1_060)) + + @pytest.mark.asyncio + async def test_secondary_window_drops_a_stamp_exactly_at_the_cutoff(self) -> None: + clock = FrozenClock(1_000.0) + m = _build_manager(time_source=clock, rate_limiter=_FullRemainingLimiter()) + tenant_key = _key(key_id="kid-a", rate_limit_rpm=1) + assert await m.check_rate_limit(tenant_key) == (True, 0, 1_060) + clock.now = 1_000.0 + DEFAULT_RATE_LIMIT_WINDOW_SECONDS # cutoff == first stamp + assert await m.check_rate_limit(tenant_key) == (True, 0, 1_120) + + +# --------------------------------------------------------------------------- # +# _legacy_env_keys(): AGENTFLOW_API_KEYS parsing. +# --------------------------------------------------------------------------- # + + +class TestLegacyEnvKeyParsing: + def test_empty_middle_segment_is_skipped_and_later_keys_still_load( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:a, ,k2:b") + keys = _build_manager()._legacy_env_keys() + assert [(k.key, k.name) for k in keys] == [("k1", "a"), ("k2", "b")] + + def test_first_colon_separates_key_from_a_name_that_has_colons( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:team:alpha") + keys = _build_manager()._legacy_env_keys() + assert [(k.key, k.name) for k in keys] == [("k1", "team:alpha")] + + def test_created_at_is_the_utc_calendar_date(self, monkeypatch: pytest.MonkeyPatch) -> None: + class _JustAfterUtcMidnight(datetime): + # 00:00:05 UTC on 2 March is still 1 March on a wall clock west of UTC. + @classmethod + def now(cls, tz: tzinfo | None = None) -> Self: + if tz is None: + return cls(2026, 3, 1, 19, 0, 5) + return cls(2026, 3, 2, 0, 0, 5, tzinfo=UTC) + + monkeypatch.setattr(manager_module, "datetime", _JustAfterUtcMidnight) + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:a") + [k] = _build_manager()._legacy_env_keys() + assert k.created_at == date(2026, 3, 2) + + def test_env_key_reads_rate_limit_rpm_from_the_module_global( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # TenantKey's Field default is the import-time value; the call in + # _legacy_env_keys reads the module global at load time. + monkeypatch.setattr(manager_module, "DEFAULT_RATE_LIMIT_RPM", 7) + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:bot") + m = _build_manager() + m.load() + assert m.keys_by_value["k1"].rate_limit_rpm == 7 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..72ec366a --- /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``, so each test names its +bucket directly. +""" + +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( diff --git a/tests/unit/test_mutation_local.py b/tests/unit/test_mutation_local.py index bfdf3fe8..f7aeb74b 100644 --- a/tests/unit/test_mutation_local.py +++ b/tests/unit/test_mutation_local.py @@ -1,8 +1,10 @@ """Unit tests for the local mutation driver's own logic. -Everything that costs minutes -- the mutmut engine and the pytest subprocesses --- is stubbed here. A real mutation run belongs on the command line -(`python scripts/mutation_local.py --module ...`), not in the unit suite. +Everything that costs minutes -- the mutmut engine and a full mutation run -- +is stubbed here. `run_mutant` is exercised against a one-test file so the +`--basetemp` parent is a real pytest mkdir, not a stub. A real mutation run +belongs on the command line (`python scripts/mutation_local.py --module ...`), +not in the unit suite. """ from __future__ import annotations @@ -325,6 +327,57 @@ def test_two_invocations_sharing_a_workspace_get_separate_basetemps( assert not set(_basetemps(first)) & set(_basetemps(second)) +def _run_mutant_against_tmp_path_test(tmp_path: Path, body: str) -> tuple[str, int | None]: + """Call `run_mutant` the way `measure_module` does: the basetemp parent is missing.""" + test_file = tmp_path / "test_tmp_path_probe.py" + test_file.write_text( + f"from pathlib import Path\n\ndef test_uses_tmp_path(tmp_path):\n{body}", + encoding="utf-8", + ) + basetemp = tmp_path / "scratch" / "run-missing" / "mutant0" + assert not basetemp.parent.exists() + return mutation_local.run_mutant( + tmp_path, + (test_file.name,), + "pkg.thing.x__mutmut_1", + python=sys.executable, + shim_dir=tmp_path / "shim", + basetemp=basetemp, + timeout=20.0, + ) + + +def test_run_mutant_survives_a_passing_tmp_path_test_when_the_basetemp_parent_is_missing( + tmp_path: Path, +): + """pytest mkdir()s --basetemp without parents; a missing parent is not a kill.""" + name, exit_code = _run_mutant_against_tmp_path_test( + tmp_path, + " (tmp_path / 'marker').write_text('ok')\n", + ) + + assert name == "pkg.thing.x__mutmut_1" + assert exit_code == 0 + assert mutation_local.classify_exit_code(exit_code) == "survived" + + +def test_run_mutant_still_kills_a_failing_tmp_path_test_when_the_basetemp_parent_is_missing( + tmp_path: Path, +): + """Creating the parent must not turn a real failure into a survival.""" + name, exit_code = _run_mutant_against_tmp_path_test( + tmp_path, + " Path('ran').write_text('yes')\n" + " (tmp_path / 'marker').write_text('ok')\n" + " assert False\n", + ) + + assert name == "pkg.thing.x__mutmut_1" + assert (tmp_path / "ran").read_text(encoding="utf-8") == "yes" + assert exit_code == 1 + assert mutation_local.classify_exit_code(exit_code) == "killed" + + def test_measure_module_scores_verdicts_and_never_counts_an_error_as_a_kill( monkeypatch, tmp_path: Path,