Skip to content

feat(cli): support header and header-command in config-ssh #10413

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
merged 10 commits into from
Feb 7, 2024
Merged

feat(cli): support header and header-command in config-ssh #10413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
42a7512
feat(configssh): add any provided header command to the SSH config also
JoshVee Oct 19, 2023
9a9ff38
chore: escape command also
JoshVee Oct 19, 2023
57355fb
Merge branch 'main' into joshvee-ssh-header-command
JoshVee Oct 19, 2023
1220039
chore: fix up the variable name
JoshVee Oct 19, 2023
a785392
chore: more test cases
JoshVee Oct 19, 2023
c5e0ec1
chore: fix tests
JoshVee Oct 19, 2023
7fc1849
Merge branch 'coder:main' into joshvee-ssh-header-command
JoshVee Oct 30, 2023
ed5e82b
Merge branch 'main' into joshvee-ssh-header-command
mafredri Feb 5, 2024
1c585c4
remove echoResponses
mafredri Feb 5, 2024
bcdb129
add support for serializing header and header-command
mafredri Feb 5, 2024
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
58 changes: 49 additions & 9 deletions cli/configssh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/cli/safeexec"
"github.com/pkg/diff"
"github.com/pkg/diff/write"
"golang.org/x/exp/constraints"
"golang.org/x/exp/slices"
"golang.org/x/sync/errgroup"
"golang.org/x/xerrors"
Expand Down Expand Up @@ -51,6 +52,8 @@ type sshConfigOptions struct {
userHostPrefix string
sshOptions []string
disableAutostart bool
header []string
headerCommand string
}

// addOptions expects options in the form of "option=value" or "option value".
Expand Down Expand Up @@ -100,15 +103,25 @@ func (o *sshConfigOptions) addOption(option string) error {
}

func (o sshConfigOptions) equal(other sshConfigOptions) bool {
// Compare without side-effects or regard to order.
opt1 := slices.Clone(o.sshOptions)
sort.Strings(opt1)
opt2 := slices.Clone(other.sshOptions)
sort.Strings(opt2)
if !slices.Equal(opt1, opt2) {
if !slicesSortedEqual(o.sshOptions, other.sshOptions) {
return false
}
return o.waitEnum == other.waitEnum && o.userHostPrefix == other.userHostPrefix && o.disableAutostart == other.disableAutostart
if !slicesSortedEqual(o.header, other.header) {
return false
}
return o.waitEnum == other.waitEnum && o.userHostPrefix == other.userHostPrefix && o.disableAutostart == other.disableAutostart && o.headerCommand == other.headerCommand
}

// slicesSortedEqual compares two slices without side-effects or regard to order.
func slicesSortedEqual[S ~[]E, E constraints.Ordered](a, b S) bool {
if len(a) != len(b) {
return false
}
a = slices.Clone(a)
slices.Sort(a)
b = slices.Clone(b)
slices.Sort(b)
Comment on lines +120 to +123
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: curious, can we replace slices.Clone with a simple copy to omit cloning and just operate on same objects?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Clone returns a copy of the slice.
// The elements are copied using assignment, so this is a shallow clone.

It's actually just a simple copy, so I think it's fine.

return slices.Equal(a, b)
}

func (o sshConfigOptions) asList() (list []string) {
Expand All @@ -124,6 +137,13 @@ func (o sshConfigOptions) asList() (list []string) {
for _, opt := range o.sshOptions {
list = append(list, fmt.Sprintf("ssh-option: %s", opt))
}
for _, h := range o.header {
list = append(list, fmt.Sprintf("header: %s", h))
}
if o.headerCommand != "" {
list = append(list, fmt.Sprintf("header-command: %s", o.headerCommand))
}

return list
}

Expand Down Expand Up @@ -230,6 +250,8 @@ func (r *RootCmd) configSSH() *clibase.Cmd {
// specifies skip-proxy-command, then wait cannot be applied.
return xerrors.Errorf("cannot specify both --skip-proxy-command and --wait")
}
sshConfigOpts.header = r.header
sshConfigOpts.headerCommand = r.headerCommand

recvWorkspaceConfigs := sshPrepareWorkspaceConfigs(inv.Context(), client)

Expand Down Expand Up @@ -393,6 +415,14 @@ func (r *RootCmd) configSSH() *clibase.Cmd {
}

if !skipProxyCommand {
rootFlags := fmt.Sprintf("--global-config %s", escapedGlobalConfig)
for _, h := range sshConfigOpts.header {
rootFlags += fmt.Sprintf(" --header %q", h)
}
if sshConfigOpts.headerCommand != "" {
rootFlags += fmt.Sprintf(" --header-command %q", sshConfigOpts.headerCommand)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: We shouldn't need to do special escaping here like we do for the coder binary, that's because we're simply passing a string argument to coder.

}

flags := ""
if sshConfigOpts.waitEnum != "auto" {
flags += " --wait=" + sshConfigOpts.waitEnum
Expand All @@ -401,8 +431,8 @@ func (r *RootCmd) configSSH() *clibase.Cmd {
flags += " --disable-autostart=true"
}
defaultOptions = append(defaultOptions, fmt.Sprintf(
"ProxyCommand %s --global-config %s ssh --stdio%s %s",
escapedCoderBinary, escapedGlobalConfig, flags, workspaceHostname,
"ProxyCommand %s %s ssh --stdio%s %s",
escapedCoderBinary, rootFlags, flags, workspaceHostname,
))
}

Expand Down Expand Up @@ -623,6 +653,12 @@ func sshConfigWriteSectionHeader(w io.Writer, addNewline bool, o sshConfigOption
for _, opt := range o.sshOptions {
_, _ = fmt.Fprintf(&ow, "# :%s=%s\n", "ssh-option", opt)
}
for _, h := range o.header {
_, _ = fmt.Fprintf(&ow, "# :%s=%s\n", "header", h)
}
if o.headerCommand != "" {
_, _ = fmt.Fprintf(&ow, "# :%s=%s\n", "header-command", o.headerCommand)
}
if ow.Len() > 0 {
_, _ = fmt.Fprint(w, sshConfigOptionsHeader)
_, _ = fmt.Fprint(w, ow.String())
Expand Down Expand Up @@ -654,6 +690,10 @@ func sshConfigParseLastOptions(r io.Reader) (o sshConfigOptions) {
o.sshOptions = append(o.sshOptions, parts[1])
case "disable-autostart":
o.disableAutostart, _ = strconv.ParseBool(parts[1])
case "header":
o.header = append(o.header, parts[1])
case "header-command":
o.headerCommand = parts[1]
default:
// Unknown option, ignore.
}
Expand Down
55 changes: 55 additions & 0 deletions cli/configssh_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,9 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) {
"# Last config-ssh options:",
"# :wait=yes",
"# :ssh-host-prefix=coder-test.",
"# :header=X-Test-Header=foo",
"# :header=X-Test-Header2=bar",
"# :header-command=printf h1=v1 h2=\"v2\" h3='v3'",
"#",
headerEnd,
"",
Expand All @@ -471,6 +474,9 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) {
"--yes",
"--wait=yes",
"--ssh-host-prefix", "coder-test.",
"--header", "X-Test-Header=foo",
"--header", "X-Test-Header2=bar",
"--header-command", "printf h1=v1 h2=\"v2\" h3='v3'",
},
},
{
Expand Down Expand Up @@ -563,6 +569,55 @@ func TestConfigSSH_FileWriteAndOptionsFlow(t *testing.T) {
regexMatch: "ProxyCommand /foo/bar/coder",
},
},
{
name: "Header",
args: []string{
"--yes",
"--header", "X-Test-Header=foo",
"--header", "X-Test-Header2=bar",
},
wantErr: false,
hasAgent: true,
wantConfig: wantConfig{
regexMatch: `ProxyCommand .* --header "X-Test-Header=foo" --header "X-Test-Header2=bar" ssh`,
},
},
{
name: "Header command",
args: []string{
"--yes",
"--header-command", "printf h1=v1",
},
wantErr: false,
hasAgent: true,
wantConfig: wantConfig{
regexMatch: `ProxyCommand .* --header-command "printf h1=v1" ssh`,
},
},
{
name: "Header command with double quotes",
args: []string{
"--yes",
"--header-command", "printf h1=v1 h2=\"v2\"",
},
wantErr: false,
hasAgent: true,
wantConfig: wantConfig{
regexMatch: `ProxyCommand .* --header-command "printf h1=v1 h2=\\\"v2\\\"" ssh`,
},
},
{
name: "Header command with single quotes",
args: []string{
"--yes",
"--header-command", "printf h1=v1 h2='v2'",
},
wantErr: false,
hasAgent: true,
wantConfig: wantConfig{
regexMatch: `ProxyCommand .* --header-command "printf h1=v1 h2='v2'" ssh`,
},
},
}
for _, tt := range tests {
tt := tt
Expand Down