mirror of
https://github.com/coder/coder.git
synced 2026-09-22 21:22:17 +08:00
Bobby Ho
66b42650ae
## Problem `coder config-ssh --ssh-host-prefix=""` (or the matching env var, `CODER_CONFIGSSH_SSH_HOST_PREFIX=`) was silently ignored, and the deprecated `Host coder.*` block was written to the SSH config anyway. The merge logic that decides whether to fall back to the server's default prefix checked `user.userHostPrefix == ""`, which is true both when the flag was never passed and when it was explicitly set to empty, so there was no way to distinguish the two. The same issue applied to `--hostname-suffix`. ## How this affects users Anyone who wants to opt out of the legacy prefix-based SSH aliases (`ssh coder.myworkspace`) in favor of the newer suffix-based ones (`ssh myworkspace.coder`) had no way to do so, the `Host coder.*` wildcard block kept reappearing on every `config-ssh` run regardless of the flag. Because that wildcard matches any hostname starting with `coder.`, not just Coder workspaces, it can silently intercept SSH connections to unrelated hosts that happen to share that prefix. It got worse on top of that: even after passing `--ssh-host-prefix=""`, running `config-ssh --use-previous-options` in a later session, a normal way to refresh local config without retyping every flag, would silently bring the block back, because the empty choice was never persisted to the file in the first place. ## Solution Track whether each option (`--ssh-host-prefix`, `--hostname-suffix`) was explicitly set by the user, as opposed to left at its zero value, and only fall back to the server default (or skip persisting the option) when it was genuinely never set. ## How it works Two new fields on `sshConfigOptions`, `userHostPrefixExplicit` and `hostnameSuffixExplicit`, carry this information: - **Live invocation**: they're set from `userSetOption(inv, ...)`, which inspects serpent's `Option.ValueSource` for the flag, right after `header`/`headerCommand` are set in the `Handler`, before any `--use-previous-options`/prompt logic can replace the struct wholesale from a prior run's saved options. - **Persistence**: `sshConfigWriteSectionHeader` now writes the `# :ssh-host-prefix=` comment line even when the value is empty, as long as it was explicit, and `sshConfigParseLastOptions` sets the field back to `true` whenever it parses that line on a later run, regardless of value. `mergeSSHOptions`'s fallback condition changed from `user.userHostPrefix == ""` to `user.userHostPrefix == "" && !user.userHostPrefixExplicit` (and the mirror for suffix). `equal()` and `asList()` were extended to include the two new fields so the `--dry-run` diff and "options differ, use new ones?" prompt stay accurate. ## Why implemented this way - Reuses `userSetOption` (`cli/util.go`), an existing helper already used for this exact "distinguish zero value from unset" problem elsewhere in the CLI (`cli/templateedit.go`), instead of inventing new machinery. - Storing the "explicit" bit as a plain field on `sshConfigOptions`, rather than as extra parameters to `mergeSSHOptions`, keeps that function dependency-free (still plain data in, plain data out, no `serpent.Invocation` coupling), while letting the same bit flow naturally through the SSH config's persisted-options comment, solving the live-flag case and the `--use-previous-options` persistence case with one mechanism instead of two. - A sentinel-value approach was considered and rejected: a self-tracking custom `serpent.Value` doesn't work because serpent applies a flag's default through the same `Value.Set()` call used for real input, so it can't tell the two apart; a plain sentinel string would work but leak into several other code paths (equality checks, diff/prompt text, the persisted comment) that would all need to filter it out. Closes https://github.com/coder/internal/issues/1208 ## Manual verification Every step below was run against a local dev server (`./scripts/develop.sh` + `./scripts/coder-dev.sh`), pointed at a throwaway `--ssh-config-file`, never a real `~/.ssh/config`. ### 1. Baseline: unchanged behavior with no flags ```sh ./scripts/coder-dev.sh config-ssh --yes --ssh-config-file "$TEST_SSH_CONFIG" cat "$TEST_SSH_CONFIG" ``` Both `Host coder.*` and `Host *.coder` are written, unchanged from before this fix (both server defaults are non-empty out of the box). <details> <summary>Output</summary> ```text Updated "/tmp/tmp.9Y7VIeuQoY" You should now be able to ssh into your workspace. For example, try running: $ ssh myworkspace.coder # ------------START-CODER----------- # This section is managed by coder. DO NOT EDIT. # # You should not hand-edit this section unless you are removing it, all # changes will be lost when running "coder config-ssh". # Host coder.* ConnectTimeout=0 StrictHostKeyChecking=no UserKnownHostsFile=/dev/null LogLevel ERROR ProxyCommand .../coder-slim ... ssh --stdio --ssh-host-prefix coder. %h Host *.coder ConnectTimeout=0 StrictHostKeyChecking=no UserKnownHostsFile=/dev/null LogLevel ERROR Match host *.coder !exec ".../coder-slim connect exists %h" ProxyCommand .../coder-slim ... ssh --stdio --hostname-suffix coder %h # ------------END-CODER------------ ``` </details> ### 2. Explicit empty `--ssh-host-prefix` omits the legacy block (the core fix) ```sh ./scripts/coder-dev.sh config-ssh --yes --ssh-config-file "$TEST_SSH_CONFIG" --ssh-host-prefix "" cat "$TEST_SSH_CONFIG" ``` `Host coder.*` is gone, only `Host *.coder` remains. The choice is now also persisted (`# :ssh-host-prefix=`). <details> <summary>Output</summary> ```text Updated "/tmp/tmp.9Y7VIeuQoY" You should now be able to ssh into your workspace. For example, try running: $ ssh myworkspace.coder # ------------START-CODER----------- # This section is managed by coder. DO NOT EDIT. # # You should not hand-edit this section unless you are removing it, all # changes will be lost when running "coder config-ssh". # # Last config-ssh options: # :ssh-host-prefix= # Host *.coder ConnectTimeout=0 StrictHostKeyChecking=no UserKnownHostsFile=/dev/null LogLevel ERROR Match host *.coder !exec ".../coder-slim connect exists %h" ProxyCommand .../coder-slim ... ssh --stdio --hostname-suffix coder %h # ------------END-CODER------------ ``` </details> ### 3. Same, via the environment variable instead of the flag ```sh CODER_CONFIGSSH_SSH_HOST_PREFIX="" ./scripts/coder-dev.sh config-ssh --yes --ssh-config-file "$TEST_SSH_CONFIG" grep -c "Host coder" "$TEST_SSH_CONFIG" ``` Confirms the fix isn't flag-only, `userSetOption` checks `ValueSource`, set the same way for `ValueSourceFlag` and `ValueSourceEnv`. <details> <summary>Output</summary> ```text Updated "/tmp/tmp.9Y7VIeuQoY" You should now be able to ssh into your workspace. For example, try running: $ ssh myworkspace.coder 0 ``` </details> ### 4. Explicit empty prefix combined with an explicit suffix ```sh ./scripts/coder-dev.sh config-ssh --yes --ssh-config-file "$TEST_SSH_CONFIG" --ssh-host-prefix "" --hostname-suffix mytest cat "$TEST_SSH_CONFIG" ``` Only `Host *.mytest` is written. Both options are correctly recorded in the persisted comment. <details> <summary>Output</summary> ```text Updated "/tmp/tmp.9Y7VIeuQoY" You should now be able to ssh into your workspace. For example, try running: $ ssh myworkspace.mytest # ------------START-CODER----------- # This section is managed by coder. DO NOT EDIT. # # You should not hand-edit this section unless you are removing it, all # changes will be lost when running "coder config-ssh". # # Last config-ssh options: # :ssh-host-prefix= # :hostname-suffix=mytest # Host *.mytest ConnectTimeout=0 StrictHostKeyChecking=no UserKnownHostsFile=/dev/null LogLevel ERROR Match host *.mytest !exec ".../coder-slim connect exists %h" ProxyCommand .../coder-slim ... ssh --stdio --hostname-suffix mytest %h # ------------END-CODER------------ ``` </details> ### 5. The explicitly-empty choice survives `--use-previous-options` with no flag repeated This is the persistence half of the fix: confirms the "omit this block" choice, once persisted, doesn't get lost on a later run that reuses previous options without repeating `--ssh-host-prefix`. Before this fix, this exact sequence would bring `Host coder.*` back. ```sh ./scripts/coder-dev.sh config-ssh --yes --ssh-config-file "$TEST_SSH_CONFIG" --ssh-host-prefix "" ./scripts/coder-dev.sh config-ssh --yes --ssh-config-file "$TEST_SSH_CONFIG" --use-previous-options cat "$TEST_SSH_CONFIG" ``` <details> <summary>Output</summary> ```text Updated "/tmp/tmp.9Y7VIeuQoY" You should now be able to ssh into your workspace. For example, try running: $ ssh myworkspace.coder No changes to make. # ------------START-CODER----------- # This section is managed by coder. DO NOT EDIT. # # You should not hand-edit this section unless you are removing it, all # changes will be lost when running "coder config-ssh". # # Last config-ssh options: # :ssh-host-prefix= # Host *.coder ConnectTimeout=0 StrictHostKeyChecking=no UserKnownHostsFile=/dev/null LogLevel ERROR Match host *.coder !exec ".../coder-slim connect exists %h" ProxyCommand .../coder-slim ... ssh --stdio --hostname-suffix coder %h # ------------END-CODER------------ ``` </details> The second command printed `No changes to make.`, and critically, `Host coder.*` did **not** reappear even though that run passed no `--ssh-host-prefix` flag at all, only `--use-previous-options`. ### 6. `--use-previous-options` still wins over this run's explicit empty flag (unaffected by this fix) Confirms this fix didn't change the pre-existing, intentional precedence of `--use-previous-options`: a previously-saved *non-empty* value still wins over an explicit empty flag passed on a later run. ```sh ./scripts/coder-dev.sh config-ssh --yes --ssh-config-file "$TEST_SSH_CONFIG" --ssh-host-prefix "custom-test." ./scripts/coder-dev.sh config-ssh --yes --ssh-config-file "$TEST_SSH_CONFIG" --use-previous-options --ssh-host-prefix "" cat "$TEST_SSH_CONFIG" ``` <details> <summary>Output</summary> ```text Updated "/tmp/tmp.9Y7VIeuQoY" You should now be able to ssh into your workspace. For example, try running: $ ssh myworkspace.coder No changes to make. # ------------START-CODER----------- # This section is managed by coder. DO NOT EDIT. # # You should not hand-edit this section unless you are removing it, all # changes will be lost when running "coder config-ssh". # # Last config-ssh options: # :ssh-host-prefix=custom-test. # Host custom-test.* ConnectTimeout=0 StrictHostKeyChecking=no UserKnownHostsFile=/dev/null LogLevel ERROR ProxyCommand .../coder-slim ... ssh --stdio --ssh-host-prefix custom-test. %h Host *.coder ConnectTimeout=0 StrictHostKeyChecking=no UserKnownHostsFile=/dev/null LogLevel ERROR Match host *.coder !exec ".../coder-slim connect exists %h" ProxyCommand .../coder-slim ... ssh --stdio --hostname-suffix coder %h # ------------END-CODER------------ ``` </details> `Host custom-test.*` is preserved verbatim, `--use-previous-options` correctly overrides the explicit empty flag when the saved value is non-empty, the mirror image of step 5's explicit-empty saved value. ### 7. End-to-end sanity check with a real workspace ```sh ./scripts/coder-dev.sh config-ssh --yes --ssh-config-file "$TEST_SSH_CONFIG" --ssh-host-prefix "" --hostname-suffix mytest ssh -F "$TEST_SSH_CONFIG" -o ConnectTimeout=15 myworkspace.mytest echo ok ``` <details> <summary>Output</summary> ```text Updated "/tmp/tmp.9Y7VIeuQoY" You should now be able to ssh into your workspace. For example, try running: $ ssh myworkspace.mytest ok ``` </details> `ok` came back from a real, running workspace, confirming the ProxyCommand and Match/exec wiring generated by the suffix-only config actually establishes a working SSH session end-to-end, not just a text-generation check.