Code review comment for lp:~thumper/juju-core/system-ssh-key

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.go#newcode262
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#newcode90
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#newcode95
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#newcode15
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#newcode28
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.

https://codereview.appspot.com/43730044/