-
Notifications
You must be signed in to change notification settings - Fork 61
🤖 feat: SSH host-key verification with interactive dialog #2399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 874c3e1cb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Add end-to-end SSH host-key verification: when connecting to an unknown host, Mux detects the host-key prompt via SSH_ASKPASS, shows a confirmation dialog, and pipes the user's decision back to complete the handshake. - Schema + types (src/common/orpc/schemas/ssh.ts) - Backend broker with dedup and 60s auto-reject (hostKeyVerificationService) - DI wiring through service container and oRPC context - SSH_ASKPASS mechanism with FIFO IPC (sshAskpass.ts) - Probe timeout extension: 10s fast-fail, extends to 60s on host-key prompt - Frontend dialog (HostKeyVerificationDialog.tsx) mounted in App - BatchMode=yes for non-interactive SSH execs (defense-in-depth) - Scoped resolvePath timeout to exclude connection acquisition wait, preventing false timeouts when host-key dialog takes >10s - Removed StrictHostKeyChecking=no from OpenSSH and SSH2 transports
874c3e1 to
f3622c3
Compare
Use static imports instead of dynamic await import() (lint rule). Use direct await instead of expect().resolves (await-thenable lint rule). Make HostKeyVerificationService timeout injectable via constructor parameter (defaults to HOST_KEY_APPROVAL_TIMEOUT_MS) so tests can use a short timeout without module mocking.
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f019508360
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed both P1 and P2:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2d6cae66e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Replace readdirSync with async readdir in fs.watch callback - Remove async from test callbacks that don't use await - Replace empty catch callbacks with () => undefined
Call iterator.return() alongside abort signal to ensure the backend subscription finally block runs, releasing responder lease and listener state. Some oRPC iterators don't reliably terminate on abort alone.
|
@codex review Pushed 5 new commits addressing both P1 findings and the P2 iterator cleanup:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b1b239e41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| // Dedup: if a prompt for this host is already pending, append another waiter | ||
| const existingId = this.inflightByHost.get(params.host); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deduplicate host-key prompts by host+port
requestVerification() coalesces pending prompts using only params.host, but the new SSH2 caller sends just resolvedConfig.hostName (no port), so concurrent connections to the same hostname on different ports can be merged into one approval flow. In that case one user decision is applied to both fingerprints, which can incorrectly accept or reject the second connection and defeats per-endpoint host-key verification for multi-port hosts.
Useful? React with 👍 / 👎.
| } finally { | ||
| setResponding(false); | ||
| setPendingQueue((prev) => prev.filter((item) => item.requestId !== requestId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserve pending prompt when respond RPC fails
The dialog always removes the current request in finally, even if api.ssh.hostKeyVerification.respond() throws (for example during renderer/backend reconnects). That drops the UI prompt while the backend request can still be pending until timeout, so the user cannot retry and the SSH handshake fails despite having clicked accept/reject.
Useful? React with 👍 / 👎.
Summary
End-to-end SSH host-key verification: when connecting to an unknown host, Mux detects the host-key prompt, shows a confirmation dialog with the fingerprint, and pipes the user's yes/no decision back to complete the SSH handshake — replacing the previous
StrictHostKeyChecking=nobypass.Closes #963
Background
Previously Mux disabled host-key checking entirely (
StrictHostKeyChecking=nofor OpenSSH,hostVerifier: () => truefor SSH2). This is a security gap — users should explicitly accept unknown host keys, matching the standard SSH experience.Implementation
The feature spans three layers:
Backend broker —
HostKeyVerificationServicededuplicates concurrent prompts for the same host with explicit waiter fanout, auto-rejects after 60s, and exposes oRPC endpoints (getHostKeyVerificationRequests,respondToHostKeyVerification).SSH_ASKPASS mechanism —
sshAskpass.tscreates a per-probe temp directory with a shell script that uses per-request file transactions (prompt.<id>.txt/response.<id>.txt) — no FIFOs ormkfifo. Each askpass invocation gets a unique ID, writes its prompt file, and polls for the matching response file.fs.watchdetects new prompt files and handles each independently, so multiple prompts per SSH handshake (e.g., host-key + passphrase) work without deadlock.Prompt classification — The askpass callback in
sshConnectionPool.tsclassifies prompts viaclassifyAskpassPrompt(): host-key prompts route throughHostKeyVerificationService; non-host-key prompts (passphrase/password) return empty string immediately with a log warning. Passphrase-protected keys are supported only when already unlocked in SSH agent — in-app passphrase entry is intentionally unsupported.Probe timeout extension — The connection probe starts with a 10s fast-fail timeout but extends to 60s when a host-key prompt is detected, giving users time to review the fingerprint. SSH2's
readyTimeoutis also extended to match when host verification is enabled.Frontend dialog —
HostKeyVerificationDialog.tsxpolls for pending verification requests and presents the host/fingerprint/key-type for user confirmation.Timeout scoping —
resolvePath()callsacquireConnection()before starting its 10s command timer, so the timer covers only command execution — not the connection wait (which can include a 60s host-key dialog). This matches the existingtransferBundleToRemote()pattern.Shared constant —
HOST_KEY_APPROVAL_TIMEOUT_MS(60s) is defined once insrc/common/constants/ssh.tsand consumed by the verification service, OpenSSH pool, and SSH2 pool.Graceful fallback — When no host-key verification UI is available (headless/test/CLI), the probe falls back to
StrictHostKeyChecking=no+UserKnownHostsFile=/dev/nullso SSH connections don't hang on unattended prompts.BatchMode=yes — Added to
spawnRemoteProcess()as defense-in-depth: if the ControlMaster socket isn't reused and SSH encounters an unexpected prompt, it fails fast instead of hanging.Responder-capability gating —
HostKeyVerificationServicenow tracks active interactive responders viaregisterInteractiveResponder()/hasInteractiveResponder(). Transports gate askpass, timeout extensions, and host-key callbacks on actual responder availability — not mere service existence. ORPC subscription lifecycle binds responder leases so headless/test contexts never block on impossible prompts. This fixed integration test timeouts caused bymux-askpasswaiting on a non-existent dialog.FIFO prompt queue —
HostKeyVerificationDialogreplaces the single-slotpendingstate with a deduplicated FIFO queue, so concurrent host-key prompts for different hosts are never silently dropped. The subscription cleanup now explicitly callsiterator.return()alongside abort signal to ensure backend responder leases are released.Runtime-aware stream timeouts — SSH integration tests now use
STREAM_TIMEOUT_SSH_MS(25s) instead of the local default (15s) forsendMessageAndWait, matching the per-test timeout differentiation already present.Validation
make static-checkpassesHostKeyVerificationServiceunit tests pass (including responder-availability + dedupe timeout regressions)sshAskpassunit tests pass (multi-prompt handling, idempotent cleanup, dedup, env vars)sshConnectionPoolunit tests pass (including prompt classification pattern matching)mux-askpasswaiting on non-existent dialog)Generated with
mux• Model:anthropic:claude-opus-4-6• Thinking:xhigh• Cost:$57.38