Improve ssh auth key utils

Improve ssh auth key utils

This branch addesses some of the issues raised by
jam in https://codereview.appspot.com/37560043/. I 
landed the branch before seeing the comments.

It's not quite perfect yet. Lines in the auth keys
file which have "command" or "from" et al directives
are preserved, but the key on those lines is not
recognised for dup checking when adding etc. Also,
when keys are changed and written out, all of the 
"non key" lines are written at the top of the file,
followed by the juju keys.

The above issues are not critical for juju. Nodes
managed by juju will have auth key files fully managed
by juju so the issues are irrelevant unless the user
edits the auth keys file by hand. For manually
provisioned nodes, the auth key file is more likely
to be hand edited. But in all cases, lines Juju doesn't
recognise are never deleted. The worst that will happen
is a duplicate key might be added if there were a manually
added line with a command prefixing the key and the same key is
added to Juju.

A future branch will address the above remaining issue.

https://code.launchpad.net/~wallyworld/juju-core/ssh-auth-keys-fixes/+merge/198747

Requires: https://code.launchpad.net/~wallyworld/juju-core/keymanager-add-delete/+merge/198673

(do not edit description out of merge proposal)

[wallyworld, 2013-12-12 13:56:59]

Please take a look.

[jameinel, 2013-12-12 14:02:48]

I think we want to use Rename rather than CopyFile, but otherwise LGTM

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

https://codereview.appspot.com/40870047/diff/1/utils/ssh/authorisedkeys.go#ne...
utils/ssh/authorisedkeys.go:65: f, err := ioutil.TempFile(os.TempDir(),
authKeysFile)
The only problem with using TempDir() is that it is likely to be /tmp, which may
be on a different filesystem, right?

Ideally we'd use a temp file in the same directory as authKeysFile and then we
can quickly rename it.

https://codereview.appspot.com/40870047/diff/1/utils/ssh/authorisedkeys.go#ne...
utils/ssh/authorisedkeys.go:77: return utils.CopyFile(sshKeyFile, f.Name())
CopyFile isn't atomic either, is it? I would have expected a os.Rename() call.

[wallyworld, 2013-12-12 14:20:45]

Please take a look.

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

https://codereview.appspot.com/40870047/diff/1/utils/ssh/authorisedkeys.go#ne...
utils/ssh/authorisedkeys.go:65: f, err := ioutil.TempFile(os.TempDir(),
authKeysFile)
On 2013/12/12 14:02:48, jameinel wrote:
> The only problem with using TempDir() is that it is likely to be /tmp, which
may
> be on a different filesystem, right?
> 
> Ideally we'd use a temp file in the same directory as authKeysFile and then we
> can quickly rename it.

Done.

https://codereview.appspot.com/40870047/diff/1/utils/ssh/authorisedkeys.go#ne...
utils/ssh/authorisedkeys.go:77: return utils.CopyFile(sshKeyFile, f.Name())
On 2013/12/12 14:02:48, jameinel wrote:
> CopyFile isn't atomic either, is it? I would have expected a os.Rename() call.

Of course. Fixed. Doh.

[jameinel, 2013-12-12 14:29:59]

A couple small points, but LGTM

https://codereview.appspot.com/40870047/diff/20001/utils/file.go
File utils/file.go (right):

https://codereview.appspot.com/40870047/diff/20001/utils/file.go#newcode53
utils/file.go:53: func CopyFile(dest, source string) error {
Since you aren't sharing the code anymore, you *could* just revert these
changes, but I don't mind having a useful CopyFile command around either.

https://codereview.appspot.com/40870047/diff/20001/utils/ssh/authorisedkeys.go
File utils/ssh/authorisedkeys.go (right):

https://codereview.appspot.com/40870047/diff/20001/utils/ssh/authorisedkeys.g...
utils/ssh/authorisedkeys.go:65: tempFile := filepath.Join(keyDir, "newkeyfile")
I think we want to spell this:

tempFile, err := ioutil.TempFile(keyDir, "newkeyfile")
if err != nil {
}
defer os.Remove(tempFile.Name)
defer tempFile.Close()
tempFile.Write(...)
etc

so that we are sure it gets a unique filename.

[rog, 2013-12-12 18:19:01]

https://codereview.appspot.com/40870047/diff/20001/utils/ssh/authorisedkeys.go
File utils/ssh/authorisedkeys.go (right):

https://codereview.appspot.com/40870047/diff/20001/utils/ssh/authorisedkeys.g...
utils/ssh/authorisedkeys.go:67: err = ioutil.WriteFile(tempFile,
[]byte(keyData), 0755)
0755 is wrong here - why would we want the key file to be executable?

And in general, we almost certainly want to keep the same permissions as the
original file (that's part of the cost of doing a rename rather than an
overwrite, I'm afraid).

[wallyworld, 2013-12-12 21:58:08]

Please take a look.

https://codereview.appspot.com/40870047/diff/20001/utils/file.go
File utils/file.go (right):

https://codereview.appspot.com/40870047/diff/20001/utils/file.go#newcode53
utils/file.go:53: func CopyFile(dest, source string) error {
On 2013/12/12 14:30:00, jameinel wrote:
> Since you aren't sharing the code anymore, you *could* just revert these
> changes, but I don't mind having a useful CopyFile command around either.

That's what I thought too, hence I left it in.

https://codereview.appspot.com/40870047/diff/20001/utils/ssh/authorisedkeys.go
File utils/ssh/authorisedkeys.go (right):

https://codereview.appspot.com/40870047/diff/20001/utils/ssh/authorisedkeys.g...
utils/ssh/authorisedkeys.go:65: tempFile := filepath.Join(keyDir, "newkeyfile")
On 2013/12/12 14:30:00, jameinel wrote:
> I think we want to spell this:
> 
> tempFile, err := ioutil.TempFile(keyDir, "newkeyfile")
> if err != nil {
> }
> defer os.Remove(tempFile.Name)
> defer tempFile.Close()
> tempFile.Write(...)
> etc
> 
> so that we are sure it gets a unique filename.

Since this is behind a mutex is doesn't really matter.
Doing the above approach is not easy because TempFile doesn't allow the file
permissions to be specified (see Roger's comment). So I used the approach of
making a temp directory (which is unique) inside the .ssh dir and then inside
that wrote a file where i could control the permissions.

https://codereview.appspot.com/40870047/diff/20001/utils/ssh/authorisedkeys.g...
utils/ssh/authorisedkeys.go:67: err = ioutil.WriteFile(tempFile,
[]byte(keyData), 0755)
On 2013/12/12 18:19:02, rog wrote:
> 0755 is wrong here - why would we want the key file to be executable?
> 
> And in general, we almost certainly want to keep the same permissions as the
> original file (that's part of the cost of doing a rename rather than an
> overwrite, I'm afraid).

Done.

[jameinel, 2013-12-13 04:08:45]

LGTM