fix: don't dial a ProxyJump hop on the target's port - #337
Open
NatLee wants to merge 1 commit into
Open
Conversation
The port of a jump host was computed in two places with different fallbacks: connecting to the hop fell back to the target's port, while the forwardOut reaching the next hop fell back to 22. A jump host does not inherit the target's Port, so a target declared on a non-default port had its first hop dialed on that port. Use one helper for both, falling back to 22 like ssh does, and cover the ProxyJump path with unit tests for the resolution order plus a two container e2e where the target is only reachable through the hop.
Contributor
Author
|
CI red is |
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.
Split out of #334 at @GitMensch's suggestion, so the host key work stays purely about host keys.
The bug
The port of a
ProxyJumphop was computed in two places, and they disagreed:With no
Porton the hop, the first falls back to the target's port and the second to 22. A jump host doesn't inherit the target'sPort, so a target declared on a non-default port had its first hop dialed on that port.That's the first half of #109:
Host tesetsPort 11111, so its jump host was dialed on 11111 — matching the report thatfirstandsecondconnect buttedoesn't. (NestedProxyJumpnot being expanded, the other half of that report, is untouched here.)Both sites now use one helper that falls back to 22, like ssh.
Tests
Proxies had no coverage at all, so this adds both levels:
test/proxy-jump-port.test.ts— 5 unit tests pinning the resolution order (hopPort> port in theProxyJumpvalue > 22). The last two fail if the fallback goes back to the target's port, so the regression is actually guarded.test/proxy-jump.test.ts— e2e over two containers on a private network. Only the jump is published; the target is reachable solely by its container name inside the network, so connecting at all proves the hop was used.Full suite passes locally (12 tests, 5 fixtures + forward-agent + the 6 added here).
One note on the e2e: container names become DNS labels on the network, so it uses a short suffix — a full UUID pushes the name past the 63 character limit and resolution fails.
Not covered
The default-to-22 path can't be exercised end to end without a jump host on port 22, which isn't reasonable to bind in CI, so that case is pinned by the unit tests instead.