Skip to content

fix: bind SSH credential handoffs to owned listeners - #1126

Merged
shanselman merged 13 commits into
mainfrom
shanselman-fix-ssh-owned-handoffs-followup
Aug 14, 2026
Merged

fix: bind SSH credential handoffs to owned listeners#1126
shanselman merged 13 commits into
mainfrom
shanselman-fix-ssh-owned-handoffs-followup

Conversation

@shanselman

@shanselman shanselman commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Narrow current-main follow-up that binds SSH restart and credential handoffs to the exact manager, registry record, tunnel generation, and WebSocket generation that authorized them.

  • manager-owned, cancellation-bounded manual SSH restart with fresh authenticated handshake and owned-listener verification
  • operator and node credential handoff reauthorization immediately before generation-bound send, with typed failures preserved
  • per-socket duplicate, malformed, late, and stale challenge suppression
  • generation-conditional tunnel stop and cleanup that cannot stop a replacement owner
  • production SSH config proof hook removed; three required test-only ownership E2Es retained

Current-main scope comparison

Scope Old head c4b4f90b vs old base Final head bafdc49e vs current main
Product 11 files, +1060/-227 LOC 11 files, +1072/-226 LOC
Tests 12 files, +1341/-688 LOC 13 files, +1460/-690 LOC
Total 23 files, +2401/-915 LOC 24 files, +2532/-916 LOC

The one additional test file is ConnectEnvelopeBuilderTests.cs, required to adapt the signer-failure proof to current main's authoritative ConnectEnvelopeBuilder and handshake gate.

Why every product file remains

File Necessity
docs/ARCHITECTURE.md Closes user SSH restart ownership in GatewayConnectionManager without restoring any current-main App or ConnectionPage responsibility.
GatewayConnectionManager.cs Owns bounded restart, registry/tunnel generation checks, operator/node handoff authorization, typed failures, and safe cleanup.
IGatewayConnectionManager.cs Exposes the manager-owned restart contract to UI callers.
ISshTunnelManager.cs Carries owned start results and generation-conditional stop primitives.
SshTunnelService.cs Implements owned generation tracking, cancellation-bounded stop, and production proof-hook removal.
OpenClawGatewayClient.cs Enforces operator per-socket challenge gating, generation-bound send, and typed diagnostic preservation.
SshTunnelCommandLine.cs Removes the production -F SSH config injection surface.
WebSocketClientBase.cs Attributes inbound processing and credential-bearing writes to the owning socket generation.
WindowsNodeClient.cs Applies the equivalent node challenge, authorization, response, and send protections.
App.xaml.cs Delegates restart to the manager and removes production E2E config injection while preserving current-main composition ownership.
ConnectionPage.xaml.cs Awaits and presents the manager-owned restart result instead of directly manipulating the tunnel.

No direct-connect extraction, DeviceIdentity change, WSL repair, browser UI, setup-token precedence, MXC behavior, or workflow change is present in the PR diff.

Validation

Exact product head before the final workflow-only main merge, with identical product binaries to bafdc49e:

  • .\build.ps1: passed
  • Shared: 3,698 passed, 32 skipped
  • Tray, isolated data directory: 2,469 passed
  • Connection: 600 passed
  • focused restart, ownership, typed handoff, and challenge tests: 28 Connection + 12 Shared passed
  • SSH ownership E2Es: 3 passed, 0 skipped
  • strict MXC not required by AGENTS.md because this PR does not change MXC or system.run
  • structured autoreview: accepted restart configuration and cleanup findings were fixed; prohibited production proof-hook restoration was rejected; final fix reviews clean
  • final full-diff rubber-duck: clean, no blocking or actionable findings
  • exact-head CI for bafdc49e: rerun pending after workflow-only current-main merge

Real behavior proof

Authorized real-profile proof against SSH host 192.168.1.10:

  • backed up and hashed all 378 profile files before proof without printing secrets
  • exact-head app owned an SSH child listening on both 127.0.0.1:18789 and [::1]:18789
  • visible Connection and saved Gateway surfaces showed Connected, via SSH tunnel, and paired via device token; node showed active
  • local MCP tools/list succeeded and benign system.which resolved git
  • paired Gateway openclaw nodes invoke ... system.which succeeded against the connected Windows node
  • forced an owned-listener conflict, verified the visible SSH recovery card, released the conflict, clicked Restart tunnel, and observed a fresh app-owned dual-stack SSH child plus return to Connected
  • normal WinAppCLI packaged launch was restored after proof and the paired Windows node returned Ready
  • final profile comparison: 378 files, no added or removed files; only gateways.json changed, solely the expected lastConnected timestamp
  • active gateway ID, .10 SSH host, SSH/local/remote ports, shared/bootstrap token values, settings file, and per-gateway identity file remained unchanged

Intended retained profile state before merge: the authorized .10 realm, its existing active gateway ID, and its per-gateway device identity remain; this proof introduced no structural profile or credential change.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 4f13b1ca-b8e7-404c-9712-e470a47a53ec
@shanselman shanselman added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Aug 8, 2026
@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. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Aug 8, 2026
@clawsweeper

clawsweeper Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 14, 2026, 5:53 PM ET / 21:53 UTC.

ClawSweeper review

What this changes

This PR makes SSH tunnel restart and operator/node gateway handoffs verify the active registry record, tunnel owner generation, listener, and WebSocket generation before credentials are sent.

Merge readiness

⚠️ Needs maintainer review before merge - 5 items remain

Keep open for maintainer review. Current main does not contain this SSH ownership hardening, and the current head has no discrete correctness finding; its intentional fail-closed credential and restart behavior needs security and availability acceptance after the in-progress exact-head checks finish.

Priority: P1
Reviewed head: bafdc49e44d27ed51e0e4fd6248a4ec26d61fd22
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The implementation and focused coverage are strong, while the reported live proof and pending exact-head checks leave this sensitive change for maintainer review.
Proof confidence 🦐 gold shrimp (3/6) Sufficient (live_output): The PR body documents an authorized real SSH profile run, an owned-listener conflict and visible recovery, local MCP and gateway invocation, and unchanged credential/profile state; redact any future attached diagnostics.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The PR body documents an authorized real SSH profile run, an owned-listener conflict and visible recovery, local MCP and gateway invocation, and unchanged credential/profile state; redact any future attached diagnostics.
Evidence reviewed 7 items Manager-owned restart verification: The current PR head serializes restart, requires a fresh connected handshake, and rechecks the active registry record, tunnel generation, configuration, and owned listener before reporting success.
Credential handoff authorization: Operator and node handoffs revalidate the active record and endpoint ownership immediately before sending, rejecting changed tunnel generations or listener ownership.
Socket-generation write guard: Credential-bearing WebSocket sends capture and check the expected socket generation while holding the send lock.
Findings None None.
Security None None.

How this fits together

The tray sends gateway and Windows-node connections through the connection manager, which may first establish an SSH loopback tunnel. This change binds listener verification and credential-bearing handshakes to that manager-owned tunnel before the gateway session becomes connected.

flowchart LR
A[Restart tunnel action] --> B[Connection manager]
B --> C[Owned SSH listener check]
C --> D[Gateway WebSocket]
D --> E[Credential handoff]
E --> F[Connected operator and node sessions]
C --> G[Fail-closed connection error]
Loading

Decision needed

Question Recommendation
Should maintainers accept the PR's fail-closed behavior, where a failed ownership recheck can interrupt a user-requested SSH restart rather than permit a credential-bearing handshake through an unverified listener? Accept fail-closed restart: Merge after the current-head checks pass, accepting a failed restart over sending credentials through an unverified SSH listener.

Why: This is an intentional security-versus-availability boundary, not a mechanical defect that automated repair can safely decide.

Before merge

  • Resolve merge risk (P1) - A false ownership or handshake failure during a user-initiated restart deliberately interrupts the existing SSH path instead of risking a credential send to an unverified listener.
  • Resolve merge risk (P1) - The branch changes the credential transport trust boundary and restart availability behavior, so a security owner should explicitly accept the fail-closed tradeoff.
  • Resolve merge risk (P1) - The exact-head C# and E2E check runs shown in the PR context are still in progress.
  • Complete next step (P2) - No mechanical repair is indicated; this collaborator-authored security and availability change needs maintainer acceptance and completion of the current-head checks.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Change surface 24 files: 11 product, 13 test/docs The security boundary spans connection lifecycle, SSH ownership, WebSocket transport, WinUI delegation, and regression coverage.
Production versus tests production +1072/-226, tests +1460/-690 The substantial behavioral change is accompanied by a larger focused test delta, including ownership E2Es.

Merge-risk options

Maintainer options:

  1. Accept verified fail-closed recovery (recommended)
    Merge after exact-head validation completes if maintainers accept that an unverified listener must terminate the restart rather than receive credentials.
  2. Pause for a recovery-policy decision
    Hold the PR if preserving an existing live SSH session through ownership uncertainty is a required product guarantee.

Technical review

Best possible solution:

Retain the manager-owned, generation-bound restart path and land it once current-head validation is green and a security owner accepts the intentional fail-closed recovery behavior.

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

Yes. The current head contains focused manager tests and three ownership E2Es covering listener replacement and withheld credential frames; this read-only review did not execute them.

Is this the best way to solve the issue?

Yes. Binding the checks at the connection-manager and generation-bound transport boundary is narrower and safer than allowing UI code or a stale socket to decide listener trust.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body documents an authorized real SSH profile run, an owned-listener conflict and visible recovery, local MCP and gateway invocation, and unchanged credential/profile state; redact any future attached diagnostics.

Label justifications:

  • P1: This changes credential-bearing SSH gateway recovery and can affect active operator and node connectivity.
  • merge-risk: 🚨 security-boundary: The PR changes when shared, bootstrap, and device credentials may cross an SSH loopback listener.
  • merge-risk: 🚨 availability: The intended fail-closed path can interrupt a user-requested tunnel restart when ownership cannot be proven.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body documents an authorized real SSH profile run, an owned-listener conflict and visible recovery, local MCP and gateway invocation, and unchanged credential/profile state; redact any future attached diagnostics.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body documents an authorized real SSH profile run, an owned-listener conflict and visible recovery, local MCP and gateway invocation, and unchanged credential/profile state; redact any future attached diagnostics.

Evidence

What I checked:

Likely related people:

  • shanselman: Current-main blame attributes the central connection-manager and SSH tunnel implementation to this contributor, and repository history shows sustained work across the affected files. (role: recent connection and SSH-area contributor; confidence: high; commits: 4206611f2b2e; files: src/OpenClaw.Connection/GatewayConnectionManager.cs, src/OpenClaw.Connection/SshTunnelService.cs, src/OpenClaw.Shared/WebSocketClientBase.cs)
  • Barbara Kudiess: Current main records this contributor's protocol-owner refactor immediately adjacent to the WebSocket and gateway-client surfaces this PR adapts. (role: recent protocol-core refactor contributor; confidence: medium; commits: 323f46d3ded7; files: src/OpenClaw.Shared/WebSocketClientBase.cs, src/OpenClaw.Shared/OpenClawGatewayClient.cs, src/OpenClaw.Shared/WindowsNodeClient.cs)

Rank-up moves

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

  • Allow the in-progress exact-head C# and E2E checks to complete before merging.

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-08-08T07:15:14.264Z sha a4ae2e4 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-08T08:39:21.772Z sha c4b4f90 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-08T09:25:07.011Z sha c4b4f90 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-14T19:53:29.744Z sha c4b4f90 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-14T20:36:01.550Z sha 6074681 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-14T21:16:42.467Z sha ed397b0 :: needs maintainer review before merge. :: none

shanselman and others added 2 commits August 8, 2026 00:52
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4f13b1ca-b8e7-404c-9712-e470a47a53ec
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4f13b1ca-b8e7-404c-9712-e470a47a53ec
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. 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. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 8, 2026
@shanselman shanselman added status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. and removed status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. labels Aug 10, 2026
shanselman and others added 10 commits August 14, 2026 12:49
…ned-handoffs-followup

# Conflicts:
#	docs/ARCHITECTURE.md
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8d3c2a8c-1e78-47ee-aa3d-bb4bee89b1b1
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ned-handoffs-followup

# Conflicts:
#	src/OpenClaw.Shared/OpenClawGatewayClient.cs
#	src/OpenClaw.Shared/WindowsNodeClient.cs
#	tests/OpenClaw.Shared.Tests/OpenClawGatewayClientTests.cs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8d3c2a8c-1e78-47ee-aa3d-bb4bee89b1b1
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 8d3c2a8c-1e78-47ee-aa3d-bb4bee89b1b1
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@clawsweeper clawsweeper Bot added the proof: sufficient Contributor real behavior proof is sufficient. label Aug 14, 2026
@shanselman
shanselman merged commit a51029e into main Aug 14, 2026
26 checks passed
@shanselman
shanselman deleted the shanselman-fix-ssh-owned-handoffs-followup branch August 14, 2026 22:08
@shanselman shanselman removed the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. 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