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

Put the secure into secure shell #7278

Merged
merged 1 commit into from May 1, 2017

Conversation

axw
Copy link
Contributor

@axw axw commented Apr 26, 2017

Description of change

Up until now, Juju bootstrap has had an insecure
initial SSH connection. The client had no way of
knowing the server's host key, and so strict
host key checking was disabled.

This branch does two main things:

  • updates to the new juju/utils version which
    defaults strict host key checking to "ask"
    (unless overridden in OpenSSH client config)
    for both the golang.org/x/crypto and openssh
    implementations
  • generates and injects an SSH host key into
    the created server via cloud-init, then uses
    the public key to perform strict host key
    checking. The key is regenerated server-side
    as the first thing the initial SSH connection
    does, so that user code deployed to controllers
    cannot sniff the keys from metadata services.

QA steps

  1. juju bootstrap localhost
  2. lxc launch ubuntu:xenial x
  3. lxc file push ~/.ssh/id_rsa.pub x/home/ubuntu/.ssh/authorized_keys
  4. juju add-machine ssh:
  5. juju ssh -m controller 0 true
  6. juju ssh 0 true

Repeat with "ssh" removed from $PATH. There should be no warnings about SSH host keys changing, nor warnings about host keys being added to known_hosts.

Documentation changes

There is one change for users: on Windows, manual provisioning will now prompt the user to verify host SSH keys.

Bug reference

Fixes https://bugs.launchpad.net/juju/+bug/1683099
Fixes https://bugs.launchpad.net/juju/+bug/1579593

@mjs
Copy link

mjs commented Apr 26, 2017

The PR description makes it seem like this PR also fixes https://bugs.launchpad.net/juju/+bug/1579593. Is that right?

@axw
Copy link
Contributor Author

axw commented Apr 26, 2017

The PR description makes it seem like this PR also fixes https://bugs.launchpad.net/juju/+bug/1579593. Is that right?

Indeed :)
Description updated.

Copy link

@mjs mjs left a comment

Choose a reason for hiding this comment

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

TYVM for doing this. No more scary/confusing/embarrassing SSH key errors on bootstrap!

Just a few little things.

} else {
cfg.UnsetAttr("ssh_keys")
}
}
Copy link

Choose a reason for hiding this comment

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

This is oddly indirect. What about just this?

func (cfg *cloudConfig) SetSSHKeys(keys SSHKeys) {
	if keys.RSA != nil {
		cfg.SetAttr("ssh_keys", map[string]interface{}{
			string(RSAPrivate): keys.RSA.Private,
			string(RSAPublic):  keys.RSA.Public,
		})
	} else {
		cfg.UnsetAttr("ssh_keys")
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was the make room for other key types, but you're right. I'll just keep it simple and we can expand later if needed.


// Public contains the public key in authorized_keys format.
Public string
}
Copy link

Choose a reason for hiding this comment

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

Why the 2 nearly identical set of types? (the others in cloudconfig/cloudinit/interface.go)

It feels like these should be defined once somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because (a) I want cloudconfig/cloudinit to be self-contained, and (b) instancecfg is meant to be independent of the delivery mechanism, and so shouldn't depend on cloudconfig/cloudinit. Eventually I'd like to split cloudconfig/cloudinit into a separate package, because most of it's not Juju-specific.

@axw axw force-pushed the ssh-stricthostkeychecking branch from eb855ec to 98badd5 Compare April 27, 2017 02:34
@axw
Copy link
Contributor Author

axw commented Apr 27, 2017

Just investigating a potential issue with RackSpace, which uses WaitSSH. I think we may have to relax host key checks there initially.

@axw
Copy link
Contributor Author

axw commented Apr 27, 2017

Just investigating a potential issue with RackSpace, which uses WaitSSH. I think we may have to relax host key checks there initially.

It would be a problem, except the firewalling code doesn't even work: https://bugs.launchpad.net/juju/+bug/1536447. I'll go ahead and merge this, and that can be fixed later. We really shouldn't be relying on SSH to manage firewalls; we should introduce a machine-level worker that manages iptables.

@axw axw force-pushed the ssh-stricthostkeychecking branch from 98badd5 to f24cbdf Compare April 27, 2017 05:04
@axw
Copy link
Contributor Author

axw commented Apr 27, 2017

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Apr 27, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot
Copy link
Collaborator

jujubot commented Apr 27, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10766

@axw axw force-pushed the ssh-stricthostkeychecking branch from f24cbdf to 826ca1d Compare April 27, 2017 06:59
@axw
Copy link
Contributor Author

axw commented Apr 27, 2017

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Apr 27, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot
Copy link
Collaborator

jujubot commented Apr 27, 2017

Build failed: Generating tarball failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10776

@axw axw force-pushed the ssh-stricthostkeychecking branch from 826ca1d to 1d4b42a Compare April 27, 2017 09:04
@axw
Copy link
Contributor Author

axw commented Apr 27, 2017

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Apr 27, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot
Copy link
Collaborator

jujubot commented Apr 27, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10779

@axw axw force-pushed the ssh-stricthostkeychecking branch from 1d4b42a to dd3de5f Compare April 28, 2017 03:38
@axw
Copy link
Contributor Author

axw commented Apr 28, 2017

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Apr 28, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot
Copy link
Collaborator

jujubot commented Apr 28, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10788

Up until now, Juju bootstrap has had an insecure
initial SSH connection. The client had no way of
knowing the server's host key, and so strict
host key checking was disabled.

This branch does two main things:
 - updates to the new juju/utils version which
   defaults strict host key checking to "ask"
   (unless overridden in OpenSSH client config)
   for both the golang.org/x/crypto and openssh
   implementations
 - generates and injects an SSH host key into
   the created server via cloud-init, then use
   the public key to perform strict host key
   checking. The key is regenerated server-side
   as the first thing the initial SSH connection
   does, so that user code deployed to controllers
   cannot sniff the keys from metadata services.

Fixes https://bugs.launchpad.net/juju/+bug/1683099
@axw axw force-pushed the ssh-stricthostkeychecking branch from dd3de5f to f189117 Compare May 1, 2017 06:38
@axw
Copy link
Contributor Author

axw commented May 1, 2017

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented May 1, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit fbe29cb into juju:develop May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants