From 0c21b8f760b49daf238523d84b25441a1b1c2816 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Tue, 6 Oct 2026 22:18:55 -0400 Subject: [PATCH 1/4] docs(eventing): fix doc/code inconsistencies from the agentdocs review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six verified inconsistencies between the eventing docs and the tree, each checked against the code before editing (the repo's own §8.5/§8.6 discipline): 1. SIGNED_ATTRS drift: DESIGN_PHASE3's header, §2.6, §7.7, §9 step 4 and §11 T4 still described T4 as adding two attributes ("userkey, depth"), contradicting §8.3's rule, §2.6's own opening, Report3's T4 row and shared/signing.py, which all say three (userkey, depth, agent) in ONE change. All five sites now agree. 2. ER_USERKEY false positive (code fix): the runner refused to start whenever REQUEST_TOPIC != "requests" and ER_USERKEY was unset, which false-positives every Phase 1 single-tenant deployment — the shipped k8s/base/configmap.yaml sets REQUEST_TOPIC: kev1-requests and boots no runner at all. The refusal now keys on the per-user topic shape ({prefix}-{userkey}-requests, §3.1) via tenancy.userkey_in_topic(), so a renamed default boots and a per-user topic still demands its matching key — mismatched too, not just missing. DESIGN_PHASE3 §3.3 and §8.2 updated to match; 10 new tests (round-trip, each suffix, renamed-default regression, mismatch refusal, non-userkey shapes). 3. EB_TOPIC_PREFIX default: §3.1 claimed "{prefix} defaults to the namespace (kev1)"; the code has a literal "kev1" default and §8.1's row agrees. Reworded to the literal default and why. 4. README_PHASE1: "submitter is not signed" — it joined SIGNED_ATTRS in Phase 2 (DESIGN_PHASE2 §4.2), kept the honest-limit framing (Kafka still plaintext, the claim is about EventBridge's assertion). "roughly 100 ms per sign/verify" — Report1 §9 measured 222/227 ms; replaced with the measured figures and the 190-230 ms re-run band. 5. README_PHASE0: pinned the suite count to Report0's record (58 passed in 7.27s, not "20 passed in ~0.6s"). 6. KubeCon §9 changelog rows reordered chronologically (10-05, 10-05, 10-06); Report1's §3 table now cites §9 for the sign/verify figures (the section they live in); DESIGN_PHASE0's CLI skeleton --watch uses argparse.BooleanOptionalAction like the shipped CLI, not store_true+default=True, a combination that can never be false; DESIGN_PHASE2 §5 gains the missing ER_VERIFY_KEY_PATH row that the keyset row already referenced; the ~150-200 ms estimates in four code docstrings replaced with the measured 222/227 ms (Report1 §9), which KubeCon M4 flags as estimates-vs-measured. eventing suite: 907 passed, 7 skipped (baseline 897 + 10 new). ruff check / format --check: clean, CI-exact invocation. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- eventing/agentdocs/DESIGN_PHASE0.md | 2 +- eventing/agentdocs/DESIGN_PHASE2.md | 1 + eventing/agentdocs/DESIGN_PHASE3.md | 32 +++++++++------ eventing/agentdocs/IMPLEMENTATION_REPORT1.md | 2 +- eventing/agentdocs/KUBECON_NA_2026.md | 2 +- eventing/agentdocs/README_PHASE0.md | 2 +- eventing/agentdocs/README_PHASE1.md | 14 ++++--- eventing/eventbridge/kafka_out.py | 3 +- eventing/eventrunner/config.py | 29 +++++++++----- eventing/eventrunner/emit.py | 3 +- eventing/shared/signing.py | 5 ++- eventing/shared/tenancy.py | 35 +++++++++++++++- eventing/tests/test_tenancy.py | 42 ++++++++++++++++++++ eventing/tests/test_topicset_wiring.py | 23 ++++++++++- 14 files changed, 156 insertions(+), 39 deletions(-) diff --git a/eventing/agentdocs/DESIGN_PHASE0.md b/eventing/agentdocs/DESIGN_PHASE0.md index 956a47d8..1a63febf 100644 --- a/eventing/agentdocs/DESIGN_PHASE0.md +++ b/eventing/agentdocs/DESIGN_PHASE0.md @@ -107,7 +107,7 @@ def cmd_cont(args): if __name__ == "__main__": p = argparse.ArgumentParser() sub = p.add_subparsers(dest="op", required=True) - r = sub.add_parser("run"); r.add_argument("prompt"); r.add_argument("--max-turns", type=int, default=3); r.add_argument("--watch", action="store_true", default=True); r.set_defaults(fn=cmd_run) + r = sub.add_parser("run"); r.add_argument("prompt"); r.add_argument("--max-turns", type=int, default=3); r.add_argument("--watch", action=argparse.BooleanOptionalAction, default=True); r.set_defaults(fn=cmd_run) w = sub.add_parser("watch"); w.add_argument("correlationid"); w.set_defaults(fn=cmd_watch) c = sub.add_parser("cont"); c.add_argument("correlationid"); c.add_argument("prompt"); c.set_defaults(fn=cmd_cont) args = p.parse_args(); args.fn(args) diff --git a/eventing/agentdocs/DESIGN_PHASE2.md b/eventing/agentdocs/DESIGN_PHASE2.md index ca5354c3..a21dd080 100644 --- a/eventing/agentdocs/DESIGN_PHASE2.md +++ b/eventing/agentdocs/DESIGN_PHASE2.md @@ -386,6 +386,7 @@ unaffected until an operator opts in: | `ER_SIGNING_KEY_PATH` | empty | Seed EventRunner signs terminal responses with. | | `ER_SIGNING_KID` | empty | Names this runner's key. Needed once more than one runner is approved. | | `ER_REQUIRE_SIGNATURE` | `false` | Refuse unsigned or badly-signed requests. | +| `ER_VERIFY_KEY_PATH` | empty | Single public key requests are verified against. Empty falls back to the public half of `ER_SIGNING_KEY_PATH`'s seed. | | `ER_VERIFY_KEYSET_PATH` | empty | Approved-key set for requests. Empty falls back to `ER_VERIFY_KEY_PATH`'s single key. | Seed paths name **Secret** mounts and are env-only, never `config.toml`. Keyset diff --git a/eventing/agentdocs/DESIGN_PHASE3.md b/eventing/agentdocs/DESIGN_PHASE3.md index c958fd7b..176306d0 100644 --- a/eventing/agentdocs/DESIGN_PHASE3.md +++ b/eventing/agentdocs/DESIGN_PHASE3.md @@ -2,7 +2,7 @@ Status: draft (revision 1) — **partly implemented.** §9's steps 1-4 are in the tree: T1 (`shared/tenancy.py`), T2 (`TopicSet` threaded through both services), T3 (`Caller`, -the registry, `ce_userkey`), T4 (`SIGNED_ATTRS` += `userkey`, `depth`), T13 (`AgentSpec`) +the registry, `ce_userkey`), T4 (`SIGNED_ATTRS` += `userkey`, `depth`, `agent`), T13 (`AgentSpec`) and T22 (the Secret-vs-ConfigMap test). **T5 is also in** — per-user stores (`store_registry.py`), the global `correlationid → userkey` index (`owner_index.py`), and the `Minter` uniqueness check that replaces the startup seeding loop (§2.6). @@ -340,7 +340,7 @@ deployment worse than an unsigned one because an operator believes it is atteste response is written into and which ntfy topic it is announced on, so an unsigned, mutable `userkey` would let anything with write access to a topic file an event into another user's history. See §8.3 on the canonicalisation break that adding it causes, -and why it has to land with `depth` in one change rather than two. +and why it has to land with `depth` and `agent` in one change rather than three. **The trap.** It is tempting to salt the session derivation — `uuid5(NAMESPACE, userkey + "/" + corr)` — so two tenants can never collide in @@ -400,8 +400,10 @@ The brief's shape, with `{userid}` replaced by `{userkey}` for the reasons in § unchanged. The two topics this table adds have no Phase 2 predecessor to stay compatible with, so they are derived from the prefix instead: `{prefix}-events` and `{prefix}-dead`. -`{prefix}` defaults to the namespace (`kev1`), configurable as `EB_TOPIC_PREFIX`, -exactly as Phase 1 §5 prefixes the shared-broker topics today. It is validated as +`{prefix}` defaults to `kev1`, configurable as `EB_TOPIC_PREFIX`. It is not derived +from the namespace: a literal default keeps the names identical to Phase 1's shipped +`kev1-` topics regardless of the namespace the chart lands in, and it matches +§8.1's row. It is validated as `[a-zA-Z0-9-]{1,32}`: it is the one component of these names `_slug` never sees, and `.` or `_` in it reproduces precisely the JMX metric collision §3.1's naming rules below warn about. Longest realistic @@ -571,10 +573,14 @@ Three things this buys that are worth naming: the ownership check on transcript writes possible at all (§6.3). `ER_USERKEY` is the new required variable in multi-tenant mode, and the runner -**refuses to start without it** when `REQUEST_TOPIC` does not equal the bridge's -single-tenant default. A runner that stamps no `userkey` on its responses produces -events EventBridge cannot file, and a silent default here would route one user's -output into another user's store. +**refuses to start without it** when `REQUEST_TOPIC` is a per-user topic +(`{prefix}-{userkey}-requests`, §3.1) whose embedded userkey it does not name. A +runner that stamps no userkey on its responses produces events EventBridge cannot +file, and a silent default here would route one user's output into another user's +store. The refusal deliberately does not key on "REQUEST_TOPIC is not the +single-tenant default": single-tenant deployments rename the default freely (the +shipped Phase 1 configmap sets `kev1-requests`), and a renamed default is not a +tenancy signal. ### 3.4 KEDA: one ScaledObject per user @@ -1948,8 +1954,8 @@ whose depth is `>= EB_TRIGGER_MAX_DEPTH` (default **3**) and records a `trigger.dropped` with reason `depth-exceeded`. `depth` joins `signing.SIGNED_ATTRS`, and it must: an unsigned counter can be reset to -zero in flight, which turns the whole control off. See §8.3 — this is the second -canonicalisation-breaking change in Phase 3 and it has to land together with `userkey`. +zero in flight, which turns the whole control off. See §8.3 — it is one of the three +canonicalisation-breaking attributes of Phase 3, and they land in one change. **2. Responses are not trigger-eligible by default.** `EB_TRIGGER_ON_RESPONSES=false`. Agent chaining — "when the triager finishes, run the fixer" — is a legitimate and @@ -2040,7 +2046,7 @@ upgrades and changes nothing behaves exactly as it did in Phase 2. | Variable | Default | Effect | |---|---|---| -| `ER_USERKEY` | empty | Which tenant this runner serves (§3.3). Stamped on every response as `ce_userkey`. Required once `REQUEST_TOPIC` is not the default. | +| `ER_USERKEY` | empty | Which tenant this runner serves (§3.3). Stamped on every response as `ce_userkey`. Required once `REQUEST_TOPIC` is a per-user topic that names a different userkey. | | `ER_AGENT_DIR` | `/etc/rossoctl/agents` | Where baked `AgentSpec`s live (§5.2). | | `ER_AGENT_NAME` | `default` | Fallback agent when neither the trigger nor the registry names one. | | `ER_SKILL_SOURCES` | empty | Allowed source prefixes. **Empty means no fetching at all** (§5.4 gate 1). | @@ -2093,7 +2099,7 @@ The dependency order is real — several steps are unsafe before the one above t 3. **The registry and `Caller`** (§2.4, §2.5), still in `single` mode. `ce_userkey` is stamped on events but nothing routes on it — the Phase 2 §4.4 "audit before enforce" pattern, applied to tenancy. -4. **`SIGNED_ATTRS` += `userkey`, `depth`** (§8.3). One change, both services. +4. **`SIGNED_ATTRS` += `userkey`, `depth`, `agent`** (§8.3). One change, both services. 5. **Per-user stores** (§6.1) and **owner-scoped reads** (§6.2), behind `EB_TENANCY_MODE=multi`. Now `multi` is usable by one real tenant. 6. **Transcript auth** (§6.3). Must come before any second tenant exists: §6's opening @@ -2163,7 +2169,7 @@ In the spirit of Phase 2 §3, the things a demo of this must not claim: | **T1** | `shared/tenancy.py`: `userkey`, `_canonicalise`, `_slug`, `TopicSet`, `ntfy_topic`. Tests: collision cases, the GitHub case-folding agreement with `ghauth.is_allowed`, topic/DNS/ntfy legality over a corpus of adversarial identifiers. | — | | **T2** | Thread `TopicSet` through `kafka_out`, `kafka_in`, `RequestsMirror`, `GroupMirror`, `eventrunner/consume`. `single` mode only; zero behaviour change. | T1 | | **T3** | `Caller`, the registry loader, `auth.resolve` returning `userkey`; `ce.EXT_USERKEY`; stamp it on requests. | T1 | -| **T4** | `SIGNED_ATTRS` += `userkey`, `depth`, in one change, both services. | T3, T13 | +| **T4** | `SIGNED_ATTRS` += `userkey`, `depth`, `agent`, in one change, both services. | T3, T13 | | **T5** | `StoreRegistry` + per-user store files + the global `correlationid → userkey` index + lazy `Minter` seeding (§2.6). | T3 | | **T6** | `Consumer.ensure_subscribed` with the blocking assignment wait, `auto_offset_reset=earliest`, and submit-path ordering. Test the race explicitly: publish before subscribe must fail the test. | T2 | | **T7** | Owner-scoped reads: `404` not `403`, the group-list filter, and the single-tenant bypass. | T5 | diff --git a/eventing/agentdocs/IMPLEMENTATION_REPORT1.md b/eventing/agentdocs/IMPLEMENTATION_REPORT1.md index 8c9b768a..ff7839fc 100644 --- a/eventing/agentdocs/IMPLEMENTATION_REPORT1.md +++ b/eventing/agentdocs/IMPLEMENTATION_REPORT1.md @@ -172,7 +172,7 @@ Highlights, each re-measuring something the design had asserted: | Concurrency (T1.10) | ≤ partition count | peak **4** replicas for 3 correlations | | Image pull | 5.1 s cold (245 MB) | 454 ms – 2.45 s warm | | Rebalance floor (Gap A) | ~3 s | consistent with the 5.0 s total | -| Ed25519 sign / verify | not estimated | **222 ms / 227 ms** — see §6 | +| Ed25519 sign / verify | not estimated | **222 ms / 227 ms** — see §9 | --- diff --git a/eventing/agentdocs/KUBECON_NA_2026.md b/eventing/agentdocs/KUBECON_NA_2026.md index f0c22ff0..c4153460 100644 --- a/eventing/agentdocs/KUBECON_NA_2026.md +++ b/eventing/agentdocs/KUBECON_NA_2026.md @@ -774,5 +774,5 @@ been read rather than run. Confirm it before spending anything on either CNI opt | Date | Change | |---|---| | 2026-10-05 | Created. Captures the accepted abstract, status against it as of `main`, the SPIRE decision, the W1–W6 plan, and the 30-minute slide/demo shape | -| 2026-10-06 | M1 and M2 filed as #895 and #896; §5 gains a tracking table | | 2026-10-05 | Added §5, the M1-M4 MVP cut (~2 days) with sequencing, explicit cuts and the talk it supports. Corrected §4/W3: Kind's default CNI is kindnetd and does not enforce NetworkPolicy, so the `NetworkPolicy` MVP was unbuildable as written — three options tabulated, M3 recommends substituting the #888 asymmetry for the egress beat. §7's risk table and §8's observations updated; the 30-minute shape renumbered §5→§6, risks §6→§7, observations §7→§8 | +| 2026-10-06 | M1 and M2 filed as #895 and #896; §5 gains a tracking table | diff --git a/eventing/agentdocs/README_PHASE0.md b/eventing/agentdocs/README_PHASE0.md index 79dd0100..ec4071a1 100644 --- a/eventing/agentdocs/README_PHASE0.md +++ b/eventing/agentdocs/README_PHASE0.md @@ -675,7 +675,7 @@ Run the unit tests any time you want to reprove the DESIGN §4.5 contract withou ```bash .venv/bin/python -m pytest tests/ -q -# → .................... 20 passed in ~0.6s +# → .......................................................... 58 passed in 7.27s ``` The four tests in `tests/test_router.py` cover FIFO ordering per correlationid, cross-correlation parallelism, the concurrency cap, and slot re-arm after drain. diff --git a/eventing/agentdocs/README_PHASE1.md b/eventing/agentdocs/README_PHASE1.md index ade31fa8..78780b7b 100644 --- a/eventing/agentdocs/README_PHASE1.md +++ b/eventing/agentdocs/README_PHASE1.md @@ -694,9 +694,11 @@ Two limits worth stating plainly: `/continue` stay open by design — `/continue` because authenticating it would put a long-lived bearer token inside every ntfy notification; it relies on the unguessable `correlationid` as a capability URL instead. -- `submitter` is **not signed**, and Kafka is plaintext. The claim is "EventBridge - refuses unauthenticated submissions and records who it believes submitted this", - not "this event proves who submitted it." +- `submitter` is signed (it joined `signing.SIGNED_ATTRS` in Phase 2, DESIGN_PHASE2 + §4.2), and Kafka is still plaintext. The claim is "EventBridge refuses + unauthenticated submissions and records who it believes submitted this — and on a + signed request, the signature proves EventBridge asserted that name", not "this + event proves who typed it." Setting `EB_AUTH_TOKENS` will 401 the `/eventbridge` CLI, the three e2e scripts and the README `curl`s until they are taught to send the header. @@ -718,8 +720,10 @@ rm /tmp/seed.hex Signing is implemented in pure Python (`cryptography` is a C extension and banned), verified against the RFC 8032 test vectors. It is off by default partly -because the pure-Python scalar multiplication costs roughly 100 ms per -sign/verify, which is real per-event overhead — see `IMPLEMENTATION_REPORT1.md`. +because the pure-Python scalar multiplication is real per-event overhead: +IMPLEMENTATION_REPORT1.md §9 measured **222 ms to sign and 227 ms to verify** on +the reference laptop, and later hardware has measured inside a 190–230 ms band — +fine for the terminal event, wrong for every intermediate one. --- diff --git a/eventing/eventbridge/kafka_out.py b/eventing/eventbridge/kafka_out.py index 63d5b1a5..6fffbf83 100644 --- a/eventing/eventbridge/kafka_out.py +++ b/eventing/eventbridge/kafka_out.py @@ -15,7 +15,8 @@ class Producer: signed input — it cannot be swapped to impersonate another key. **Cost.** The Ed25519 here is pure Python (`cryptography` is a C extension and is - banned by §1.1) and takes ~150-200 ms per signature. `publish_request` is called + banned by §1.1) and takes 190-230 ms per signature (measured: 222 ms sign / 227 ms + verify, IMPLEMENTATION_REPORT1 §9). `publish_request` is called once per group member by `GroupService.submit_members`, in a loop, inside one HTTP request — so a 100-member batch spends ~20 s signing while the caller waits. That is accepted: a batch launch is an operator action, not a hot path. It is recorded diff --git a/eventing/eventrunner/config.py b/eventing/eventrunner/config.py index a6624916..694aba1b 100644 --- a/eventing/eventrunner/config.py +++ b/eventing/eventrunner/config.py @@ -6,6 +6,8 @@ from collections.abc import Mapping from dataclasses import dataclass +from shared import tenancy + # Env vars that carry an Anthropic credential. Their presence is what decides # whether we default to real `claude` or to mock mode — a container with no key # would otherwise spawn claude only to have it fail on auth, turning every @@ -110,9 +112,10 @@ class Cfg: # §3.3 — which tenant this runner serves. Stamped on every response as # `ce_userkey`, which is what lets EventBridge file the response in the right # store. Empty is single-tenant mode (the default, = Phase 2). Required once - # REQUEST_TOPIC is not the default: a runner that stamps no userkey produces - # events EventBridge cannot attribute, and a silent default here would route one - # user's output into another user's store. `load` enforces that, loudly. + # REQUEST_TOPIC is a per-user topic naming a different userkey: a runner that + # stamps no userkey produces events EventBridge cannot attribute, and a silent + # default here would route one user's output into another user's store. `load` + # enforces that, loudly. userkey: str = "" # §5.2 — where baked AgentSpecs live. A directory in the image, not a mount: @@ -175,14 +178,20 @@ def load() -> Cfg: cfg.userkey = (e("ER_USERKEY") or "").strip() cfg.agent_dir = e("ER_AGENT_DIR", cfg.agent_dir) cfg.agent_name = e("ER_AGENT_NAME", cfg.agent_name) - # A per-user runner is pointed at a per-user topic, so a non-default REQUEST_TOPIC - # with no ER_USERKEY means the deployment was rendered wrong. Refusing to start is - # the only safe direction: the alternative is responses EventBridge files into the - # `shared` store or drops, which looks exactly like an agent that never answered. - if cfg.request_topic != Cfg.request_topic and not cfg.userkey: + # A per-user runner is pointed at a per-user topic, so a REQUEST_TOPIC that embeds + # a userkey with no matching ER_USERKEY means the deployment was rendered wrong. + # Refusing to start is the only safe direction: the alternative is responses + # EventBridge files into the `shared` store or drops, which looks exactly like an + # agent that never answered. The check keys on the per-user topic shape + # (`{prefix}-{userkey}-requests`, §3.1) rather than on "not the default name": + # single-tenant deployments rename the default freely (the shipped K8s configmap + # sets `kev1-requests`) and renaming alone is not a tenancy signal. + embedded = tenancy.userkey_in_topic(cfg.request_topic) + if embedded is not None and embedded != cfg.userkey: raise SystemExit( - f"ER_USERKEY is required when REQUEST_TOPIC is not {Cfg.request_topic!r} " - f"(got {cfg.request_topic!r}). A runner that stamps no userkey on its " + f"ER_USERKEY is required when REQUEST_TOPIC is a per-user topic " + f"(got {cfg.request_topic!r}, whose embedded userkey is " + f"{embedded!r}). A runner that stamps no userkey on its " f"responses produces events EventBridge cannot attribute. " f"See DESIGN_PHASE3.md §3.3.") diff --git a/eventing/eventrunner/emit.py b/eventing/eventrunner/emit.py index 57735262..15ea071a 100644 --- a/eventing/eventrunner/emit.py +++ b/eventing/eventrunner/emit.py @@ -13,7 +13,8 @@ class Emitter: """§11: signs **terminal events only**, and what that does and does not prove. `emit()` runs for every `stdout` frame an agent produces, and the pure-Python - Ed25519 here costs ~150-200 ms per signature, so signing every frame would add + Ed25519 here costs 190-230 ms per signature (measured: 222 ms sign / 227 ms + verify, IMPLEMENTATION_REPORT1 §9), so signing every frame would add minutes to a chatty run. Terminal events are one per run, where the cost is invisible against an agent that already took seconds. diff --git a/eventing/shared/signing.py b/eventing/shared/signing.py index cfcf9103..98e6ed6c 100644 --- a/eventing/shared/signing.py +++ b/eventing/shared/signing.py @@ -89,7 +89,7 @@ def _scalar_mult(p: tuple[int, int], e: int) -> tuple[int, int]: strictly true: with `EB_SIGNING_KEY_PATH` set, EventBridge signs on the HTTP request path, so a caller who can time `POST /v0/agents` observes something correlated with the scalar. What still makes it acceptable is that the signal is - buried under a Kafka round trip and a ~150 ms pure-Python operation whose variance + buried under a Kafka round trip and a ~200 ms pure-Python operation whose variance dwarfs the leak, the seed never leaves the pod, and signing remains opt-in. It is a real if impractical weakness rather than a non-issue — do not promote this to a trust boundary that assumes constant time. @@ -464,7 +464,8 @@ def response_decision(event, ks, *, require: bool, **Unsigned non-terminal frames are accepted, and that is not a loophole being left open — it is the signing policy on the other side.** `emit()` signs terminal events only, because it runs for every `stdout` frame and a signature costs - ~150-200 ms; verifying all-or-nothing would rewrite every streamed frame of every + ~190-230 ms (measured: 222 ms sign / 227 ms verify, IMPLEMENTATION_REPORT1 §9); + verifying all-or-nothing would rewrite every streamed frame of every genuine run to `phase=error`. So an event that carries no signature AND is not terminal is passed through, while an unsigned **terminal** event is still refused — that is the one the transcript presents as the answer, and refusing it is the whole diff --git a/eventing/shared/tenancy.py b/eventing/shared/tenancy.py index 8d473f7b..69aca44c 100644 --- a/eventing/shared/tenancy.py +++ b/eventing/shared/tenancy.py @@ -162,8 +162,15 @@ def userkey(issuer: str, userid: str) -> str: # # The slug is at most _SLUG_MAX characters, starts with an alphanumeric (`_slug` strips # leading dashes) and contains only `[a-z0-9-]`. -USERKEY_RE = re.compile( - rf"^[a-z]{{2}}-[a-z0-9][a-z0-9-]{{0,{_SLUG_MAX - 1}}}-[0-9a-f]{{{_DIGEST_HEX}}}$") +_USERKEY_CORE = rf"[a-z]{{2}}-[a-z0-9][a-z0-9-]{{0,{_SLUG_MAX - 1}}}-[0-9a-f]{{{_DIGEST_HEX}}}" +USERKEY_RE = re.compile(rf"^{_USERKEY_CORE}$") + +# A per-user topic name, `{prefix}-{userkey}-{suffix}` (§3.1): the prefix agrees with +# EventBridge's `EB_TOPIC_PREFIX` validation and the suffixes are `TopicSet`'s four. +# `userkey_in_topic` uses it to recognise — not interpret — a per-user topic on the +# runner side, where the topic arrives as a bare string with no TopicSet in hand. +PER_USER_TOPIC_RE = re.compile( + rf"^[a-zA-Z0-9-]{{1,32}}?-(?P{_USERKEY_CORE})-(?:requests|responses|events|dead)$") def is_valid_userkey(value: str | None) -> bool: @@ -192,6 +199,30 @@ def is_valid_userkey(value: str | None) -> bool: return bool(value) and bool(USERKEY_RE.fullmatch(value)) +def userkey_in_topic(topic: str | None) -> str | None: + """The userkey a per-user topic name embeds, or `None` if it embeds none. + + A per-user topic is `{prefix}-{userkey}-{suffix}` (§3.1) and `userkey` itself + contains `-`, so the name cannot be split — the key has to be found by shape. + This is the inverse of `TopicSet.requests(userkey)` and lives next to + `is_valid_userkey` for the same reason: this module is the only place an + identity becomes a name, so recognising one inside a name belongs here too. + + `None` is the answer for every single-tenant topic — including renamed defaults + like `kev1-requests` — because a renamed default is not a tenancy signal. The + caller decides what a per-user name without a matching key means; here it is + just "this is not a per-user topic". + + False negatives are safe here: the runner's refusal is a deployment guard, not + the last line of defense — EventBridge's `StoreRegistry.for_event` is what + actually files or rejects a response, and it counts rather than guesses. + """ + if not topic: + return None + m = PER_USER_TOPIC_RE.fullmatch(topic) + return m.group("userkey") if m else None + + def ntfy_topic(prefix: str, userkey: str, secret: bytes) -> str: """An unguessable, stable, per-user ntfy topic that contains no identifier. diff --git a/eventing/tests/test_tenancy.py b/eventing/tests/test_tenancy.py index 947db03b..68e40866 100644 --- a/eventing/tests/test_tenancy.py +++ b/eventing/tests/test_tenancy.py @@ -238,6 +238,48 @@ def test_single_mode_inbox_and_dead_are_derived_from_the_prefix(): assert ts.dead() == "kev1-dead" +# ---- userkey_in_topic ------------------------------------------------------ + +def test_userkey_in_topic_round_trips_topicset(): + """The inverse of `TopicSet`'s derivation: whatever the topic layout produces, + `userkey_in_topic` must recognise the key back out of it.""" + uk = T.userkey("github", "Mrsabath") + ts = T.TopicSet("kev1", tenancy=T.MULTI) + for method in (ts.requests, ts.responses, ts.events, ts.dead): + assert T.userkey_in_topic(method(uk)) == uk + + +@pytest.mark.parametrize("suffix", ["requests", "responses", "events", "dead"]) +def test_userkey_in_topic_recognises_each_suffix(suffix): + uk = T.userkey("oidc", "alice@example.com") + assert T.userkey_in_topic(f"kev1-{uk}-{suffix}") == uk + + +def test_userkey_in_topic_returns_none_for_single_tenant_names(): + """A renamed default is not a tenancy signal — this is the case the runner's + old refusal got wrong, refusing to boot `k8s/base/configmap.yaml`'s + `REQUEST_TOPIC: kev1-requests`.""" + assert T.userkey_in_topic("requests") is None + assert T.userkey_in_topic("kev1-requests") is None + assert T.userkey_in_topic("kev1-kev1-requests") is None + assert T.userkey_in_topic("") is None + assert T.userkey_in_topic(None) is None + + +def test_userkey_in_topic_requires_a_per_user_suffix(): + uk = T.userkey("github", "Mrsabath") + assert T.userkey_in_topic(f"kev1-{uk}") is None + assert T.userkey_in_topic(f"kev1-{uk}-other") is None + + +def test_userkey_in_topic_rejects_names_that_are_not_userkey_products(): + """The embedded component must have the `userkey()` shape — anything else is a + coincidence of dashes, not a tenant.""" + assert T.userkey_in_topic("kev1-shared-requests") is None + assert T.userkey_in_topic("kev1-GH-MRSABATH-4C1D9E07-requests") is None + assert T.userkey_in_topic("kev1-gh-alice-notahex-requests") is None + + # ---- ntfy_topic ------------------------------------------------------------ SECRET = b"a-test-ntfy-topic-secret" diff --git a/eventing/tests/test_topicset_wiring.py b/eventing/tests/test_topicset_wiring.py index ed3aec24..b077c7c5 100644 --- a/eventing/tests/test_topicset_wiring.py +++ b/eventing/tests/test_topicset_wiring.py @@ -175,7 +175,7 @@ def test_the_emitter_omits_the_userkey_in_single_mode(): assert ce.EXT_USERKEY not in _sent_event(em._prod).attrs -def test_the_runner_refuses_a_non_default_request_topic_without_a_userkey(monkeypatch): +def test_the_runner_refuses_a_per_user_topic_without_a_userkey(monkeypatch): """§3.3: a runner that stamps no userkey produces events EventBridge cannot attribute, and a silent default would route one user's output into another's store.""" from eventrunner import config as ercfg @@ -185,6 +185,27 @@ def test_the_runner_refuses_a_non_default_request_topic_without_a_userkey(monkey ercfg.load() +def test_the_runner_refuses_a_per_user_topic_with_a_mismatched_userkey(monkeypatch): + """The topic embeds one tenant; ER_USERKEY names another. Attributing the + responses to the named one would file them in the wrong store — refuse.""" + from eventrunner import config as ercfg + monkeypatch.setenv("REQUEST_TOPIC", f"kev1-{UK}-requests") + monkeypatch.setenv("ER_USERKEY", "gh-alice-00000000") + with pytest.raises(SystemExit, match="ER_USERKEY is required"): + ercfg.load() + + +def test_the_runner_accepts_a_renamed_single_tenant_topic_without_a_userkey(monkeypatch): + """`k8s/base/configmap.yaml` ships `REQUEST_TOPIC: kev1-requests` for the + Phase 1 single-tenant deployment, with no ER_USERKEY. Renaming the default is + not a tenancy signal; the old refusal — any non-default topic — refused to boot + exactly this deployment.""" + from eventrunner import config as ercfg + monkeypatch.setenv("REQUEST_TOPIC", "kev1-requests") + monkeypatch.delenv("ER_USERKEY", raising=False) + assert ercfg.load().userkey == "" + + def test_the_runner_accepts_a_per_user_topic_with_a_userkey(monkeypatch): from eventrunner import config as ercfg monkeypatch.setenv("REQUEST_TOPIC", f"kev1-{UK}-requests") From 370ef6b93f0575821da203c2cfa636cf65de984b Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Tue, 6 Oct 2026 23:25:23 -0400 Subject: [PATCH 2/4] fix(eventing): name the mismatch case in the ER_USERKEY refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The §3.3 refusal covers two distinct faults but reported both as "ER_USERKEY is required": an unset variable, and one that disagrees with the userkey embedded in REQUEST_TOPIC. For the second the message is wrong — the variable *was* supplied, it just names another tenant — and an operator told a variable they set is missing looks in the wrong place. The exit now names which fault it is and, on a mismatch, prints both the supplied key and the embedded one, so a mis-rendered deployment is diagnosable from the exit line without reading the chart. Behaviour is unchanged: the same four inputs boot or refuse exactly as before, only the text differs. The mismatch test asserts the new wording, that "is required" is absent, and that both keys appear. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- eventing/eventrunner/config.py | 16 ++++++++++++---- eventing/tests/test_topicset_wiring.py | 13 +++++++++++-- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/eventing/eventrunner/config.py b/eventing/eventrunner/config.py index 694aba1b..a3da4f6e 100644 --- a/eventing/eventrunner/config.py +++ b/eventing/eventrunner/config.py @@ -188,11 +188,19 @@ def load() -> Cfg: # sets `kev1-requests`) and renaming alone is not a tenancy signal. embedded = tenancy.userkey_in_topic(cfg.request_topic) if embedded is not None and embedded != cfg.userkey: + # Two distinct faults share this exit, and the message names which one: an + # unset ER_USERKEY (the runner stamps nothing) and one that disagrees with the + # topic (the runner stamps the wrong tenant). Saying "is required" for the + # second would be wrong — it was supplied, it just names someone else. + problem = ("ER_USERKEY is required when REQUEST_TOPIC is a per-user topic" + if not cfg.userkey else + f"ER_USERKEY must match the userkey embedded in REQUEST_TOPIC, " + f"but names {cfg.userkey!r}") raise SystemExit( - f"ER_USERKEY is required when REQUEST_TOPIC is a per-user topic " - f"(got {cfg.request_topic!r}, whose embedded userkey is " - f"{embedded!r}). A runner that stamps no userkey on its " - f"responses produces events EventBridge cannot attribute. " + f"{problem} (got {cfg.request_topic!r}, whose embedded userkey is " + f"{embedded!r}). A runner that stamps no userkey — or the wrong one — on " + f"its responses produces events EventBridge cannot attribute, or files " + f"them in another user's store. " f"See DESIGN_PHASE3.md §3.3.") base = e("TMPDIR", "/tmp").rstrip("/") diff --git a/eventing/tests/test_topicset_wiring.py b/eventing/tests/test_topicset_wiring.py index b077c7c5..000a9879 100644 --- a/eventing/tests/test_topicset_wiring.py +++ b/eventing/tests/test_topicset_wiring.py @@ -187,12 +187,21 @@ def test_the_runner_refuses_a_per_user_topic_without_a_userkey(monkeypatch): def test_the_runner_refuses_a_per_user_topic_with_a_mismatched_userkey(monkeypatch): """The topic embeds one tenant; ER_USERKEY names another. Attributing the - responses to the named one would file them in the wrong store — refuse.""" + responses to the named one would file them in the wrong store — refuse. + + The message must not say "is required": it *was* supplied, it just names + someone else, and an operator told a set variable is missing looks in the + wrong place. Both the supplied and the embedded key are named so the + rendering bug is diagnosable from the exit line alone.""" from eventrunner import config as ercfg monkeypatch.setenv("REQUEST_TOPIC", f"kev1-{UK}-requests") monkeypatch.setenv("ER_USERKEY", "gh-alice-00000000") - with pytest.raises(SystemExit, match="ER_USERKEY is required"): + with pytest.raises(SystemExit, match="ER_USERKEY must match") as exc: ercfg.load() + msg = str(exc.value) + assert "is required" not in msg + assert "gh-alice-00000000" in msg # what the operator set + assert UK in msg # what the topic says it should be def test_the_runner_accepts_a_renamed_single_tenant_topic_without_a_userkey(monkeypatch): From 2e94bcb0caa3277aabf493db96a6da026066d07c Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Wed, 7 Oct 2026 10:32:09 -0400 Subject: [PATCH 3/4] =?UTF-8?q?docs(eventing):=20address=20review=20of=20#?= =?UTF-8?q?900=20=E2=80=94=20unambiguous=20topic-key=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review: #900, three comments — one must-fix, one suggestion, one nit. - must-fix: `userkey_in_topic`'s split is ambiguous, and the non-greedy quantifier picked the wrong end — a legal dashed prefix (`prod-eu-…`) made a correctly-configured runner refuse to boot; the greedy flip moves the failure to multi-dash slugs, which userkey()'s own docstring shows are ordinary. The grammar cannot be fixed by quantifier, so the guard no longer decomposes the name at all: `userkey_in_topic` keeps only the yes/no per-user question (every valid split means per-user), its docstring now forbids using the captured key for identity, and a new `tenancy.topic_names_userkey(topic, userkey)` anchors on the key the runner already holds — `-{userkey}-{suffix}`, no split. All eight of the reviewer's cases pass, plus dashed-prefix and multi-dash-slug boot tests, and the dashed-prefix case he asked for in test_userkey_in_topic_*. - suggestion (taken): the inverse misconfiguration — ER_USERKEY set while REQUEST_TOPIC is a shared topic — now refuses too. emit.py keys its stamp on userkey presence, not tenancy mode, so that runner would have consumed the shared topic and filed every response as one tenant's: the harm §3.3 names, from the other direction. ER_USERKEY is also now shape-checked (`is_valid_userkey`) at boot — a typo'd key names nobody and StoreRegistry counts every stamped response `unattributed`, and a topic rendered from the same typo'd key anchors against it consistently, so only the shape check catches it. - nit: §3.1's EB_TOPIC_PREFIX paragraph reflowed to the file's width. Rebased onto 370ef6b, which split the refusal message by fault ("is required" for unset, "must match" for a wrong key). That split is preserved and carried into the anchored check: the message names the fault and the supplied key, and REQUEST_TOPIC rides in the message because the anchoring check does not extract an embedded key to print. DESIGN_PHASE3 §3.3 and §8.2 describe the two-directional refusal and the anchoring rationale; the Cfg docstring matches. eventing suite: 915 passed, 7 skipped (897 baseline; +18 across the three commits). ruff check / format --check: clean, CI-exact invocation. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- eventing/agentdocs/DESIGN_PHASE3.md | 44 ++++++++++------ eventing/eventrunner/config.py | 69 ++++++++++++++++++-------- eventing/shared/tenancy.py | 52 ++++++++++++++----- eventing/tests/test_tenancy.py | 40 +++++++++++++++ eventing/tests/test_topicset_wiring.py | 46 +++++++++++++++++ 5 files changed, 202 insertions(+), 49 deletions(-) diff --git a/eventing/agentdocs/DESIGN_PHASE3.md b/eventing/agentdocs/DESIGN_PHASE3.md index 176306d0..4d0261b5 100644 --- a/eventing/agentdocs/DESIGN_PHASE3.md +++ b/eventing/agentdocs/DESIGN_PHASE3.md @@ -402,13 +402,12 @@ with, so they are derived from the prefix instead: `{prefix}-events` and `{prefi `{prefix}` defaults to `kev1`, configurable as `EB_TOPIC_PREFIX`. It is not derived from the namespace: a literal default keeps the names identical to Phase 1's shipped -`kev1-` topics regardless of the namespace the chart lands in, and it matches -§8.1's row. It is validated as -`[a-zA-Z0-9-]{1,32}`: it is the one component of these names `_slug` never sees, and `.` -or `_` in it reproduces precisely the JMX metric collision §3.1's naming rules below warn -about. Longest realistic -name: `kev1-` + 2 + 1 + 24 + 1 + 8 + `-responses` = 51 characters, against Kafka's -249 — comfortable, and worth having checked rather than assumed. +`kev1-` topics regardless of the namespace the chart lands in, and it matches §8.1's +row. It is validated as `[a-zA-Z0-9-]{1,32}`: it is the one component of these names +`_slug` never sees, and `.` or `_` in it reproduces precisely the JMX metric collision +§3.1's naming rules below warn about. Longest realistic name: `kev1-` + 2 + 1 + 24 + +1 + 8 + `-responses` = 51 characters, against Kafka's 249 — comfortable, and worth +having checked rather than assumed. Two naming rules that bite if ignored: @@ -573,14 +572,27 @@ Three things this buys that are worth naming: the ownership check on transcript writes possible at all (§6.3). `ER_USERKEY` is the new required variable in multi-tenant mode, and the runner -**refuses to start without it** when `REQUEST_TOPIC` is a per-user topic -(`{prefix}-{userkey}-requests`, §3.1) whose embedded userkey it does not name. A -runner that stamps no userkey on its responses produces events EventBridge cannot -file, and a silent default here would route one user's output into another user's -store. The refusal deliberately does not key on "REQUEST_TOPIC is not the -single-tenant default": single-tenant deployments rename the default freely (the -shipped Phase 1 configmap sets `kev1-requests`), and a renamed default is not a -tenancy signal. +**refuses to start** on a mismatch between it and `REQUEST_TOPIC`, in either +direction: a per-user topic (`{prefix}-{userkey}-requests`, §3.1) the key does not +name, and — the inverse, which is just as wrong — a key set while `REQUEST_TOPIC` +is not that user's per-user topic. In the inverse direction the runner would +consume the shared topic while stamping `ce_userkey` on every response (the stamp +keys on the key's presence, not on tenancy mode), filing every response as one +tenant's. A runner that stamps no userkey on its responses produces events +EventBridge cannot file, and a silent default here would route one user's output +into another user's store. The key must also have `userkey()`'s shape +(`is_valid_userkey`): a typo'd key names nobody, and StoreRegistry counts every +stamped response `unattributed`. + +The match itself anchors on the key the runner holds rather than decomposing the +topic name: `{prefix}-{userkey}-{suffix}` is an ambiguous grammar — dashes are +legal in both the prefix and the slug inside `userkey` — so any split picks an +arbitrary answer (`prod-eu-gh-alice-…-requests` splits shortest-first and embeds +the wrong key), and a split-based check would refuse a correctly-configured +deployment with a dashed prefix. `topic_names_userkey` needs no split. The refusal +deliberately does not key on "REQUEST_TOPIC is not the single-tenant default" +either: single-tenant deployments rename the default freely (the shipped Phase 1 +configmap sets `kev1-requests`), and a renamed default is not a tenancy signal. ### 3.4 KEDA: one ScaledObject per user @@ -2046,7 +2058,7 @@ upgrades and changes nothing behaves exactly as it did in Phase 2. | Variable | Default | Effect | |---|---|---| -| `ER_USERKEY` | empty | Which tenant this runner serves (§3.3). Stamped on every response as `ce_userkey`. Required once `REQUEST_TOPIC` is a per-user topic that names a different userkey. | +| `ER_USERKEY` | empty | Which tenant this runner serves (§3.3). Stamped on every response as `ce_userkey`. Must name the tenant `REQUEST_TOPIC` serves, in either direction: a per-user topic without the matching key refuses, and a key without its per-user topic refuses too. | | `ER_AGENT_DIR` | `/etc/rossoctl/agents` | Where baked `AgentSpec`s live (§5.2). | | `ER_AGENT_NAME` | `default` | Fallback agent when neither the trigger nor the registry names one. | | `ER_SKILL_SOURCES` | empty | Allowed source prefixes. **Empty means no fetching at all** (§5.4 gate 1). | diff --git a/eventing/eventrunner/config.py b/eventing/eventrunner/config.py index a3da4f6e..4b07b4cd 100644 --- a/eventing/eventrunner/config.py +++ b/eventing/eventrunner/config.py @@ -111,11 +111,10 @@ class Cfg: # §3.3 — which tenant this runner serves. Stamped on every response as # `ce_userkey`, which is what lets EventBridge file the response in the right - # store. Empty is single-tenant mode (the default, = Phase 2). Required once - # REQUEST_TOPIC is a per-user topic naming a different userkey: a runner that - # stamps no userkey produces events EventBridge cannot attribute, and a silent - # default here would route one user's output into another user's store. `load` - # enforces that, loudly. + # store. Empty is single-tenant mode (the default, = Phase 2). Must agree with + # REQUEST_TOPIC in either direction — a per-user topic without this key, or + # this key without its per-user topic, is a deployment rendered wrong — and + # must have userkey()'s shape. `load` enforces all three, loudly. userkey: str = "" # §5.2 — where baked AgentSpecs live. A directory in the image, not a mount: @@ -176,31 +175,61 @@ def load() -> Cfg: # §3.3, §5.1-§5.2. cfg.userkey = (e("ER_USERKEY") or "").strip() + if cfg.userkey and not tenancy.is_valid_userkey(cfg.userkey): + # A key without tenancy.userkey()'s shape (--<8 hex>) could not + # have come from the registry, so it names nobody: EventBridge's + # StoreRegistry would count every stamped response `unattributed`. Catching + # the typo at boot is cheaper than finding it on /healthz after the work is + # lost — and the topic-name checks below cannot catch it, because a topic + # rendered from the same typo'd key anchors against it consistently. + raise SystemExit( + f"ER_USERKEY {cfg.userkey!r} does not have the shape tenancy.userkey() " + f"produces (--<8 hex>, e.g. gh-alice-a1b2c3d4). Check the " + f"user registry entry it was rendered from. See DESIGN_PHASE3.md §3.3.") cfg.agent_dir = e("ER_AGENT_DIR", cfg.agent_dir) cfg.agent_name = e("ER_AGENT_NAME", cfg.agent_name) - # A per-user runner is pointed at a per-user topic, so a REQUEST_TOPIC that embeds - # a userkey with no matching ER_USERKEY means the deployment was rendered wrong. + # A per-user runner is pointed at a per-user topic. Both directions of a mismatch + # between the two are a deployment rendered wrong, and both refuse to start: + # - a per-user REQUEST_TOPIC the configured ER_USERKEY does not name — the + # responses would be stamped for a tenant whose topic this is not; + # - ER_USERKEY set while REQUEST_TOPIC is NOT a per-user topic — the runner + # would consume the shared topic and stamp `ce_userkey` on every response + # (`emit.py` keys the stamp on userkey presence, not on tenancy mode), filing + # every response into one tenant's store. # Refusing to start is the only safe direction: the alternative is responses - # EventBridge files into the `shared` store or drops, which looks exactly like an - # agent that never answered. The check keys on the per-user topic shape - # (`{prefix}-{userkey}-requests`, §3.1) rather than on "not the default name": - # single-tenant deployments rename the default freely (the shipped K8s configmap - # sets `kev1-requests`) and renaming alone is not a tenancy signal. - embedded = tenancy.userkey_in_topic(cfg.request_topic) - if embedded is not None and embedded != cfg.userkey: + # EventBridge files into the `shared` store or another user's, which looks + # exactly like an agent that never answered. `topic_names_userkey` anchors on the + # key in hand rather than splitting the name — the `{prefix}-{userkey}-{suffix}` + # grammar is ambiguous when both prefix and slug contain dashes, so a split-based + # check false-positives on legal dashed prefixes (`prod-eu-…`). The per-user + # question itself is shape-only, and every valid split still means "per-user". + # A single-tenant deployment renaming the default (the shipped K8s configmap sets + # `kev1-requests`) is not a tenancy signal and still boots. + if tenancy.userkey_in_topic(cfg.request_topic) is not None \ + and not tenancy.topic_names_userkey(cfg.request_topic, cfg.userkey): # Two distinct faults share this exit, and the message names which one: an # unset ER_USERKEY (the runner stamps nothing) and one that disagrees with the # topic (the runner stamps the wrong tenant). Saying "is required" for the - # second would be wrong — it was supplied, it just names someone else. + # second would be wrong — it was supplied, it just names someone else. The + # topic rides in the message because it carries the key it should name: the + # anchoring check does not extract one, and the exit line stays diagnosable + # without reading the chart. problem = ("ER_USERKEY is required when REQUEST_TOPIC is a per-user topic" if not cfg.userkey else - f"ER_USERKEY must match the userkey embedded in REQUEST_TOPIC, " + f"ER_USERKEY must match the tenant REQUEST_TOPIC serves, " f"but names {cfg.userkey!r}") raise SystemExit( - f"{problem} (got {cfg.request_topic!r}, whose embedded userkey is " - f"{embedded!r}). A runner that stamps no userkey — or the wrong one — on " - f"its responses produces events EventBridge cannot attribute, or files " - f"them in another user's store. " + f"{problem} (got {cfg.request_topic!r}). A runner that stamps no " + f"userkey — or the wrong one — on its responses produces events " + f"EventBridge cannot attribute, or files them in another user's store. " + f"See DESIGN_PHASE3.md §3.3.") + if cfg.userkey and not tenancy.topic_names_userkey(cfg.request_topic, cfg.userkey): + raise SystemExit( + f"ER_USERKEY is set but REQUEST_TOPIC {cfg.request_topic!r} is not this " + f"userkey's per-user topic. A runner configured this way consumes the " + f"shared topic while stamping every response as {cfg.userkey!r} — in " + f"multi-tenant mode that files other users' responses into one user's " + f"store. Set REQUEST_TOPIC to the per-user topic, or clear ER_USERKEY. " f"See DESIGN_PHASE3.md §3.3.") base = e("TMPDIR", "/tmp").rstrip("/") diff --git a/eventing/shared/tenancy.py b/eventing/shared/tenancy.py index 69aca44c..658bfa9b 100644 --- a/eventing/shared/tenancy.py +++ b/eventing/shared/tenancy.py @@ -165,12 +165,16 @@ def userkey(issuer: str, userid: str) -> str: _USERKEY_CORE = rf"[a-z]{{2}}-[a-z0-9][a-z0-9-]{{0,{_SLUG_MAX - 1}}}-[0-9a-f]{{{_DIGEST_HEX}}}" USERKEY_RE = re.compile(rf"^{_USERKEY_CORE}$") +# TopicSet's four per-user suffixes (§3.1). One tuple because two recognisers need +# the same list: the shape regex below and `topic_names_userkey`'s anchored check. +_SUFFIXES = ("requests", "responses", "events", "dead") + # A per-user topic name, `{prefix}-{userkey}-{suffix}` (§3.1): the prefix agrees with # EventBridge's `EB_TOPIC_PREFIX` validation and the suffixes are `TopicSet`'s four. -# `userkey_in_topic` uses it to recognise — not interpret — a per-user topic on the -# runner side, where the topic arrives as a bare string with no TopicSet in hand. +# Used to recognise — not interpret — a per-user topic: see `userkey_in_topic` for why +# the split it captures cannot be trusted for identity. PER_USER_TOPIC_RE = re.compile( - rf"^[a-zA-Z0-9-]{{1,32}}?-(?P{_USERKEY_CORE})-(?:requests|responses|events|dead)$") + rf"^[a-zA-Z0-9-]{{1,32}}?-(?P{_USERKEY_CORE})-(?:{'|'.join(_SUFFIXES)})$") def is_valid_userkey(value: str | None) -> bool: @@ -200,18 +204,22 @@ def is_valid_userkey(value: str | None) -> bool: def userkey_in_topic(topic: str | None) -> str | None: - """The userkey a per-user topic name embeds, or `None` if it embeds none. - - A per-user topic is `{prefix}-{userkey}-{suffix}` (§3.1) and `userkey` itself - contains `-`, so the name cannot be split — the key has to be found by shape. - This is the inverse of `TopicSet.requests(userkey)` and lives next to - `is_valid_userkey` for the same reason: this module is the only place an - identity becomes a name, so recognising one inside a name belongs here too. + """Whether a topic name is per-user, and a key it *might* embed — ambiguous by + construction, so only the yes/no answer is reliable. + + A per-user topic is `{prefix}-{userkey}-{suffix}` (§3.1) and both `prefix` + (`[a-zA-Z0-9-]{1,32}`, dashes allowed) and the slug inside `userkey` can contain + `-`, so the grammar has several valid splits and any one regex quantifier picks + one: non-greedy prefers the shortest prefix (`prod-eu-gh-alice-ed66acf0-requests` + yields `eu-gh-alice-ed66acf0`), greedy prefers the longest (`kev1-oi-alice-ex- + ample-com-1f806a21-requests` yields the slug-only tail). Both are ordinary + configurations. **The returned key must therefore never be compared for + identity** — use `topic_names_userkey`, which anchors on the key the caller + already holds and needs no split at all. This function's value is its yes/no + answer: every valid split still means "per-user". `None` is the answer for every single-tenant topic — including renamed defaults - like `kev1-requests` — because a renamed default is not a tenancy signal. The - caller decides what a per-user name without a matching key means; here it is - just "this is not a per-user topic". + like `kev1-requests` — because a renamed default is not a tenancy signal. False negatives are safe here: the runner's refusal is a deployment guard, not the last line of defense — EventBridge's `StoreRegistry.for_event` is what @@ -223,6 +231,24 @@ def userkey_in_topic(topic: str | None) -> str | None: return m.group("userkey") if m else None +def topic_names_userkey(topic: str | None, userkey: str | None) -> bool: + """Whether `topic` is this tenant's per-user topic. No split, so no ambiguity. + + The check the runner actually needs — "does this topic belong to a tenant other + than the one I am configured as?" — does not require decomposing the name, only + matching the key already in hand against the name's end: `{prefix}-{userkey}- + {suffix}` anchored on `-{userkey}-{suffix}` can be satisfied by exactly one + key, whatever dashes the prefix and slug contain. + + The caller supplies the key, so a typo'd or forged `ER_USERKEY` answers `False` + against a real per-user topic — which is the safe direction for a boot-time + guard: refuse, loudly, and let the operator read the error. + """ + if not topic or not userkey: + return False + return any(topic.endswith(f"-{userkey}-{s}") for s in _SUFFIXES) + + def ntfy_topic(prefix: str, userkey: str, secret: bytes) -> str: """An unguessable, stable, per-user ntfy topic that contains no identifier. diff --git a/eventing/tests/test_tenancy.py b/eventing/tests/test_tenancy.py index 68e40866..ab56b24e 100644 --- a/eventing/tests/test_tenancy.py +++ b/eventing/tests/test_tenancy.py @@ -280,6 +280,46 @@ def test_userkey_in_topic_rejects_names_that_are_not_userkey_products(): assert T.userkey_in_topic("kev1-gh-alice-notahex-requests") is None +def test_userkey_in_topic_split_is_ambiguous_and_only_the_yes_no_is_reliable(): + """Dashes are legal in both the prefix and the slug, so the split a regex + quantifier picks is arbitrary: `prod-eu-…` (dashed prefix) splits shortest-first + and embeds the wrong key, and a multi-dash slug splits the other way under a + greedy quantifier. Both answers are still 'per-user', which is the only claim + the caller may rely on — identity goes through `topic_names_userkey`.""" + # Non-greedy picks the shortest prefix, so the captured key is wrong here… + assert T.userkey_in_topic("prod-eu-gh-alice-ed66acf0-requests") == "eu-gh-alice-ed66acf0" + # …yet the same name, anchored on the real key, is recognised as that tenant's. + assert T.topic_names_userkey("prod-eu-gh-alice-ed66acf0-requests", "gh-alice-ed66acf0") + assert not T.topic_names_userkey("prod-eu-gh-alice-ed66acf0-requests", "eu-gh-xxxxxxx") + + +def test_topic_names_userkey_matches_by_suffix_anchor(): + uk = T.userkey("github", "Mrsabath") + for suffix in ("requests", "responses", "events", "dead"): + assert T.topic_names_userkey(f"kev1-{uk}-{suffix}", uk) + assert not T.topic_names_userkey(f"kev1-{uk}-{suffix}", "gh-someone-00000000") + + +def test_topic_names_userkey_rejects_shared_and_missing_names(): + uk = T.userkey("github", "Mrsabath") + assert not T.topic_names_userkey("kev1-requests", uk) # single-tenant rename + assert not T.topic_names_userkey("requests", uk) + assert not T.topic_names_userkey("", uk) + assert not T.topic_names_userkey(None, uk) + assert not T.topic_names_userkey("kev1-requests", "") # no key in hand + assert not T.topic_names_userkey("kev1-requests", None) + assert not T.topic_names_userkey(None, None) + + +def test_topic_names_userkey_needs_the_suffix_boundary(): + """`-{userkey}-requests` is the anchor; a name merely ending in the key, or + embedding it without the suffix, must not match.""" + uk = T.userkey("github", "Mrsabath") + assert not T.topic_names_userkey(f"kev1-{uk}", uk) + assert not T.topic_names_userkey(f"kev1-{uk}-other", uk) + assert not T.topic_names_userkey(f"prefix-{uk}-requests-extra", uk) + + # ---- ntfy_topic ------------------------------------------------------------ SECRET = b"a-test-ntfy-topic-secret" diff --git a/eventing/tests/test_topicset_wiring.py b/eventing/tests/test_topicset_wiring.py index 000a9879..57498d09 100644 --- a/eventing/tests/test_topicset_wiring.py +++ b/eventing/tests/test_topicset_wiring.py @@ -204,6 +204,52 @@ def test_the_runner_refuses_a_per_user_topic_with_a_mismatched_userkey(monkeypat assert UK in msg # what the topic says it should be +def test_the_runner_accepts_a_per_user_topic_with_a_dashed_prefix(monkeypatch): + """`{prefix}-{userkey}-{suffix}` is an ambiguous grammar — dashes are legal in + both the prefix and the slug — so a split-based check false-positives on a + correctly-configured deployment with a dashed prefix: the non-greedy split of + `prod-eu-gh-alice-ed66acf0-requests` yields `eu-gh-alice-ed66acf0`. The guard + anchors on the key in hand instead (`topic_names_userkey`), which needs no + split and cannot be wrong about the one key it is asked about.""" + from eventrunner import config as ercfg + monkeypatch.setenv("REQUEST_TOPIC", "prod-eu-gh-alice-ed66acf0-requests") + monkeypatch.setenv("ER_USERKEY", "gh-alice-ed66acf0") + assert ercfg.load().userkey == "gh-alice-ed66acf0" + + +def test_the_runner_accepts_a_per_user_topic_with_a_multi_dash_slug(monkeypatch): + """The greedy fix's failure mode: slugs themselves contain dashes + (`oi-alice-example-com-9e8f7a6b` is userkey()'s own docstring example), so a + longest-prefix split is wrong the other way. The anchored check is indifferent.""" + from eventrunner import config as ercfg + monkeypatch.setenv("REQUEST_TOPIC", "kev1-oi-alice-ex-ample-com-1f806a21-requests") + monkeypatch.setenv("ER_USERKEY", "oi-alice-ex-ample-com-1f806a21") + assert ercfg.load().userkey == "oi-alice-ex-ample-com-1f806a21" + + +def test_the_runner_refuses_a_userkey_on_a_shared_topic(monkeypatch): + """The inverse misconfiguration: ER_USERKEY set while REQUEST_TOPIC is not that + user's per-user topic. `emit.py` keys its stamp on userkey presence, not tenancy + mode, so this runner would consume the shared topic and file every response as + one tenant's — the harm §3.3 names, from the other direction.""" + from eventrunner import config as ercfg + monkeypatch.setenv("REQUEST_TOPIC", "kev1-requests") + monkeypatch.setenv("ER_USERKEY", UK) + with pytest.raises(SystemExit, match="is not this userkey's per-user topic"): + ercfg.load() + + +def test_the_runner_refuses_an_invalid_userkey_shape(monkeypatch): + """A key without tenancy.userkey()'s shape names nobody — StoreRegistry counts + every stamped response `unattributed` — and a topic rendered from the same + typo'd key anchors against it consistently, so only a shape check catches it.""" + from eventrunner import config as ercfg + monkeypatch.setenv("REQUEST_TOPIC", f"kev1-{UK}-requests") + monkeypatch.setenv("ER_USERKEY", "gh-alice-typo") + with pytest.raises(SystemExit, match="does not have the shape"): + ercfg.load() + + def test_the_runner_accepts_a_renamed_single_tenant_topic_without_a_userkey(monkeypatch): """`k8s/base/configmap.yaml` ships `REQUEST_TOPIC: kev1-requests` for the Phase 1 single-tenant deployment, with no ER_USERKEY. Renaming the default is From a3af30aff93b77f8da57e9455c7673ac284b6853 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Wed, 7 Oct 2026 13:27:40 -0400 Subject: [PATCH 4/4] =?UTF-8?q?docs(eventing):=20address=20review=20of=20#?= =?UTF-8?q?900=20=E2=80=94=20stop=20asserting=20the=20ambiguous=20split?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review: #900, the remaining suggestion on the approval review — the two tests that still pinned the split for identity, the use the new userkey_in_topic docstring rules out. test_userkey_in_topic_round_trips_topicset asserted that whatever TopicSet derives, userkey_in_topic recognises the key back out of it — a round-trip contract the module disowns: it holds only because the fixture prefix "kev1" has no dash, and the ambiguity test twelve lines below asserts the opposite ("prod-eu-…" splits shortest-first). So the file asserted both "the split round-trips" and "the split is arbitrary", and a future narrowing (return None on several valid splits, or a quantifier flip) would fail these two as if regressions when they actually encode a contract the fix repudiated. Narrowed both to the claim that survives, per the reviewer's shape: - renamed to test_userkey_in_topic_recognises_topicset_names_as_per_user; recognition (is not None) plus the anchored round-trip topic_names_userkey(method(uk), uk), which holds for every prefix and slug rather than only the dash-free ones. - test_userkey_in_topic_recognises_each_suffix likewise asserts recognition plus the anchored check, with a docstring pointing at the ambiguity test. The captured key is now asserted nowhere, matching the docstring. eventing suite: 915 passed, 7 skipped. ruff check / format --check: clean, CI-exact invocation. Assisted-By: Claude (Anthropic AI) Signed-off-by: Mariusz Sabath --- eventing/tests/test_tenancy.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/eventing/tests/test_tenancy.py b/eventing/tests/test_tenancy.py index ab56b24e..eb01c3e5 100644 --- a/eventing/tests/test_tenancy.py +++ b/eventing/tests/test_tenancy.py @@ -240,19 +240,23 @@ def test_single_mode_inbox_and_dead_are_derived_from_the_prefix(): # ---- userkey_in_topic ------------------------------------------------------ -def test_userkey_in_topic_round_trips_topicset(): - """The inverse of `TopicSet`'s derivation: whatever the topic layout produces, - `userkey_in_topic` must recognise the key back out of it.""" +def test_userkey_in_topic_recognises_topicset_names_as_per_user(): + """`TopicSet`'s four per-user names are recognised as per-user. The captured key + is deliberately not asserted — see the ambiguity test below.""" uk = T.userkey("github", "Mrsabath") ts = T.TopicSet("kev1", tenancy=T.MULTI) for method in (ts.requests, ts.responses, ts.events, ts.dead): - assert T.userkey_in_topic(method(uk)) == uk + assert T.userkey_in_topic(method(uk)) is not None + assert T.topic_names_userkey(method(uk), uk) @pytest.mark.parametrize("suffix", ["requests", "responses", "events", "dead"]) def test_userkey_in_topic_recognises_each_suffix(suffix): + """Recognition only — the split's captured key is not asserted, per the + ambiguity test below.""" uk = T.userkey("oidc", "alice@example.com") - assert T.userkey_in_topic(f"kev1-{uk}-{suffix}") == uk + assert T.userkey_in_topic(f"kev1-{uk}-{suffix}") is not None + assert T.topic_names_userkey(f"kev1-{uk}-{suffix}", uk) def test_userkey_in_topic_returns_none_for_single_tenant_names():