Skip to content
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.

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

Allow vault ssh to accept ssh commands in any ssh compatible format #4710

Merged
merged 2 commits into from
Jun 14, 2018

Conversation

Crazybus
Copy link
Contributor

@Crazybus Crazybus commented Jun 6, 2018

Previously vault ssh required ssh commands to be in the format username@hostname <flags> command. While this works just fine for human users this breaks a lot of automation workflows and is not compatible with the options that the ssh client supports.

Motivation

We currently run ansible which uses vault ssh to connect to hosts. Ansible generates ssh commands with the format ssh <flags> -o User=username hostname command. While this is a valid ssh command it currently breaks with vault because vault expects the format to be username@hostname. To work around this we currently use a wrapper script to parse the correct username being set by ansible and translate this into a vault ssh compatible username@hostname format

Changes

  • You can now specify arguments in any order that ssh client allows. All arguments are passed directly to the ssh command and the format isn't modified in any way.
  • The username and port are parsed from the specified ssh command. It will accept all of the options supported by the ssh command and also will properly prefer -p and user@ if both options are specified.
  • The ssh port is only added from the vault credentials if it hasn't been specified on the command line

@jefferai jefferai added this to the 0.10.3 milestone Jun 6, 2018
@jefferai
Copy link
Member

jefferai commented Jun 6, 2018

This seems to revert #4673 which you yourself created (to use the hostname instead of IP) -- is that what you meant to do?

@Crazybus
Copy link
Contributor Author

Crazybus commented Jun 6, 2018

@jefferai Yes. Before vault was replacing the original ssh command and setting it to be username+"@"+ip. Changing it to hostname was a nice quick fix. However with these changes this fix is no longer needed since the original command is passed through directly to ssh without any modifications (only extra arguments are added as needed).

@Crazybus
Copy link
Contributor Author

Crazybus commented Jun 7, 2018

Let me add a few more examples in case that didn't clear things up.

In the examples my hostname is localhost which resolves to 127.0.0.1 and my current user is mick.

Before #4673

$ vault ssh -role bastion -mode otp localhost
ssh mick@127.0.0.1 -p 22

After #4673

$ vault ssh -role bastion -mode otp localhost
ssh mick@localhost -p 22

Now

$ vault ssh -role bastion -mode otp localhost
ssh localhost -p 22

But also any form of username/port specification allowed by ssh also works as expected. The -p arg is only appended with the port value from the creds api if it wasn't specified in the original ssh command.

$ vault ssh -role bastion -mode otp -- -o User=jeff -o Port=2222 localhost
ssh -o User=jeff -o Port=2222 localhost

@Crazybus
Copy link
Contributor Author

Crazybus commented Jun 8, 2018

@jefferai I just learned why that go test -race flag is so useful :)

command/ssh_test.go:143: Errorf format %q reads arg #1, but call has
only 0 args

Which I have now fixed in 942fd2d

Previously vault ssh required ssh commands to be in the format
`username@hostname <flags> command`. While this works just fine for human
users this breaks a lot of automation workflows and is not compatible
with the options that the ssh client supports.

Motivation

We currently run ansible which uses vault ssh to connect to hosts.
Ansible generates ssh commands with the format `ssh <flags> -o User=username hostname
command`. While this is a valid ssh command it currently breaks with
vault because vault expects the format to be `username@hostname`. To work
around this we currently use a wrapper script to parse the correct username being set
by ansible and translate this into a vault ssh compatible `username@hostname` format

Changes

* You can now specify arguments in any order that ssh client allows. All
arguments are passed directly to the ssh command and the format isn't
modified in any way.
* The username and port are parsed from the specified ssh command. It
will accept all of the options supported by the ssh command and also
will properly prefer `-p` and `user@` if both options are specified.
* The ssh port is only added from the vault credentials if it hasn't
been specified on the command line
When tested with the -race flag in travis it was properly catching that
these arguments to Errorf had not been set properly.

```
command/ssh_test.go:143: Errorf format %q reads arg hashicorp#1, but call has
only 0 args
```
Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -460,10 +470,6 @@ func (c *SSHCommand) handleTypeCA(username, hostname, ip string, sshArgs []strin
)
}

args = append(args,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed entirely? We don't need to pass in the username, IP, and port to the SSH command explicitly any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. My comment here should clear things up for you: #4710 (comment)

The current behaviour was to expect username@hostname as the first argument. This was then added to the actual ssh command with username@hostname plus the remaining ssh arguments. This PR removes that limitation of needing to use username@hostname and instead allows you to use any valid ssh command so that vault doesn't need to modify the actual command.

@jefferai jefferai merged commit caf3b94 into hashicorp:master Jun 14, 2018
@jefferai
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants