fix: write workspace SSH hosts to a shared glob include that ssh reads first - #1061
fix: write workspace SSH hosts to a shared glob include that ssh reads first#1061EhabY wants to merge 6 commits into
Conversation
EhabY
changed the title
5368805 to
b4db800
Compare
dc556b6 to
216491f
Compare
EhabY
changed the title
code-asher
left a comment
code-asher
left a comment
There was a problem hiding this comment.
Nice, I like how minimal the edits are to my main config now. One comment on the path for our config.
7095b82 to
53ae245
Compare
EhabY
changed the title
9a97ce0 to
90df8e5
Compare
EhabY
changed the title
058b794 to
1355573
Compare
SSH uses the first value it obtains for each option, so a catch-all "Host *" in the user's config beat the block we appended to the end of it, and connections aborted with "Unexpected SSH Config Option". Writing the block higher up would not be enough: it still loses to hosts pulled in by an Include above it, and a Host line moved over the options someone wrote outside any block would capture them. Write the blocks to ~/.ssh/coder/config instead and include that file from the first line of the user's config, where nothing can be parsed before it. Their config is written once to add the include, and the deployment's old block moves out of it on the next connect. The include path keeps its tilde: relative includes resolve against ~/.ssh no matter where the including file lives, and an absolute path would not survive a config synced between machines. Since placement now guarantees the options apply, the block that recomputed them and aborted the connection on a mismatch is gone. What remains of computeSshProperties reads RemoteCommand, which can only come from the user's config.
Antigravity and Windsurf/Devin renamed the setting to remote.antigravitySSH.configFile and remote.devinSSH.configFile, then spawn ssh without -F, so ssh reads ~/.ssh/config no matter what any of them say. The setting only feeds their own host tree. Honoring it, or a stale remote.SSH.configFile synced in from another editor, writes the workspace host to a file the connection never reads. Ignore it on those two and keep reading remote.SSH.configFile elsewhere: Microsoft's extension and Cursor's fork pass it to ssh with -F, and VSCodium's fork parses the file itself instead of running ssh. This drops the per-extension section map from #1060: the three extensions that do connect through the setting all read remote.SSH.
1355573 to
3369b82
Compare
code-asher
left a comment
code-asher
left a comment
There was a problem hiding this comment.
I think this is the right direction! The main concern I have is if we should use a different file structure to account for concurrency, or just not worry about concurrency at all.
Also I have not been able to test it yet because every time I build I get a timeout for one of the dependencies. Not sure why pnpm insists on downloading every time I build...
| merged = raw ? `${raw.trimEnd()}\n\n${block}` : block; | ||
| } | ||
| await this.save(); | ||
| return merged.startsWith(CODER_SSH_CONFIG_HEADER) |
There was a problem hiding this comment.
I had a silly thought, if the user edits this file (despite the warning lol) and prepends something, we would add a duplicate comment.
Maybe we should check to see if the comment is elsewhere in the config and remove it so we can put it back to the top?
Or alternatively we read in all the blocks, and then regenerate the entire file so anything the user added outside the blocks gets wiped.
Edit: may not be an issue if we end up changing the file structure.
There was a problem hiding this comment.
No longer an issue now
| let rest = raw; | ||
| const editorBlock = this.findBlock(rest, blockMarkers(editorId)); | ||
| if (editorBlock) { | ||
| rest = this.removeRange(rest, editorBlock); |
There was a problem hiding this comment.
It occurs to me that if you use multiple editors they will keep moving their blocks on top of each other.
Not an actual issue or anything, but it could be noisy if the user has their SSH config source controlled or automatic backups or something like that and the file keeps changing unnecessarily.
Maybe not worth solving (up to you), but we could extract all the include blocks, order them and re-insert so they are stable. Or leave them in place if they already exist (I know we talked about not doing that so the user has less control but idk if it would really be an issue).
Edit: may not be an issue if we end up changing the file structure.
There was a problem hiding this comment.
Solved with the file structure change
| ); | ||
| }); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I see the notes about the race being acceptable but if we are anticipating this to be a problem enough to try to address it then I feel like we should solve it fully.
Which I think would mean a lock in $XDG_DATA_HOME.
Alternatively...we could put the configs in $XDG_DATA_HOME, then use a wildcard include for the main config, that way even if they step on each other's toes writing the main config the end result would always be correct.
Multiple instances of one editor on different deployments could still step on each other's toes for their own configs though. We could do a config per deployment. A setup where we have something like these files:
~/.local/share/coder.coder-remote/ssh/vscode--dev.coder.com.conf
~/.local/share/coder.coder-remote/ssh/vscode--cdrstable.dev.conf
~/.local/share/coder.coder-remote/ssh/cursor--dev.coder.com.conf
And this include:
Include ~/.local/share/coder.coder-remote/ssh/*.conf
There was a problem hiding this comment.
While that is def cleaner, I think the fact that people use multiple VS Code editors is very unlikely and so we can even live with the race fully (what are the chances you try to open two editors at the same exact time AND this is the first time that we add it?).
One improvement we can do, is that we do not move the include to the top if the top has other "vscode" includes (since those are mutually exclusive).
My issue with XDG_DATA_HOME is that we have to handle each platform, and we have to ensure it's not deleted or cleaned up by mistake. While using the global storage seems more like a natural place IMO
There was a problem hiding this comment.
Actually the more that I think about it the more I like your idea since it means we have a single file for each (editor, deployment), though in all fairness we can duplicate this in the globalStorage as well.
I'll double check if we can find this path easily on all platforms and make sure globs work on all platforms equally, if so then yeah maybe that's the way to go.
The only downside is file cleanup, maybe we always write the full file, and then we run a cleanup on editor startup we clean up those files if they haven't been touched in a week or something. Potentially we can let ANY editor remove these files so even for other editors to fix the staleness 🤔
There was a problem hiding this comment.
Moved, will test on Linux and Windows 🙏
- Rename the ambiguous getSshConfigPath pair to getMainSshConfigPath and getIncludedSshConfigPath. - Move the generated-file header back to the top if the user prepended content, instead of duplicating it. - Drop the legacy marker lookup in mergeDeployment; the editor-owned file is new, so it can never contain legacy blocks. - Report unbalanced START/END marker counts accurately. - Note that mutate() only retries conflicts and blame concurrent editors in its failure message. - Rename readForConflict to read. - classifySshHost always returns a classification; foreign and undefined were handled identically everywhere.
f8a9f7f to
5b5e0a9
Compare
EhabY
changed the title
80f6001 to
f1c13d9
Compare
Replace the per-editor include block with a single editor-agnostic block: Include "~/.local/share/coder.coder-remote/ssh/*.conf" Each (editor, deployment) pair owns one file in that shared directory (vscode--dev.coder.com.conf), fully regenerated on connect, so concurrent writers are single-writer per file and last-writer-wins is correct. The include line is identical no matter which editor writes it, so the user's config stops churning once it is in place and the cross-editor race on it disappears in steady state; the optimistic-retry machinery now only guards the include placement and legacy cleanup. The directory lives in the platform data dir (XDG_DATA_HOME, Application Support, APPDATA) instead of per-editor global storage so every editor emits the same include. OpenSSH resolves glob includes through glob(3) on every platform, including Win32-OpenSSH since v7.7, and a missing directory is a non-fatal no-match, verified by the real-ssh tests. Connects always rewrite the deployment file so its mtime marks the last connect, and any editor sweeps files older than a week on connect; the next connect to that deployment recreates its file.
f1c13d9 to
b388518
Compare
2 participants
Fixes SSH config precedence without letting VS Code-based editors overwrite one another's generated Coder hosts.
Approach
Shared SSH config directory
Every editor writes one generated file per deployment into a directory shared across editors, under the platform data dir (
$XDG_DATA_HOME,~/Library/Application Support,%APPDATA%):The effective user SSH config gets a single editor-agnostic block, placed first because OpenSSH uses the first value it finds:
On connection, the extension:
*.conffiles (from any editor) not connected to for a week; the next connect recreates them.Each generated file has a single writer, and concurrent writers of the same file produce equivalent content, so last-writer-wins is correct without locking. The Include block is byte-identical no matter which editor writes it, so once placed the user's config stops changing and concurrent editors converge instead of racing. Host patterns are namespaced per editor and deployment, making glob ordering between the files irrelevant.
Other deployments' direct blocks and the deployment-unaware historical block are preserved. Hosts matching the historical
coder-vscode--*pattern parse as foreign, so their windows keep connecting through the preserved block instead of failing setup.The generated files start with a do-not-edit header, since they are rewritten on every connection. A new Coder: Open Generated SSH Configuration command opens the file in a read-only editor: a connected window opens its own deployment's file, and a local window offers a deployment picker.
Include paths inside
$HOMEare emitted as~/..., so quirks in the home path (spaces,%, glob characters) never reach the argument ssh reads. Paths outside$HOMEare quoted with Windows separators normalized and glob characters escaped; CR, LF, NUL,", and%are rejected because ssh cannot read them back (OpenSSH 9.9+ percent-expands Include arguments and exits fatally on unknown tokens).Editor-specific authorities
The SSH host prefix uses
vscode.env.uriSchemeunchanged:coder-vscodecoder-cursorcoder-windsurfcoder-vscode-insidersThe generated wildcard host and CLI
--ssh-host-prefixuse the same prefix.Legacy authority migration
Only the actual historical
coder-vscodeauthority is migrated in other editors. Foreigncoder-*authorities are ignored rather than retargeted.Important
Migration reopens the window automatically. Connecting to a legacy authority in another editor retargets it and reopens the same folder or saved workspace once, preserving the full URI (scheme, path, query, and fragment). Empty windows reopen with the retargeted authority.
An untitled multi-root workspace cannot be reopened without dropping folders, so it keeps connecting through the old host and a modal explains how to finish the switch (save the workspace, then reload). Recent-folder entries with legacy authorities stay compatible when reopening a workspace.
Plain, URI-wrapped, and nested
ssh-remote+authorities retain their surrounding wrappers when retargeted.Concurrent config changes
All writes use atomic temporary-file rename with retries for transient Windows errors, and follow the same shape as the CLI's
config-ssh: read, merge, skip when unchanged, write atomically. No locks or optimistic retries; every editor writes an identical Include block, so concurrent updates converge, and a lost update stays shadowed by the include (first value wins) until the next connect rewrites it.Compatibility
ssh -Gagainst generated configs: a quoted glob include in a path with spaces wins over a laterHost *, escaped glob characters in the directory name resolve, two deployments and two editors are served through one glob include, and deleting the generated directory degrades gracefully instead of breaking ssh.jeanp413/open-remote-sshsource (VSCodium's Remote-SSH): it parses the config in-process viassh-config, follows top-level quoted Includes (untildify+ glob), and implements first-value-wins, on both current and years-old releases.PowerShell/openssh-portablesource and release history: every in-box Windows ssh.exe resolves Include arguments throughglob(3)(wildcard or not) since the first bundled 7.7p1, 8.1p1+ (Windows 10 2004+) also honors quoted forward-slash absolute paths, and a missing include directory is a non-fatal "matched no files".Diff size
Relative to
7382ee1:src/)test/)Validation
pnpm test: 2,316 passed, 1 skipped (includes the real-OpenSSH tests, skipped where ssh is absent)pnpm typecheckNODE_OPTIONS=--max-old-space-size=4096 pnpm lintpnpm format:checkpnpm buildgit diff --checkImplementation plan and decisions
vscode.env.uriSchemeunchanged and require a non-empty editor ID where it is consumed.START|END CODER <hostname>markers in generated files and one parameterlessSTART|END CODERinclude block in the user's config. LegacyCODER VSCODE <hostname>markers are still matched for user-config cleanup.coder-vscode, preserving wrappers and complete URI/workspace state; parse deployment-unawarecoder-vscode--*hosts as foreign.typeof-based filesystem interface; tests mocknode:fs/promiseswith memfs instead of hand-rolled fixtures.Generated by Coder Agents for @EhabY.