cmd/jujud: do not change password

cmd/jujud: do not change password

This prepares the way for the password to be changed
when connecting to the API instead of the state.

We also change bootstrap-state to change the machine
agent's password, so the bootstrap machine agent
won't *need* to change its password, and hence won't
need a connection to the at-that-time-nonexistent
API server.

Until agents are actually connecting to the API,
juju will be a tiny bit less secure, so I plan on
waiting until that's ready to land before landing this
branch.

https://code.launchpad.net/~rogpeppe/juju-core/311-juju-bootstrap-state-change-password-1.5/+merge/165675

(do not edit description out of merge proposal)

[rog, 2013-05-24 19:02:16]

Please take a look.

[dimitern, 2013-05-24 19:30:16]

LGTM, nice!

https://codereview.appspot.com/9738043/diff/5001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/juju/bootstrap_test.go#n...
cmd/juju/bootstrap_test.go:58: func (s *BootstrapSuite) TestTest(c *C) {
TestTest :)

[fwereade, 2013-05-27 20:13:26]

reserving judgment for now, until I see the rest of the pipeline, but I have
suspicions.

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

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap.go#newco...
cmd/jujud/bootstrap.go:140: }
I guess if we fail here, we have a borked environment regardless. So, given the
failure modes we've already embraced, ok, I guess... but it's a bit surprising
nonetheless. Comment perhaps?

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:198: info.Tag, info.Password = "", testPasswordHash
wait, why can user-admin log in as ""?

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:220: c.Assert(machineConf1.StateInfo.Password,
Equals, machineConf1.APIInfo.Password)
Are we duplicating tag/password for the same entities everywhere? If so, why?

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go#newc...
environs/agent/agent.go:208: func (c *Conf) OpenAPI() (st *api.State,
newPassword string, err error) {
Why are we duplicating this back-to-front newPassword-juggling behaviour?

https://codereview.appspot.com/9738043/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (left):

https://codereview.appspot.com/9738043/diff/5001/environs/dummy/environs.go#o...
environs/dummy/environs.go:465: // logic is done.
"TODO: this whole test-only parallel implementation of bootstrap-state is an
open invitation to bugs and insanity", perhaps?

[rog, 2013-06-07 17:01:55]

Please take a look.

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

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap.go#newco...
cmd/jujud/bootstrap.go:140: }
On 2013/05/27 20:13:26, fwereade wrote:
> I guess if we fail here, we have a borked environment regardless. So, given
the
> failure modes we've already embraced, ok, I guess... but it's a bit surprising
> nonetheless. Comment perhaps?

i'm not sure what kind of comment you're thinking here.
how is failing to write the configuration any different
from any of the above failure modes?

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:198: info.Tag, info.Password = "", testPasswordHash
On 2013/05/27 20:13:26, fwereade wrote:
> wait, why can user-admin log in as ""?

the empty string means "admin" when logging into the state. see the docs on
state.Info.Tag. i disclaim all responsibility for that decision :-)

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:220: c.Assert(machineConf1.StateInfo.Password,
Equals, machineConf1.APIInfo.Password)
On 2013/05/27 20:13:26, fwereade wrote:
> Are we duplicating tag/password for the same entities everywhere? If so, why?

we use the same password for the state that we do for the API.
is that bad?

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go#newc...
environs/agent/agent.go:208: func (c *Conf) OpenAPI() (st *api.State,
newPassword string, err error) {
On 2013/05/27 20:13:26, fwereade wrote:
> Why are we duplicating this back-to-front newPassword-juggling behaviour?

what do you suggest instead? we still need to do exactly the same thing AFAICS.
i wasn't thinking of redesigning this logic at this point, but i can do if you'd
like it to change.

https://codereview.appspot.com/9738043/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (left):

https://codereview.appspot.com/9738043/diff/5001/environs/dummy/environs.go#o...
environs/dummy/environs.go:465: // logic is done.
On 2013/05/27 20:13:26, fwereade wrote:
> "TODO: this whole test-only parallel implementation of bootstrap-state is an
> open invitation to bugs and insanity", perhaps?

that's not really a TODO. i've added something slightly different.

[fwereade, 2013-06-12 16:50:37]

LGTM; I do find all this stuff a bit tricky to follow, though, and I feel like
there's a simpler model waiting to come out. Let's wait and see if one emerges
when we're actually using the API.

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

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap.go#newco...
cmd/jujud/bootstrap.go:140: }
On 2013/06/07 17:01:55, rog wrote:
> On 2013/05/27 20:13:26, fwereade wrote:
> > I guess if we fail here, we have a borked environment regardless. So, given
> the
> > failure modes we've already embraced, ok, I guess... but it's a bit
surprising
> > nonetheless. Comment perhaps?
> 
> i'm not sure what kind of comment you're thinking here.
> how is failing to write the configuration any different
> from any of the above failure modes?

Yeah, I guess it's no different; this is just the latest expression of my
paranoia that one-shotting bootstrap, rather than ensuring it actually
completes, will cause us surprising and upsetting problems one day. No action
needed here.

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:198: info.Tag, info.Password = "", testPasswordHash
On 2013/06/07 17:01:55, rog wrote:
> On 2013/05/27 20:13:26, fwereade wrote:
> > wait, why can user-admin log in as ""?
> 
> the empty string means "admin" when logging into the state. see the docs on
> state.Info.Tag. i disclaim all responsibility for that decision :-)

ok, thanks for pointing that out

https://codereview.appspot.com/9738043/diff/5001/cmd/jujud/bootstrap_test.go#...
cmd/jujud/bootstrap_test.go:220: c.Assert(machineConf1.StateInfo.Password,
Equals, machineConf1.APIInfo.Password)
On 2013/06/07 17:01:55, rog wrote:
> On 2013/05/27 20:13:26, fwereade wrote:
> > Are we duplicating tag/password for the same entities everywhere? If so,
why?
> 
> we use the same password for the state that we do for the API.
> is that bad?

I reserve judgment on that; I think it smells a little bit, because API access
is a very different thing to state access... but I'm more thinking that the
*.Info types are feeling a bit overloaded, and that -- if the two chunks of data
are fundamentally the same rather than just coincidentally so -- separating that
common auth data is probably a pretty good idea.

Really, I was just looking for clarification.

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/9738043/diff/5001/environs/agent/agent.go#newc...
environs/agent/agent.go:208: func (c *Conf) OpenAPI() (st *api.State,
newPassword string, err error) {
On 2013/06/07 17:01:55, rog wrote:
> On 2013/05/27 20:13:26, fwereade wrote:
> > Why are we duplicating this back-to-front newPassword-juggling behaviour?
> 
> what do you suggest instead? we still need to do exactly the same thing
AFAICS.
> i wasn't thinking of redesigning this logic at this point, but i can do if
you'd
> like it to change.

Not worth making speculative effort, I suppose. But I always found that bit
demanded a surprising amount of concentration, and was vaguely hoping something
leaner might emerge.

[rog, 2013-06-18 11:19:53]

Please take a look.

[rog, 2013-06-21 14:40:00]

Please take a look.

[rog, 2013-06-21 15:26:38]

Please take a look.