Skip to content

fix: isolate ASCII Box host trust per lease - #1785

Open
steipete wants to merge 1 commit into
mainfrom
codex/fix-ascii-known-hosts-c396
Open

fix: isolate ASCII Box host trust per lease#1785
steipete wants to merge 1 commit into
mainfrom
codex/fix-ascii-known-hosts-c396

Conversation

@steipete

@steipete steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix #1748 by applying the existing protected lease-scoped SSH trust helper at the ASCII Box adapter boundary. Both readiness/reuse and ownership-guarded teardown receive the canonical local lease ID. The native Box authentication key, advertised gateway endpoint, legacy IP fallback, and disabled ControlMaster behavior are preserved.

A new Box can reuse an endpoint without inheriting another lease's host key. A changed key within the same lease remains rejected. Retained leases from older clients explicitly enroll on their first connection with this client; shared pins are neither copied into an unproven lease scope nor deleted. The provider docs and Unreleased changelog explain the behavior and credit @shunkakinoki for the report.

Verification

  • Red control: the new adapter regressions failed on the previous source for both IP and gateway endpoints, and unsafe lease paths reached readiness.
  • go test -race ./internal/providers/asciibox -count=1 passed after the fix, including claim/cleanup and guarded-teardown regressions.
  • Real native OpenSSH proof replaces a synthetic server key at the exact same loopback host/port: a fresh lease connects; the old lease and provider-wide file reject the changed key; both old trust files remain byte-identical. It reuses the existing forwarding fixture, with no account keys, sshd, or external destinations.
  • Broader native SSH forwarding, coordinator cleanup, and host-trust regressions passed under the race detector.
  • go vet ./internal/providers/asciibox ./internal/cli, docs-link checks, and git diff --check passed. Managed Codex review returned no actionable findings in its default P0 scope.
  • Source-bound real ASCII lifecycle proof is in progress. The official CLI and existing E2E credential passed authenticated read-only doctor checks; create/sync/run/reuse/destroy and independent native absence must finish before landing.
  • Full hosted CI passed on the exact PR head: five substantive workflows and 22 jobs, including CI. Native deletion completion remains the landing hold.

Boundaries

The core host-trust implementation and production ownership/deletion fences are unchanged. Provider tests reuse the existing isolated-user-directory utility; they do not touch operator configuration. Native loopback SSH proof is separate from the real provider lifecycle requirement, and neither a timeout nor deletion acceptance alone will be treated as completed cleanup.

Closes #1748.

Real provider proof and current hold

The frozen clean candidate is 0d29ac2ab4d784329476d4efd00f7f4eb6f16cf9; its built CLI SHA-256 is 495251babd02ad9010bc1e3f05289fad01998ffe6a35ad0cd7ea2d4170ab9c10. Tested with the official ASCII CLI 0.1.211-ascii-prod1 and one task-owned Box.

  • Warmup succeeded. A deliberately wrong synthetic key was then placed only in the task-owned provider-wide known-hosts file, leaving the correct lease-scoped pin untouched.
  • Sync transferred 2,169 files (33.7 MiB), remote execution printed ascii-host-trust-pass, and the 22-byte artifact downloaded correctly. The remote source hash matched the candidate.
  • A separate retained --no-sync invocation printed ascii-reuse-pass. The lease-scoped trust and stale shared trust files remained byte-identical through both runs.
  • Guarded stop reached its three-minute deadline and returned context deadline exceeded. The local claim correctly retained its bound deletion-operation reference.
  • Independent native reads report Box bx_pp783kt6 absent, but operation bdop_53e2ade7b3f34fdc83113cd1cbc6a4d0 remains blocked, with completedAt: null. Absence is not completion: cleanup proof remains incomplete, and this PR must not land yet. No claim/trust file was forcibly removed or completion fabricated.

The long silent wait also reproduces the diagnostic problem tracked in #1730. That is separate from the host-trust fix.

Keep the native provider authentication key while using the existing protected lease-specific known-hosts directory for readiness, reuse, and guarded teardown. Preserve rejection of changed keys within a lease and document first enrollment for retained legacy leases.

Add adapter regressions and a real OpenSSH recycled-endpoint proof. Reuse the existing isolated user-directory test utility so host-trust fixtures never touch operator state. Addresses #1748, reported by @shunkakinoki.
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 4, 2026
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 4, 2026, 12:45 AM ET / 04:45 UTC.

ClawSweeper review

What this changes

This PR gives each ASCII Box lease a separate SSH known-hosts file for readiness, reuse, and guarded teardown while retaining the provider’s shared authentication key.

Merge readiness

Blocked before merge - 4 items remain

Keep open: the adapter change directly addresses the reported host-trust defect and current main does not contain it, but merge still requires removal of the contributor-authored Unreleased entry and complete real-provider guarded-teardown proof.

Priority: P2
Reviewed head: 0d29ac2ab4d784329476d4efd00f7f4eb6f16cf9

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The implementation is focused and well-covered, but the submitted real-provider lifecycle evidence explicitly leaves its changed cleanup path incomplete.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: The changed production owner is ASCII Box SSH-target construction for readiness, reuse, and guarded teardown. The submitted terminal-style provider evidence shows successful sync/run/reuse, but explicitly lacks an observed successful guarded-teardown completion because the deletion operation remained blocked. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The changed production owner is ASCII Box SSH-target construction for readiness, reuse, and guarded teardown. The submitted terminal-style provider evidence shows successful sync/run/reuse, but explicitly lacks an observed successful guarded-teardown completion because the deletion operation remained blocked. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 7 items Provider integration: The introduced provider code passes the canonical lease ID into SSH-target construction for both acquisition/reuse and ownership-guarded cleanup, then applies the existing lease-scoped host-trust helper.
Host-trust invariant: The existing helper creates a private lease directory and assigns its known-hosts file; SSH uses strict accept-new verification rather than disabling host-key checking.
Regression coverage: Added tests cover legacy-IP and gateway endpoints, separate lease trust files, shared-key preservation, unsafe lease IDs, guarded-teardown targeting, and a native OpenSSH endpoint/key replacement scenario.
Findings 1 actionable finding [P3] Leave the Unreleased entry to maintainers
Security None None.

How this fits together

The ASCII Box provider converts a provider Box and Crabbox lease ID into an SSH target for readiness, workload reuse, and guarded cleanup. The target’s known-hosts path determines which remote host key native SSH will trust.

flowchart LR
A[ASCII Box record] --> C[Lease target construction]
B[Crabbox lease ID] --> C
C --> D[Lease-specific host trust]
D --> E[SSH readiness and reuse]
D --> F[Guarded remote teardown]
E --> G[Remote workload]
F --> H[Lease cleanup]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The changed production owner is ASCII Box SSH-target construction for readiness, reuse, and guarded teardown. The submitted terminal-style provider evidence shows successful sync/run/reuse, but explicitly lacks an observed successful guarded-teardown completion because the deletion operation remained blocked. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Leave the Unreleased entry to maintainers (P3) - The repository policy assigns Unreleased entries to maintainers for contributor PRs. This PR-introduced entry remains unchanged since the prior review; remove it and let a maintainer add the release note when the work lands.
  • Resolve merge risk (P1) - The submitted real-provider trace does not show guarded teardown completing: its deletion operation remains blocked, so the changed cleanup path lacks an observed after-fix completion result.
  • Complete next step (P2) - Remove the contributor-authored Unreleased changelog entry and attach a redacted real-provider trace that completes guarded teardown.

Findings

  • [P3] Leave the Unreleased entry to maintainers — CHANGELOG.md:5
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Implementation versus regression coverage production +9/-5, tests +176/-2, docs +9 The small provider change is accompanied by endpoint, trust-isolation, and cleanup regressions.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #1748
Summary: This PR is the explicit candidate fix for the recycled ASCII Box endpoint host-trust bug.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Keep the provider-boundary implementation, have a maintainer own the release note, and land after a redacted real ASCII Box lifecycle trace demonstrates successful guarded teardown with lease-scoped trust intact.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Keep the provider-boundary implementation, have a maintainer own the release note, and land after a redacted real ASCII Box lifecycle trace demonstrates successful guarded teardown with lease-scoped trust intact.

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

Yes. Two lease targets using the same endpoint with different host keys exercise the reported failure, and the added native OpenSSH test makes that path concrete without this review executing it.

Is this the best way to solve the issue?

Yes. Applying the existing generic lease-scoped trust helper at the provider adapter is the narrow, provider-neutral repair and preserves strict SSH verification.

Full review comments:

  • [P3] Leave the Unreleased entry to maintainers — CHANGELOG.md:5
    The repository policy assigns Unreleased entries to maintainers for contributor PRs. This PR-introduced entry remains unchanged since the prior review; remove it and let a maintainer add the release note when the work lands.
    Confidence: 0.99

Overall correctness: patch is correct
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 62c008266bd5.

Labels

Label justifications:

  • P2: This is a bounded provider SSH-trust reliability repair without evidence of an active broad outage.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The changed production owner is ASCII Box SSH-target construction for readiness, reuse, and guarded teardown. The submitted terminal-style provider evidence shows successful sync/run/reuse, but explicitly lacks an observed successful guarded-teardown completion because the deletion operation remained blocked. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Acceptance criteria:

  • [P1] git diff --check.

What I checked:

  • Provider integration: The introduced provider code passes the canonical lease ID into SSH-target construction for both acquisition/reuse and ownership-guarded cleanup, then applies the existing lease-scoped host-trust helper. (internal/providers/asciibox/backend.go:323, 0d29ac2ab4d7)
  • Host-trust invariant: The existing helper creates a private lease directory and assigns its known-hosts file; SSH uses strict accept-new verification rather than disabling host-key checking. (internal/cli/lease.go:259, 0d29ac2ab4d7)
  • Regression coverage: Added tests cover legacy-IP and gateway endpoints, separate lease trust files, shared-key preservation, unsafe lease IDs, guarded-teardown targeting, and a native OpenSSH endpoint/key replacement scenario. (internal/providers/asciibox/backend_test.go:583, 0d29ac2ab4d7)
  • Current-main check: The PR head is not an ancestor of current main, and the current-main comparison removes the lease-ID parameter and helper call from the ASCII Box target path; the central fix is therefore not already implemented on main. (internal/providers/asciibox/backend.go, 62c008266bd5)
  • Repository contribution policy: Root policy assigns Unreleased changelog entries to maintainers when the PR author is a contributor; this PR introduces such an entry. (AGENTS.md:41, 0d29ac2ab4d7)
  • Submitted real-provider evidence: The PR body reports successful provider sync, execution, artifact download, and retained-lease reuse, but explicitly records a blocked deletion operation with no completion timestamp and says cleanup proof remains incomplete. (0d29ac2ab4d7)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • shunkakinoki: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

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

  • Remove the contributor-authored Unreleased changelog entry.
  • Attach a redacted real-provider trace showing guarded teardown completes while lease-scoped host trust remains intact; update the PR body to trigger re-review.

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 (2 earlier review cycles)
  • reviewed 2026-09-04T04:15:32.220Z sha 0d29ac2 :: needs real behavior proof before merge. :: [P3] Leave the Unreleased entry to maintainers
  • reviewed 2026-09-04T04:35:30.461Z sha 0d29ac2 :: needs real behavior proof before merge. :: [P3] Leave the Unreleased entry to maintainers

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 4, 2026
@shunkakinoki

Copy link
Copy Markdown
Contributor

Fresh live evidence for exact head 0d29ac2ab4d784329476d4efd00f7f4eb6f16cf9:

  • Built with repository-pinned Go 1.26.5; focused internal/providers/asciibox tests passed.
  • A real ASCII Box was allocated and reached SSH readiness through the lease-scoped target; sync and the remote command completed successfully (command=2.916s, run total=55.062s). This reproduces the downstream path without the prior shared known-hosts mismatch.
  • Guarded release then exhausted its bounded deletion wait with context deadline exceeded; a subsequent direct provider read returned authoritative 404 for the exact Box. Crabbox retained the claim because it did not confirm the asynchronous deletion before its deadline.

This is positive live proof for the host-trust portion, but not a successful Crabbox teardown, so I am not claiming this PR fully cleared. The remaining lifecycle blocker is the separate cleanup/absence-reconciliation work in #1730 and #1789. No host-key verification was disabled and no trust entry was removed.

The remaining branch action here is maintainer-owned removal of the contributor-authored Unreleased entry.

steipete pushed a commit that referenced this pull request Sep 6, 2026
Stop readiness retries on OpenSSH host-key rejection without changing host trust, including direct, proxy, and WSL SFTP probes. Match diagnostics across arbitrary write boundaries and output volume using fixed-size state without retaining stderr.

Preserve authentication/connection startup retries, cancellation, workspace ownership errors, and the original exit cause. Add regression coverage, docs, and maintainer changelog credit.

Verified real OpenSSH loopback checks, related CLI and ASCII race tests, and current-head CI, Release Check, Connector E2E, and Docs UI workflows. Managed review was P0 scoped-clean. Optional bot admission remains separately blocked; no bot verdict or CodeQL fork result is claimed.

Complements #1785 and #1748; does not replace provider lease-scoped trust work.

Co-authored-by: Shun Kakinoki <shunkakinoki@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ASCII Box reused IP is blocked by provider-wide known_hosts state

2 participants