Create a system ssh identity on bootstrap

Create a system ssh identity on bootstrap

When an environment is bootstrapped, a system ssh
identity is created and written to the data directory.

The public key part is added to the authorized keys.

https://code.launchpad.net/~thumper/juju-core/system-ssh-key/+merge/199572

(do not edit description out of merge proposal)

[thumper, 2013-12-18 21:57:27]

Please take a look.

[axw, 2013-12-19 01:50:05]

https://codereview.appspot.com/43730044/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/43730044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:262: identityFile :=
ssh.SystemIdentityFilename(cfg.DataDir)
See comment on ssh.SystemIdentityFilename: the path here should be obtained with
path.Join, not filepath.Join. If you expose ssh.SystemIdentity, you can just use
cfg.dataFile(ssh.SystemIdentity) here.

https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:90: func GenerateSystemSSHKey(env environs.Environ)
(privateKey string, err error) {
Doc comment please. Please mention that the function records the public key in
the environment config.

https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:90: func GenerateSystemSSHKey(env environs.Environ)
(privateKey string, err error) {
Can you please add a test for this function in bootstrap_test.go, to make sure
the public key is added to the environment's config?

https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:95: return "", fmt.Errorf("failed to create system
key: %v", err)
I'm told the errors should be worded like "creating system key", so it ends up
as
error: creating system key: <thing>

(I've traditionally done it the same way as you have here, mind.)

https://codereview.appspot.com/43730044/diff/1/utils/ssh/generate.go
File utils/ssh/generate.go (right):

https://codereview.appspot.com/43730044/diff/1/utils/ssh/generate.go#newcode19
utils/ssh/generate.go:19: func GenerateKey(comment string) (private, public
string, err error) {
Doc comment please. What are the properties of the keys? It's RSA, 2048-bits,
unencrypted (i.e. no passphrase), with the private key encoded as PEM and the
public key encoded in authorized_keys format. I think it'd be good to spell all
of that out.

https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go
File utils/ssh/systemidentity.go (right):

https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go#ne...
utils/ssh/systemidentity.go:15: func WriteSystemIdentity(dataDir string,
privateKey string) error {
This seems a bit too high-level for the utils/ssh package. It probably ought to
go in environs/ssh, as a "system identity" is a Juju-level concept rather than
an SSH-level one.

I'm probably being a bit too anal though, so I won't insist.

https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go#ne...
utils/ssh/systemidentity.go:28: return filepath.Join(dataDir, systemIdentity)
Using filepath.Join isn't correct in the case of bootstrapping from Windows. I'd
say just expose the systemIdentity constant, and use the appropriate
path/filepath.Join depending on the context.

[rog, 2013-12-19 11:59:17]

Looks reasonable if we actually want to do this, but I have my reservations. I'd
like more information on why we're doing this.

https://codereview.appspot.com/43730044/diff/1/doc/system-ssh-key.txt
File doc/system-ssh-key.txt (right):

https://codereview.appspot.com/43730044/diff/1/doc/system-ssh-key.txt#newcode2
doc/system-ssh-key.txt:2: potential use-cases.
I'd like to see a few of those use cases outlined here.

Also, we already have a system private key. Can't we just use that?

https://codereview.appspot.com/43730044/diff/1/doc/system-ssh-key.txt#newcode5
doc/system-ssh-key.txt:5: as part of the clout-init machine creation process.
The public key part is
s/clout/cloud/

https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go
File utils/ssh/systemidentity.go (right):

https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go#ne...
utils/ssh/systemidentity.go:15: func WriteSystemIdentity(dataDir string,
privateKey string) error {
On 2013/12/19 01:50:05, axw wrote:
> This seems a bit too high-level for the utils/ssh package. It probably ought
to
> go in environs/ssh, as a "system identity" is a Juju-level concept rather than
> an SSH-level one.

+1
This definitely feels like the wrong place to me. Doesn't environs/cloudinit
encapsulate most of this kind of knowledge?

[thumper, 2013-12-19 22:16:10]

Please take a look.

https://codereview.appspot.com/43730044/diff/1/doc/system-ssh-key.txt
File doc/system-ssh-key.txt (right):

https://codereview.appspot.com/43730044/diff/1/doc/system-ssh-key.txt#newcode2
doc/system-ssh-key.txt:2: potential use-cases.
On 2013/12/19 11:59:17, rog wrote:
> I'd like to see a few of those use cases outlined here.
> 
> Also, we already have a system private key. Can't we just use that?

Added some. Also made the decision to use different keys for different uses
rather than overloading use.

https://codereview.appspot.com/43730044/diff/1/doc/system-ssh-key.txt#newcode5
doc/system-ssh-key.txt:5: as part of the clout-init machine creation process.
The public key part is
On 2013/12/19 11:59:17, rog wrote:
> s/clout/cloud/

Done.

https://codereview.appspot.com/43730044/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/43730044/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:262: identityFile :=
ssh.SystemIdentityFilename(cfg.DataDir)
On 2013/12/19 01:50:05, axw wrote:
> See comment on ssh.SystemIdentityFilename: the path here should be obtained
with
> path.Join, not filepath.Join. If you expose ssh.SystemIdentity, you can just
use
> cfg.dataFile(ssh.SystemIdentity) here.

Done.

https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:90: func GenerateSystemSSHKey(env environs.Environ)
(privateKey string, err error) {
On 2013/12/19 01:50:05, axw wrote:
> Doc comment please. Please mention that the function records the public key in
> the environment config.

Done.

https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:95: return "", fmt.Errorf("failed to create system
key: %v", err)
On 2013/12/19 01:50:05, axw wrote:
> I'm told the errors should be worded like "creating system key", so it ends up
> as
> error: creating system key: <thing>
> 
> (I've traditionally done it the same way as you have here, mind.)

I'd say that this is dumb.

https://codereview.appspot.com/43730044/diff/1/utils/ssh/generate.go
File utils/ssh/generate.go (right):

https://codereview.appspot.com/43730044/diff/1/utils/ssh/generate.go#newcode19
utils/ssh/generate.go:19: func GenerateKey(comment string) (private, public
string, err error) {
On 2013/12/19 01:50:05, axw wrote:
> Doc comment please. What are the properties of the keys? It's RSA, 2048-bits,
> unencrypted (i.e. no passphrase), with the private key encoded as PEM and the
> public key encoded in authorized_keys format. I think it'd be good to spell
all
> of that out.

OK

https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go
File utils/ssh/systemidentity.go (right):

https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go#ne...
utils/ssh/systemidentity.go:15: func WriteSystemIdentity(dataDir string,
privateKey string) error {
On 2013/12/19 01:50:05, axw wrote:
> This seems a bit too high-level for the utils/ssh package. It probably ought
to
> go in environs/ssh, as a "system identity" is a Juju-level concept rather than
> an SSH-level one.
> 
> I'm probably being a bit too anal though, so I won't insist.

Moved it.

https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go#ne...
utils/ssh/systemidentity.go:28: return filepath.Join(dataDir, systemIdentity)
On 2013/12/19 01:50:05, axw wrote:
> Using filepath.Join isn't correct in the case of bootstrapping from Windows.
I'd
> say just expose the systemIdentity constant, and use the appropriate
> path/filepath.Join depending on the context.

Ah... hadn't thought of the windows side.  Removed this and exporting the
SystemIdentity.

Ideally what I wanted was a single place to abstract away the location of the
system identity file. Now it is spread about.

[axw, 2013-12-20 01:16:53]

On 2013/12/19 22:16:10, thumper wrote:
> Please take a look.
> 
> https://codereview.appspot.com/43730044/diff/1/doc/system-ssh-key.txt
> File doc/system-ssh-key.txt (right):
> 
> https://codereview.appspot.com/43730044/diff/1/doc/system-ssh-key.txt#newcode2
> doc/system-ssh-key.txt:2: potential use-cases.
> On 2013/12/19 11:59:17, rog wrote:
> > I'd like to see a few of those use cases outlined here.
> > 
> > Also, we already have a system private key. Can't we just use that?
> 
> Added some. Also made the decision to use different keys for different uses
> rather than overloading use.
> 
> https://codereview.appspot.com/43730044/diff/1/doc/system-ssh-key.txt#newcode5
> doc/system-ssh-key.txt:5: as part of the clout-init machine creation process.
> The public key part is
> On 2013/12/19 11:59:17, rog wrote:
> > s/clout/cloud/
> 
> Done.
> 
> https://codereview.appspot.com/43730044/diff/1/environs/cloudinit/cloudinit.go
> File environs/cloudinit/cloudinit.go (right):
> 
>
https://codereview.appspot.com/43730044/diff/1/environs/cloudinit/cloudinit.g...
> environs/cloudinit/cloudinit.go:262: identityFile :=
> ssh.SystemIdentityFilename(cfg.DataDir)
> On 2013/12/19 01:50:05, axw wrote:
> > See comment on ssh.SystemIdentityFilename: the path here should be obtained
> with
> > path.Join, not filepath.Join. If you expose ssh.SystemIdentity, you can just
> use
> > cfg.dataFile(ssh.SystemIdentity) here.
> 
> Done.
> 
> https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go
> File provider/common/bootstrap.go (right):
> 
>
https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go#n...
> provider/common/bootstrap.go:90: func GenerateSystemSSHKey(env
environs.Environ)
> (privateKey string, err error) {
> On 2013/12/19 01:50:05, axw wrote:
> > Doc comment please. Please mention that the function records the public key
in
> > the environment config.
> 
> Done.
> 
>
https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go#n...
> provider/common/bootstrap.go:95: return "", fmt.Errorf("failed to create
system
> key: %v", err)
> On 2013/12/19 01:50:05, axw wrote:
> > I'm told the errors should be worded like "creating system key", so it ends
up
> > as
> > error: creating system key: <thing>
> > 
> > (I've traditionally done it the same way as you have here, mind.)
> 
> I'd say that this is dumb.
> 
> https://codereview.appspot.com/43730044/diff/1/utils/ssh/generate.go
> File utils/ssh/generate.go (right):
> 
> https://codereview.appspot.com/43730044/diff/1/utils/ssh/generate.go#newcode19
> utils/ssh/generate.go:19: func GenerateKey(comment string) (private, public
> string, err error) {
> On 2013/12/19 01:50:05, axw wrote:
> > Doc comment please. What are the properties of the keys? It's RSA,
2048-bits,
> > unencrypted (i.e. no passphrase), with the private key encoded as PEM and
the
> > public key encoded in authorized_keys format. I think it'd be good to spell
> all
> > of that out.
> 
> OK
> 
> https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go
> File utils/ssh/systemidentity.go (right):
> 
>
https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go#ne...
> utils/ssh/systemidentity.go:15: func WriteSystemIdentity(dataDir string,
> privateKey string) error {
> On 2013/12/19 01:50:05, axw wrote:
> > This seems a bit too high-level for the utils/ssh package. It probably ought
> to
> > go in environs/ssh, as a "system identity" is a Juju-level concept rather
than
> > an SSH-level one.
> > 
> > I'm probably being a bit too anal though, so I won't insist.
> 
> Moved it.
> 
>
https://codereview.appspot.com/43730044/diff/1/utils/ssh/systemidentity.go#ne...
> utils/ssh/systemidentity.go:28: return filepath.Join(dataDir, systemIdentity)
> On 2013/12/19 01:50:05, axw wrote:
> > Using filepath.Join isn't correct in the case of bootstrapping from Windows.
> I'd
> > say just expose the systemIdentity constant, and use the appropriate
> > path/filepath.Join depending on the context.
> 
> Ah... hadn't thought of the windows side.  Removed this and exporting the
> SystemIdentity.
> 
> Ideally what I wanted was a single place to abstract away the location of the
> system identity file. Now it is spread about.

LGTM, thanks.

[rog, 2013-12-20 09:19:41]

https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/43730044/diff/1/provider/common/bootstrap.go#n...
provider/common/bootstrap.go:95: return "", fmt.Errorf("failed to create system
key: %v", err)
On 2013/12/19 22:16:11, thumper wrote:
> On 2013/12/19 01:50:05, axw wrote:
> > I'm told the errors should be worded like "creating system key", so it ends
up
> > as
> > error: creating system key: <thing>
> > 
> > (I've traditionally done it the same way as you have here, mind.)
> 
> I'd say that this is dumb.

I agree with both of you here. I think it's good if the error message still
makes sense even when the subsidiary error after the colon is removed.

https://codereview.appspot.com/43730044/diff/20001/doc/system-ssh-key.txt
File doc/system-ssh-key.txt (right):

https://codereview.appspot.com/43730044/diff/20001/doc/system-ssh-key.txt#new...
doc/system-ssh-key.txt:10: Juju already creates a private key to connect to the
mongo database. It was an
Strictly speaking it creates a private key for *serving* the mongo database and
the API server.

https://codereview.appspot.com/43730044/diff/20001/doc/system-ssh-key.txt#new...
doc/system-ssh-key.txt:12: different purposes just seems like a more robust
idea.
I'd like a less hand-wavy justification than this.
Every extra secret lying around is another potential system vulnerability.

On the other hand, if there *is* a good reason for using different keys for
different purposes, perhaps we should consider using different keys for serving
the API server and for the mongo server.

https://codereview.appspot.com/43730044/diff/20001/environs/ssh/systemidentit...
File environs/ssh/systemidentity.go (right):

https://codereview.appspot.com/43730044/diff/20001/environs/ssh/systemidentit...
environs/ssh/systemidentity.go:18: func WriteSystemIdentity(filename string,
privateKey string) error {
We're creating an entire new package for a single constant and a function that's
semantically almost identical to ioutil.WriteFile?

Please let's just define SystemIdentityFilename in environs/cloudinit and call
WriteFile directly inside provider/local, the only caller.

https://codereview.appspot.com/43730044/diff/20001/environs/ssh/systemidentit...
environs/ssh/systemidentity.go:21: logger.Errorf("failed writing system
identity: %v", err)
If we've got an error return, please return the more informative error rather
than logging it. The error should be logged later at a higher level.

[thumper, 2014-01-05 22:33:13]

Please take a look.

https://codereview.appspot.com/43730044/diff/20001/doc/system-ssh-key.txt
File doc/system-ssh-key.txt (right):

https://codereview.appspot.com/43730044/diff/20001/doc/system-ssh-key.txt#new...
doc/system-ssh-key.txt:12: different purposes just seems like a more robust
idea.
On 2013/12/20 09:19:42, rog wrote:
> I'd like a less hand-wavy justification than this.
> Every extra secret lying around is another potential system vulnerability.
> 
> On the other hand, if there *is* a good reason for using different keys for
> different purposes, perhaps we should consider using different keys for
serving
> the API server and for the mongo server.

When I originally wrote this, I wasn't really intending to commit it to the
tree, so the prose was somewhat fast and loose.

https://codereview.appspot.com/43730044/diff/20001/environs/ssh/systemidentit...
File environs/ssh/systemidentity.go (right):

https://codereview.appspot.com/43730044/diff/20001/environs/ssh/systemidentit...
environs/ssh/systemidentity.go:18: func WriteSystemIdentity(filename string,
privateKey string) error {
On 2013/12/20 09:19:42, rog wrote:
> We're creating an entire new package for a single constant and a function
that's
> semantically almost identical to ioutil.WriteFile?
> 
> Please let's just define SystemIdentityFilename in environs/cloudinit and call
> WriteFile directly inside provider/local, the only caller.

Yeah, I have done this now.

Originally I was going to have this module abstract away more of the information
of the system identity file, but now as you can see it doesn't do much.  I have
now removed this and just call write from the local provider.