Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 33 additions & 12 deletions eventing/agentdocs/DESIGN_PHASE2.md
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,9 @@ knowing before a demo.

## 8. What the docs review found (added after rossoctl/rossoctl#2609)

**Scope.** §8.1–§8.4 are about **Phase 2** specifically: thirteen claims *this
**Scope.** §8.1–§8.3 are about **Phase 2** specifically: thirteen claims *this
document* made about the identity and signing controls, which do not hold in the code.
§8.4 is commentary on why they all failed the same way, and enumerates none of them.
§8.5–§8.8 are the general procedure that came out of them, and apply to a claim made in
any phase — Phase 3's design makes claims of the same shape about tenancy and the signed
attribute set, and nothing here is Phase 2 only.
Expand All @@ -458,6 +459,13 @@ document for a reader who cannot see the code. Six review rounds then checked ea
restated claim **by running it**, against `eventing/` at `d9677ddd`. Thirteen did
not hold.

The tally, because a count that cannot be checked is the same failure this section is
about: **five in §8.1** (four table rows plus the fifth, promised nowhere), **four in
§8.2**, **four in §8.3**. An earlier revision stated thirteen while enumerating eleven,
because two bullets each carried two findings in one paragraph — §8.2's capability-URL
entry covered the group list and the member list, and §8.3's SPIRE entry covered the
algorithm and the file format. They are split now, so the list reads 13.

They are recorded here rather than edited into §3 and §4.4 in place, so the reasoning
above stays readable as the record of what was intended, and this section says what
the code does. That convention is this repository's own: where a later phase supersedes
Expand All @@ -469,7 +477,7 @@ deletes the original intent loses the more useful half.
| §4.4 says | The code does | Issue |
|---|---|---|
| "A keyset alone verifies **and logs**" | Verifies and returns a verdict. Nothing is logged or counted, so the observable reject rate the two-flag rollout depends on does not exist. | [#886](https://github.com/rossoctl/examples/issues/886) |
| The verifier accepts "**only**" EventBridge's kid for group events, so an approved runner "cannot forge a `group.completed` and end a batch early" | Flags it, then applies it anyway. `kafka_in.py` rewrites `phase`/`data` but keeps `final` and `groupid`, so the rewritten event still reaches `on_group_event`. The group mirror replays group events unverified on restart. | [#885](https://github.com/rossoctl/examples/issues/885) |
| The verifier accepts "**only**" EventBridge's kid for group events, so an approved runner "cannot forge a `group.completed` and end a batch early" | Flags it, then applies it anyway. `kafka_in.py` routes on `ce.is_group_event(evt)` — the *original* event, independently of the rewrite — and the rewrite replaces only `phase`/`data`, so `type` and `groupid` survive and `on_group_event` ends the batch. The group mirror replays group events unverified on restart. | [#885](https://github.com/rossoctl/examples/issues/885) |
| Step 2: "this proves *who finished a run*" | Not once stored. `insert_response` uses `INSERT OR REPLACE` on `(correlationid, sequence)`, so a later **unsigned** frame reusing a sequence number replaces the verified terminal row. | [#885](https://github.com/rossoctl/examples/issues/885) |
| "EventBridge refuses to start with a keyset but no `EB_SIGNING_KID`" | True — but nothing checks that the kid is *in* the keyset. EventBridge starts, then flags its own group events. | [#888](https://github.com/rossoctl/examples/issues/888) |

Expand All @@ -486,10 +494,12 @@ opposite directions, and only one of them is loud.**

### 8.2 §3's "deliberately left open" is wider than stated

- **§3.1's capability-URL argument rests on ids being unguessable.** `GET /v0/groups`
returns the 100 most recent groups without sign-in, and `GET /v0/groups/{id}/status`
lists every member's correlation id. For any conversation in a group, the capability
is published. ([#887](https://github.com/rossoctl/examples/issues/887))
- **§3.1's capability-URL argument rests on ids being unguessable, and the group list
publishes them.** `GET /v0/groups` returns the 100 most recent groups without sign-in.
([#887](https://github.com/rossoctl/examples/issues/887))
- **The member list publishes them too.** `GET /v0/groups/{id}/status` lists every
member's correlation id, so for any conversation in a group the capability is published
rather than merely guessable. ([#887](https://github.com/rossoctl/examples/issues/887))
- **§3.2 understates what `PUT /transcript` allows.** It is the checkpoint EventRunner
**resumes from** on a cold pod, so an unauthenticated write changes what the agent
continues with. Request signing does not cover it.
Expand All @@ -499,16 +509,27 @@ opposite directions, and only one of them is loud.**
non-terminal frame without it is pushed, stored in the member's transcript and shown
on the group page; a forged terminal without it is pushed at priority 5 even with
`NTFY_GROUP_NOTIFY_ERRORS` off, and does not count the member as failed.
([#891](https://github.com/rossoctl/examples/issues/891)) — §8.5.4 is the general rule
this is an instance of, and the fix is to decide on group membership from the store
rather than on the groupid the frame supplies.

### 8.3 Four things about the keys

### 8.3 Three things about the keys
**No issue links in this subsection, deliberately.** §8.1 and §8.2 record code bugs and
every item carries the issue filed for it. These four are **documentation drift** — claims
this document made that the code never matched, or stopped matching — so the correction
*is* the fix and there is nothing to track. §8.7 says to file the issue; it is worth
saying out loud where that does not apply, since an unlinked finding otherwise reads as
an oversight.

- **§2.6 records `submitter`/`submitteriss` as unsigned.** They joined `SIGNED_ATTRS`
in §4.2, so the signature covers them. The limit that remains is a different one and
worth stating as such: a signature proves EventBridge *asserted* the name.
- **§4.3's "the verification code does not change" under SPIRE does not hold.** The
verifier accepts EdDSA only, and SPIRE issues EC or RSA keys. The keyset is also a
flat JSON map of kid to key, and `keyset.load` rejects a JWKS document outright. The
kid lookup survives; the algorithm and the file format do not.
- **§4.3's "the verification code does not change" under SPIRE does not hold, on
algorithm.** The verifier accepts EdDSA only, and SPIRE issues EC or RSA keys.
- **Nor on file format.** The keyset is a flat JSON map of kid to key, and `keyset.load`
rejects a JWKS document outright. The kid lookup survives; the algorithm and the file
format do not.
- **A flat keyset makes the asymmetric keys attribution, not restriction.** §4.4 step 5
says a flat keyset "is not enough" for group events, and pins them. The same
reasoning applies to member answers and is not drawn: EventBridge's own key is in
Expand All @@ -529,7 +550,7 @@ document is written.

### 8.5 Checking a control claim before it ships

§8.1–§8.4 are what thirteen unchecked claims cost. What follows is the procedure that
§8.1–§8.3 are what thirteen unchecked claims cost. What follows is the procedure that
came out of them: five questions to ask of any sentence that says a security control
does something, how to check it cheaply, and what a correction should preserve.

Expand Down
21 changes: 21 additions & 0 deletions eventing/k8s/overlays/kind-signed/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ patches:
# `env` does not exist on the base container (only `envFrom`), so this adds the
# whole list rather than appending to it. A trailing-slash `env/-` op would fail
# with "path does not exist" at render time.
#
# **The silent hazard is the other direction:** `op: add` on `…/env` REPLACES the
# list when one already exists. Safe here because this overlay builds on
# `../kind`, whose containers have no `env`. That is a property of this base, not
# a general guarantee — see the eventrunner patch below, where `../demo` does add
# an `env` list that a `demo-signed` overlay would silently replace. `kustomize
# build` succeeds and nothing warns either way. Whoever writes that overlay wants
# `op: add` on `env/-` per entry, against a base that already has the key.
- op: add
path: /spec/template/spec/containers/0/env
value:
Expand All @@ -83,6 +91,19 @@ patches:
# `env` does not exist on the base container (only `envFrom`), so this adds the
# whole list rather than appending to it. A trailing-slash `env/-` op would fail
# with "path does not exist" at render time.
#
# **The silent hazard is the other direction:** `op: add` on `…/env` REPLACES the
# list when one already exists. Safe here because this overlay builds on
# `../kind`, whose containers have no `env` — but `../demo` has three entries, so
# a `demo-signed` overlay built by changing only the base would silently drop
# `ER_MOCK_CLAUDE=false`, `HOME` and `CLAUDE_CONFIG_DIR`. Mock mode is *not* the
# failure: `envFrom` is a sibling field and survives, so the credential is still
# present and config.py auto-detects **real** mode ("auto: API credential
# present"). What is lost is `HOME`/`CLAUDE_CONFIG_DIR`, so claude writes session
# transcripts to the container's ephemeral layer instead of the mounted volume and
# a restart loses every resumable session on the pod — the §16 Gap B failure.
# `kustomize build` succeeds and nothing warns. Whoever writes that overlay wants
# `op: add` on `env/-` per entry, against a base that already has the key.
- op: add
path: /spec/template/spec/containers/0/env
value:
Expand Down
Loading