fix(ssh): follow a jump host's own ProxyJump and name the tunnel before connecting (#2687) - #2691
Merged
Merged
Conversation
…re connecting (#2687) Claude-Session: https://claude.ai/code/session_01H3kBkbE5KT2gYwy3xFW7UQ
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.
Follow-up to #2689. Two defects found while investigating #2687, verified before building.
A jump host's own
ProxyJumpwas never followedbuildAuthenticatedChainbuilt the hop list fromresolvedPrimary.proxyJumpalone, so a bastion that declares a bastion of its own was skipped.Measured with OpenSSH_10.3p1 on:
ssh -v targetissues two proxy commands,-W [bee.example.com]:22 aythen-W [10.0.0.9]:22 bee, so the real chain isaythenbeethentarget. TablePro stopped atbeeand tried to reach it directly, which is either unreachable or the wrong machine.resolveJumpChainnow walks each hop's ownProxyJumpfirst, so the hop nearest the user comes first. It stops at ten levels, which is where ssh stops, so a pair of hosts naming each other terminates instead of exhausting the stack.The confirmation never named the SSH server
The external-link alert listed host, user and database only. A tunnelled connection's database host is the far end of the tunnel, usually
localhost, so the alert asked "connect only if you trust the source of this link" while naming none of the machines the session actually crosses. It now lists the SSH server and every jump host.The import sheet had the SSH host and auth method but not the SSH username or the jump hosts; both are shown now.
Two related claims that turned out to be wrong
Worth recording, because they look plausible and I reported them before checking:
scheme+ssh://form (ConnectionURLParser.swift:289), and every other path passessshHost: nilexplicitly.Info.plistregisters no+sshscheme, so those URLs never reach the app. No externally-supplied SSH target can reach the trust store, so the missing fields cannot be exploited.ExternalConnectionGatehas one production call site, but the four automatic paths that were said to bypass it do not carry externally-supplied connections: reopen-last-session filters to saved ids, Handoff carries only a connection id for a window the user already opened, tunnel recovery rebuilds a tunnel for a session that is already connected, and MCP runs every call throughMCPAuthPolicy.resolveAndAuthorizefirst.Verification
build: PASStest: PASS, 132 executed, 132 passedscripts/check-ssh-config-parity.sh: 16 of 16 fixtures still agree withsshlint: no violations in the changed filesmainafter fix(ssh): expand ~/.ssh/config tokens with the scope each keyword allows (#2687) #2689 merged; re-verified after the rebase.The jump-chain change has unit coverage for the nested case, a three-level chain, several hops each expanding in place, and a cycle. The alert change is covered by tests that read the built
NSAlert's text, including a disabled tunnel that must stay unmentioned. No UI automation: the alert is modal and the import sheet needs a deeplink, neither of which runs deterministically under XCUITest.https://claude.ai/code/session_01H3kBkbE5KT2gYwy3xFW7UQ