Merge lp:~axwalk/juju-core/api-disable-secrets-pushing into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

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://codereview.appspot.com/37090043/

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://codereview.appspot.com/37090043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+197673_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/37090043/

Affected files (+7, -71 lines):
   A [revision details]
   M environs/cloudinit_test.go
   M environs/config.go
   M environs/config_test.go
   M juju/api.go
   M juju/apiconn_test.go
   M juju/conn_test.go
   M juju/export_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131203182636-on3s53irtmfaw1di
+New revision: <email address hidden>

Index: environs/cloudinit_test.go
=== modified file 'environs/cloudinit_test.go'
--- environs/cloudinit_test.go 2013-12-03 06:04:43 +0000
+++ environs/cloudinit_test.go 2013-12-04 10:18:57 +0000
@@ -124,7 +124,6 @@

   oldAttrs["ca-private-key"] = ""
   oldAttrs["admin-secret"] = ""
- delete(oldAttrs, "secret")
   c.Check(mcfg.Config.AllAttrs(), gc.DeepEquals, oldAttrs)
   srvCertPEM := mcfg.StateServerCert
   srvKeyPEM := mcfg.StateServerKey

Index: environs/config.go
=== modified file 'environs/config.go'
--- environs/config.go 2013-11-13 08:15:10 +0000
+++ environs/config.go 2013-12-04 10:18:57 +0000
@@ -202,23 +202,12 @@
  // secret attributes removed. If the resulting config is not suitable
  // for bootstrapping an environment, an error is returned.
  func BootstrapConfig(cfg *config.Config) (*config.Config, error) {
- p, err := Provider(cfg.Type())
- if err != nil {
- return nil, err
- }
- secrets, err := p.SecretAttrs(cfg)
- if err != nil {
- return nil, err
- }
   m := cfg.AllAttrs()
- for k := range secrets {
- delete(m, k)
- }
-
   // We never want to push admin-secret or the root CA private key to the
cloud.
   delete(m, "admin-secret")
   delete(m, "ca-private-key")
- if cfg, err = config.New(config.NoDefaults, m); err != nil {
+ cfg, err := config.New(config.NoDefaults, m)
+ if err != nil {
    return nil, err
   }
   if _, ok := cfg.AgentVersion(); !ok {

Index: environs/config_test.go
=== modified file 'environs/config_test.go'
--- environs/config_test.go 2013-11-20 04:28:46 +0000
+++ environs/config_test.go 2013-12-04 10:18:57 +0000
@@ -296,7 +296,6 @@
   c.Assert(err, gc.IsNil)

   expect := cfg.AllAttrs()
- delete(expect, "secret")
   expect["admin-secret"] = ""
   expect["ca-private-key"] = ""
   c.Assert(cfg1.AllAttrs(), gc.DeepEquals,...

[William Reade (fwereade)]

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...

https://codereview.appspot.com/37090043/

[William Reade (fwereade)]

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#oldcode130
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.

https://codereview.appspot.com/37090043/

[Andrew Wilkins (axwalk)]

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#oldcode130
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.

https://codereview.appspot.com/37090043/

[Roger Peppe (rogpeppe)]

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#oldcode130
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
sta...

[Andrew Wilkins (axwalk)]

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#oldcode299
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.

https://codereview.appspot.com/37090043/

[Roger Peppe (rogpeppe)]

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.

https://codereview.appspot.com/37090043/

[Andrew Wilkins (axwalk)]

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.

https://codereview.appspot.com/37090043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/37090043/

[William Reade (fwereade)]

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#oldcode130
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 secur...

[Andrew Wilkins (axwalk)]

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#oldcode130
> 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, w...

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-12-10 6:54, Andrew Wilkins wrote:
> 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.
>

Pyjuju did that, but it was *not* a good user experience.
'time ssh host' is much longer than TLS.Connect.

"openssl s_time $EC2INSTANCE:17070"

says that I can connect to my EC2 instance 45 times in 31s. (682ms per
connect)

time for i in `seq 10`; do ssh ubuntu@$EC2INSTANCE /bin/false; done
is 39.2s, or 3.92s per connect.

Roughly 5-6 times slower.

So yes, it would be possible, and it might be a mode that we want to
allow you to enable "ssh-tunneling-mode: true" or something like that.
But I don't think we want it to be the default mode because the
experience is pretty terrible.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlKonYcACgkQJdeBCYSNAAOPdgCeJtM/Scc4ZzMx1zkG5+gYEBzt
DcAAn1lt8YHLMbK0+UhKQymlHhF8Q/kl
=eVxp
-----END PGP SIGNATURE-----

[Andrew Wilkins (axwalk)]

On Thu, Dec 12, 2013 at 1:14 AM, John Arbash Meinel
<email address hidden>wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 2013-12-10 6:54, Andrew Wilkins wrote:
> > 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.
> >
>
> Pyjuju did that, but it was *not* a good user experience.
> 'time ssh host' is much longer than TLS.Connect.
>
> "openssl s_time $EC2INSTANCE:17070"
>
> says that I can connect to my EC2 instance 45 times in 31s. (682ms per
> connect)
>
> time for i in `seq 10`; do ssh ubuntu@$EC2INSTANCE /bin/false; done
> is 39.2s, or 3.92s per connect.
>
> Roughly 5-6 times slower.
>
> So yes, it would be possible, and it might be a mode that we want to
> allow you to enable "ssh-tunneling-mode: true" or something like that.
> But I don't think we want it to be the default mode because the
> experience is pretty terrible.
>

Yeah, I expected the time to connect would be an issue. I agree that it
probably should be off by default, if we do it at all. The problem can be
mitigated somewhat by having a long-running background process on the CLI
machine that holds an SSH "ControlMaster" socket open; the first connection
will still be slow of course.

John
> =:->
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (Cygwin)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlKonYcACgkQJdeBCYSNAAOPdgCeJtM/Scc4ZzMx1zkG5+gYEBzt
> DcAAn1lt8YHLMbK0+UhKQymlHhF8Q/kl
> =eVxp
> -----END PGP SIGNATURE-----
>

=== modified file 'environs/cloudinit_test.go'
--- environs/cloudinit_test.go	2013-12-03 06:04:43 +0000
+++ environs/cloudinit_test.go	2013-12-06 09:49:29 +0000
@@ -124,7 +124,6 @@
 
 	oldAttrs["ca-private-key"] = ""
 	oldAttrs["admin-secret"] = ""
-	delete(oldAttrs, "secret")
 	c.Check(mcfg.Config.AllAttrs(), gc.DeepEquals, oldAttrs)
 	srvCertPEM := mcfg.StateServerCert
 	srvKeyPEM := mcfg.StateServerKey

=== modified file 'environs/config.go'
--- environs/config.go	2013-11-13 08:15:10 +0000
+++ environs/config.go	2013-12-06 09:49:29 +0000
@@ -198,27 +198,17 @@
 	return environsFilepath, nil
 }
 
-// BootstrapConfig returns a copy of the supplied configuration with
-// secret attributes removed. If the resulting config is not suitable
-// for bootstrapping an environment, an error is returned.
+// BootstrapConfig returns a copy of the supplied configuration with the
+// admin-secret and ca-private-key attributes removed. If the resulting
+// config is not suitable for bootstrapping an environment, an error is
+// returned.
 func BootstrapConfig(cfg *config.Config) (*config.Config, error) {
-	p, err := Provider(cfg.Type())
-	if err != nil {
-		return nil, err
-	}
-	secrets, err := p.SecretAttrs(cfg)
-	if err != nil {
-		return nil, err
-	}
 	m := cfg.AllAttrs()
-	for k := range secrets {
-		delete(m, k)
-	}
-
 	// We never want to push admin-secret or the root CA private key to the cloud.
 	delete(m, "admin-secret")
 	delete(m, "ca-private-key")
-	if cfg, err = config.New(config.NoDefaults, m); err != nil {
+	cfg, err := config.New(config.NoDefaults, m)
+	if err != nil {
 		return nil, err
 	}
 	if _, ok := cfg.AgentVersion(); !ok {

=== modified file 'environs/config_test.go'
--- environs/config_test.go	2013-11-20 04:28:46 +0000
+++ environs/config_test.go	2013-12-06 09:49:29 +0000
@@ -296,7 +296,6 @@
 	c.Assert(err, gc.IsNil)
 
 	expect := cfg.AllAttrs()
-	delete(expect, "secret")
 	expect["admin-secret"] = ""
 	expect["ca-private-key"] = ""
 	c.Assert(cfg1.AllAttrs(), gc.DeepEquals, expect)

=== modified file 'juju/api.go'
--- juju/api.go	2013-11-21 15:22:31 +0000
+++ juju/api.go	2013-12-06 09:49:29 +0000
@@ -13,7 +13,6 @@
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/configstore"
 	"launchpad.net/juju-core/errors"
-	"launchpad.net/juju-core/rpc"
 	"launchpad.net/juju-core/state/api"
 )
 
@@ -56,47 +55,12 @@
 	if err != nil {
 		return nil, err
 	}
-	// TODO(axw) remove this once we have synchronous bootstrap.
-	if err := updateSecrets(environ, st); err != nil {
-		apiClose(st)
-		return nil, err
-	}
 	return &APIConn{
 		Environ: environ,
 		State:   st,
 	}, nil
 }
 
-// updateSecrets pushes environment secrets to the API server.
-// NOTE: this is a temporary hack, and will disappear when we
-// have synchronous bootstrap.
-var updateSecrets = func(environ environs.Environ, st *api.State) error {
-	secrets, err := environ.Provider().SecretAttrs(environ.Config())
-	if err != nil {
-		return err
-	}
-	client := st.Client()
-	cfg, err := client.EnvironmentGet()
-	if rpc.IsNoSuchRequest(err) {
-		// Ignore checking for secrets when using an older (1.16) API
-		// server. This should be removed in 1.18.
-		logger.Warningf("running in 1.16 compatibility mode; connection may fail if environment is just bootstrapped")
-		return nil
-	}
-	if err != nil {
-		return err
-	}
-	for k, v := range secrets {
-		if _, exists := cfg[k]; exists {
-			// Environment already has secrets. Won't send again.
-			return nil
-		} else {
-			cfg[k] = v
-		}
-	}
-	return client.EnvironmentSet(cfg)
-}
-
 // Close terminates the connection to the environment and releases
 // any associated resources.
 func (c *APIConn) Close() error {

=== modified file 'juju/apiconn_test.go'
--- juju/apiconn_test.go	2013-11-07 06:48:00 +0000
+++ juju/apiconn_test.go	2013-12-06 09:49:29 +0000
@@ -65,8 +65,9 @@
 	c.Assert(conn.Environ, gc.Equals, env)
 	c.Assert(conn.State, gc.NotNil)
 
+	// the secrets will not be updated, as they already exist
 	attrs, err := conn.State.Client().EnvironmentGet()
-	c.Assert(attrs["secret"], gc.Equals, "fnord")
+	c.Assert(attrs["secret"], gc.Equals, "pork")
 
 	c.Assert(conn.Close(), gc.IsNil)
 }
@@ -137,7 +138,6 @@
 		return expectState, nil
 	}
 	defer testbase.PatchValue(juju.APIOpen, apiOpen).Restore()
-	defer testbase.PatchValue(juju.UpdateSecrets, updateSecretsNoop).Restore()
 	st, err := juju.NewAPIFromName("noconfig", store)
 	c.Assert(err, gc.IsNil)
 	c.Assert(st, gc.Equals, expectState)
@@ -186,7 +186,6 @@
 		return nil, expectErr
 	}
 	defer testbase.PatchValue(juju.APIOpen, apiOpen).Restore()
-	defer testbase.PatchValue(juju.UpdateSecrets, updateSecretsNoop).Restore()
 	st, err := juju.NewAPIFromName("noconfig", store)
 	c.Assert(err, gc.Equals, expectErr)
 	c.Assert(st, gc.IsNil)
@@ -213,7 +212,6 @@
 		return cfgOpenedState, nil
 	}
 	defer testbase.PatchValue(juju.APIOpen, apiOpen).Restore()
-	defer testbase.PatchValue(juju.UpdateSecrets, updateSecretsNoop).Restore()
 
 	stateClosed, restoreAPIClose := setAPIClosed()
 	defer restoreAPIClose.Restore()
@@ -278,7 +276,6 @@
 		return cfgOpenedState, nil
 	}
 	defer testbase.PatchValue(juju.APIOpen, apiOpen).Restore()
-	defer testbase.PatchValue(juju.UpdateSecrets, updateSecretsNoop).Restore()
 
 	stateClosed, restoreAPIClose := setAPIClosed()
 	defer restoreAPIClose.Restore()

=== modified file 'juju/conn_test.go'
--- juju/conn_test.go	2013-11-19 17:25:24 +0000
+++ juju/conn_test.go	2013-12-06 09:49:29 +0000
@@ -134,19 +134,31 @@
 	st, err := state.Open(info, state.DefaultDialOpts())
 	c.Assert(err, gc.IsNil)
 
-	// Verify we have no secret in the environ config
-	cfg, err = st.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	c.Assert(cfg.UnknownAttrs()["secret"], gc.IsNil)
+	// Verify we have secrets in the environ config already.
+	statecfg, err := st.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	c.Assert(statecfg.UnknownAttrs()["secret"], gc.Equals, "pork")
+
+	// Remove the secret from state, and then make sure it gets
+	// pushed back again.
+	attrs = statecfg.AllAttrs()
+	delete(attrs, "secret")
+	newcfg, err := config.New(config.NoDefaults, attrs)
+	c.Assert(err, gc.IsNil)
+	err = st.SetEnvironConfig(newcfg, statecfg)
+	c.Assert(err, gc.IsNil)
+	statecfg, err = st.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	c.Assert(statecfg.UnknownAttrs()["secret"], gc.IsNil)
 
 	// Make a new Conn, which will push the secrets.
 	conn, err := juju.NewConn(env)
 	c.Assert(err, gc.IsNil)
 	defer conn.Close()
 
-	cfg, err = conn.State.EnvironConfig()
+	statecfg, err = conn.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
-	c.Assert(cfg.UnknownAttrs()["secret"], gc.Equals, "pork")
+	c.Assert(statecfg.UnknownAttrs()["secret"], gc.Equals, "pork")
 
 	// Reset the admin password so the state db can be reused.
 	err = conn.State.SetAdminMongoPassword("")

=== modified file 'juju/export_test.go'
--- juju/export_test.go	2013-11-07 06:48:00 +0000
+++ juju/export_test.go	2013-12-06 09:49:29 +0000
@@ -5,5 +5,4 @@
 	APIClose             = &apiClose
 	ProviderConnectDelay = &providerConnectDelay
 	NewAPIFromName       = newAPIFromName
-	UpdateSecrets        = &updateSecrets
 )