picatz / picatz/flowstate

auth: one bearer admission path, with surface parity, a single challenge builder, and the proto-first policy question

Open
#896 5 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

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

Description

Flowstate now admits a bearer token on two surfaces, and they were built at different times against different obligations. Most of what each does is right. What is not right is that they are two, and the places where they differ are not differences anyone chose.

Verified against origin/main at 98ca34ce124c024d5a5f4cbaf75c5615d790af7b. Three findings, at three different stages: one in flight, one that will break on a scheduled change, one that is a standing design question for the owner.

1. Surface parity: two checks the MCP surface performs and the Connect surface does not (in flight)

MCPTokenVerifier (pkg/flowstate/v1/auth/mcpverifier.go:99) wraps the shared Verifier with two additional refusals:

  • Delegation claims are refused outright. refusedDelegationClaim (pkg/flowstate/v1/auth/mcpverifier.go:205) rejects a token carrying RFC 8693 act or may_act (:44, :47). The reasoning at :21-41 is exactly right and is the fail-closed rule: admitting such a token as its bare sub would let a request proceed while the audit record says nothing was delegated.
  • The audience is narrowed to this specific resource. principal.HasAudience(resource) at :129 checks the token against the MCP resource identifier, on top of the Verifier's own check. The doc at :52-60 explains why that is not redundant: a TrustedIssuer entry lists every audience that issuer may mint, so an issuer serving both a general API audience and an MCP resource would otherwise let a token minted for one be spent at the other.

Neither refusal exists on the Connect RPC path. grep for may_act across the non-test Go in pkg/flowstate/v1/auth/ returns mcpverifier.go and nothing else, and Authenticator.Authenticate (pkg/flowstate/v1/auth/connect.go) performs no per-RPC audience narrowing. So the same token this deployment refuses at the MCP door is admitted at the RPC door, and both arguments above apply verbatim to the RPC surface: a delegation claim nobody interprets is the confused-deputy shape whichever port it arrives on.

Status: being fixed now, on claude/bearer-surface-unification. Recorded here so the finding is durable and so items 2 and 3 have their context; not an open ask. The branch was not yet on origin when this was written, so treat the name as in-flight rather than as a cite.

2. Two WWW-Authenticate builders, and the change that will make them disagree

The 401 challenge is constructed in two places today:

  • Hand-built, on the Connect surface. Authenticator.unauthenticated (pkg/flowstate/v1/auth/connect.go:219-227) assembles Bearer error="invalid_token" and appends resource_metadata=%q when a protected-resource metadata URL is configured.
  • By the SDK, on the MCP surface. mcpauth.RequireBearerToken with ResourceMetadataURL set (cmd/flow/mcpserve.go:380-386) emits the challenge itself.

Both currently omit scope, and both do so deliberately: the doc at connect.go:216-219 says so in as many words, and mcpserve.go:384 carries the matching comment. So they agree today by both being empty in the same place, which is agreement by coincidence of a deferral, not by construction.

That deferral has an end date. #567's D1 is the action/scope vocabulary decision, and docs/MCP_AUTHORIZATION.md (## What this deliberately does not do (yet)) records that scopes_supported and the scope challenge parameter are both waiting on it. The day D1 lands, someone adds a scope parameter to one of these two builders, and the other keeps answering without one. A client doing step-up then sees a different challenge depending on which port it hit.

The fix is one challenge builder, and it belongs on ProtectedResource (pkg/flowstate/v1/auth/protectedresource.go:66), which is already the single object that holds the resource identifier (:168), its metadata URL (:190), and the policy it was validated against (:92). It is the derived view the house rule prefers: the challenge would be computed from the one configured resource rather than declared twice.

Illustrative, not the landed shape:

// Challenge renders the RFC 6750 challenge for this resource. Both the Connect
// authenticator and the MCP bearer middleware render through this, so a scope
// vocabulary added once cannot reach one surface and miss the other.
func (p *ProtectedResource) Challenge(err error, required ...string) string

The cost: the MCP SDK builds its own challenge from RequireBearerTokenOptions, so matching it means either passing our scope set into the SDK's options and asserting the rendered bytes in a test, or handling 401 ahead of the SDK middleware. Neither is free, and that is the argument for gating this on D1 rather than doing it now: refactoring two builders into one while both correctly render the same empty thing buys nothing, and the requirement is not yet known.

3. The standing question: auth policy is hand-written Go while the schema holds the same idea

Policy (pkg/flowstate/v1/auth/policy.go:26), TrustedIssuer (:165) and ClaimRule (:353) are hand-written Go structs with hand-written validation. The schema already holds a structured claims requirement doing the same job: SignalPolicyRule.claims (proto/flowstate/v1/signal.proto:95), a map<string, string> of exact-match claim requirements with protovalidate rules, gating an RPC.

CLAUDE.md's #726 analysis names this precisely: there are several spellings of "this claim must carry this value" in this tree, and the odd one out is usually the oldest rather than the best. Migrating auth's policy types into proto/flowstate/v1 with protovalidate would converge the claim-rule spellings, and would make the policy reference doc #726 asks for a derived view rather than a fourth hand-kept copy.

Against that: it is an L, multi-PR change to the package where every authentication decision is made, and the cost is monotonically increasing. #893 alone adds 152 lines to policy.go (pkg/flowstate/v1/auth/policy.go +152/-0). Every such addition is more surface to migrate and more behavior a migration could silently alter, in the one package where a silent alteration is a security bug.

There is also a real argument for the status quo that should not be skipped: a Policy is loaded from an operator's YAML on the machine that serves requests and is not a thing that travels between processes, which is the boundary CLAUDE.md's proto-first section names as the exception. Whether that exception applies here is genuinely arguable, and it is the crux.

This is a direction decision, not a proposal. Nobody should start it without an answer, and nobody should keep adding to policy.go while pretending the question is not open.

Questions for the owner

  1. Policy in the schema, or staying in Go? (No recommendation offered; the boundary argument above cuts both ways and this is the owner's call. An answer either way should land as a sentence in pkg/flowstate/v1/auth/doc.go, so the next addition to policy.go is made against a written decision.)
  2. Is the single challenge builder gated on #567's D1, as recommended above? (Recommended: yes. Doing it before D1 unifies two renderings of the same empty string.)
  3. Does the parity rule get written down as a rule? That is: any new check on one bearer surface is either applied to both or carries a comment saying why it is one-sided. Cheap, and it is the mechanism that would have caught item 1 at review time instead of at audit time.

Related: #567 (the identity track and D1), #726 (the policy reference doc and the claim-rule spelling analysis), #558 (the MCP protected-resource story), #560 (what a claim may say and delegation that is checkable).


Generated by Claude Code

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 by reading pkg/flowstate/v1/auth/mcpverifier.go, connect.go, protectedresource.go, and cmd/flow/mcpserve.go, then review #567, #726, and docs/MCP_AUTHORIZATION.md. This issue is a direction decision rather than an implementation task: the owner must decide where policy belongs, whether challenge unification waits for D1, and whether the parity rule is documented in pkg/flowstate/v1/auth/doc.go.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
api, authentication, security
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.