Skip to content

acp: gate a relay-signed workflow message as its owner - #6801

Open
Ayavuzer wants to merge 2 commits into
block:mainfrom
Ayavuzer:pr/acp-workflow-gate-relay-owner
Open

acp: gate a relay-signed workflow message as its owner#6801
Ayavuzer wants to merge 2 commits into
block:mainfrom
Ayavuzer:pr/acp-workflow-gate-relay-owner

Conversation

@Ayavuzer

Copy link
Copy Markdown

Workflow messages are signed by the relay keypair, not by the person who
created the workflow. Under the default owner-only gate that makes every
scheduled message invisible to the agent: the event lands in the channel,
a human reads it, and the agent's session never sees it. Observed live —
a scheduled morning briefing drew no reply, and the same text pasted by
hand minutes later drew one immediately. Same channel, same agent, same
words; only the signer differed.

A relay-signed event carrying the workflow marker is now gated as the
workflow's owner (recovered from the first p tag). The marker alone is
not enough: a non-relay signer never resolves to the owner, so channel
members cannot forge the attribution. Unconfigured behaviour is
unchanged (fail-closed).

Four tests cover recovery, forgery, the unconfigured path, and
relay-signed events without the marker.

Workflow messages are signed by the relay keypair, not by the person who
created the workflow. Under the default owner-only gate that makes every
scheduled message invisible to the agent: the event lands in the channel,
a human reads it, and the agent's session never sees it. Observed live —
a scheduled morning briefing drew no reply, and the same text pasted by
hand minutes later drew one immediately. Same channel, same agent, same
words; only the signer differed.

A relay-signed event carrying the workflow marker is now gated as the
workflow's owner (recovered from the first p tag). The marker alone is
not enough: a non-relay signer never resolves to the owner, so channel
members cannot forge the attribution. Unconfigured behaviour is
unchanged (fail-closed).

Four tests cover recovery, forgery, the unconfigured path, and
relay-signed events without the marker.
@Ayavuzer
Ayavuzer requested a review from a team as a code owner August 25, 2026 18:02

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the symptom is convincing and the fix is the right idea — a relay-signed workflow message under an owner-only gate is invisible to the agent, and the 05:30-vs-07:13 comparison isolates the signer as the only variable. i checked the ordering contract you rely on and it holds: workflow_sink.rs:262-268 builds [p(author), h(channel), buzz:workflow] and only pushes mention p tags later at :342, so find_map over all tags does land on the author. worth citing those line numbers in the doc comment, because that's the whole load-bearing assumption.

the justification for the signer check is wrong, and that's the risky part.

The relay guards its own side the same way — handlers/event.rs pairs the tag with pubkey == relay_keypair.public_key() — and this mirrors it.

that code is not a validation. handlers/event.rs:521-526 computes is_relay_workflow_msg and uses it at :528-531 to skip workflow triggering, so a relay-signed workflow message doesn't recursively fire more workflows. it does not reject a member-submitted event carrying buzz:workflow — nothing does. the tag really is plain text anyone can attach.

your check is still safe, because it requires event.pubkey.to_hex() == signer before it looks at the tag at all. but the comment tells the next reader that the relay already filters the tag, and that's exactly the belief under which someone relaxes the signer check later. i'd rewrite it as "nothing validates this tag on ingest; the signer equality is the only thing standing between a member and owner attribution."

the trust boundary moves and it's worth stating. before, "this event is from the owner" was a signature by the owner's key. after, for one signer, it's "the relay said so". everything the configured relay signs with a buzz:workflow tag now speaks as whatever pubkey sits in its first p tag. today the only producer is workflow_sink.rs, but the relay signs other events (there's a relay-signed KIND_PRESENCE_UPDATE right there in handlers/event.rs's tests), so any future relay code path that adds that tag becomes an owner-impersonation vector without anyone touching this file. a comment at workflow_sink.rs:263 saying "the first p tag is the identity buzz-acp will gate this event as — do not reorder" would make the contract two-sided instead of one-sided.

related: BUZZ_ACP_WORKFLOW_SIGNER is an env var the operator sets to a relay pubkey. the client already knows which relay it's connected to and authenticates against it — is there a reason not to derive this from that, rather than a second, independently-settable source of truth that can silently point at a different key than the one the socket is talking to? if the env var is deliberate (e.g. to keep the feature opt-in), saying so in the doc comment would help.

one concrete thing: workflow_author calls std::env::var("BUZZ_ACP_WORKFLOW_SIGNER") per event. that's a lock plus an allocation on the ingest path for a value that cannot change during the process. reading it once into a OnceLock<Option<String>> next to the other config keeps the testable workflow_author_for_signer split you've already made — which is a good split, and the reason for it in the comment is the right reason.

- rewrote the workflow_author doc comment: the claim that the relay
  'guards its own side the same way' was wrong (handlers/event.rs's
  is_relay_workflow_msg only skips re-triggering workflows for
  relay-signed events, it never validates/rejects a member-submitted
  buzz:workflow tag) — replaced with the accurate statement that the
  signer-equality check is the only thing standing between a member and
  owner attribution
- documented the trust-boundary shift explicitly (owner-signature ->
  relay-attestation for one configured signer) and the future-owner-
  impersonation risk if another relay code path ever emits buzz:workflow
- added a matching comment at workflow_sink.rs:262 (buzz-relay) marking
  the first p tag as the identity buzz-acp gates the event as — makes the
  ordering contract two-sided instead of only documented on the reading
  side
- explained why BUZZ_ACP_WORKFLOW_SIGNER is a separate trust anchor
  instead of being derived from the connected relay
- fixed an unrelated pre-existing bug found while rewriting this: the new
  workflow_author doc comment had been spliced directly onto the end of
  author_allowed's doc comment with no separating blank line, so rustdoc
  attached the whole merged block to workflow_author and author_allowed
  had no doc comment at all. Reordered so each function owns its own doc.
- workflow_author now reads BUZZ_ACP_WORKFLOW_SIGNER once via a
  std::sync::LazyLock instead of std::env::var per event (repo
  convention: LazyLock over OnceLock when the initializer needs no
  runtime input)

Not addressed: none — all concrete points from the review are covered.

Verified: cargo clippy -p buzz-acp -p buzz-relay --all-targets -- -D
warnings clean; cargo test -p buzz-acp --lib: 805 passed, 0 failed
(env-pollution-free).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants