[ignaciojaneiro]

Follow-up to #742, reworked as suggested in review.

What

• Stale comment fixed. The FactActorNode comment in api/datalog.go and §4.3 of security-architecture.md said actor_node feeds act.sub in border JWTs. It doesn't: act.sub is the peer ID of the egress node that called /sts/token, as sts.go, identity.md and the rest of security-architecture.md define it. The border JWT is unchanged.
• Origin peer in the audit log. Both Border Crossing lines at POST /sts/token now log client_peer_id next to actor_node. Member, Task and Delegated Session Biscuits all carry client_peer_id in Block 0, while only Delegated Session Biscuits carry actor_node. Audit logs can now join the origin peer and the egress node for every Biscuit kind.
  • On the allow line, client_peer_id is the origin peer.
  • On the deny line, authorizeBiscuitForEgress now returns the verified claims along with the refusal, so refused attempts name the origin too. The field is empty only when the Biscuit itself did not verify.
• What actor_node is. The FactActorNode comment and §4.3 now say what it holds for /oauth/* Biscuits: the node whose credential came with the request, or else a claimed peer (the actor_peer_id form value, or the control plane's own peer ID).
• Log keys. The Mint endpoint row says what actor_node and client_peer_id mean in each Border Crossing line. actor_node is the egress node in /sts/token lines and the exchanging node in /token/exchange lines.

Tests

• TestSTSTokenBorderCrossingNamesTheOriginPeer reads the control plane's log through a go-log pipe. It checks client_peer_id on both the allow and the deny line for a Task Biscuit attenuated from a Member Biscuit, which is the case with client_peer_id and no actor_node.
• go test -race ./internal/controlplane/ ./api/ ./internal/identity/ passes, and golangci-lint v2.14.0 is clean.

🤖 Generated with Claude Code

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gemini-code-assist]

Code Review

This pull request introduces support for nesting the origin actor node as a prior actor (act.act.sub) in border JWTs when a node requests a token using a delegated Biscuit. This change is reflected in the STS token handler, associated unit tests, and documentation. The review feedback highlights a style guide violation where raw peer ID strings are compared directly in borderJWTActClaim instead of being canonicalized first. Additionally, the reviewer noted that the unit tests must be updated to use valid, dynamically generated peer IDs to prevent decoding failures once canonicalization is implemented.

[aojea]

Thanks for digging into this and writing up #742 and this PR!

After looking closely at how POST /sts/token, Member vs. Delegated Biscuits, and /oauth/* interact here, I don't think we should nest actor_node into act.act.sub in the border JWT:

1. The doc discrepancy was just a stale comment on FactActorNode:
   sts.go, identity.md, security-architecture.md (§4.4 and §6), and google-cloud-credentials.md all intentionally define act.sub as the peer ID of the egress node that served the request. The mention of act.sub on FactActorNode in api/datalog.go and security-architecture.md §4.3 was a leftover comment from when FactActorNode was first introduced.

2. Member/Task Biscuits vs. Delegated Session Biscuits:
   In mintBiscuitWithBindingAndRand, both Member Biscuits and Delegated Session Biscuits carry client_peer_id(<origin_peer>) in Block 0; a Delegated Session Biscuit only uses actor_node(<origin_peer>) instead of node(<origin_peer>) so node:<peer> policy bindings do not leak to the delegated subject.
   When a node enrolled with an OIDC or workload JWT (or a Task Biscuit attenuated from its Member Biscuit) calls a separate egress node, claims.Principal() returns claims.Email or claims.User (not node:<peer>), while claims.ActorNodePeerID is "". Keying on claims.ActorNodePeerID drops the origin node for Member/Task Biscuits while including it for Delegated Biscuits (TestSTSTokenPriorActorComesFromActorNode).

3. Control plane /oauth/* fallback & declared actors:
   When a Biscuit is minted at the control plane's /oauth/authorize or /oauth/token without a node credential, resolveDefaultActorPeer sets actor_node to the control plane's own Ed25519 signing key peer ID (or to an unauthenticated actor_peer_id form parameter). Nesting claims.ActorNodePeerID into act.act.sub causes /sts/token to sign the control plane's signing key peer ID (or an unverified actor_peer_id) into the border JWT as if it were a prior actor node.

Suggested direction

Instead of adding act.act.sub to the border JWT (or the multi-hop chain machinery proposed in #742 Step 2 across Block 0, TaskAuthorizationRule, and the Go/TS/Python verifiers):

1. Keep act in the border JWT as {"sub": nodeRecord.PeerID}.
2. Fix the stale comment on FactActorNode in api/datalog.go and site/content/docs/contributing/security-architecture.md §4.3 (removing the mention of act.sub in outbound border JWTs).
3. In HandleSTSToken (internal/controlplane/sts.go), add "client_peer_id", claims.ClientPeerID to the Border Crossing allow log line so audit logs can be joined across the origin peer and the egress node for both Member Biscuits and Delegated Session Biscuits.

[ignaciojaneiro]

Thanks for the careful look. I agree on all three points, and I've reworked the PR to exactly that:

• act stays {"sub": nodeRecord.PeerID}.
• The stale act.sub mention is gone from the FactActorNode comment and from §4.3.
• The allow Border Crossing line at /sts/token now logs client_peer_id.

I'll take the real-time side of the question to #742.

[asivanadi0]

I read both commits at be008a5 and ran go test -race ./internal/controlplane/ ./api/ ./internal/identity/, which passes. I also put sts.go back to main with the new test kept, and TestSTSTokenBorderCrossingNamesTheOriginPeer fails on the allow line (client_peer_id = <nil>), so the test pins the change.

The contract change in authorizeBiscuitForEgress looks safe. Its only callers are HandleSTSToken and the ban test at sts_test.go:738, which discards the claims. The deny line can't log an unvalidated peer string either: InspectVerifiedBiscuit already rejects a client_peer_id that doesn't decode (canonicalPeer, internal/identity/biscuit.go:958), so whatever reaches the log is the canonical form, or empty when the Biscuit has none.

One doc suggestion inline, plus a note for #742 that is out of scope here.

Not for this PR, just a note for #742. The served_by refusal (sts.go:1025) and the audience refusal (:1036) return 403 before authorizeBiscuitForEgress and write no Border Crossing line, so an audit join on deny lines won't see those attempts. The new table text ("on deny once the Biscuit has verified") is accurate as written, because both happen before verification, so nothing needs to change here.

[asivanadi0]

Since this comment is now the place that says what actor_node is for, it should also mention that it's one of the two authority facts the request binding checks. For a Delegated Session Biscuit, identity.RequireAuthorityRequestBinding (internal/identity/biscuit.go:247) accepts a token without node() only when both actor_node(peer) and client_peer_id(peer) match the presenting peer, and /sts/token runs it at sts.go:1201. "It grants nothing" stays true for Datalog policy, but someone reading "for audit logs and for ext_proc attributes" could fairly conclude the fact is safe to drop. Maybe something like:

// ... It is for audit logs, for ext_proc attributes["mesh"].actor_node, and,
// together with client_peer_id, for the authority-block request binding
// (identity.RequireAuthorityRequestBinding). No baseline rule and no policy
// binding reads it, so it grants nothing.

The same clause would fit the actor_node bullet in security-architecture.md (line 309).

[ignaciojaneiro]

Good point. Done in 2b6fee0: both the FactActorNode comment and the §4.3 bullet now say that actor_node, together with client_peer_id, is how identity.RequireAuthorityRequestBinding binds a token without node() to the presenting peer. They also say that no Datalog rule reads it, so it still grants nothing.

[ignaciojaneiro]

@asivanadi0 thanks for the review, and for putting sts.go back to main to check that the test pins the change. The binding clause is in 2b6fee0.

Agreed on the served_by and audience refusals: they return 403 before the Biscuit is verified and write no Border Crossing line. I'll note it in #742.

[asivanadi0]

Re-checked at 2b6fee0. The new commit only touches comments and docs. It adds the request-binding clause to the FactActorNode comment and to the actor_node bullet in §4.3, and both now match what identity.RequireAuthorityRequestBinding does. go test -race ./api/ ./internal/controlplane/ still passes. LGTM.

[aojea]

Thanks