picatz / picatz/flowstate

audit: `Run` and `SignalWithStart` refuse a manual start under the workflow's `manual:` block after the admission ALLOW is already written, and record no DENY — the trail for a refused `manual: denied` or `allowed_principals` start reads as allowed

Open
#1,889 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

auth engine kind/bug security
Dominant language
Go
Stars
9
Forks
0
Avg merge
3h 3m
Merged PRs (30d)
509

Description

Observed behavior

At 7530b29 (origin/main is b015dc6; the one commit between them touches flowtest only), Run writes its audit record at admission, pkg/flowstate/v1/server/server.go:1317, and the comment above it says why it is written there: "this RPC's authorization question is 'may this caller start work in their own namespace' and nothing between here and there can change the answer: what follows is the specification being checked, which is a question about the file rather than about the caller."

That stopped being true when manual: landed. server.go:1428 reaches a second question about the caller, v1.CheckManualStart(workflow, manualStartPrincipal(ctx), req.Msg.GetReason()), which refuses under three rules the workflow declares (trigger.proto:112-135): denied, require_reason, and allowed_principals matched against the caller's issuer-qualified principal. A refusal is return nil, connect.NewError(connect.CodePermissionDenied, err) with no auditDeny. The request leaves one record: rpc: Run, decision: ALLOW, resource kind NAMESPACE.

SignalWithStart has the same shape at lifecycle.go:834: the admission ALLOW is written at :721, CheckManualStart refuses at :834, and nothing records it. That is a third unaudited refusal on that handler beside the two authorizeSignal refusals #1883 reports.

Two things keep this out of the existing guard. TestTheAuditSeamIsNotBypassed (auditseam_test.go:88) tracks callers of authorizeRunDecision and authorizeScheduleDecision only; CheckManualStart is a function in pkg/flowstate/v1, not a server decision function, so the seam test does not see it as a decision at all. And the tests that prove the refusal (pkg/flowstate/v1/trigger_manual_test.go, flowfile/manualtrigger_test.go) assert the refusal and nothing about records.

The assessing pass drove both manual: denied and an unsatisfied allowed_principals through the handler with a recording sink and observed exactly one record each, rpc: Run / decision: ALLOW. I confirmed the control flow from source and did not re-run the probe.

Enforcement holds. The defect is evidence: an operator asking "who tried to start the break-glass rotation and was refused" finds the refused caller recorded as allowed, the same failure #1883 describes for signal delivery, on the RPC that flow run and every MCP agent reach.

Desired outcome

A refused manual start records a DENY under the same RPC name as the admission record, before the refusal is returned, coded POLICY_DENIED (the code authorizeSignal's name-level refusal already uses, audit.proto:542) with the rule field naming which of denied, require_reason, or allowed_principals refused. The admission ALLOW stays: it is a true record of the admission decision, and this is a second decision, which is the same one-record-per-decision reading #1883 takes.

The seam test should cover it. Either CheckManualStart is called only through an audited server wrapper (authorizeManualStart) that the seam test tracks the way it tracks authorizeRunDecision, or analyzeServerSource learns that v1.CheckManualStart is a decision needing a named-caller exemption. The first is smaller and matches the pattern the file already has.

The comment at server.go:1310-1316 is rewritten so it no longer asserts that nothing after the admission record can change the answer.

Acceptance criteria

  • A test in the shape of TestSignalWithStartAuthorizesCreateAndDeliverySeparately runs Run with a recording sink under each of the three rules and asserts: one ALLOW at admission, one DENY coded POLICY_DENIED naming the rule, and nothing else. The same for SignalWithStart under denied and allowed_principals (it has no reason field, so require_reason refuses unconditionally there and that case is asserted too).
  • Under --audit-required with a failing sink, the refused caller sees the sink's failure surfaced, matching auditDeny's existing contract.
  • TestTheAuditSeamIsNotBypassed fails if a future caller reaches CheckManualStart without the audited form.
  • docs/DEPLOYMENT.md "What the trail records" lists manual-start refusals among the decisions it records.

Constraints and dependencies

  • Sibling of #1883, same fix shape, different decision. Land them together if the record shape #1733's per-action design needs is decided in one place; otherwise this one is independent and smaller.
  • The webhook bridge does not call CheckManualStart (a webhook is not a manual start), so it is out of scope here.
  • Adjacent: #1883, #1733, #1774, #1797, audit.go's one-record rule.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with pkg/flowstate/v1/server/server.go at Run and SignalWithStart, then read CheckManualStart and the existing auditDeny path. Run the manual-start tests, auditseam_test.go, and the recording-sink cases to understand the expected decision records. Done means the listed refusal rules produce the required DENY evidence, seam coverage passes, and docs/DEPLOYMENT.md describes manual-start refusals.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.