Merge lp:~dimitern/juju-core/147-apiprovisioner-blank-env-secrets into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

apiserver/provisioner: Mask out secret env attrs

This changes the provisioner API EnvironConfig()
call to mask out any secret attributes before
returning the config to non-manager agents.
The environment manager agents still get everything.

This closes off a security issue with LXC provisioner
having access to the environment configuration and
its secrets via the API.

Live tested on EC2 successfully.

https://codereview.appspot.com/13964043/

R=dimitern, jameinel

apiserver/provisioner: Mask out secret env attrs

This changes the provisioner API EnvironConfig()
call to mask out any secret attributes before
returning the config to non-manager agents.
The environment manager agents still get everything.

This closes off a security issue with LXC provisioner
having access to the environment configuration and
its secrets via the API.

Live tested on EC2 successfully.

https://codereview.appspot.com/13964043/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+187738_code.launchpad.net,

Message:
Please take a look.

Description:
apiserver/provisioner: Mask out secret env attrs

This changes the provisioner API EnvironConfig()
call to mask out any secret attributes before
returning the config to non-manager agents.
The environment manager agents still get everything.

This closes off a security issue with LXC provisioner
having access to the environment configuration and
its secrets via the API.

Live tested on EC2 successfully.

https://code.launchpad.net/~dimitern/juju-core/147-apiprovisioner-blank-env-secrets/+merge/187738

Requires:
https://code.launchpad.net/~dimitern/juju-core/146-apiprovisioner-addresses/+merge/187719

(do not edit description out of merge proposal)

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

Affected files (+38, -1 lines):
   A [revision details]
   M state/apiserver/provisioner/provisioner.go
   M state/apiserver/provisioner/provisioner_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:
<email address hidden>
+New revision:
<email address hidden>

Index: state/apiserver/provisioner/provisioner.go
=== modified file 'state/apiserver/provisioner/provisioner.go'
--- state/apiserver/provisioner/provisioner.go 2013-09-26 10:38:59 +0000
+++ state/apiserver/provisioner/provisioner.go 2013-09-26 11:15:31 +0000
@@ -157,7 +157,25 @@
   if err != nil {
    return result, err
   }
- result.Config = config.AllAttrs()
+ allAttrs := config.AllAttrs()
+ if !p.authorizer.AuthEnvironManager() {
+ // Mask out any secrets in the environment configuration
+ // with values of the same type, so it'll pass validation.
+ //
+ // TODO(dimitern) 201309-26 bug #1231384
+ // This needs to change so we won't return anything to
+ // entities other than the environment manager, but the
+ // provisioner code should be refactored first.
+ env, err := environs.New(config)
+ if err != nil {
+ return result, err
+ }
+ secretAttrs, err := env.Provider().SecretAttrs(config)
+ for k := range secretAttrs {
+ allAttrs[k] = "not available"
+ }
+ }
+ result.Config = allAttrs
   return result, nil
  }

Index: state/apiserver/provisioner/provisioner_test.go
=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go 2013-09-26 10:38:59
+0000
+++ state/apiserver/provisioner/provisioner_test.go 2013-09-26 11:15:31
+0000
@@ -422,6 +422,23 @@
   c.Assert(err, gc.IsNil)
   c.Assert(result.Error, gc.IsNil)
   c.Assert(result.Config, gc.DeepEquals,
params.Config(envConfig.AllAttrs()))
+
+ // Now test it with a non-environment manager and make sure
+ // the secret attributes are masked.
+ anAuthorizer := s.authorizer
+ anAuthorizer.MachineAgent = true
+ anAuthorizer.Manager = false
+ aProvisioner, err := provisioner.NewProvisionerAPI(s.State, s.resources,
+ anAuthorizer)
+ c.Assert(err, gc.IsNil)
+
+ // We need to see the secret attributes masked out, and for
+ // the dummy pro...

[John A Meinel (jameinel)]

LGTM given it is the same as the previous submisison which had already
been approved.

https://codereview.appspot.com/13964043/

[John A Meinel (jameinel)]

On 2013/09/26 13:10:37, jameinel wrote:
> LGTM given it is the same as the previous submisison which had already
been
> approved.

I do still have the question why we set the values to "not available"
rather than just deleting them. I guess so Env creation doesn't complain
about missing entries?

https://codereview.appspot.com/13964043/

[Dimiter Naydenov (dimitern)]

On 2013/09/26 13:12:01, jameinel wrote:
> On 2013/09/26 13:10:37, jameinel wrote:
> > LGTM given it is the same as the previous submisison which had
already been
> > approved.

> I do still have the question why we set the values to "not available"
rather
> than just deleting them. I guess so Env creation doesn't complain
about missing
> entries?

Setting them to something is needed for validation. And since this is
temporary,
it'll go away soon (when the provisioner is properly refactored).

https://codereview.appspot.com/13964043/

[Go Bot (go-bot)]

The attempt to merge lp:~dimitern/juju-core/147-apiprovisioner-blank-env-secrets into lp:juju-core failed. Below is the output from the failed tests.

# launchpad.net/juju-core/state/apiserver/provisioner
state/apiserver/provisioner/provisioner.go:169: undefined: environs

[Dimiter Naydenov (dimitern)]

Self-approving: Added a missing import to fix a build failure.

review: Approve

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/13964043/

=== modified file 'state/apiserver/provisioner/provisioner.go'
--- state/apiserver/provisioner/provisioner.go	2013-09-26 13:20:24 +0000
+++ state/apiserver/provisioner/provisioner.go	2013-09-26 16:02:23 +0000
@@ -7,6 +7,7 @@
 	"fmt"
 
 	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
@@ -157,7 +158,25 @@
 	if err != nil {
 		return result, err
 	}
-	result.Config = config.AllAttrs()
+	allAttrs := config.AllAttrs()
+	if !p.authorizer.AuthEnvironManager() {
+		// Mask out any secrets in the environment configuration
+		// with values of the same type, so it'll pass validation.
+		//
+		// TODO(dimitern) 201309-26 bug #1231384
+		// This needs to change so we won't return anything to
+		// entities other than the environment manager, but the
+		// provisioner code should be refactored first.
+		env, err := environs.New(config)
+		if err != nil {
+			return result, err
+		}
+		secretAttrs, err := env.Provider().SecretAttrs(config)
+		for k := range secretAttrs {
+			allAttrs[k] = "not available"
+		}
+	}
+	result.Config = allAttrs
 	return result, nil
 }
 
 
=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go	2013-09-26 10:38:59 +0000
+++ state/apiserver/provisioner/provisioner_test.go	2013-09-26 16:02:23 +0000
@@ -422,6 +422,23 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(result.Error, gc.IsNil)
 	c.Assert(result.Config, gc.DeepEquals, params.Config(envConfig.AllAttrs()))
+
+	// Now test it with a non-environment manager and make sure
+	// the secret attributes are masked.
+	anAuthorizer := s.authorizer
+	anAuthorizer.MachineAgent = true
+	anAuthorizer.Manager = false
+	aProvisioner, err := provisioner.NewProvisionerAPI(s.State, s.resources,
+		anAuthorizer)
+	c.Assert(err, gc.IsNil)
+
+	// We need to see the secret attributes masked out, and for
+	// the dummy provider it's only one: "secret".
+	expectedConfig := envConfig.AllAttrs()
+	expectedConfig["secret"] = "not available"
+	result, err = aProvisioner.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	c.Assert(result.Error, gc.IsNil)
 }
 
 func (s *provisionerSuite) TestStatus(c *gc.C) {