Skip to content

Commit 663ab4a

Browse files
committed
fix(sshConn): evaluate Match exec blocks and honour ProxyJump=none
pretty resolves ssh_config entries for every target and every jump host, but the sshconfig.Context passed to the resolver left Context.Exec unset. The ssh_config library silently evaluates every 'Match host X exec "..."' block as non-matching when Exec is nil, so directives that pick a reachable jump host dynamically - common in corporate configs, e.g. Match host jump-alias exec "nc -zG 1 primary.example.net 22" HostName primary.example.net Match host jump-alias exec "nc -zG 1 secondary.example.net 22" HostName secondary.example.net never applied. Their HostName override was skipped, the alias stayed literal, and the connection failed at DNS resolution with 'no such host'. Fix: wire an Exec callback through the resolver. - Introduce shellMatchExec: runs the already-token-expanded command via /bin/sh -c (cmd.exe /C on Windows), with stdin/stdout/stderr detached and a 10s timeout cap so a misbehaving probe can't hang the CLI. - Expose it via a package-level matchExecFunc var so tests can stub exec evaluation deterministically without shelling out. This matches the existing stubbing pattern used for connectionFunc / sessionFunc. - Pass matchExecFunc as Context.Exec from SSHConfigResolver.resolve so every host and jump-host resolution benefits without any caller changes in cmd/. Follow-on correctness fix surfaced by the above: once Match exec starts actually firing, 'Match originalhost "jump-*" ProxyJump none' patterns produce ProxyJump="none". OpenSSH treats that as an explicit opt-out that cancels inherited ProxyJump rules, but pretty was parsing it as a literal jump host named "none" and trying to dial it. ResolveHost now drops 'none' before calling ParseProxyJump, and ParseProxyJump also collapses to an empty slice whenever any component equals "none" (case insensitive), guaranteeing no caller ever sees the sentinel. Tests: - TestResolveHostMatchExecApplies: first matching exec probe wins and its HostName is returned. - TestResolveHostMatchExecAllFailKeepsAlias: when every probe fails the alias is left unchanged. - TestShellMatchExecSucceedsForTrue / FailsForFalse / EmptyCmd: pin the default shell callback's semantics on POSIX. - TestParseProxyJumpNoneDisablesJumps: covers 'none', 'None', 'NONE', and surrounding whitespace. - TestResolveHostProxyJumpNoneClearsJumps: end-to-end sanity check that a specific-block ProxyJump=none beats a wildcard block and results in zero jumps. No changes required in ncode/ssh_config: it already parses Match exec and expands %-tokens correctly; it just needs the caller to provide a runtime Exec callback, which is this change.
1 parent 45b9942 commit 663ab4a

2 files changed

Lines changed: 213 additions & 4 deletions

File tree

internal/sshConn/config.go

Lines changed: 72 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,64 @@
11
package sshConn
22

33
import (
4+
"context"
45
"errors"
56
"fmt"
67
"os"
8+
"os/exec"
79
"os/user"
810
"path/filepath"
11+
"runtime"
912
"strconv"
1013
"strings"
14+
"time"
1115

1216
sshconfig "github.com/ncode/ssh_config"
1317
"golang.org/x/crypto/ssh"
1418
)
1519

20+
// matchExecTimeout bounds how long a `Match ... exec "..."` probe may run.
21+
// OpenSSH itself does not impose a timeout, but pretty resolves many hosts in
22+
// a row (including per-host ProxyJump resolution) so we cap each probe to
23+
// avoid hanging the CLI if a user's exec command never returns.
24+
const matchExecTimeout = 10 * time.Second
25+
26+
// matchExecFunc evaluates `Match ... exec "<cmd>"` directives. It is a package
27+
// variable so tests can substitute a deterministic stub without shelling out.
28+
// It must return (true, nil) if the command exits with status 0, (false, nil)
29+
// otherwise, and only return a non-nil error for conditions the caller should
30+
// surface (currently unused by the resolver because we do not opt into strict
31+
// mode).
32+
var matchExecFunc = shellMatchExec
33+
34+
// shellMatchExec runs cmd via the local shell (sh -c / cmd /C on Windows) and
35+
// reports whether it succeeded. The command has already had its %-tokens
36+
// expanded by ssh_config before this function is invoked.
37+
func shellMatchExec(cmd string) (bool, error) {
38+
if strings.TrimSpace(cmd) == "" {
39+
return false, nil
40+
}
41+
ctx, cancel := context.WithTimeout(context.Background(), matchExecTimeout)
42+
defer cancel()
43+
44+
var c *exec.Cmd
45+
if runtime.GOOS == "windows" {
46+
c = exec.CommandContext(ctx, "cmd", "/C", cmd)
47+
} else {
48+
c = exec.CommandContext(ctx, "/bin/sh", "-c", cmd)
49+
}
50+
c.Stdin = nil
51+
c.Stdout = nil
52+
c.Stderr = nil
53+
54+
if err := c.Run(); err != nil {
55+
// Any non-zero exit or failure to launch counts as "did not match",
56+
// mirroring OpenSSH's treatment of Match exec probes.
57+
return false, nil
58+
}
59+
return true, nil
60+
}
61+
1662
type SSHConfigPaths struct {
1763
User string
1864
System string
@@ -134,7 +180,10 @@ func (r *SSHConfigResolver) ResolveHost(spec HostSpec, fallbackUser string) (Res
134180
if err != nil {
135181
return ResolvedHost{}, err
136182
}
137-
if proxyJump != "" {
183+
// OpenSSH treats `ProxyJump none` as an explicit opt-out that cancels
184+
// ProxyJump inherited from broader-matching blocks. Skip parsing in that
185+
// case so we don't try to dial the literal host "none".
186+
if proxyJump != "" && !strings.EqualFold(strings.TrimSpace(proxyJump), "none") {
138187
resolved.ProxyJump = ParseProxyJump(proxyJump)
139188
}
140189

@@ -200,7 +249,18 @@ func (r *SSHConfigResolver) resolve(cfg *sshconfig.Config, alias string) (*sshco
200249
if cfg == nil {
201250
return nil, nil
202251
}
203-
ctx := sshconfig.Context{HostArg: alias, OriginalHost: alias, LocalUser: currentUser()}
252+
ctx := sshconfig.Context{
253+
HostArg: alias,
254+
OriginalHost: alias,
255+
LocalUser: currentUser(),
256+
// Providing Exec enables `Match host X exec "..."` blocks to be
257+
// evaluated. Without it the library silently treats every exec
258+
// predicate as non-matching, which causes directives like
259+
// Match host jump-alias exec "nc -zG 1 primary.example.net 22"
260+
// HostName primary.example.net
261+
// to be skipped, leaving the alias unresolvable via DNS.
262+
Exec: matchExecFunc,
263+
}
204264
return cfg.Resolve(ctx)
205265
}
206266

@@ -231,14 +291,22 @@ func expandPath(path string) string {
231291
return path
232292
}
233293

294+
// ParseProxyJump splits a ProxyJump value into individual jump hosts. Empty
295+
// components are dropped, and the OpenSSH "none" sentinel (which disables
296+
// ProxyJump) collapses the result to an empty slice so callers never try to
297+
// dial a literal host named "none".
234298
func ParseProxyJump(value string) []string {
235299
parts := strings.Split(value, ",")
236300
jumps := make([]string, 0, len(parts))
237301
for _, part := range parts {
238302
trimmed := strings.TrimSpace(part)
239-
if trimmed != "" {
240-
jumps = append(jumps, trimmed)
303+
if trimmed == "" {
304+
continue
305+
}
306+
if strings.EqualFold(trimmed, "none") {
307+
return nil
241308
}
309+
jumps = append(jumps, trimmed)
242310
}
243311
return jumps
244312
}

internal/sshConn/config_test.go

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"os"
1111
"path/filepath"
1212
"reflect"
13+
"runtime"
1314
"testing"
1415

1516
"golang.org/x/crypto/ssh"
@@ -181,6 +182,112 @@ func TestLoadIdentityFilesReturnsErrorForMalformedPrivateKey(t *testing.T) {
181182
}
182183
}
183184

185+
func TestResolveHostMatchExecApplies(t *testing.T) {
186+
// Models the production pattern where a jump alias resolves to a concrete
187+
// HostName only when the probe command succeeds, e.g.:
188+
// Match host jump-alias exec "nc -zG 1 primary.example.net 22"
189+
// HostName primary.example.net
190+
cfg := `Match host jump-alias exec "probe-primary"
191+
HostName primary.example.net
192+
Match host jump-alias exec "probe-secondary"
193+
HostName secondary.example.net
194+
`
195+
userCfg := writeTempConfig(t, cfg)
196+
resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg})
197+
if err != nil {
198+
t.Fatalf("unexpected error: %v", err)
199+
}
200+
201+
var seen []string
202+
stubExec(t, func(cmd string) (bool, error) {
203+
seen = append(seen, cmd)
204+
return cmd == "probe-primary", nil
205+
})
206+
207+
resolved, err := resolver.ResolveHost(HostSpec{Host: "jump-alias"}, "me")
208+
if err != nil {
209+
t.Fatalf("unexpected error: %v", err)
210+
}
211+
if resolved.Host != "primary.example.net" {
212+
t.Fatalf("expected primary HostName from matching exec block, got %q", resolved.Host)
213+
}
214+
if len(seen) == 0 {
215+
t.Fatalf("expected exec callback to be invoked, got no calls")
216+
}
217+
}
218+
219+
func TestResolveHostMatchExecAllFailKeepsAlias(t *testing.T) {
220+
cfg := `Match host jump-alias exec "probe-primary"
221+
HostName primary.example.net
222+
Match host jump-alias exec "probe-secondary"
223+
HostName secondary.example.net
224+
`
225+
userCfg := writeTempConfig(t, cfg)
226+
resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg})
227+
if err != nil {
228+
t.Fatalf("unexpected error: %v", err)
229+
}
230+
231+
stubExec(t, func(cmd string) (bool, error) { return false, nil })
232+
233+
resolved, err := resolver.ResolveHost(HostSpec{Host: "jump-alias"}, "me")
234+
if err != nil {
235+
t.Fatalf("unexpected error: %v", err)
236+
}
237+
// With every exec probe failing, no HostName override applies and the
238+
// alias stays as-is - this is the same state that previously surfaced as
239+
// a DNS "no such host" when wiring was missing entirely.
240+
if resolved.Host != "jump-alias" {
241+
t.Fatalf("expected alias to remain when all exec probes fail, got %q", resolved.Host)
242+
}
243+
}
244+
245+
func TestShellMatchExecSucceedsForTrue(t *testing.T) {
246+
if runtime.GOOS == "windows" {
247+
t.Skip("POSIX shell not available on Windows CI images")
248+
}
249+
ok, err := shellMatchExec("true")
250+
if err != nil {
251+
t.Fatalf("unexpected error: %v", err)
252+
}
253+
if !ok {
254+
t.Fatal("expected true command to succeed")
255+
}
256+
}
257+
258+
func TestShellMatchExecFailsForFalse(t *testing.T) {
259+
if runtime.GOOS == "windows" {
260+
t.Skip("POSIX shell not available on Windows CI images")
261+
}
262+
ok, err := shellMatchExec("false")
263+
if err != nil {
264+
t.Fatalf("unexpected error: %v", err)
265+
}
266+
if ok {
267+
t.Fatal("expected false command to report non-match")
268+
}
269+
}
270+
271+
func TestShellMatchExecEmptyCmd(t *testing.T) {
272+
ok, err := shellMatchExec(" ")
273+
if err != nil {
274+
t.Fatalf("unexpected error: %v", err)
275+
}
276+
if ok {
277+
t.Fatal("expected empty command to report non-match")
278+
}
279+
}
280+
281+
// stubExec replaces the package-level matchExecFunc for the duration of a test
282+
// and restores it on cleanup. This keeps Match exec evaluation deterministic
283+
// without shelling out.
284+
func stubExec(t *testing.T, fn func(cmd string) (bool, error)) {
285+
t.Helper()
286+
orig := matchExecFunc
287+
matchExecFunc = fn
288+
t.Cleanup(func() { matchExecFunc = orig })
289+
}
290+
184291
func TestClientConfigForCombinedAuth(t *testing.T) {
185292
socketPath := startTestAgent(t)
186293
t.Setenv("SSH_AUTH_SOCK", socketPath)
@@ -206,6 +313,40 @@ func TestParseProxyJump(t *testing.T) {
206313
}
207314
}
208315

316+
func TestParseProxyJumpNoneDisablesJumps(t *testing.T) {
317+
// OpenSSH: `ProxyJump none` cancels any inherited ProxyJump; it must not
318+
// be treated as a literal hop.
319+
for _, in := range []string{"none", "None", "NONE", " none "} {
320+
if got := ParseProxyJump(in); len(got) != 0 {
321+
t.Fatalf("ParseProxyJump(%q) = %v, want empty", in, got)
322+
}
323+
}
324+
}
325+
326+
func TestResolveHostProxyJumpNoneClearsJumps(t *testing.T) {
327+
// OpenSSH: first matching directive wins for single-valued options, so
328+
// the specific block declaring `ProxyJump none` must come before the
329+
// wildcard block that sets a jump. The resolved value must collapse to
330+
// no jumps rather than the literal host "none".
331+
cfg := `Host solo.internal
332+
ProxyJump none
333+
Host *.internal
334+
ProxyJump jump1
335+
`
336+
userCfg := writeTempConfig(t, cfg)
337+
resolver, err := LoadSSHConfig(SSHConfigPaths{User: userCfg})
338+
if err != nil {
339+
t.Fatalf("unexpected error: %v", err)
340+
}
341+
resolved, err := resolver.ResolveHost(HostSpec{Host: "solo.internal"}, "me")
342+
if err != nil {
343+
t.Fatalf("unexpected error: %v", err)
344+
}
345+
if len(resolved.ProxyJump) != 0 {
346+
t.Fatalf("expected no proxy jumps when ProxyJump=none wins, got %v", resolved.ProxyJump)
347+
}
348+
}
349+
209350
func TestResolveProxyJumpSingle(t *testing.T) {
210351
cfg := "Host target\n ProxyJump jump1\n"
211352
userCfg := writeTempConfig(t, cfg)

0 commit comments

Comments
 (0)