Repository navigation
Enforce single-occupancy slot pinning with human-approved moves - #16
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
3ac96c9 to
97e6c39
Compare
Review: slot pinning (verified against the branch head)Each finding below was checked against the code, not just flagged. Fixes are in progress locally. Should fix before merge
Needs a decision
Minor
🤖 Generated with Claude Code |
Addresses the verified findings from the PR #16 review: - rebind: never is enforced on the approval path. The plan gate no longer stamps a move for a never slot (new PlanGateDeps.SlotRebind), and moveApprovedPins refuses one as a backstop. - First-approval and whole-plan cards show the move. The plan's own slots now carry MovedFrom, the whole-plan renderer prints the move and the revocation, and CardLine.MovedFrom becomes a separate revocation line on the wire. - The AgentSession admission sweep runs before the finalizer is added, so a failed sweep is retried on requeue instead of skipped forever. - moveApprovedPins refuses deterministic conflicts (a never slot, or a second instance of the moving type in the same approval) before any MovePin. A grant failure after a committed move returns ErrSlotMoveCommitted, and a MUST_MATCH failure returns ErrSlotMoveDrifted, so the approver notice says what actually changed. - GrantSlots collects EnsurePin/WriteGrantsPinned errors instead of returning early, so later types and the multi-occupancy write land. - The slot occupancy lookup logs the error it used to drop. - The occupancy: single default is documented as an upgrade break (CRD UPGRADE NOTE, security model), with CRDs, install.yaml and CRD docs regenerated. mage test:unit passes. mage test:integration and mage test:e2e were not run: Docker was unavailable, so every SpiceDB-backed test failed to start. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Fixes for the review findings (1db0c42)Pushed fixes for the findings in the review above.
Not changed: scope-first ordering in TestsNew regression tests:
🤖 Generated with Claude Code |
Addresses the verified findings from the PR #16 review: - rebind: never is enforced on the approval path. The plan gate no longer stamps a move for a never slot (new PlanGateDeps.SlotRebind), and moveApprovedPins refuses one as a backstop. - First-approval and whole-plan cards show the move. The plan's own slots now carry MovedFrom, the whole-plan renderer prints the move and the revocation, and CardLine.MovedFrom becomes a separate revocation line on the wire. - The AgentSession admission sweep runs before the finalizer is added, so a failed sweep is retried on requeue instead of skipped forever. - moveApprovedPins refuses deterministic conflicts (a never slot, or a second instance of the moving type in the same approval) before any MovePin. A grant failure after a committed move returns ErrSlotMoveCommitted, and a MUST_MATCH failure returns ErrSlotMoveDrifted, so the approver notice says what actually changed. - GrantSlots collects EnsurePin/WriteGrantsPinned errors instead of returning early, so later types and the multi-occupancy write land. - The slot occupancy lookup logs the error it used to drop. - The occupancy: single default is documented as an upgrade break (CRD UPGRADE NOTE, security model), with CRDs, install.yaml and CRD docs regenerated. mage test:unit passes. mage test:integration and mage test:e2e were not run: Docker was unavailable, so every SpiceDB-backed test failed to start. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1db0c42 to
a712433
Compare
Addresses the verified findings from the PR #16 review: - rebind: never is enforced on the approval path. The plan gate no longer stamps a move for a never slot (new PlanGateDeps.SlotRebind), and moveApprovedPins refuses one as a backstop. - First-approval and whole-plan cards show the move. The plan's own slots now carry MovedFrom, the whole-plan renderer prints the move and the revocation, and CardLine.MovedFrom becomes a separate revocation line on the wire. - The AgentSession admission sweep runs before the finalizer is added, so a failed sweep is retried on requeue instead of skipped forever. - moveApprovedPins refuses deterministic conflicts (a never slot, or a second instance of the moving type in the same approval) before any MovePin. A grant failure after a committed move returns ErrSlotMoveCommitted, and a MUST_MATCH failure returns ErrSlotMoveDrifted, so the approver notice says what actually changed. - GrantSlots collects EnsurePin/WriteGrantsPinned errors instead of returning early, so later types and the multi-occupancy write land. - The slot occupancy lookup logs the error it used to drop. - The occupancy: single default is documented as an upgrade break (CRD UPGRADE NOTE, security model), with CRDs, install.yaml and CRD docs regenerated. mage test:unit passes. mage test:integration and mage test:e2e were not run: Docker was unavailable, so every SpiceDB-backed test failed to start. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
a712433 to
48f371e
Compare
Review of the fix commits (afbc1d8, b5f447d, 48f371e, 05928fe)Every finding below was checked against Security summary: none of these lets a session reach an instance nobody approved. The pin still fails closed, and every path ends with less access, not more. The weak spot is the human's consent and the information they get:
Weakens the approval guarantee1. A merged whole-plan line hides the revocation ( 2. Store errors are folded into pin refusals (
Effect: a small information leak (internal error detail and endpoints reach the model and chat users) and a misleading answer to the approver. 3. "Move committed" can name the wrong failure ( 4. A 5. Refusal advice that can't work (
"To target the committed instance" also points at the wrong instance; it should be the new target. channelsd already changes its wording for Fail-closed availability bugs (access lost, never gained)6. A failed later move drops an earlier move's new grant ( 7. Pre-move refusals block unrelated types ( 8. Later phases depend on phase 1's move ( 9. Suggestion: a simpler fix for item 8 ( Suggested orderFix items 1 and 2 first. Most of 3, 6, 7 and 8 go away if 🤖 Generated with Claude Code |
Fixes for the second-round findings (b1910e0)All nine addressed. The consent-accuracy and leak issues are fixed at the surface; 3/6/7/8/9 are gone by construction — the approved move now runs inside
|
AuthzSlot gains occupancy (single|multi, default single) and rebind (approval|never, default approval). The installer completes both before SSA apply, the same way membership is completed, so a byte-identical re-install stays a no-op. Class admission rejects a single-occupancy slot declaring more than one default. AgentSession status gains a display-only slotPins mirror type; SpiceDB remains the only enforcement source of truth.
The schema composer adds a non-expiring slot_pin relation to every slot resource definition. GrantSlots — the one funnel every slot-grant write passes through — gates single-occupancy bindings: the first bind writes the pin under a MUST_NOT_MATCH precondition, grant writes carry a MUST_MATCH guard so a bind can never race an approved move, a different instance is refused with a message routed by the slot's rebind policy, and a single call naming two instances of one single-occupancy type binds nothing for that type while other types still land. Grants keep their mandatory expiry; the pin does not expire. Tuple validation refuses tool-authored writes to slot_pin and slot_grant relations, and a guard test over pkg/, internal/ and cmd/ asserts no production write bypasses the gate. Pinned grant writes and moves advance the read-your-writes freshness floor.
…d amendment Every producer that reaches GrantSlots — defaults, extracted and observed promotion, thread seeding, trigger binding, tool approval and the plan gate — copies the slot's occupancy and rebind onto its bindings. A plan amendment that names a different instance for a filled single-occupancy slot records the displaced instance on the approval card at build time; the approved move executes atomically (the pin repoints under a MUST_MATCH guard and the displaced instance's actual grants are revoked in the same request), and the card shows the move as a move: current instance, proposed instance, and the revocation. A move whose recorded starting point no longer matches the live pin fails the approval loudly rather than moving something the approver never saw.
The runner mirrors pins onto AgentSession.status.slotPins — derived from SpiceDB reads after a successful bind, never from candidate lists, so the mirror cannot claim a pin that was never written — and oap session show renders them. Pin refusals from the promotion paths are recorded per resource type (newest wins, cleared when that type later binds or moves) and annotate the next denied tool call for that type, so the model is told the pinned instance and the route out rather than reading a bare denial as a system fault. A pin failure after a plan-gate approval tells the approver whether the slot had drifted or was already committed elsewhere, with accurate claims about what was granted.
channelsd pre-stamps the session finalizer at its create site and sweeps stale slot tuples itself before trigger and thread-seed binding, so the operator's admission sweep can never race the mint and silently delete legitimate authority; a sweep failure fails the session start rather than proceeding past a possible leak. Mint-time pin refusals record a SlotBindRefused session notice with the refusal text, with the suggested next step routed by the slot's rebind policy. A tool approval that lands on a pinned slot in a class with no plan gate is rejected naming the pinned instance and the new-session route.
Fork and restart copy grant and pin tuples verbatim — a lifecycle clone is not a new arrival and never re-enters the gate — while session teardown deletes pins alongside grants (pins never expire, so GC is the only thing that removes them). A new session's first reconcile sweeps stale slot tuples left under its name by a dead predecessor, except for fork children, whose tuples are the parent's legitimate copy and whose generated names make name reuse moot. The status webhook waives slotPins for the runner: it is a display-only mirror no authorization reads.
A scenario suite drives the full arc on the in-process harness: the first bind pins, a second instance is refused and never written, an approved amendment moves the pin and revokes the displaced instance's grants, rebind: never admits no move, and two concurrent first binds resolve to exactly one winner through the precondition rather than process ordering. A scenario bundle proves the move card on a real published approval; the existing re-point bundle keeps its re-ask coverage, and two fixtures that bind a constant workspace instance alongside the chosen repository under one resource type now declare that slot multi-occupancy, which is what they are.
The security model's slot section now describes what the code enforces: the commitment is a non-expiring pinned relationship written under an atomic precondition; a filled single-occupancy slot cannot be silently reopened — a different instance needs a human-approved plan amendment that moves the pin and revokes the old instance's access, and rebind: never admits no move. The stated limits are documented plainly: pins are per resource type, pin identity is derived-ID string equality, moving a pin requires the plan gate, and a slot binding a constant instance alongside the chosen one needs multi occupancy or split types.
Addresses the verified findings from the PR #16 review: - rebind: never is enforced on the approval path. The plan gate no longer stamps a move for a never slot (new PlanGateDeps.SlotRebind), and moveApprovedPins refuses one as a backstop. - First-approval and whole-plan cards show the move. The plan's own slots now carry MovedFrom, the whole-plan renderer prints the move and the revocation, and CardLine.MovedFrom becomes a separate revocation line on the wire. - The AgentSession admission sweep runs before the finalizer is added, so a failed sweep is retried on requeue instead of skipped forever. - moveApprovedPins refuses deterministic conflicts (a never slot, or a second instance of the moving type in the same approval) before any MovePin. A grant failure after a committed move returns ErrSlotMoveCommitted, and a MUST_MATCH failure returns ErrSlotMoveDrifted, so the approver notice says what actually changed. - GrantSlots collects EnsurePin/WriteGrantsPinned errors instead of returning early, so later types and the multi-occupancy write land. - The slot occupancy lookup logs the error it used to drop. - The occupancy: single default is documented as an upgrade break (CRD UPGRADE NOTE, security model), with CRDs, install.yaml and CRD docs regenerated. mage test:unit passes. mage test:integration and mage test:e2e were not run: Docker was unavailable, so every SpiceDB-backed test failed to start. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
moveApprovedPins' ListGrantsFor error path returned a bare error even after an earlier type's move had already committed (pin repointed, prior grants revoked), so BindApproved reported it as an ordinary failure. The approver was then told nothing was granted while access had in fact been removed — the exact case ErrSlotMoveCommitted exists to flag, and which the sibling MovePin error path already wraps. Wrap it the same way once a move landed.
A plan-scoped approval is resolved one BindApproved call per covered phase, and stampMovedFrom stamps the same pin move onto every covered phase that names the moved-to instance. A single-occupancy pin moves at most once per approval: the first phase repointed repoA->repoB, and each later phase then re-attempted the identical move with a now-stale PriorID=repoA whose MUST_MATCH drift-failed — silently dropping that phase's grant while still recording it approved, and making a re-approval drift-fail every phase. dedupeCoveredMoves keeps the move on only the first covered record per type; the rest bind same-instance against the pin the first move already set. Run before the delegable lookup, since a SlotRef's identity includes MovedFrom and clearing it afterward would leave a later phase's request ref no longer matching the delegable key looked up for it.
…rror A plain pinned refusal on the approval path — the slot is committed to a different instance and no approved move applies — was published to the approver under categories.InternalError, whose meaning is "we broke, and retrying is the remedy." It is neither: re-approving the same card cannot change it, as the notice copy itself says. Publish it under the refusal-flavored SlotBindRefused instead (same ToneDegraded weight; the category is what a surface reads to tell a fault from an answer). The fault and committed-move cases keep InternalError.
Fixes the nine findings from the review of the fix commits: - Whole-plan card: pre-scan for a move per derived-id so the merged (git/gh) line still shows the revocation when the moving type loses the dedup, instead of rendering it as a plain first-fill. - GrantSlots store failures carry a distinct ErrSlotStore fault sentinel: a transient blip joined with a real pin refusal now classifies as a retryable fault, and its raw store text no longer leaks to the model or the chat notice. - Fold the approved pin move into GrantSlots' per-type loop and delete moveApprovedPins, so one type's move failure can no longer mislabel, strand, or block a sibling type's grant. - MovePin is idempotent at the target (pin already names toID -> no-op, not drift); dedupeCoveredMoves removed, so multi-phase approvals are order-independent and every covered phase keeps its MovedFrom audit. - A rebind:never slot pinned elsewhere renders as a blocked line (new PinnedImmovable field) rather than a grantable-looking first-fill. - Pinned-refusal notice routes by rebind policy (new session for never, amendment for movable) and names the instance the approval wanted, not the committed one. allRebindNever lifted to shared authz.AllRebindNever. All three suites green: mage test:unit, test:integration, test:e2e.
b1910e0 to
4982a52
Compare
samkim
left a comment
samkim
left a comment
There was a problem hiding this comment.
Review of the latest fix commits (d8fa0b4..4982a52), focused on security and info leaks. Each finding below was reproduced with a test that asserts the correct behavior and fails at 4982a52. Three other candidates were checked and dropped: MovePin's no-op path (grant writes are pin-guarded, confirmed against real SpiceDB), card recipients (plan-gate cards resolve only agentsession#approve), and the %T writer error (both production writers implement SlotPinner).
Should fix, in priority order:
forkedFrom(and a pre-stamped finalizer) skip the admission sweep: controller.go:1188- Cross-phase moves to different targets commit half a move and report "nothing granted": slot_grant.go:371
- Fault copy claims nothing was granted after a partial grant: plangate_notice.go:90
AllRebindNeveris computed over all bindings, not the refused ones: host_approval.go:2577, backfill.go:308- A store fault on one type hides a genuine pin refusal on another: plangate_notice.go:303 and three sibling sites
- The per-phase loop publishes one failure notice per phase: host_approval.go:2577
Inline comments have the repro and the suggested fixes.
| // synchronously ahead of its mint-time binds (pipeline.go), so it never | ||
| // reaches this branch. | ||
| if !controllerutil.ContainsFinalizer(&sess, spiceboxv1alpha1.FinalizerAgentSession) && | ||
| r.SpiceDBDeleter != nil && sess.Spec.ForkedFrom == "" { |
There was a problem hiding this comment.
Security: the admission sweep can be skipped by anyone who can create the CR.
sess.Spec.ForkedFrom == "" is the only thing gating the sweep, and forkedFrom is user-written spec. Nothing checks that the named parent exists or forked this session. The field's doc says "Immutable", but no CEL rule or webhook enforces that. Slot tuples are keyed by (ns, name) with no UID, so a new session that reuses a dead predecessor's name and sets forkedFrom: anything inherits any slot_pin (never expires) or slot_grant that survived the predecessor's finalize().
Repro (envtest, -tags=integration): seed stale pin and grant tuples for ns/name, create the session with forkedFrom: "no-such-parent", then reconcile. DeleteSlotGrants is called 0 times (want 1) and the stale tuples survive. The control case with forkedFrom empty sweeps correctly. A separate update that changes spec.forkedFrom on an existing session is accepted.
Second route to the same skip: the sweep only runs when the finalizer is absent. channelsd pre-stamps it deliberately (pipeline.go:~1531, it sweeps itself before its mint-time binds), but any creator can put the finalizer in their manifest and get the same skip without touching forkedFrom.
Suggested fix:
- Gate the fork exemption on something the parent proves, not the child's spec: the parent exists in the same namespace and points back at this session (
status.supersededBy, or apendingRestart.targetSessionNamewhose marker passesrestartmarker.Verify). - Bind that proof to the child's UID, not just its name.
status.supersededByis never cleared (restart.go:86), so once child X has been deleted, a new X withforkedFrom: Pwould still match P's stalesupersededBy: Xand replay the skip. Record the child UID on the parent whenBuildChildSessioncreates it (e.g.status.supersededByUID) and compare it againstsess.UID. - Add a CEL rule making
forkedFromimmutable (add, remove and repoint all refused). - For the pre-stamped finalizer: the cleanest option is a ValidatingAdmissionPolicy that refuses a CREATE carrying the agentsession finalizer unless
request.userInfo.usernameis the channelsd service account. It runs in the apiserver and adds no webhook availability dependency. Alternatives: a channelsd-signed mint marker bound to the specific creation (with a nonce or expiry so a predecessor's annotation can't be replayed), or dropping the exemption and having channelsd wait for the operator's sweep before binding.
Exploitability is narrow (it needs direct create on AgentSession plus a tuple that survived finalize()), but this is a fail-open authz gate, and closing that window is the stated purpose of the sweep.
There was a problem hiding this comment.
Fixed in e28cd74, with follow-ups in 43aff91 and e1c8d3a. forkedFrom no longer gates the sweep and is now immutable (CEL). The only skip left is a pre-stamped finalizer. A new ValidatingAdmissionPolicy (config/admission/) lets only the operator and channelsd service accounts add that finalizer. Fork and subagent children are pre-stamped by the operator, which sweeps their name before creating them. Takeover children are not pre-stamped. channelsd now sweeps before Create, so a crash or redelivery cannot leave a session unswept. Both sweeps also delete data-slot grants. The policy test runs against envtest with impersonated users.
| // card showed nor the target (MovePin's own no-op already | ||
| // absorbed the already-at-target case). Nothing moved, nothing | ||
| // revoked; re-approving rebuilds the card from the current pin. | ||
| refusals = append(refusals, fmt.Errorf("%w: %w: execute approved slot move for %s", ErrSlotMoveDrifted, err, rt)) |
There was a problem hiding this comment.
Correctness: one approval whose phases move the same pin to different instances commits the first move, then reports "nothing was granted".
stampMovedFrom (hooks/plangate.go) stamps MovedFrom=<current pin> on every covered ref that differs from the pin, so a plan where phase 1 names repo B and phase 2 names repo C (pin = A) yields a card with {B: A, C: A}. Nothing rejects that at card build, and the only distinct-instance guard (len(grp.ids) > 1, ~L322) applies within a single BindApproved call. The approval loop (recordPlanGateDecision, host_approval.go ~L2385) calls it once per phase. The old cross-phase "two distinct move targets" check went away in d8fa0b4.
So the sequence is:
- Phase 1:
MovePin(A→B)commits, A's grants are revoked and B's grant is written. - Phase 2:
MovePin(A→C)fails its precondition,ReadPinreturns B ≠ C, and the error is wrapped here asErrSlotMoveDrifted. classifyApprovalBindFailuremaps that toapprovalFault, and the approver is told "nothing was granted… try approving once more".- Retrying rebuilds the card as B→C and revokes B.
Repro (driving the real recordPlanGateDecision loop): result was pin=repoB realMoves=1 grantWrittenOnRepoB=true, with one notice category=internal_error lead="Your approval could not be applied, so nothing was granted…". A real two-phase plan through requestPhaseApproval produces this exact card.
Suggested fix: before the per-phase BindApproved loop, collect the distinct move targets per single-occupancy type across approvedPhaseRecords(pg). If any type has more than one, refuse the whole approval before any write, with a pinned-refusal-style notice ("this plan moves to more than one instance; a session can hold only one"), not approvalFault. Optionally flag it at card build in stampMovedFrom too. Separately, a drift that follows a move this same approval committed shouldn't be classified as "nothing granted".
There was a problem hiding this comment.
Fixed in 5594096 and 7cafaf2. All covered phases are resolved before anything is written. conflictingMoveTargets refuses the whole approval when a single-occupancy type would end up on more than one instance and at least one phase moves it. Nothing is moved, granted or recorded as approved, and the approver gets a SlotBindRefused notice. That also covers one phase staying on the pin while another moves off it.
| // Recoverable-fault copy (the default): nothing bound, and re-approving may | ||
| // take. Bounded rather than "retry"/"don't retry" — one more attempt | ||
| // distinguishes a transient blip from a persistent one; a third never does. | ||
| lead := "Your approval could not be applied, so nothing was granted and the plan " + |
There was a problem hiding this comment.
The approver is told the wrong thing about what access the session holds.
The approvalFault copy says "nothing was granted and the plan has not started… nothing has been granted either way". But GrantSlots deliberately keeps going after a per-type failure ("so the remaining types still land", slot_grant.go ~L306): each single-occupancy type does its own EnsurePin/WriteGrantsPinned, and multi-occupancy types are written in a separate WriteRelationships. So when type Y's write fails with ErrSlotStore, types X and Z are already granted. The pinned-refusal copy in this same function already acknowledges that "other instances may have bound". This branch is the one that contradicts the code.
Repro: a writer where git_repo:repoB push succeeds and another type's WriteGrantsPinned returns an ErrSlotStore-wrapped error. The require that repoB was granted passes, and the notice is still internal_error with "…so nothing was granted…". The cross-phase drift case on slot_grant.go:371 lands on this same copy after a real revoke and grant.
Suggested fix: change the copy to "could not be fully applied, so some or all of the access it covers was not granted", and drop the "nothing has been granted either way" next step. If it's cheap, name the types that did and didn't bind.
There was a problem hiding this comment.
Fixed in 5594096. Bind-time faults now say the approval was recorded but could not be fully applied, and name the types that were not granted. The "nothing was granted, try once more" wording is kept only for the standing-lookup failure, where nothing is recorded and it is true.
| // only be worked in a new session — no amendment moves it — so the | ||
| // notice must route there, not to a widening that would be refused | ||
| // every time. Same split channelsd makes. | ||
| h.publishPlanGateApprovalFailed(ctx, err, kind, authz.AllRebindNever(bindings)) |
There was a problem hiding this comment.
Two issues on this line.
(a) AllRebindNever(bindings) is passed every binding in the phase, not the refused ones. The doc on AllRebindNever (slot_grant.go:175) and the nextStep parameter both say "every refused binding". With a mix (ticket rebind default binds fine, git_repo rebind never is refused because it's pinned elsewhere) the result is false, so the approver is told "an approved amendment moves the commitment to it". GrantSlots refuses that amendment every time (priorID != id && rebind == never, ~L349). channelsd has the same bug at backfill.go:308.
Repro (real driveDecision → narrowToApproved → BindApproved path, class with git_repo single/never and ticket single/default): the ticket grant is written, and NextStep is "…an approved amendment moves the commitment…" when it should say "start a new session". The channelsd publishSlotBindRefusedNotice path gives "propose an updated plan… moves the commitment" in the same scenario.
Fix: GrantSlots joins fmt.Errorf strings, so the call site can't tell which types were refused. Return a typed refusal per type, e.g. *SlotPinRefusal{ResourceType, Rebind, Reason} that unwraps to ErrSlotPinned, collect them with errors.As over the joined error, and compute the rebind-never predicate over those only. The same type fixes the store-fault masking on plangate_notice.go:303.
(b) This publishes once per covered phase. recordPlanGateDecision loops approvedPhaseRecords(pg) and calls narrowToApproved per record. Each runs its own BindApproved and publishes on failure here, and the return only exits narrowToApproved, so the loop moves on. Each notice gets a fresh newRequestID(), so nothing dedupes. Repro: one approval covering 3 phases that each name a refused pinned instance publishes 3 SlotBindRefused notices (want 1). The same structure can make notices contradict each other: phase 1 reports a committed move whose new grant failed, and a later phase's no-op move then writes that grant.
Fix: have narrowToApproved return its classified outcome instead of publishing it. Aggregate across the loop and publish once at the end. A later per-type success should supersede an earlier fault or committed-move outcome for the same type.
There was a problem hiding this comment.
Both fixed in 5594096. (a) GrantSlots returns a typed *authz.SlotBindFailure per type, and AllRebindNever now takes SlotPinRefusals(err), here and in channelsd. (b) Outcomes are folded per (type, permission) across phases and published once. A later bind of the same grant supersedes an earlier fault or committed move, and a refusal is sticky.
| return approvalMoveCommitted, true | ||
| case errors.Is(err, authz.ErrSlotMoveDrifted): | ||
| return approvalFault, true | ||
| case errors.Is(err, authz.ErrSlotStore): |
There was a problem hiding this comment.
A transient store fault on one type hides a genuine pin refusal on another.
GrantSlots returns errors.Join of all per-type outcomes, so one call can carry an ErrSlotPinned for type X (pinned elsewhere, permanent) and an ErrSlotStore for type Y (EnsurePin timeout). All four consumers check the store fault first:
- here: classified
approvalFault, so the approver is told to "try once more", and that can never succeed for X - loop_autofill.go:380 and loop_dispatch.go:789:
ErrSlotPinned && !ErrSlotStoreis false, sorecordSlotPinRefusalis skipped and the model gets a bare denial for X with no explanation - channelsd backfill.go:294: the SlotBindRefused notice is suppressed entirely
I get that the ordering is intentional, to keep raw store detail out of user and model text. But it currently drops the refusal along with the store detail. Repros: the classifier returns fault for the mixed error. The real dispatch/observed-promotion path logs git_commit refusal recorded=false, though the joined error contains "this session is pinned to git_commit:…". channelsd publishes 0 notices (want 1).
Fix: with the typed refusal suggested on host_approval.go:2577, walk the joined error, surface only the SlotPinRefusal members to the model and user (and classify as a refusal), and keep the ErrSlotStore members in logs. Update the comments here and at the sibling sites that describe the old ordering. TestBindTriggerSlots_PinRefusalWithStoreFault_NoNotice asserts the current suppression and would need to flip.
There was a problem hiding this comment.
Fixed in 5594096. All four sites now surface only the refusals, quoted via SlotRefusalText, so no store text reaches the model or users. Faults stay in the logs. TestBindTriggerSlots_PinRefusalWithStoreFault_NoNotice is replaced by ..._NoticeQuotesOnlyTheRefusal.
GrantSlots now returns a typed *authz.SlotBindFailure per resource type (refused, fault, drifted, move-committed), so callers can separate a permanent pin refusal from a transient store fault in the same call. - Model- and user-facing surfaces quote only the refusals (SlotPinRefusals / SlotRefusalText); faults stay in operator logs and no longer suppress a refusal on another type. - AllRebindNever is computed over the refused types only, in the runner and in channelsd. - A plan-gate approval resolves every covered phase before writing, refuses the whole approval when phases move one pin to two different instances, and publishes one notice aggregated per type across phases. - Bind-time fault copy no longer claims nothing was granted.
…alizer - The sweep is skipped only when the session finalizer was present at first reconcile; spec.forkedFrom no longer gates it. - A ValidatingAdmissionPolicy refuses any create/update that adds the agentsession finalizer unless the caller is the operator or channelsd service account. - BuildChildSession pre-stamps the finalizer so a fork child keeps the grants and pin its parent copies onto it. - spec.forkedFrom is immutable after create (CEL). - oap install knows the two admission kinds are cluster-scoped.
…plans - An amendment card renders any covered slot whose pin it moves or blocks, not only its delta, so approving never revokes an instance the approver was not shown. - The whole-approval conflict check counts every instance of a moving single-occupancy type, so a phase staying on the pin beside one moving off it is refused instead of silently revoking the stay's grant. - Approval outcomes are tracked per (type, permission): a later phase's grant no longer hides an earlier phase's failure on the same type. - Occupancy and rebind are read straight off the spec, so a precondition compile failure can no longer make a rebind:never slot movable. - GrantSlots lets rebind:never win a mixed merge and keeps the first PriorID, as its comment always said.
- CopySlotGrants skips any type the fork child already pins and guards each copied type with MUST_NOT_MATCH, so a re-run of restart step 6c cannot give the child two pins. - Takeover children are no longer pre-stamped with the finalizer; other fork modes sweep the target name before creating the child. - Admission sweeps (operator and channelsd) also delete data-slot grants. - channelsd sweeps before Create, failing closed, so a crash or redelivery can no longer leave a minted session unswept. - DeleteSlotGrants deletes pins before revoking grants.
Before any class becomes valid SpiceDB has no agentsession definition, so the reads behind the admission and mint sweeps fail FailedPrecondition. Nothing can be granted against an absent definition, so the sweep is satisfied; failing closed there refused every first session on a fresh install.
The admission sweep now also deletes data-slot grants, and a subagent child's first reconcile raced the data-slot grants the SubagentRequest controller writes onto it right after Create, wiping them. The controller now sweeps the child's (ns, name) itself before Create, fail-closed, and pre-stamps the session finalizer so the child's own reconcile does not sweep again. An existing child (a retry) is never swept.
Third-round fixes + pre-merge audit (5594096..e1c8d3a)All six findings from the last review are addressed; replies are inline. I also ran the repo's ten-lens audit ( Fixed
Not fixed in this PR (follow-ups)
Deploy note: the new admission policy needs Kubernetes ≥ 1.30. It hard-codes the Gate: |
What this changes
The security model has long claimed that a slot, once filled, "cannot be reopened" and that the slot write "checks that the slot is still empty as a precondition." Neither was enforced: slot grants were plain TOUCH writes, and a session bound to resource A could silently bind resource B of the same type through any bind path. This PR makes the claim true by construction, with one honest amendment: a slot cannot be silently reopened — a different instance needs a human's approval.
How it works
Declaration.
AuthzSlotgains two fields:occupancy: single | multi(defaultsingle) andrebind: approval | never(defaultapproval). The installer completes both before apply so re-installs stay SSA no-ops, and class admission rejects a single-occupancy slot declaring multiple defaults.Enforcement. The schema composes a non-expiring
slot_pinrelation onto every slot resource type. The first bind of a single-occupancy slot writes the pin under an atomicMUST_NOT_MATCHprecondition atauthz.GrantSlots— the single funnel every production slot-grant write already passes through — and grant writes for pinned slots carry aMUST_MATCHguard so a bind can never race a move. A different instance is refused with a message that names the pinned instance and the route out, routed by the slot'srebindpolicy. Grants keep their mandatory expiry and revocability; the pin persists for the session's life, so expiry never silently reopens the slot. Tool-authored writes toslot_pin/slot_grant_*relations are refused at tuple validation, and a guard test asserts no production write bypasses the gate.The approved move. A plan amendment naming a different instance records the displaced instance on the approval card at build time. Approval executes the move atomically: the pin repoints under a
MUST_MATCHguard and the displaced instance's actual grants are revoked in the same request. The card shows the move as a move — current instance, proposed instance, and the revocation — and a move whose starting point no longer matches the live pin fails the approval loudly instead of moving something the approver never saw.Lifecycle. Fork/restart copies grant and pin tuples verbatim (a clone is not a new arrival); teardown deletes pins alongside grants; a new session's first reconcile sweeps stale tuples left under a reused name, with channelsd sweeping before its own mint-time binds so the two can never race.
Visibility. Pins mirror onto
AgentSession.status.slotPins(display-only; SpiceDB stays the only enforcement input) and render inoap session show. Pin refusals annotate the model's next denial for that type with the pinned instance and the route out, and mint-time refusals record a session notice.Documented limits
occupancy: multi(or split types).Migration
Existing classes pick up the defaults on their next apply. Multi-instance classes must declare
occupancy: multi. Pre-existing sessions have no pins; the first bind after upgrade writes one.Verification
All three suites green at the tip (
mage test:unit,mage test:integration,mage test:e2e, including all 82 scenario bundles). New coverage includes a real-SpiceDB end-to-end arc (pin → refusal → approved move → revocation), a concurrent first-bind race resolved by the precondition rather than process ordering, a red-first regression test for the fork/sweep interaction, and a scenario bundle asserting the move card on a real published approval.