Skip to content

Refactor ssh option passing #723

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
selzoc merged 1 commit into from
May 5, 2026
+109 −13
Merged

Refactor ssh option passing #723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions ssh/ssh_args.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func NewSSHArgs(connOpts ConnectionOpts, result boshdir.SSHResult, forceTTY bool
}

func (a SSHArgs) LoginForHost(host boshdir.Host) []string {
return []string{host.Host, "-l", host.Username}
return []string{"-l", host.Username, "--", host.Host}
}

func formProxyOpt(existenceChecker cmdExistenceChecker, proxyHostString string) string {
Expand All @@ -71,6 +71,12 @@ func fmtAsProxyCommandOpt(command, proxyHost string) string {
return fmt.Sprintf("ProxyCommand=%s", fmt.Sprintf(command, proxyHost))
}

// shellQuote wraps s in POSIX single-quotes, escaping any embedded single-quote
// with the standard '\” idiom.
func shellQuote(s string) string {
return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
}

func (a SSHArgs) OptsForHost(host boshdir.Host) []string {
// Options are used for both ssh and scp
cmdOpts := []string{}
Expand Down Expand Up @@ -132,8 +138,8 @@ func (a SSHArgs) OptsForHost(host boshdir.Host) []string {
// Always force TTY for gateway ssh
"ProxyCommand=ssh -tt -W %s -l %s %s %s",
proxyHostPortTmpl,
gwUsername,
gwHost,
shellQuote(gwUsername),
shellQuote(gwHost),
strings.Join(gwCmdOpts, " "),
)

Expand Down
110 changes: 100 additions & 10 deletions ssh/ssh_args_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ var _ = Describe("SSHArgs", func() {
return SSHArgs{}.LoginForHost(host)
}

It("returns login details with IPv4", func() {
Expect(act()).To(Equal([]string{"127.0.0.1", "-l", "user"}))
It("returns login details with IPv4, placing '--' before host to stop option parsing", func() {
Expect(act()).To(Equal([]string{"-l", "user", "--", "127.0.0.1"}))
})

It("returns login details with IPv6 non-bracketed", func() {
It("returns login details with IPv6, placing '--' before host to stop option parsing", func() {
host.Host = "::1"
Expect(act()).To(Equal([]string{"::1", "-l", "user"}))
Expect(act()).To(Equal([]string{"-l", "user", "--", "::1"}))
})
})

Expand Down Expand Up @@ -118,7 +118,7 @@ var _ = Describe("SSHArgs", func() {
"-o", "IdentitiesOnly=yes",
"-o", "IdentityFile=/tmp/priv-key",
"-o", "UserKnownHostsFile=/tmp/known-hosts",
"-o", "ProxyCommand=ssh -tt -W %h:%p -l gw-user gw-host -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
"-o", "ProxyCommand=ssh -tt -W %h:%p -l 'gw-user' 'gw-host' -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
}))
})

Expand All @@ -135,7 +135,7 @@ var _ = Describe("SSHArgs", func() {
"-o", "IdentitiesOnly=yes",
"-o", "IdentityFile=/tmp/priv-key",
"-o", "UserKnownHostsFile=/tmp/known-hosts",
"-o", "ProxyCommand=ssh -tt -W %h:%p -l gw-user gw-host -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o PasswordAuthentication=no -o IdentitiesOnly=yes -o IdentityFile=/tmp/gw-priv-key",
"-o", "ProxyCommand=ssh -tt -W %h:%p -l 'gw-user' 'gw-host' -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o PasswordAuthentication=no -o IdentitiesOnly=yes -o IdentityFile=/tmp/gw-priv-key",
}))
})

Expand All @@ -153,7 +153,7 @@ var _ = Describe("SSHArgs", func() {
"-o", "IdentitiesOnly=yes",
"-o", "IdentityFile=/tmp/priv-key",
"-o", "UserKnownHostsFile=/tmp/known-hosts",
"-o", "ProxyCommand=ssh -tt -W %h:%p -l user-gw-user user-gw-host -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
"-o", "ProxyCommand=ssh -tt -W %h:%p -l 'user-gw-user' 'user-gw-host' -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
}))
})

Expand Down Expand Up @@ -249,7 +249,7 @@ var _ = Describe("SSHArgs", func() {
"-o", "IdentitiesOnly=yes",
"-o", "IdentityFile=/tmp/priv-key",
"-o", "UserKnownHostsFile=/tmp/known-hosts",
"-o", "ProxyCommand=ssh -tt -W [%h]:%p -l gw-user gw-host -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
"-o", "ProxyCommand=ssh -tt -W [%h]:%p -l 'gw-user' 'gw-host' -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
}))
})

Expand All @@ -264,7 +264,7 @@ var _ = Describe("SSHArgs", func() {
"-o", "IdentitiesOnly=yes",
"-o", "IdentityFile=/tmp/priv-key",
"-o", "UserKnownHostsFile=/tmp/known-hosts",
"-o", "ProxyCommand=ssh -tt -W %h:%p -l gw-user ::1 -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
"-o", "ProxyCommand=ssh -tt -W %h:%p -l 'gw-user' '::1' -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
}))
})

Expand All @@ -280,10 +280,37 @@ var _ = Describe("SSHArgs", func() {
"-o", "IdentitiesOnly=yes",
"-o", "IdentityFile=/tmp/priv-key",
"-o", "UserKnownHostsFile=/tmp/known-hosts",
"-o", "ProxyCommand=ssh -tt -W [%h]:%p -l gw-user ::1 -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
"-o", "ProxyCommand=ssh -tt -W [%h]:%p -l 'gw-user' '::1' -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
}))
})

It("single-quotes gateway username", func() {
result.GatewayUsername = "vcap; exit 1"
result.GatewayHost = "gw-host"

Expect(act()).To(ContainElement(
"ProxyCommand=ssh -tt -W %h:%p -l 'vcap; exit 1' 'gw-host' -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
))
})

It("single-quotes gateway host", func() {
result.GatewayUsername = "gw-user"
result.GatewayHost = "x; exit 1 #"

Expect(act()).To(ContainElement(
"ProxyCommand=ssh -tt -W %h:%p -l 'gw-user' 'x; exit 1 #' -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
))
})

It("escapes embedded single-quotes in gateway fields using the POSIX '\\'' idiom", func() {
result.GatewayUsername = "gw-user"
result.GatewayHost = "host'with'quotes"

Expect(act()).To(ContainElement(
"ProxyCommand=ssh -tt -W %h:%p -l 'gw-user' 'host'\\''with'\\''quotes' -o ServerAliveInterval=30 -o ForwardAgent=no -o ClearAllForwardings=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
))
})

It("returns ssh options non-bracketed if host is IPv6 and SOCKS5Proxy is set", func() {
host = boshdir.Host{Host: "::1", Username: "user"}
connOpts.SOCKS5Proxy = "socks5://some-proxy"
Expand All @@ -299,4 +326,67 @@ var _ = Describe("SSHArgs", func() {
}))
})
})

Describe("combined OptsForHost + LoginForHost assembly", func() {
buildArgs := func() []string {
args := SSHArgs{
ConnOpts: connOpts,
Result: result,
ForceTTY: forceTTY,
PrivKeyFile: privKeyFile,
KnownHostsFile: knownHostsFile,
CmdExistenceChecker: connectProxyCmdRunner,
Socks5Proxy: proxy.NewSocks5Proxy(proxy.NewHostKey(), log.New(io.Discard, "", log.LstdFlags), 1*time.Minute),
}
return append(args.OptsForHost(host), args.LoginForHost(host)...)
}

It("places '--' immediately before the host in the combined argument slice", func() {
combined := buildArgs()

separatorIdx := -1
for i, arg := range combined {
if arg == "--" {
separatorIdx = i
break
}
}

Expect(separatorIdx).To(BeNumerically(">", 0), "\"--\" must be present in the combined argument slice")
Expect(combined[separatorIdx+1]).To(Equal(host.Host), "host must be the element immediately after \"--\"")
})

It("places all -o flags before '--'", func() {
combined := buildArgs()

separatorIdx := -1
for i, arg := range combined {
if arg == "--" {
separatorIdx = i
break
}
}
Expect(separatorIdx).To(BeNumerically(">", 0))

for i, arg := range combined[:separatorIdx] {
if arg == "-o" {
Expect(i).To(BeNumerically("<", separatorIdx),
"\"-o\" at index %d must appear before \"--\" at index %d", i, separatorIdx)
}
}
})

It("ensures '--' does not appear anywhere in OptsForHost output", func() {
args := SSHArgs{
ConnOpts: connOpts,
Result: result,
ForceTTY: forceTTY,
PrivKeyFile: privKeyFile,
KnownHostsFile: knownHostsFile,
CmdExistenceChecker: connectProxyCmdRunner,
Socks5Proxy: proxy.NewSocks5Proxy(proxy.NewHostKey(), log.New(io.Discard, "", log.LstdFlags), 1*time.Minute),
}
Expect(args.OptsForHost(host)).NotTo(ContainElement("--"))
})
})
})