fix(ssh): expand ~/.ssh/config tokens with the scope each keyword allows (#2687) - #2689
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2687.
The bug
~/.ssh/configcontainingmade every SSH-tunnelled connection to a dotted host fail with
The two characters
%hreachedgetaddrinfoverbatim.Root cause
SSHConfigResolver.resolveTargettook the rawHostNamevalue as the connect host, andSSHTokenContext.expandwas wired into only two places: the identity-file list andMatch exec. There was no per-keyword token scope, no${VAR}handling, and no tilde handling outside identity files.It was not limited to tunnels.
LibSSH2SFTPSession.opengoes through the samebuildAuthenticatedChain, so remote-file and backup transfers failed the same way.Approach
Expansion is a property of the keyword, and nothing in the code said so.
SSHTokenScopenow encodes the four sentences ssh_config(5) devotes to this, and one left-to-right scanner replaces the old chain ofreplacingOccurrencescalls. That chain re-scanned its own output, so a home directory containing the characters%hcame back with the hostname spliced into it, and it needed a U+FFFF sentinel to protect%%. Both problems go away with a single pass.Hostnameis expanded inside the block loop rather than after it, becauseMatch hosthas to compare against the substituted value. That is what OpenSSH's ownmatch_cfg_linedoes.Everything asserted here was measured against OpenSSH_10.3p1, not read off the man page.
Also fixed
The audit of the two files this rewrites found nine more divergences, each with its own
ssh -Greproduction:HostName db.example.com # productionproduced %h (Tokens) in ~/.ssh/config not supported #2687's exact error from a second cause, andPort 7777 # csilently fell back to 22.Hostpatterns were matched against a substitutedHostName, so a wildcard block for a private domain applied to an alias that never mentioned it.Match execran with%pand%runexpanded, sonc -z %h %pfailed and silently dropped whatever the block set.Includenaming several files globbed the whole line as one pattern and read none of them, with nothing logged.Includeinside aHostblock leaked its directives to every connection: a production key reached a staging session.Hostblock contributed nothing, because the cycle guard never forgot a file it had finished reading.Match !host prod-dbmatched every host, including the one it named, so a dev key was offered to production.Match finaloverrode values earlier blocks had set, and a later block replaced the identity-file list instead of appending to it. ssh is first-wins across both passes.Host a,bwas read as a list. OnlyMatch hosttakes commas; on aHostline the comma is an ordinary character.Plus a
Match execwhose command ignoresSIGTERMhung the connect path forever: the timeout sentSIGTERMand then blocked inwaitUntilExit. Measured,trap '' TERM; sleep 300never returned.Review findings folded in
A security pass found no new vulnerability but flagged two things this change introduced, both fixed and both now covered by a test:
ProxyJumpwas expanded before it was split on commas, so a username containing a comma could add a jump host the config never named; and an unevaluableMatch execreported "did not hold", which satisfied a negated criterion by its own failure.A Codex review found seven more, all confirmed against ssh before acting:
${VAR}was expanded inMatch exec,HostnameandProxyJump. ssh expands it only in the file keywords and hands the command to the shell intact, sotest x${MODE:-dev} = xdevlooked up a variable literally namedMODE:-devand failed the tunnel.Match !finaland!canonicalwere routed to the pass where their un-negated form is true, so they could never apply.Match execsaw only values accumulated so far, so%pwas empty when the port was the implicit 22 and ignored a port set on the connection.%jused the first jump host; ssh uses the last, the hop nearest the target, which changes every multi-hop%C.HostKeyAliaswas never parsed, so%kalways fell back to the original host.${with no closing brace was copied through instead of reported.DEVELOPER_DIRto Xcode-beta, so it failed before running on a machine with only the standard Xcode.Deliberately not adopted
ssh lowercases the resolved hostname.
HostKeyStorekeysknown_hostson a case-sensitive[host]:port, so adopting it would fire "SSH Host Key Changed" on every mixed-case trusted entry.Include's host-dependent tokens are also out: the config is parsed once and shared by every connection, before any host is known, so those are skipped with a log line rather than resolved against the wrong target.Verification
scripts/check-ssh-config-parity.shis committed with the fix. It builds the real parser and resolver withswiftcand diffs 16 fixtures against thesshon the machine, so a future change re-checks the behaviour instead of trusting a transcription.Run against the code before this change it reports 8 of 12 fixtures disagreeing, including
hostname %h. After it reports 16 of 16 agreeing.build: PASStest: PASS, 222 executed, 222 passed, across every suite that owns a type this toucheslint: no violations in the changed files (two pre-existinglegacy_swiftui_aspect_ratiohits inSupportView.swiftandImportFromAppSourcePicker.swiftare untouched by this branch)docs: bothcheck-writing-style.shandcheck-docs-against-source.pypassNo UI automation: ssh_config resolution is headless and has no deterministic UI surface.
SSHPathUtilitiesTestswas removed because every case it held is reproduced and extended in the newSSHConfigTokensTestsandSSHPathExpansionTests.Verified in an isolated worktree, because the main checkout holds another session's in-flight work.
https://claude.ai/code/session_01H3kBkbE5KT2gYwy3xFW7UQ