Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Add ability to set a default SSH user for kssh#18

Merged
ddworken merged 6 commits intomasterfrom
david/kssh-default-ssh-user
Aug 19, 2019
Merged

Add ability to set a default SSH user for kssh#18
ddworken merged 6 commits intomasterfrom
david/kssh-default-ssh-user

Conversation

@ddworken
Copy link
Copy Markdown
Contributor

This feature is useful when using jumpboxes combined with ssh configs. For example, imagine you have an SSH
config that translates to this command:

ssh -J jump.example.com server.internal

When run via kssh, you want it to use the developer user but when run via ssh you want it to use your
username as the user (the default when no user is specified). Prior to this change, that was not possible.
Now, kssh does two new things:

  1. kssh --set-default-user
    It is now possible to set a default SSH user to use with kssh. This is implemented via an SSH config
    file that applies a User directive to all hosts. This config file then inherits from the default ssh
    config file at ~/.ssh/config. This makes it possible to set a default user without modifying the
    user's ssh config file.
  2. kssh now adds to the SSH agent by default
    Now by default kssh adds to the running ssh agent. This is necessary in order for jumpboxes to work
    since they rely on ssh agent forwarding.

As part of this, I also had to tweak the tests to make all of this work properly.

This feature is useful when using jumpboxes combined with ssh configs. For example, imagine you have an SSH
config that translates to this command:

```
ssh -J jump.example.com server.internal
```

When run via kssh, you want it to use the `developer` user but when run via ssh you want it to use your
username as the user (the default when no user is specified). Prior to this change, that was not possible.
Now, kssh does two new things:

1. kssh --set-default-user
	It is now possible to set a default SSH user to use with kssh. This is implemented via an SSH config
	file that applies a User directive to all hosts. This config file then inherits from the default ssh
	config file at ~/.ssh/config. This makes it possible to set a default user without modifying the
	user's ssh config file.
2. kssh now adds to the SSH agent by default
	Now by default kssh adds to the running ssh agent. This is necessary in order for jumpboxes to work
	since they rely on ssh agent forwarding.

As part of this, I also had to tweak the tests to make all of this work properly.
@ddworken ddworken requested a review from xgess August 16, 2019 21:35
Comment thread docs/troubleshooting.md Outdated
Comment thread src/cmd/kssh/kssh.go Outdated
Comment thread README.md Outdated
Comment thread src/kssh/ssh.go Outdated

var AlternateSSHConfigFile = shared.ExpandPathWithTilde("~/.ssh/kssh-config")

func CreateDefaultUserConfigFile() (bool, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

am i reading this correctly?

this function:

  1. checks ~/.ssh/kssh.config for an ssh username
  2. if it's there, writes it into ~/.ssh/kssh-config
  3. returns true if it did that, and false in any other case.

my first thought is that these two configs should have names that are more different from one another. maybe the first one could be ~/.kssh/config.json or ~/.ssh/kssh.json. just something that distinguishes these two a little better.
my second thought is that this function is doing a weird combination of things. maybe it should be two functions: one that ensures local ssh config reflects any changes in local kssh config (e.g. PushKsshConfigToSsh or UpdateLocalSshConfig or ...?) and another that fetches whatever the current state is of our configured situation.

Or maybe GetSshUserFromConfig that returns the username from the config(s), keeps them in sync, etc.
This seems a little more decoupled / extensible than called Create and returning a boolean.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think you're right that having it return a boolean was a little messy. Changed to have it only return an error. What do you think of this change?

Comment thread src/cmd/kssh/kssh_test.go

func BenchmarkLoadConfigs(b *testing.B) {
os.Remove("~/.ssh/kssh.config")
os.Remove("~/.ssh/kssh-config.json")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NAB: it's a little sloppy that this file is named in so many different places. it would be nicer if it were in just one place and pulled in everywhere it's needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I definitely agree. I've tried to keep it to a minimum but the split between the go codebase and the python integration tests make it difficult (at least without adding some avdl etc).

Copy link
Copy Markdown
Contributor

@xgess xgess left a comment

Choose a reason for hiding this comment

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

little request for text/name changes in warnAboutAlternateConfig. then :shipit:

Comment thread src/cmd/kssh/kssh.go Outdated
@ddworken ddworken merged commit fc863af into master Aug 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants