state/api: enable password checking

state/api: enable password checking



https://code.launchpad.net/~rogpeppe/juju-core/197-api-passwords/+merge/147223

Requires: https://code.launchpad.net/~rogpeppe/juju-core/176-rpc-spike/+merge/138089

(do not edit description out of merge proposal)

[rog, 2013-02-08 11:10:27]

Please take a look.

https://codereview.appspot.com/7299066/diff/4002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7299066/diff/4002/state/state.go#newcode256
state/state.go:256: EntityName() string
i don't know why the changes in this file are
showing up - they're not changes from trunk.

i *have* merged the prereq and pushed it.

https://codereview.appspot.com/7299066/diff/4002/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/7299066/diff/4002/state/state_test.go#newcode1252
state/state_test.go:1252: func (s *StateSuite) TestAuthEntity(c *C) {
same here. this change is already merged into trunk.

https://codereview.appspot.com/7299066/diff/4002/state/user.go
File state/user.go (right):

https://codereview.appspot.com/7299066/diff/4002/state/user.go#newcode46
state/user.go:46: err = notFoundf("user %q", name)
this change is already merged into trunk

https://codereview.appspot.com/7299066/diff/4002/state/user_test.go
File state/user_test.go (right):

https://codereview.appspot.com/7299066/diff/4002/state/user_test.go#newcode51
state/user_test.go:51: func (s *UserSuite) TestName(c *C) {
this change is already merged into trunk.

[TheMue, 2013-02-08 11:43:29]

LGTM, only very small comments.

https://codereview.appspot.com/7299066/diff/3007/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/7299066/diff/3007/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:218: stm, conf, _ := s.primeAgent(c,
state.JobServeAPI)
Name "stm" has been a bit misleading because we mostly talk about state.Machine
here. Maybe better "pam" as prime agent machine.

https://codereview.appspot.com/7299066/diff/3007/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/7299066/diff/3007/juju/api.go#newcode36
juju/api.go:36: // TODO(rog): updateSecrets
Would like a more detailed comment here in case somebody has to continue your
work here.

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go#newcod...
state/api/api_test.go:114: func opUnitSetPassword(unitName string) func(c *C, st
*api.State) (func(), error) {
Funny, triple func in one line. Nice. ;)

[dimitern, 2013-02-08 16:29:55]

LGTM with a few comments. Nice!

https://codereview.appspot.com/7299066/diff/3007/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/7299066/diff/3007/environs/dummy/environs.go#n...
environs/dummy/environs.go:463: // TODO(rog) use hash of password when we can
change passwords
Good point, definitely hashing is the way to go.

https://codereview.appspot.com/7299066/diff/3007/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/7299066/diff/3007/juju/api.go#newcode36
juju/api.go:36: // TODO(rog): updateSecrets
On 2013/02/08 11:43:29, TheMue wrote:
> Would like a more detailed comment here in case somebody has to continue your
> work here.
+1

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go#newcode29
state/api/api_test.go:29: var operationPermTests = []struct {
Nice tests!

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go#newcode71
state/api/api_test.go:71: loop:
Am I getting this right - deny should take precedence over allow, right?

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go#newcod...
state/api/api_test.go:114: func opUnitSetPassword(unitName string) func(c *C, st
*api.State) (func(), error) {
On 2013/02/08 11:43:29, TheMue wrote:
> Funny, triple func in one line. Nice. ;)
+1

https://codereview.appspot.com/7299066/diff/3007/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/client.go#newcode31
state/api/client.go:31: // If this and the password are empty, no login attempt
will be made.
And what then? Shouldn't this be a reported error?

[fwereade, 2013-02-12 13:52:20]

LGTM assuming agreement below.

https://codereview.appspot.com/7299066/diff/3007/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/7299066/diff/3007/cmd/jujud/bootstrap.go#newco...
cmd/jujud/bootstrap.go:61: _, err = st.AddUser("admin", c.Conf.OldPassword)
Do we really want to use the same password across the board here?

https://codereview.appspot.com/7299066/diff/3007/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/7299066/diff/3007/environs/dummy/environs.go#n...
environs/dummy/environs.go:463: // TODO(rog) use hash of password when we can
change passwords
On 2013/02/08 16:29:55, dimitern wrote:
> Good point, definitely hashing is the way to go.

I thought we could change passwords already..? Bit more context in the comment
please.

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go#newcode71
state/api/api_test.go:71: loop:
On 2013/02/08 16:29:55, dimitern wrote:
> Am I getting this right - deny should take precedence over allow, right?

I think it's just that you're expected to specify one or the other, dependng on
which is more convenient.

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go#newcod...
state/api/api_test.go:167: //	jobs=manage-environ
Could we make this one a machine with an id other than 0 please? I'd prefer to
avoid potentially-misleading assumptions.

https://codereview.appspot.com/7299066/diff/3007/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/apiclient.go#newco...
state/api/apiclient.go:51: func (st *State) Login(entityName, password string)
error {
This is only happening over TLS, right? (sorry, I just don't really approve of
passwords... but as long as clients can verify they're really talking to the
right env (with the certs) I can't find anything concrete to complain about ;))

https://codereview.appspot.com/7299066/diff/3007/state/api/apiclient.go#newco...
state/api/apiclient.go:78: // state. TODO(rog) It returns a NotFoundError if the
machine has been removed.
I think error handling is a really high priority for a followup -- please bug
it.

https://codereview.appspot.com/7299066/diff/3007/state/api/apiclient.go#newco...
state/api/apiclient.go:123: func UnitEntityName(unitName string) string {
Why are we duplicating this code?

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:69: // Safeguard id for possible future use.
This feels a bit creaky... but not entirely unreasonable.

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:76: // current client may access the agent APIs.
This only means something to me now that I've read the document you proposed :).
Bit more detail here would be good, since it seems to be the fundamental check
that we go through for everything except clients.

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:77: func (r *srvRoot) accessAgentAPI() error {
A thought: requireAgent(/requireClient)?

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:117: // Any entity is allowed to change its own password.
So the only possible reason to get a srvUser is to change its password? If so,
this filter seems a bit redundant.

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:190: // filling them in on a need-to-know basis.
Long-term, maybe +1; short-term, just fill in everything as we need it please
:).

https://codereview.appspot.com/7299066/diff/3007/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/client.go#newcode31
state/api/client.go:31: // If this and the password are empty, no login attempt
will be made.
On 2013/02/08 16:29:55, dimitern wrote:
> And what then? Shouldn't this be a reported error?

+1

[rog, 2013-02-12 15:02:48]

Please take a look.

https://codereview.appspot.com/7299066/diff/3007/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/7299066/diff/3007/cmd/jujud/bootstrap.go#newco...
cmd/jujud/bootstrap.go:61: _, err = st.AddUser("admin", c.Conf.OldPassword)
On 2013/02/12 13:52:21, fwereade wrote:
> Do we really want to use the same password across the board here?

currently we need to, i think, but it won't be necessary when:
1) the bootstrap command actually starts the machine agent, meaning that it can
generate a config for the first machine agent containing a new password.
2) all external clients connect through the API rather than directly to mongo,
so we don't need a state password that the user knows about.

i've added a kanban ticket for the first.

https://codereview.appspot.com/7299066/diff/3007/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/7299066/diff/3007/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:218: stm, conf, _ := s.primeAgent(c,
state.JobServeAPI)
On 2013/02/08 11:43:29, TheMue wrote:
> Name "stm" has been a bit misleading because we mostly talk about
state.Machine
> here. Maybe better "pam" as prime agent machine.

as discussed online, i think i'll leave this as is. the alternatives aren't much
better.

https://codereview.appspot.com/7299066/diff/3007/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/7299066/diff/3007/environs/dummy/environs.go#n...
environs/dummy/environs.go:463: // TODO(rog) use hash of password when we can
change passwords
On 2013/02/12 13:52:21, fwereade wrote:
> On 2013/02/08 16:29:55, dimitern wrote:
> > Good point, definitely hashing is the way to go.
> 
> I thought we could change passwords already..? Bit more context in the comment
> please.

Done.

https://codereview.appspot.com/7299066/diff/3007/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/7299066/diff/3007/juju/api.go#newcode36
juju/api.go:36: // TODO(rog): updateSecrets
On 2013/02/08 16:29:55, dimitern wrote:
> On 2013/02/08 11:43:29, TheMue wrote:
> > Would like a more detailed comment here in case somebody has to continue
your
> > work here.
> +1

Done.

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go#newcode29
state/api/api_test.go:29: var operationPermTests = []struct {
On 2013/02/08 16:29:55, dimitern wrote:
> Nice tests!

thanks! they got compressed substantially over time.

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go#newcode71
state/api/api_test.go:71: loop:
On 2013/02/08 16:29:55, dimitern wrote:
> Am I getting this right - deny should take precedence over allow, right?

no, as i tried (but evidently failed) to say in the comment, if an allow list is
given, the deny list is ignored.

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go#newcode71
state/api/api_test.go:71: loop:
On 2013/02/12 13:52:21, fwereade wrote:
> On 2013/02/08 16:29:55, dimitern wrote:
> > Am I getting this right - deny should take precedence over allow, right?
> 
> I think it's just that you're expected to specify one or the other, dependng
on
> which is more convenient.

yup.

https://codereview.appspot.com/7299066/diff/3007/state/api/api_test.go#newcod...
state/api/api_test.go:167: //	jobs=manage-environ
On 2013/02/12 13:52:21, fwereade wrote:
> Could we make this one a machine with an id other than 0 please? I'd prefer to
> avoid potentially-misleading assumptions.

as discussed online, i've left as-is, with an added comment.

https://codereview.appspot.com/7299066/diff/3007/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/apiclient.go#newco...
state/api/apiclient.go:51: func (st *State) Login(entityName, password string)
error {
On 2013/02/12 13:52:21, fwereade wrote:
> This is only happening over TLS, right? (sorry, I just don't really approve of
> passwords... but as long as clients can verify they're really talking to the
> right env (with the certs) I can't find anything concrete to complain about
;))

yeah, i was in the same boat a while ago.
the concrete problem with passwords is that they give
the receiver unneeded (and possibly compromising) information,
because people have a tendency to reuse their passwords.

yes, it's only happening over TLS.

i'd be happier with a public-key based authentication system, but that has its
own issues (it's hard for a web-based user to type in a key pair).

https://codereview.appspot.com/7299066/diff/3007/state/api/apiclient.go#newco...
state/api/apiclient.go:78: // state. TODO(rog) It returns a NotFoundError if the
machine has been removed.
On 2013/02/12 13:52:21, fwereade wrote:
> I think error handling is a really high priority for a followup -- please bug
> it.

made a ticket for it.

https://codereview.appspot.com/7299066/diff/3007/state/api/apiclient.go#newco...
state/api/apiclient.go:123: func UnitEntityName(unitName string) string {
On 2013/02/12 13:52:21, fwereade wrote:
> Why are we duplicating this code?

because it belongs here, i think. eventually clients won't import state, so
they'll need this functionality from state/api.

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:76: // current client may access the agent APIs.
On 2013/02/12 13:52:21, fwereade wrote:
> This only means something to me now that I've read the document you proposed
:).
> Bit more detail here would be good, since it seems to be the fundamental check
> that we go through for everything except clients.

Done.

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:77: func (r *srvRoot) accessAgentAPI() error {
On 2013/02/12 13:52:21, fwereade wrote:
> A thought: requireAgent(/requireClient)?

Done.

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:117: // Any entity is allowed to change its own password.
On 2013/02/12 13:52:21, fwereade wrote:
> So the only possible reason to get a srvUser is to change its password? If so,
> this filter seems a bit redundant.

it's not redundant as such, as the check needs to happen somewhere, but perhaps
i could move it into srvUser.SetPassword itself. would you prefer that?

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:190: // filling them in on a need-to-know basis.
On 2013/02/12 13:52:21, fwereade wrote:
> Long-term, maybe +1; short-term, just fill in everything as we need it please
> :).

changed the comment.

https://codereview.appspot.com/7299066/diff/3007/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/client.go#newcode31
state/api/client.go:31: // If this and the password are empty, no login attempt
will be made.
On 2013/02/08 16:29:55, dimitern wrote:
> And what then? Shouldn't this be a reported error?

this allows the client to do what any other
API client can do - connect without logging in.
(the login operation is something that happens
after connection).

this allows the tests to check that operations
are disallowed when the user has not logged in.

[rog, 2013-02-12 16:48:39]

Please take a look.

[rog, 2013-02-12 16:52:44]

Please take a look.

[fwereade, 2013-02-12 16:59:30]

LGTM

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:117: // Any entity is allowed to change its own password.
On 2013/02/12 15:02:48, rog wrote:
> On 2013/02/12 13:52:21, fwereade wrote:
> > So the only possible reason to get a srvUser is to change its password? If
so,
> > this filter seems a bit redundant.
> 
> it's not redundant as such, as the check needs to happen somewhere, but
perhaps
> i could move it into srvUser.SetPassword itself. would you prefer that?

+1, I think. Feels neater to me. Your call.

https://codereview.appspot.com/7299066/diff/3007/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/client.go#newcode31
state/api/client.go:31: // If this and the password are empty, no login attempt
will be made.
On 2013/02/12 15:02:48, rog wrote:
> On 2013/02/08 16:29:55, dimitern wrote:
> > And what then? Shouldn't this be a reported error?
> 
> this allows the client to do what any other
> API client can do - connect without logging in.
> (the login operation is something that happens
> after connection).
> 
> this allows the tests to check that operations
> are disallowed when the user has not logged in.
> 

Fair enough, +1 for consistency. Maybe a comment?

[rog, 2013-02-12 17:31:49]

*** Submitted:

state/api: enable password checking

R=TheMue, dimitern, fwereade
CC=
https://codereview.appspot.com/7299066

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/apiserver.go#newco...
state/api/apiserver.go:117: // Any entity is allowed to change its own password.
On 2013/02/12 16:59:30, fwereade wrote:
> On 2013/02/12 15:02:48, rog wrote:
> > On 2013/02/12 13:52:21, fwereade wrote:
> > > So the only possible reason to get a srvUser is to change its password? If
> so,
> > > this filter seems a bit redundant.
> > 
> > it's not redundant as such, as the check needs to happen somewhere, but
> perhaps
> > i could move it into srvUser.SetPassword itself. would you prefer that?
> 
> +1, I think. Feels neater to me. Your call.

hmm, i think i'll leave it as is. i'm not sure we want one user to be able to
probe other current user names. i'll add a comment though.

https://codereview.appspot.com/7299066/diff/3007/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/7299066/diff/3007/state/api/client.go#newcode31
state/api/client.go:31: // If this and the password are empty, no login attempt
will be made.
On 2013/02/12 16:59:30, fwereade wrote:
> On 2013/02/12 15:02:48, rog wrote:
> > On 2013/02/08 16:29:55, dimitern wrote:
> > > And what then? Shouldn't this be a reported error?
> > 
> > this allows the client to do what any other
> > API client can do - connect without logging in.
> > (the login operation is something that happens
> > after connection).
> > 
> > this allows the tests to check that operations
> > are disallowed when the user has not logged in.
> > 
> 
> Fair enough, +1 for consistency. Maybe a comment?

Done.