Add a credentials service

Add a credentials service

A new crdentials service provides 2 APIs:
- AuthorisedKeys
- WatchAuthorisedKeys

These will be used by a new machine agent worker to update
the local copy of ~/.ssh/authorised_keys

I added also a AuthStateManager API to compliment the
AuthEnvironManager API. I want the ability to query
ssh keys to be limited to machines running the StateManager
job, which is not guaranteed to be the same machine as
used for provisioning.

https://code.launchpad.net/~wallyworld/juju-core/ssh-key-watcher/+merge/197814

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

(do not edit description out of merge proposal)

[wallyworld, 2013-12-05 03:53:37]

Please take a look.

[wallyworld, 2013-12-06 00:35:55]

Please take a look.

[wallyworld, 2013-12-06 01:21:16]

Please take a look.

[thumper, 2013-12-06 03:37:46]

LGTM - with one import tweak

https://codereview.appspot.com/37570043/diff/40001/state/apiserver/credential...
File state/apiserver/credentials/authorisedkeys.go (right):

https://codereview.appspot.com/37570043/diff/40001/state/apiserver/credential...
state/apiserver/credentials/authorisedkeys.go:11: "strings"
standard libraries at the top :-)

[rog, 2013-12-06 08:55:23]

LGTM modulo the below

https://codereview.appspot.com/37570043/diff/40001/state/api/credentials/auth...
File state/api/credentials/authorisedkeys.go (right):

https://codereview.appspot.com/37570043/diff/40001/state/api/credentials/auth...
state/api/credentials/authorisedkeys.go:4: package credentials
I'm not sure where you're intending this package to be used,
but all the other api packages are named after the client
functionality that's using them (e.g. machiner, client, etc).

If you're creating a new worker to manage the keys, perhaps
this should be named "credentialupdater" or something like that?

[fwereade, 2013-12-09 10:37:18]

record of live chat: we should separate auth from implementation, as we do in
the various embeddable types in apiserver/common, before we land this. WIPping
in LP.

[wallyworld, 2013-12-09 11:42:13]

Please take a look.

https://codereview.appspot.com/37570043/diff/40001/state/apiserver/credential...
File state/apiserver/credentials/authorisedkeys.go (right):

https://codereview.appspot.com/37570043/diff/40001/state/apiserver/credential...
state/apiserver/credentials/authorisedkeys.go:11: "strings"
On 2013/12/06 03:37:46, thumper wrote:
> standard libraries at the top :-)

Done.

[fwereade, 2013-12-11 09:21:47]

Couple of questions -- I won't be present at the standup, but please discuss it
with dimitern/jam.

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/keyupdater...
File state/apiserver/keyupdater/authorisedkeys_test.go (right):

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys_test.go:139: {Error:
apiservertesting.NotFoundError("machine 42")},
I thought we were generally treating NotFound as Unauthorized, on the principle
that betraying existence of irrelevant entities is unnecessary information
leakage? I had a feeling this was standard somewhere... maybe in ServerError?

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/root.go#ne...
state/apiserver/root.go:301: // machine with running the ManageState job.
Is this just a statement of "Manager is stupid, we have two kinds of manager"? I
don't see how it's relevant to this CL otherwise.

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter_test.go:78: EnvironManager: false,
I'd leave out the empty values across the board (or, maybe, be explicit about
everything across the board, but I'm not quite so keen there).

[wallyworld, 2013-12-11 09:40:20]

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/keyupdater...
File state/apiserver/keyupdater/authorisedkeys_test.go (right):

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys_test.go:139: {Error:
apiservertesting.NotFoundError("machine 42")},
On 2013/12/11 09:21:48, fwereade wrote:
> I thought we were generally treating NotFound as Unauthorized, on the
principle
> that betraying existence of irrelevant entities is unnecessary information
> leakage? I had a feeling this was standard somewhere... maybe in ServerError?

I cargo culted this behaviour from elsewhere. So maybe we should leave as is and
fix across the board if we want to change it?

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/root.go#ne...
state/apiserver/root.go:301: // machine with running the ManageState job.
On 2013/12/11 09:21:48, fwereade wrote:
> Is this just a statement of "Manager is stupid, we have two kinds of manager"?
I
> don't see how it's relevant to this CL otherwise.

Yeah, since I added a new StateManager flag, I also wanted to be sure there was
a test for it.

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter_test.go:78: EnvironManager: false,
On 2013/12/11 09:21:48, fwereade wrote:
> I'd leave out the empty values across the board (or, maybe, be explicit about
> everything across the board, but I'm not quite so keen there).

Ok, will change. I think I copied this from elsewhere.

[wallyworld, 2013-12-11 10:58:09]

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/keyupdater...
File state/apiserver/keyupdater/authorisedkeys_test.go (right):

https://codereview.appspot.com/37570043/diff/60001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys_test.go:139: {Error:
apiservertesting.NotFoundError("machine 42")},
On 2013/12/11 09:40:20, wallyworld wrote:
> On 2013/12/11 09:21:48, fwereade wrote:
> > I thought we were generally treating NotFound as Unauthorized, on the
> principle
> > that betraying existence of irrelevant entities is unnecessary information
> > leakage? I had a feeling this was standard somewhere... maybe in
ServerError?
> 
> I cargo culted this behaviour from elsewhere. So maybe we should leave as is
and
> fix across the board if we want to change it?

We confirmed at standup that we want to move to returning EPerm for not found
but that there are probably still cases that don't do that yet. I'll fix the
cases in the branch before landing.

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

Please take a look.

[fwereade, 2013-12-12 09:34:10]

Couple of quick KeyUpdater implementation comments, going to do a pass over the
rest in a sec.

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
File state/apiserver/keyupdater/authorisedkeys.go (right):

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:56: getCanRead, err :=
api.getCanRead()
canRead, err := api.getCanRead()?

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:62: results[i].Error =
common.ServerError(common.ErrPerm)
I think we should probably not treat *all* errors as EPERM, just ENOTFOUND.

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:68: }
Would be nice to check canRead before bothering to hit the DB with FindEntity.
That'd then lend itself to a structure like:

_, err := api.state.Find...
if err == nil {
    watch := api.state.WatchFor...
    // Consume
    ...
}
results[i].Error = common.ServerError(err)

...which may not be perfect but does mirror the structure of a bunch of other
API calls. YMMV, I guess.

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:74: err = nil
I think the set to nil is redundant regardless.

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:103: }
Again probably good to get a canRead func before other DB hits...

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:111: continue
...and good to check canRead before hitting the DB for the entity itself.

And, similar structural comments to the above.

[wallyworld, 2013-12-12 11:04:59]

Please take a look.

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
File state/apiserver/keyupdater/authorisedkeys.go (right):

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:56: getCanRead, err :=
api.getCanRead()
On 2013/12/12 09:34:11, fwereade wrote:
> canRead, err := api.getCanRead()?

ffs. how did i miss that

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:62: results[i].Error =
common.ServerError(common.ErrPerm)
On 2013/12/12 09:34:11, fwereade wrote:
> I think we should probably not treat *all* errors as EPERM, just ENOTFOUND.

Done.

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:68: }
On 2013/12/12 09:34:11, fwereade wrote:
> Would be nice to check canRead before bothering to hit the DB with FindEntity.
> That'd then lend itself to a structure like:
> 
> _, err := api.state.Find...
> if err == nil {
>     watch := api.state.WatchFor...
>     // Consume
>     ...
> }
> results[i].Error = common.ServerError(err)
> 
> ...which may not be perfect but does mirror the structure of a bunch of other
> API calls. YMMV, I guess.


I tweaked it to use this approach:

I like the 3 little separate blocks rather than an if {} with lots inside as i
think it's easier to read. Agree??

for each entity:
    // 1. check for perms
    if !canRead(entity.Tag) {
	results[i].Error = common.ServerError(common.ErrPerm)
	continue
    }
    // 2. check entity exists
    if _, err := api.state.FindEntity(entity.Tag); err != nil {
	if errors.IsNotFoundError(err) {
		results[i].Error = common.ServerError(common.ErrPerm)
	} else {
		results[i].Error = common.ServerError(err)
	}
	continue
    }
    // 3. do stuff
    var err error
    err = do stuff
    results[i].Error = common.ServerError(err)

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:74: err = nil
On 2013/12/12 09:34:11, fwereade wrote:
> I think the set to nil is redundant regardless.

I'm sure I cargo culted this from somewhere. Not sure now. Fixed

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:103: }
On 2013/12/12 09:34:11, fwereade wrote:
> Again probably good to get a canRead func before other DB hits... 

Done.

https://codereview.appspot.com/37570043/diff/80001/state/apiserver/keyupdater...
state/apiserver/keyupdater/authorisedkeys.go:111: continue
On 2013/12/12 09:34:11, fwereade wrote:
> ...and good to check canRead before hitting the DB for the entity itself.
> 
> And, similar structural comments to the above.

Done. See above

[jameinel, 2013-12-12 12:19:11]

So the file is called .ssh/authorized-keys and not .ssh/authorised-keys

I'm usually pretty lax about British vs American spelling, but in this case we
do map onto something else that is spelled in a particular way.