Route amika sandbox ssh through the managed host alias - #296
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36951370e1
ℹ️ 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".
| if forcePTY { | ||
| dest := sshArgs[len(sshArgs)-1] | ||
| sshArgs = append(sshArgs[:len(sshArgs)-1], "-t", dest) | ||
| entry, err := NewHostEntry(sandboxID, sandboxName, info.SSHDestination, info.ExpiresAt) |
There was a problem hiding this comment.
When the API returns an ssh_destination that needs options beyond user/host/port, such as -i, -F, or -o ProxyCommand, this conversion drops them: NewHostEntry parses Destination.Options but the rendered Host block never carries those options. The previous ExecSSH passed strings.Fields(info.SSHDestination) directly, and go/cmd/amika/scp/scp.go:564-567 documents that these server-provided options must be forwarded; I also checked ssh -G against the rendered block and only HostName/User/Port/StrictHostKeyChecking are applied. In that environment, amika sandbox ssh/remote connect/agent no-wait now invoke ssh amika-... without the required options and cannot connect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 575f6be. NewHostEntry/ResolveHost now surface the parsed Destination.Options, and ExecSSH forwards them on the ssh command line ahead of the alias (mirroring how scp already forwards them via resolveSandboxDestination). The alias still supplies HostName/User/Port and accept-new, so both concerns are covered. Cursor connects via its own Remote-SSH machinery and can't take extra ssh flags, so options remain dropped on that path (now documented at the call site).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 575f6bec8b
ℹ️ 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".
| if forcePTY { | ||
| sshArgs = append(sshArgs, "-t") | ||
| } | ||
| sshArgs = append(sshArgs, alias) |
There was a problem hiding this comment.
When a sandbox is reached through a new gateway HostName, or multiple sandboxes share one gateway with different host keys, connecting to amika-<id> is not enough to key known_hosts by the sandbox ID: OpenSSH uses HostKeyAlias for that behavior, otherwise the real HostName is used for host-key lookup/saving. This means reconnects via a rotated HostName can still accept a fresh key instead of verifying the sandbox's previous key, and shared HostNames can still fail as changed keys; please render HostKeyAlias <alias> in the managed Host block before relying on this alias here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — fixed in 2e42be3. Render now emits HostKeyAlias <alias> in each managed Host block, so OpenSSH keys known_hosts by the stable amika-<id> rather than the rotating HostName. First connect records the sandbox's key under its id; every reconnect verifies against it even when the gateway HostName rotates, and sandboxes sharing a gateway no longer collide as changed keys.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4263aafc24
ℹ️ 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".
| // managed config; the options carry anything that block cannot express. | ||
| func buildSSHArgs(alias string, options []string, forcePTY bool, extraArgs []string) []string { | ||
| var sshArgs []string | ||
| sshArgs = append(sshArgs, options...) |
There was a problem hiding this comment.
When the server-returned SSH destination contains -F <configfile> (which ParseDestination preserves as an extra option), forwarding it here while replacing the raw user@host with amika-<id> makes OpenSSH read that config file instead of the user's ~/.ssh/config that EnsureInclude just updated. OpenSSH usage lists [-F configfile] ... destination, and I verified with ssh -G -F /dev/null amika-sb_abc that the managed alias block is not loaded, so hostname remains amika-sb_abc; those sandboxes now try to connect to the alias as a real host whereas the previous raw destination still worked. Handle/reject -F or inject the managed alias into the selected config before execing ssh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in c94db53. ResolveHost now rejects -F (both -F file and attached -Ffile forms) with a clear error, since it replaces the config file ssh reads and is fundamentally incompatible with resolving through the managed alias. Every other forwarded option is additive, so -F is the only special case.
For context: none of the current providers (Daytona, Freestyle, Vercel) emit any options in ssh_destination — they return bare user@host — so this path is unreachable today; the guard is defensive against a future provider.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 985563e96b
ℹ️ 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".
| if forcePTY { | ||
| dest := sshArgs[len(sshArgs)-1] | ||
| sshArgs = append(sshArgs[:len(sshArgs)-1], "-t", dest) | ||
| entry, options, err := NewHostEntry(sandboxID, sandboxName, info.SSHDestination, info.ExpiresAt) |
There was a problem hiding this comment.
When the API returns a valid SSH destination that selects a cipher, such as -c aes128-ctr user@host (ssh -h lists [-c cipher_spec]), routing through NewHostEntry here makes ParseDestination handle the string before execing. That parser does not treat -c as an option that consumes the following token, so it records the cipher name as the target and then rejects the real target as “more than one host”; these sandboxes can no longer be reached even though the previous strings.Fields(info.SSHDestination) path passed the args to ssh unchanged. Add c to the arg-taking option set and cover it before relying on this parser for amika sandbox ssh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I believe this one is a false positive. argTakingOptions (config.go:65) is "bBcDEeFIiJLmOoPQRSWw" — it already includes lowercase c, so consumesFollowingArg("-c") returns true and ParseDestination treats the following token as the cipher argument rather than the target.
Verified empirically:
ParseDestination("-c aes128-ctr user@host")
=> user="user" host="host" port=0 options=["-c" "aes128-ctr"], err=<nil>
The real target is parsed correctly and forwarded via the options, so -c aes128-ctr reaches ssh unchanged. Happy to add a regression test if useful, but no code change seems needed. Let me know if I'm missing a case.
985563e to
87ead45
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87ead45e93
ℹ️ 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".
| extraArgs = args[1:] | ||
| } | ||
| return ssh.ExecSSH(client, name, forcePTY, extraArgs) | ||
| return ssh.ExecSSH(client, basedir.New(""), name, forcePTY, extraArgs) |
There was a problem hiding this comment.
When AMIKA_STATE_DIRECTORY is set, this new basedir.New("") resolver still writes ssh-hosts.json through XDG/default state paths rather than the configured state directory; ResolveHost now persists SSH host state for amika sandbox ssh, so users isolating state in CI or with the documented override still get state created under ~/.local/state/amika instead. Use a paths resolver that is backed by config.StateDir() or otherwise teaches this path about the override.
AGENTS.md reference: AGENTS.md:L130-L130
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The SSH commands now resolve paths via a new config.SSHPaths() (backed by basedir.NewWithStateDir), so ssh-hosts.json honors AMIKA_STATE_DIRECTORY like the other state files. The managed ~/.ssh/config and ~/.ssh/amika.conf stay home-relative (they aren't state files). Applied to all call sites — ssh, agent --no-wait, create --connect, attach, and the code/Cursor path. Added coverage in config_test.go (override honored + ~/.ssh files unmoved) and basedir_test.go (NewWithStateDir).
87ead45 to
5a9947c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a9947c279
ℹ️ 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".
| // Cursor connects via the alias through its own Remote-SSH machinery and | ||
| // takes no extra ssh command-line options, so any server-provided options | ||
| // are necessarily dropped here (unlike the CLI ssh path). | ||
| alias, info, _, err := ssh.ResolveHost(client, paths, name) |
There was a problem hiding this comment.
When the API returns an ssh_destination that requires extra options such as -i or -o ProxyCommand, this path still ignores the options returned by ResolveHost and launches Cursor with only ssh-remote+<alias>. Fresh evidence after the CLI fix is that ExecSSH now forwards those options, but prepareCursorSSHTarget discards them here while the rendered alias only contains HostName/User/Port/HostKeyAlias/StrictHostKeyChecking, so amika sandbox code cannot connect in the same environments unless the options are rendered into the managed Host block or the command fails clearly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 01c5a7f. Rendering arbitrary CLI options (-i, -o …, -J, boolean flags) into ssh_config directives robustly is fragile, and Cursor drives its own Remote-SSH so it can't take ssh flags — so prepareCursorSSHTarget now fails clearly when ResolveHost returns options, pointing the user at amika sandbox ssh (which forwards them). That matches how -F is handled: fail loud rather than launch a misconfigured connection. No provider emits options today, so this is unreachable in practice; if one ever needs it, rendering options into the managed block would be the follow-up. Added TestPrepareCursorSSHTargetRejectsOptions.
`amika sandbox ssh` failed with "Host key verification failed" on first connect to a fresh sandbox host (KAPRO-561). `ExecSSH` execed `ssh` against the raw connection string from the API (e.g. `tok@ssh.app.daytona.io`), which never matched the `Host amika-<id>` block in `~/.ssh/amika.conf`, so that block's `StrictHostKeyChecking accept-new` was never applied. With no `known_hosts` entry ssh prompts, and in non-interactive use (a remote command, piped stdin, CI) the prompt cannot be answered. `amika sandbox code` was unaffected because it already connects via the stable alias. Extract the "resolve sandbox id, upsert the managed host, return the alias" logic (previously inline in `prepareCursorSSHTarget`) into a shared `ssh.ResolveHost` helper and route `ExecSSH` through the alias instead of the raw destination. All `ExecSSH` call sites (`ssh`, `agent --no-wait`, `create --connect`, `attach`) benefit from the fix. Alongside the core change: - Forward server-provided ssh options. The raw destination may carry options beyond user/host/port (e.g. `-i`, `-o ...`) that the alias block cannot express, so `ResolveHost` surfaces them and `ExecSSH` forwards them on the command line ahead of the alias, mirroring how scp already forwards them. The Cursor path cannot pass ssh flags, so `amika sandbox code` fails clearly when a destination carries options rather than launching Cursor without them. - Render `HostKeyAlias <alias>`. OpenSSH keys `known_hosts` by the real HostName unless `HostKeyAlias` is set, so without it `accept-new` would silently accept a fresh key whenever the gateway hostname rotates and would flag changed keys when sandboxes share a gateway. Keying by the stable alias makes the first connect record the key and every reconnect verify it. - Reject `-F` in forwarded options. Unlike other options `-F` replaces the config file ssh reads, so forwarding it would make ssh ignore the managed block and dial the literal alias as a host. - Fail closed on control characters in `Render`. `amika.conf` is included by `~/.ssh/config`, so an embedded newline in an interpolated field could inject arbitrary ssh_config directives. Upstream schemas already forbid such characters; the render seam now enforces it itself, before the state file is persisted so a bad entry cannot wedge later regenerations. - Honor `AMIKA_STATE_DIRECTORY` for the SSH hosts state. The commands now resolve paths via `config.SSHPaths()` (backed by a new `basedir.NewWithStateDir`) so `ssh-hosts.json` respects the documented state-directory override like the other state files, rather than always landing under `~/.local/state/amika`. No current provider emits options in `ssh_destination`, so the option-forwarding, `-F`, and injection paths are defense-in-depth against a future provider rather than live bugs.
5a9947c to
01c5a7f
Compare
2 participants
Problem
amika sandbox sshfails with "Host key verification failed" on first connect to a fresh sandbox host (KAPRO-561).ExecSSHexecedsshagainst the raw connection string from the API (e.g.tok@ssh.app.daytona.io). That destination never matched theHost amika-<id>block in~/.ssh/amika.conf, so the block'sStrictHostKeyChecking accept-newwas never applied. With noknown_hostsentry, ssh falls back to prompting — and in non-interactive use (a remote command, piped stdin, CI) the prompt can't be answered, so the connection fails.amika sandbox codewas unaffected because it connects via the stable aliasamika-<id>, which does carryaccept-new.Note: simply calling
EnsureInclude()from the ssh path — as the issue originally proposed — would not have fixed it, because the raw destination still wouldn't match the alias block.Fix
Core change — route
amika sandbox sshthrough the managed alias:prepareCursorSSHTarget) into a sharedssh.ResolveHosthelper.ExecSSHthrough the aliasamika-<id>instead of the raw destination, soStrictHostKeyChecking accept-newapplies and first connect no longer fails.ExecSSHcall sites (ssh,agent --no-wait,create --connect,attach) now pass base-dir paths and benefit from the fix.Correctness / robustness follow-ups (surfaced by review):
-i,-o …) that the alias block cannot express.ResolveHostnow surfaces them andExecSSHforwards them on the command line ahead of the alias, mirroring howscpalready forwards them.HostKeyAlias <alias>. OpenSSH keysknown_hostsby the realHostName, not the alias, unlessHostKeyAliasis set. Without it,accept-newwould silently accept a fresh key whenever the gateway hostname rotates and would flag changed keys when sandboxes share a gateway. With it, the first connect records the sandbox's key under its stable id and every reconnect verifies against it.-Fin forwarded options. Unlike other options,-Freplaces the config file ssh reads, so forwarding it would make ssh ignore the managed block and dial the literal alias as a host.ResolveHostrejects it (all getopt spellings) with a clear error.Security hardening:
Render.~/.ssh/amika.confisIncluded by~/.ssh/config; an embedded newline in an interpolated field (sandbox name/id, host, user) could inject arbitrary ssh_config directives (e.g.ProxyCommand). Upstream schemas already forbid such characters, so this is unreachable today, but the render seam now enforces it itself rather than trusting distant invariants. Validation runs before the state file is persisted so a bad entry can't wedge later regenerations.No current provider (Daytona, Freestyle, Vercel) emits options in
ssh_destination, so the option-forwarding,-F, and injection paths are defense-in-depth against a future provider rather than live bugs.Testing
go build ./...,go vet ./internal/ssh/... ./cmd/amika/sandbox/...— cleango test ./internal/ssh/... ./cmd/amika/sandbox/...— passgofmtcleanbuildSSHArgsarg ordering (options /-t/ alias / command);ResolveHost(writes alias block +accept-new+HostKeyAlias, surfaces options, sandbox-id fallback, empty-destination error,-Frejection in all forms);configFileOptiongetopt-cluster matching; andRenderrejecting control characters in every interpolated field.Review
Cleared Codex review (P1 option forwarding + two P2s:
HostKeyAlias,-F) and an independent review pass (bundled--Fguard hole, validate-before-persist ordering), all addressed.