fix: use authorized broker sessions for admin commands - #1714
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 security-boundary
|
Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 8:20 PM ET / September 2, 2026, 00:20 UTC. ClawSweeper reviewWhat this changesThis PR lets coordinator-backed admin and image commands send the configured broker credential, including a GitHub login session, when no explicit admin token is configured while retaining explicit-token precedence. Merge readiness⛔ Blocked until stronger real behavior proof is added - 4 items remain Keep open: the focused CLI change aligns with the existing coordinator authorization design, but this external PR still lacks real successful-path proof for the newly reachable GitHub-admin session flow. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s CLI selects a broker credential before calling coordinator admin routes. The coordinator revalidates the credential, derives admin status from immutable GitHub-owner grants, and either permits or rejects the route. flowchart LR
A[Operator admin command] --> B[CLI credential selection]
B --> C[Coordinator request]
C --> D[GitHub session validation]
D --> E[Admin-route authorization]
E --> F[Admin operation]
E --> G[403 rejection]
Decision needed
Why: The patch intentionally makes an existing server-side grant reachable from the CLI, and only a real coordinator can prove the final authorization effect for the newly allowed principal. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Record a redacted real-coordinator run where an already granted immutable GitHub owner succeeds on a non-destructive admin route and a comparable ungranted session receives 403 without provider activity. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: the base helper rejected every admin call without an explicit admin token before the coordinator could evaluate a signed GitHub session. The deployed successful grant path has not been exercised. Is this the best way to solve the issue? Yes: retaining the coordinator as the sole authority and only changing CLI credential selection is the narrowest maintainable repair; it needs final-effect evidence before merge. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c9cdc3598753. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
6d503e2 to
abab288
Compare
|
@clawsweeper re-review The branch is mechanically refreshed onto current main at The PR body still explicitly records the missing successful live path for an already-granted GitHub admin principal. No grant or credential was changed, and the denied login was not retried. This refresh does not claim to close that proof gap or satisfy GitHub's independent approval requirement. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
🦞👀 Re-review progress:
|
1 participant
What Problem This Solves
Admin and image commands reject an operator's configured GitHub session locally unless a separate admin token is configured, even though the coordinator already supports admin grants for immutable GitHub owners. This blocks legitimate host-management workflows before the server can authorize the request.
Why This Change Was Made
Use the normal configured broker authentication when no explicit admin token exists. An explicit admin token still takes precedence and suppresses the normal token command. Missing broker credentials still fail locally before any guest-side checkpoint preparation. A server denial is authoritative: there is no retry with another credential and no server policy or privilege change.
User Impact
Operators with an existing coordinator-side GitHub admin grant can use their login for admin and image commands. Ordinary users and shared-token callers remain subject to the same server-side refusal. Documentation now describes the existing authorization boundary consistently.
Evidence
go test ./internal/cli -run '^TestAdminLeasesUsesConfiguredAuthorization$' -count=1 -v.go build -trimpath -o <task-binary> ./cmd/crabboxpassed.go vet ./internal/cliandnode scripts/build-docs-site.mjsalso passed.Follow-up on CI
The first CI run caught a missing-credential regression in the legacy checkpoint path: it reached guest preparation before refusing absent broker authentication. The follow-up restores that early guard using the existing client authentication predicate, without requiring a separate admin token. Both regression cases failed on the previous head and now pass alongside all six authorization controls (eight leaf cases total). Fresh managed Codex review found no accepted P0 findings.
The broader checkpoint-inclusive local race run did not complete green: an existing built-CLI abandonment fixture exceeded its unchanged 15-second deadline, then the package hit the unchanged 10-minute deadline. It recorded 191 passing leaf executions, one failure and one intentional skip before termination. The exact failed fixture passed once standalone (7.23 seconds) without changing its deadline; that diagnostic does not replace missing broad coverage or establish the original cause. Fresh vet and build passed on the follow-up. The earlier narrower race run and docs build above apply to the first head. Fresh CI passed on the follow-up, including full Go tests, Go coverage, Windows cancellation, Apple VM, connector lifecycles, scripts and docs (run 33465959137).
The live successful-admin proof requested by review remains unavailable: the current account is denied by the broker. No admin grant was changed and no AWS Mac host was allocated.
Current-main refresh (2026-09-01)
Mechanically refreshed onto
c9cdc35987531d6f71c317693cb18cbbb0dbe8d9; the current head isabab2880c31f1d33fa06fa94baa4c3ae7a7cb1e8. The sole merge conflict was the old unreleased changelog entry. The finalized main changelog is retained byte-for-byte and this PR no longer changes it. All eight remaining implementation, test, and documentation files are byte-identical to the previously reviewed6d503e20c525e7219d89b8ff539d00518f579fb0head; the two-commit range comparison differs only by that removed changelog entry.Fresh validation on the refreshed head:
GOMAXPROCS=4 go test -race ./internal/cli -run '^Test(Admin|CreateAWSAMICheckpointRejectsMissingBrokerAuthBeforeCloudInit$|Coordinator.*(Token|Auth)|GitHubLogin|Login|Whoami|WriteBrokerLogin)' -count=1 -jsonpassed: 49 top-level tests, 61 leaf cases, no failures or skips. Package time was 113.074 seconds; the command exited 0 after 449.275 seconds including compilation and Go cleanup. The process was allowed to finish normally; no deadline was changed.git diff --check origin/main...HEAD,node scripts/check-docs-links.mjs, andnode scripts/build-docs-site.mjspassed.The earlier negative live-authentication evidence above applies to the original head. Exact-head validation is now terminal green: full CI and the required release check both passed on attempt 1; connector smokes, CodeQL, and docs visual proof also passed. The final check rollup has 24 successful checks, one neutral check, one skipped optional check, and no pending or failed checks. The successful GitHub-admin live proof remains outstanding: an already-granted immutable GitHub owner must succeed on a non-destructive admin route, alongside the nearest unauthorized-principal denial. Mocked tests and explicit-admin-token success are not substitutes. The known denied login was not retried, and no credential, grant, server configuration, or cloud resource was changed for this refresh. GitHub also still requires an independent approval. Auto-merge remains off while the live authorization proof is missing.