Skip to content

docs: design broker SSH credential handoff - #1204

Open
steipete wants to merge 1 commit into
mainfrom
codex/1074-handoff-design
Open

docs: design broker SSH credential handoff#1204
steipete wants to merge 1 commit into
mainfrom
codex/1074-handoff-design

Conversation

@steipete

Copy link
Copy Markdown
Contributor

Summary

  • define a coordinator-issued, hash-only borrow grant bound to the borrower owner/org, pool, lease, borrow generation, borrow token verifier, and hard expiry
  • specify client-generated ephemeral keys, 15-minute maximum credentials, renewal, bounded per-owner/per-borrow state, idempotent response-loss handling, and fail-closed authorization revalidation
  • require credential revocation, generation-scoped SSH session fencing, authenticated host-key provenance, and linearizable ambiguous-issue cancellation before a lease may return to ready; otherwise quarantine or drain
  • keep issuance, network access, revocation, fencing, and reconciliation behind a provider capability, with a current survey showing that no adapter yet implements the complete contract
  • document legacy CLI/worker behavior, clean unsupported-provider degradation, a phased implementation plan, and explicit maintainer decisions

This is the remaining credential-handoff design for #1074. The capacity half landed in #1197. The grant and revalidation conventions are aligned with the device-pairing work in #1199.

Verification

git diff --check origin/main
  clean

scripts/check-docs.sh
  checked 56 command docs: command surface ok
  checked provider matrix: 79 providers
  checked 239 markdown files: internal links ok
  generated docs tests: 14 passed

node scripts/build-docs-site.mjs
  built docs site: dist/docs-site

.agents/skills/autoreview/scripts/autoreview --mode local --stream-engine-output
  trufflehog: clean
  autoreview clean: no accepted/actionable findings reported

Runtime proof is not applicable: this PR changes documentation only and explicitly marks the design as unimplemented.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Jul 29, 2026
@clawsweeper

clawsweeper Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 31, 2026, 11:22 AM ET / 15:22 UTC.

ClawSweeper review

What this changes

This PR adds a proposed provider-neutral design for short-lived, revocable SSH credentials when an independently configured client borrows a ready-pool lease, and links it from the broker specification.

Merge readiness

⚠️ Ready for maintainer review - 5 items remain

Keep this PR open for maintainer review: it documents a new security-sensitive SSH credential contract rather than an implemented behavior, and its access, revocation, and rollout choices should be explicitly accepted before becoming repository guidance. Likely related people: steipete (medium confidence).

Priority: P3
Reviewed head: 51232fe755ac798d74490d0e7352c9dc6a12a849
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The documentation is focused and backed by stated docs validation, but its substantive security and compatibility contract remains a maintainer decision rather than an independently verifiable implementation.
Proof confidence 🌊 off-meta tidepool Not applicable: Real behavior proof is not required because this PR only changes files under docs/.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Real behavior proof is not required because this PR only changes files under docs/.
Evidence reviewed 4 items Repository inspection blocked: The required read-only repository inspection command failed before it could read files or history with bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted; this review therefore relies on the supplied PR metadata and does not claim current-main source or git-history verification.
Proposed, unimplemented contract: The supplied PR diff adds a new design document whose opening status says no runtime implementation exists yet; the PR body likewise frames the work as the remaining design for the SSH-handoff portion of the ready-pool proposal.
Main broker documentation is intentionally forward-looking: The supplied diff changes the broker specification from an unconditional statement that the coordinator does not issue SSH credentials to a statement that it does not do so today, then links the proposed handoff design. That makes this documentation a prospective compatibility and security contract rather than a description of shipped behavior.
Findings None None.
Security Needs attention Confirm the future SSH security contract: The new design makes concrete commitments about scoped grants, fail-closed revalidation, revocation, session fencing, and quarantine before any adapter implements them; maintainers should approve those obligations and the legacy rollout policy before publishing them as the intended contract.

How this fits together

Crabbox ready pools let keepers register prepared remote leases and let clients borrow and return them through the Worker coordinator. This proposed handoff sits between broker authorization and provider-managed remote access, defining how a borrower would receive scoped SSH access without sharing a standing keeper key.

flowchart LR
  A[Authorized client] --> B[Worker coordinator]
  B --> C[Borrow authorization]
  C --> D[Short-lived SSH grant]
  D --> E[Provider access capability]
  E --> F[Borrowed ready-pool lease]
  F --> G[Return and revocation]
  G --> H[Ready or quarantined lease]
Loading

Decision needed

Question Recommendation
Should Crabbox adopt this proposed broker-issued SSH credential lifecycle as the security and compatibility contract that future provider adapters must implement? Approve the proposed contract: Accept the design as the repository's intended provider-neutral handoff contract and require future implementation PRs to meet its stated rollout and revocation constraints.

Why: The patch contains no runtime implementation defect to repair, but it defines authentication scope, credential expiry, revocation, session fencing, and legacy degradation that affect the product's future security boundary and upgrade behavior.

Before merge

  • Resolve security concern: Confirm the future SSH security contract - The new design makes concrete commitments about scoped grants, fail-closed revalidation, revocation, session fencing, and quarantine before any adapter implements them; maintainers should approve those obligations and the legacy rollout policy before publishing them as the intended contract.
  • Resolve merge risk (P1) - Merging the proposal would establish expectations for short credential lifetime, revocation proof, session fencing, and quarantine behavior before maintainers choose the implementation and provider rollout policy.
  • Resolve merge risk (P1) - The broker-spec wording changes the documented future compatibility path for legacy borrowers, while no provider adapter is described as implementing the complete contract yet.
  • Complete next step (P2) - A maintainer must choose whether the proposed SSH credential lifecycle and legacy rollout policy should become the repository's future public contract before this documentation merges.

Findings

  • [medium] Confirm the future SSH security contract — docs/spec/broker-ssh-handoff.md:1
Agent review details

Security

Needs attention: This docs-only patch adds no executable dependency or secret-handling code, but it proposes a security-sensitive SSH credential, revocation, and fencing contract that needs explicit maintainer acceptance.

Review metrics

Metric Value Why it matters
Documentation surface 1 new 663-line design document; 1 broker-spec link changed The small file count masks a broad proposed contract covering authorization, remote access, revocation, and provider obligations.

Merge-risk options

Maintainer options:

  1. Approve the documented security contract (recommended)
    Merge after a maintainer explicitly accepts the grant, revocation, fencing, provider-capability, and legacy-borrow commitments as the intended future API contract.
  2. Defer unresolved policy details
    Keep the PR open or request a narrower revision if provider support, rollout sequencing, or legacy compatibility must remain undecided until implementation work begins.

Technical review

Best possible solution:

Approve and retain a single provider-neutral design only after maintainers explicitly choose the supported grant lifecycle, legacy-borrow compatibility behavior, provider capability contract, and phased rollout criteria; implement those choices later with focused provider and coordinator tests.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR documents an explicitly unimplemented future credential-handoff design rather than reporting a current runtime failure.

Is this the best way to solve the issue?

Unclear: a provider-neutral design is the right place to record cross-provider invariants, but maintainers must first confirm that its security and legacy-compatibility commitments are the intended permanent contract.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5aa1c3983874.

Labels

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

  • P3: This is a proposed documentation and design change with no current runtime behavior change or demonstrated user regression.
  • merge-risk: 🚨 compatibility: The proposal defines a future replacement for the current client-owned SSH access model and specifies legacy-borrow degradation behavior.
  • merge-risk: 🚨 security-boundary: The proposed contract governs scoped credential issuance, expiry, revocation, host-key provenance, and SSH session fencing.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Real behavior proof is not required because this PR only changes files under docs/.

Evidence

Security concerns:

  • [medium] Confirm the future SSH security contract — docs/spec/broker-ssh-handoff.md:1
    The new design makes concrete commitments about scoped grants, fail-closed revalidation, revocation, session fencing, and quarantine before any adapter implements them; maintainers should approve those obligations and the legacy rollout policy before publishing them as the intended contract.
    Confidence: 0.86

What I checked:

  • Repository inspection blocked: The required read-only repository inspection command failed before it could read files or history with bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted; this review therefore relies on the supplied PR metadata and does not claim current-main source or git-history verification.
  • Proposed, unimplemented contract: The supplied PR diff adds a new design document whose opening status says no runtime implementation exists yet; the PR body likewise frames the work as the remaining design for the SSH-handoff portion of the ready-pool proposal. (docs/spec/broker-ssh-handoff.md:1, 51232fe755ac)
  • Main broker documentation is intentionally forward-looking: The supplied diff changes the broker specification from an unconditional statement that the coordinator does not issue SSH credentials to a statement that it does not do so today, then links the proposed handoff design. That makes this documentation a prospective compatibility and security contract rather than a description of shipped behavior. (docs/spec/broker.md:75, 51232fe755ac)
  • Related capacity work is already merged but distinct: The provided related-item context says feat(pool): reconcile desired ready capacity #1197 merged on 2026-07-28 and deliberately excluded broker-mediated SSH credentials, access-grant issuance, and provider access installation; it does not supersede this design. (5a17cf07ac56)

Likely related people:

  • steipete: The supplied related-item history identifies steipete as author of this design and of the merged ready-pool capacity and device-pairing work that this proposal explicitly builds on; local history attribution could not be completed because read-only inspection failed before execution. (role: recent area contributor; confidence: medium; commits: 5a17cf07ac56, e1e5d143ef1f; files: docs/spec/broker-ssh-handoff.md, docs/spec/broker.md)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Obtain explicit maintainer approval or narrowing of the credential lifecycle, provider-capability, and legacy-borrow commitments before merge.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (6 earlier review cycles)
  • reviewed 2026-07-29T13:16:22.481Z sha 51232fe :: needs maintainer review before merge. :: none
  • reviewed 2026-07-30T01:13:44.261Z sha 51232fe :: needs maintainer review before merge. :: none
  • reviewed 2026-07-30T11:52:43.138Z sha 51232fe :: needs maintainer review before merge. :: none
  • reviewed 2026-07-30T13:37:32.864Z sha 51232fe :: needs maintainer review before merge. :: none
  • reviewed 2026-07-30T18:26:15.917Z sha 51232fe :: needs maintainer review before merge. :: none
  • reviewed 2026-07-31T06:31:17.820Z sha 51232fe :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 30, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant