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.
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.)
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.
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.
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 ssh-key. txt (right):
> File doc/system-
https:/ /codereview. appspot. com/43730044/ diff/1/ doc/system- ssh-key. txt#newcode2 ssh-key. txt:2: potential use-cases.
> doc/system-
> 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 ssh-key. txt:5: as part of the clout-init machine creation
> doc/system-
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 cloudinit/ cloudinit. go (right):
> File environs/
https:/ /codereview. appspot. com/43730044/ diff/1/ environs/ cloudinit/ cloudinit. go#newcode262 cloudinit/ cloudinit. go:262: identityFile := ityFilename( cfg.DataDir) ityFilename: the path here should be ssh.SystemIdent ity) here.
> environs/
> ssh.SystemIdent
> On 2013/12/19 01:50:05, axw wrote:
> > See comment on ssh.SystemIdent
obtained
> with
> > path.Join, not filepath.Join. If you expose ssh.SystemIdentity, you
can just
> use
> > cfg.dataFile(
> Done.
https:/ /codereview. appspot. com/43730044/ diff/1/ provider/ common/ bootstrap. go common/ bootstrap. go (right):
> File provider/
https:/ /codereview. appspot. com/43730044/ diff/1/ provider/ common/ bootstrap. go#newcode90 common/ bootstrap. go:90: func GenerateSystemS SHKey(env
> provider/
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 common/ bootstrap. go:95: return "", fmt.Errorf("failed to
> provider/
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 generate. go (right):
> File utils/ssh/
https:/ /codereview. appspot. com/43730044/ diff/1/ utils/ssh/ generate. go#newcode19 generate. go:19: func GenerateKey(comment string) (private,
> utils/ssh/
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 systemidentity. go (right):
> File utils/ssh/
https:/ /codereview. appspot. com/43730044/ diff/1/ utils/ssh/ systemidentity. go#newcode15 systemidentity. go:15: func WriteSystemIden tity(dataDir
> utils/ssh/
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 systemidentity. go:28: return filepath. Join(dataDir,
> utils/ssh/
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.
https:/ /codereview. appspot. com/43730044/