Remove secrets-pushing for new bootstraps, API

Remove secrets-pushing for new bootstraps, API

- environs/BootstrapConfig now no longer removes
  secret attributes from configuration. This is
  on account of configuration not being populated
  into cloud-init config now.
- juju/NewAPIConn now no longer pushes secrets,
  as it is not necessary. This was not present in
  1.16, so it is not a compatibility concern.

If we're okay with disabling new CLI from initialising
an old juju server, then we can also remove secrets
pushing from juju/NewConn. Doing this would allow us
to drop all notion of "secret attributes".

https://code.launchpad.net/~axwalk/juju-core/api-disable-secrets-pushing/+merge/197673

(do not edit description out of merge proposal)

[axw, 2013-12-04 10:29:26]

Please take a look.

[fwereade, 2013-12-04 14:06:27]

I think we should keep the secret-handling in Conn, until we actually lose Conn
itself -- I'm sure *someone* will make the first connection to a new env with an
old client, and I'd rather not break whoever that turns out to be.

And I don't think we can drop the notion of secret attrs: (1) we're still using
them in the API server, although that can go any time we like, but (2) more
importantly, we'll need to restrict their display to non-admin users in the very
near future.

On to the review...

[fwereade, 2013-12-04 14:13:08]

This is great, just a few notes. Rog, can you think of any disadvantage to just
pushing the CA private key now?

...or even generating the whole thing on the server side, and never even putting
the key on the user's machine?

https://codereview.appspot.com/37090043/diff/1/environs/cloudinit_test.go
File environs/cloudinit_test.go (left):

https://codereview.appspot.com/37090043/diff/1/environs/cloudinit_test.go#old...
environs/cloudinit_test.go:130: srvKeyPEM := mcfg.StateServerKey
It concerns me a little that this is increasingly poorly-named, because it's now
about initializing juju not driving cloudinit. Can we just rename
environs/cloudinit and this file (and associated types...) now please?

https://codereview.appspot.com/37090043/diff/1/environs/config.go
File environs/config.go (left):

https://codereview.appspot.com/37090043/diff/1/environs/config.go#oldcode218
environs/config.go:218: // We never want to push admin-secret or the root CA
private key to the cloud.
FWIW, we rather do want to push the CA private key now, because we'll need to
generate new certs for new state servers soon. Maybe not necessary to change it
now.

https://codereview.appspot.com/37090043/diff/1/juju/api.go
File juju/api.go (left):

https://codereview.appspot.com/37090043/diff/1/juju/api.go#oldcode98
juju/api.go:98: }
\o/

https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go#newcode137
juju/conn_test.go:137: // Verify we have secrets in the environ config already.
I think we should still test that Conn still does the dance if it's required.

[axw, 2013-12-05 07:40:34]

Please take a look.

https://codereview.appspot.com/37090043/diff/1/environs/cloudinit_test.go
File environs/cloudinit_test.go (left):

https://codereview.appspot.com/37090043/diff/1/environs/cloudinit_test.go#old...
environs/cloudinit_test.go:130: srvKeyPEM := mcfg.StateServerKey
On 2013/12/04 14:13:09, fwereade wrote:
> It concerns me a little that this is increasingly poorly-named, because it's
now
> about initializing juju not driving cloudinit. Can we just rename
> environs/cloudinit and this file (and associated types...) now please?

Sure. environs/jujuinit?

Can I do this in a followup? There are quite a few files to touch.

https://codereview.appspot.com/37090043/diff/1/environs/config.go
File environs/config.go (left):

https://codereview.appspot.com/37090043/diff/1/environs/config.go#oldcode218
environs/config.go:218: // We never want to push admin-secret or the root CA
private key to the cloud.
On 2013/12/04 14:13:09, fwereade wrote:
> FWIW, we rather do want to push the CA private key now, because we'll need to
> generate new certs for new state servers soon. Maybe not necessary to change
it
> now.

Okay. Regarding your question about "generating the [ca private key] on the
server side": it wouldn't be *too* hard, but it will mean we'll need to
 - update jujud to be able to generate certs/keys, so we can initialise mongo
 - update agent config code to take the cert/key from a file (?)
 - update worker/localstorage to generate a new key server-side also. This would
mean threading the CA cert/key through somehow.

Not overly difficult, but I think it warrants a CL to itself.

https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go#newcode137
juju/conn_test.go:137: // Verify we have secrets in the environ config already.
On 2013/12/04 14:13:09, fwereade wrote:
> I think we should still test that Conn still does the dance if it's required.

Done. Adding the test in is slightly tricky, given that we always have secrets
now. I've gotten around this by modifying provider/dummy to elide the standard
secret if it's set to a special value, and add in another (unknown attr) secret
if it is present. It's a bit ugly - if you have a better idea of how to do this,
please let me know.

[rog, 2013-12-05 13:20:21]

Looks great. A few remarks below.

https://codereview.appspot.com/37090043/diff/1/environs/cloudinit_test.go
File environs/cloudinit_test.go (left):

https://codereview.appspot.com/37090043/diff/1/environs/cloudinit_test.go#old...
environs/cloudinit_test.go:130: srvKeyPEM := mcfg.StateServerKey
On 2013/12/04 14:13:09, fwereade wrote:
> It concerns me a little that this is increasingly poorly-named, because it's
now
> about initializing juju not driving cloudinit. Can we just rename
> environs/cloudinit and this file (and associated types...) now please?

I'm not entirely sure. The principal function of this package is to transform a
MachineConfig structure into a cloudinit structure, which seems quite
cloudinit-specific to me and reasonably well focused.

What's changed about this package that leads to to think that it's changed
enough to warrant a rename?

BTW, I could easily see the MachineConfig type moving out of this package FWIW.

If we *are* to rename this package, then perhaps environs/init might work -
after all it's all juju around here :-)

https://codereview.appspot.com/37090043/diff/1/environs/config.go
File environs/config.go (left):

https://codereview.appspot.com/37090043/diff/1/environs/config.go#oldcode218
environs/config.go:218: // We never want to push admin-secret or the root CA
private key to the cloud.
On 2013/12/05 07:40:35, axw wrote:
> On 2013/12/04 14:13:09, fwereade wrote:
> > FWIW, we rather do want to push the CA private key now, because we'll need
to
> > generate new certs for new state servers soon. Maybe not necessary to change
> it
> > now.
> 
> Okay. Regarding your question about "generating the [ca private key] on the
> server side": it wouldn't be *too* hard, but it will mean we'll need to
>  - update jujud to be able to generate certs/keys, so we can initialise mongo
>  - update agent config code to take the cert/key from a file (?)
>  - update worker/localstorage to generate a new key server-side also. This
would
> mean threading the CA cert/key through somehow.
> 
> Not overly difficult, but I think it warrants a CL to itself.

If we generate the root CA private key on the server side, otherwise how do we
verify that we're talking to the right server when we make our initial
connection? If we don't do key verification then ISTM that we're vulnerable to a
MitM attack even if the transport to the provider API is secure (if it isn't
then all bets are off, of course).

BTW even if we don't push the root CA private key, we can still push a cert/key
that can be used to sign new state server certs - we'd just need to turn on the
delegation flag.

https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go#newcode137
juju/conn_test.go:137: // Verify we have secrets in the environ config already.
On 2013/12/05 07:40:35, axw wrote:
> On 2013/12/04 14:13:09, fwereade wrote:
> > I think we should still test that Conn still does the dance if it's
required.
> 
> Done. Adding the test in is slightly tricky, given that we always have secrets
> now. I've gotten around this by modifying provider/dummy to elide the standard
> secret if it's set to a special value, and add in another (unknown attr)
secret
> if it is present. It's a bit ugly - if you have a better idea of how to do
this,
> please let me know.

Ah, now I understand the dummy thing. Wouldn't it be easier to make the test
specifically delete the secret from the config in state after bootstrapping?
Would we still need the ugly dummy logic then?

https://codereview.appspot.com/37090043/diff/10001/environs/config_test.go
File environs/config_test.go (left):

https://codereview.appspot.com/37090043/diff/10001/environs/config_test.go#ol...
environs/config_test.go:299: delete(expect, "secret")
Was this broken before? I thought BootstrapConfig was supposed to elide secrets.

https://codereview.appspot.com/37090043/diff/10001/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/37090043/diff/10001/provider/dummy/environs.go...
provider/dummy/environs.go:476: // in the state server.
Sorry for my obtuseness, but I don't understand this. The context of the remark
(talking about "pushing") seems largely unrelated to the code that it's in.

I also don't really understand what the "extra secret" thing is about - aren't
attributes either secret or non-secret?

[axw, 2013-12-05 15:05:25]

https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go#newcode137
juju/conn_test.go:137: // Verify we have secrets in the environ config already.
On 2013/12/05 13:20:21, rog wrote:
> On 2013/12/05 07:40:35, axw wrote:
> > On 2013/12/04 14:13:09, fwereade wrote:
> > > I think we should still test that Conn still does the dance if it's
> required.
> > 
> > Done. Adding the test in is slightly tricky, given that we always have
secrets
> > now. I've gotten around this by modifying provider/dummy to elide the
standard
> > secret if it's set to a special value, and add in another (unknown attr)
> secret
> > if it is present. It's a bit ugly - if you have a better idea of how to do
> this,
> > please let me know.
> 
> Ah, now I understand the dummy thing. Wouldn't it be easier to make the test
> specifically delete the secret from the config in state after bootstrapping?
> Would we still need the ugly dummy logic then?

Sadly, it's not really easier. I would prefer to do that, but SetEnvironConfig
in state never removes any old settings, it only updates. There's a bug about
that somewhere, but it's not clear to me whether this is required behaviour.

https://codereview.appspot.com/37090043/diff/10001/environs/config_test.go
File environs/config_test.go (left):

https://codereview.appspot.com/37090043/diff/10001/environs/config_test.go#ol...
environs/config_test.go:299: delete(expect, "secret")
On 2013/12/05 13:20:21, rog wrote:
> Was this broken before? I thought BootstrapConfig was supposed to elide
secrets.

It used to, but not anymore (that's the primary change).
The assertion here used to be that the config returned by BootstrapConfig didn't
have secrets; the assertion is now that it does.

[rog, 2013-12-05 15:35:09]

https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go#newcode137
juju/conn_test.go:137: // Verify we have secrets in the environ config already.
On 2013/12/05 15:05:25, axw wrote:
> On 2013/12/05 13:20:21, rog wrote:
> > On 2013/12/05 07:40:35, axw wrote:
> > > On 2013/12/04 14:13:09, fwereade wrote:
> > > > I think we should still test that Conn still does the dance if it's
> > required.
> > > 
> > > Done. Adding the test in is slightly tricky, given that we always have
> secrets
> > > now. I've gotten around this by modifying provider/dummy to elide the
> standard
> > > secret if it's set to a special value, and add in another (unknown attr)
> > secret
> > > if it is present. It's a bit ugly - if you have a better idea of how to do
> > this,
> > > please let me know.
> > 
> > Ah, now I understand the dummy thing. Wouldn't it be easier to make the test
> > specifically delete the secret from the config in state after bootstrapping?
> > Would we still need the ugly dummy logic then?
> 
> Sadly, it's not really easier. I would prefer to do that, but SetEnvironConfig
> in state never removes any old settings, it only updates. There's a bug about
> that somewhere, but it's not clear to me whether this is required behaviour.

If that's the problem, rather than the ugly and non-obvious code in dummy, why
not write a little function on State specifically to delete environment config
attributes? We might only use the code in this test, but at least its purpose
would be straightforward and its structure not predicated on some far removed
test.

dummy is already too complex - adding warts like this won't help.

https://codereview.appspot.com/37090043/diff/10001/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/37090043/diff/10001/environs/config.go#newcode202
environs/config.go:202: // secret attributes removed. If the resulting config is
not suitable
This comment is wrong now because we're not removing all secret attributes.

[axw, 2013-12-06 02:35:50]

On 2013/12/05 15:35:09, rog wrote:
> https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go
> File juju/conn_test.go (right):
> 
> https://codereview.appspot.com/37090043/diff/1/juju/conn_test.go#newcode137
> juju/conn_test.go:137: // Verify we have secrets in the environ config
already.
> On 2013/12/05 15:05:25, axw wrote:
> > On 2013/12/05 13:20:21, rog wrote:
> > > On 2013/12/05 07:40:35, axw wrote:
> > > > On 2013/12/04 14:13:09, fwereade wrote:
> > > > > I think we should still test that Conn still does the dance if it's
> > > required.
> > > > 
> > > > Done. Adding the test in is slightly tricky, given that we always have
> > secrets
> > > > now. I've gotten around this by modifying provider/dummy to elide the
> > standard
> > > > secret if it's set to a special value, and add in another (unknown attr)
> > > secret
> > > > if it is present. It's a bit ugly - if you have a better idea of how to
do
> > > this,
> > > > please let me know.
> > > 
> > > Ah, now I understand the dummy thing. Wouldn't it be easier to make the
test
> > > specifically delete the secret from the config in state after
bootstrapping?
> > > Would we still need the ugly dummy logic then?
> > 
> > Sadly, it's not really easier. I would prefer to do that, but
SetEnvironConfig
> > in state never removes any old settings, it only updates. There's a bug
about
> > that somewhere, but it's not clear to me whether this is required behaviour.
> 
> If that's the problem, rather than the ugly and non-obvious code in dummy, why
> not write a little function on State specifically to delete environment config
> attributes? We might only use the code in this test, but at least its purpose
> would be straightforward and its structure not predicated on some far removed
> test.
> 
> dummy is already too complex - adding warts like this won't help.

Yeah, fair enough.
I'm going to look at fixing https://bugs.launchpad.net/juju-core/+bug/1167616.
I'll come back to this once that's done (or shown to take too much time).

> https://codereview.appspot.com/37090043/diff/10001/environs/config.go
> File environs/config.go (right):
> 
>
https://codereview.appspot.com/37090043/diff/10001/environs/config.go#newcode202
> environs/config.go:202: // secret attributes removed. If the resulting config
is
> not suitable
> This comment is wrong now because we're not removing all secret attributes.

Well it doesn't say it takes away *all* the secrets ;)
I'll update the comment and repropose when I've fixed the other thing.

[axw, 2013-12-06 09:52:02]

Please take a look.

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

Essentially LGTM -- you can land this now, but let's come to a decision re the
CA cert private key and whether we push it during bootstrap, or enable
delegation; and land that followup sooner rather than later. Does anyone feel
strongly either way?

https://codereview.appspot.com/37090043/diff/1/environs/cloudinit_test.go
File environs/cloudinit_test.go (left):

https://codereview.appspot.com/37090043/diff/1/environs/cloudinit_test.go#old...
environs/cloudinit_test.go:130: srvKeyPEM := mcfg.StateServerKey
On 2013/12/05 13:20:21, rog wrote:
> On 2013/12/04 14:13:09, fwereade wrote:
> > It concerns me a little that this is increasingly poorly-named, because it's
> now
> > about initializing juju not driving cloudinit. Can we just rename
> > environs/cloudinit and this file (and associated types...) now please?
> 
> I'm not entirely sure. The principal function of this package is to transform
a
> MachineConfig structure into a cloudinit structure, which seems quite
> cloudinit-specific to me and reasonably well focused.
> 
> What's changed about this package that leads to to think that it's changed
> enough to warrant a rename?
> 
> BTW, I could easily see the MachineConfig type moving out of this package
FWIW.
> 
> If we *are* to rename this package, then perhaps environs/init might work -
> after all it's all juju around here :-)

I think we've got our concepts a bit tangled around here. We're treating
cloudinit as the canonical representation and generating different forms of
initialization from one of those, rather than generating an initialization type
and rendering it into a cloudinit form. (That's why I was grumpy about creating
environs/cloudinit in the first place -- I feared it'd entrench exactly this
ill-factoring -- but I guess I didn't express myself well.)

https://codereview.appspot.com/37090043/diff/1/environs/config.go
File environs/config.go (left):

https://codereview.appspot.com/37090043/diff/1/environs/config.go#oldcode218
environs/config.go:218: // We never want to push admin-secret or the root CA
private key to the cloud.
On 2013/12/05 13:20:21, rog wrote:
> On 2013/12/05 07:40:35, axw wrote:
> > On 2013/12/04 14:13:09, fwereade wrote:
> > > FWIW, we rather do want to push the CA private key now, because we'll need
> to
> > > generate new certs for new state servers soon. Maybe not necessary to
change
> > it
> > > now.
> > 
> > Okay. Regarding your question about "generating the [ca private key] on the
> > server side": it wouldn't be *too* hard, but it will mean we'll need to
> >  - update jujud to be able to generate certs/keys, so we can initialise
mongo
> >  - update agent config code to take the cert/key from a file (?)
> >  - update worker/localstorage to generate a new key server-side also. This
> would
> > mean threading the CA cert/key through somehow.
> > 
> > Not overly difficult, but I think it warrants a CL to itself.
> 
> If we generate the root CA private key on the server side, otherwise how do we
> verify that we're talking to the right server when we make our initial
> connection? If we don't do key verification then ISTM that we're vulnerable to
a
> MitM attack even if the transport to the provider API is secure (if it isn't
> then all bets are off, of course).
> 
> BTW even if we don't push the root CA private key, we can still push a
cert/key
> that can be used to sign new state server certs - we'd just need to turn on
the
> delegation flag.

Good points, well made. Am undecided as to whether keeping the root CA on the
bootstrapping client and delegating is actually a win over pushing the private
key up once we've got a secure connection... you're right it shouldn't be part
of this CL though.

https://codereview.appspot.com/37090043/diff/30001/juju/api.go
File juju/api.go (left):

https://codereview.appspot.com/37090043/diff/30001/juju/api.go#oldcode98
juju/api.go:98: }
I think I already made this comment earlier, but it deserves a repeat: \o/

[axw, 2013-12-10 02:53:08]

On 2013/12/09 10:33:10, fwereade wrote:
> Essentially LGTM -- you can land this now, but let's come to a decision re the
> CA cert private key and whether we push it during bootstrap, or enable
> delegation; and land that followup sooner rather than later. Does anyone feel
> strongly either way?

Just throwing an alternative out there (based on the "push during bootstrap"
option). It has come up on the mailing list that it would be useful to operate
over SSH, to better deal with restricted access environments (i.e. so you don't
need to use sshuttle.)

So, we could generate the cert/keys entirely server side, and never use it for
the CLI. The CLI could SSH to the machine with a forwarded port (or multiplex
over the stdio), and the API server could allow plaintext communication to
localhost. This way we get SSH proxying, with only SSH keys stored outside the
environment.

> https://codereview.appspot.com/37090043/diff/1/environs/cloudinit_test.go
> File environs/cloudinit_test.go (left):
> 
>
https://codereview.appspot.com/37090043/diff/1/environs/cloudinit_test.go#old...
> environs/cloudinit_test.go:130: srvKeyPEM := mcfg.StateServerKey
> On 2013/12/05 13:20:21, rog wrote:
> > On 2013/12/04 14:13:09, fwereade wrote:
> > > It concerns me a little that this is increasingly poorly-named, because
it's
> > now
> > > about initializing juju not driving cloudinit. Can we just rename
> > > environs/cloudinit and this file (and associated types...) now please?
> > 
> > I'm not entirely sure. The principal function of this package is to
transform
> a
> > MachineConfig structure into a cloudinit structure, which seems quite
> > cloudinit-specific to me and reasonably well focused.
> > 
> > What's changed about this package that leads to to think that it's changed
> > enough to warrant a rename?
> > 
> > BTW, I could easily see the MachineConfig type moving out of this package
> FWIW.
> > 
> > If we *are* to rename this package, then perhaps environs/init might work -
> > after all it's all juju around here :-)
> 
> I think we've got our concepts a bit tangled around here. We're treating
> cloudinit as the canonical representation and generating different forms of
> initialization from one of those, rather than generating an initialization
type
> and rendering it into a cloudinit form. (That's why I was grumpy about
creating
> environs/cloudinit in the first place -- I feared it'd entrench exactly this
> ill-factoring -- but I guess I didn't express myself well.)

Fair call. When I did the synchronous bootstrap work, I did start down the road
of creating a new canonical representation. It would've essentially ended up as
a clone of a subset of cloudinit.Config, though, so it didn't seem worthwhile at
the time. It may be worthwhile revisiting.

> https://codereview.appspot.com/37090043/diff/1/environs/config.go
> File environs/config.go (left):
> 
> https://codereview.appspot.com/37090043/diff/1/environs/config.go#oldcode218
> environs/config.go:218: // We never want to push admin-secret or the root CA
> private key to the cloud.
> On 2013/12/05 13:20:21, rog wrote:
> > On 2013/12/05 07:40:35, axw wrote:
> > > On 2013/12/04 14:13:09, fwereade wrote:
> > > > FWIW, we rather do want to push the CA private key now, because we'll
need
> > to
> > > > generate new certs for new state servers soon. Maybe not necessary to
> change
> > > it
> > > > now.
> > > 
> > > Okay. Regarding your question about "generating the [ca private key] on
the
> > > server side": it wouldn't be *too* hard, but it will mean we'll need to
> > >  - update jujud to be able to generate certs/keys, so we can initialise
> mongo
> > >  - update agent config code to take the cert/key from a file (?)
> > >  - update worker/localstorage to generate a new key server-side also. This
> > would
> > > mean threading the CA cert/key through somehow.
> > > 
> > > Not overly difficult, but I think it warrants a CL to itself.
> > 
> > If we generate the root CA private key on the server side, otherwise how do
we
> > verify that we're talking to the right server when we make our initial
> > connection? If we don't do key verification then ISTM that we're vulnerable
to
> a
> > MitM attack even if the transport to the provider API is secure (if it isn't
> > then all bets are off, of course).
> > 
> > BTW even if we don't push the root CA private key, we can still push a
> cert/key
> > that can be used to sign new state server certs - we'd just need to turn on
> the
> > delegation flag.
> 
> Good points, well made. Am undecided as to whether keeping the root CA on the
> bootstrapping client and delegating is actually a win over pushing the private
> key up once we've got a secure connection... you're right it shouldn't be part
> of this CL though.
> 
> https://codereview.appspot.com/37090043/diff/30001/juju/api.go
> File juju/api.go (left):
> 
> https://codereview.appspot.com/37090043/diff/30001/juju/api.go#oldcode98
> juju/api.go:98: }
> I think I already made this comment earlier, but it deserves a repeat: \o/