Merge lp:~hazmat/pyjuju/states-with-principals into lp:pyjuju

[Kapil Thangavelu]

Security integration with state package.

Provides for principals created when corrresponding states are
created that have agents associated.

https://codereview.appspot.com/5966076/

[Kapil Thangavelu (hazmat)]

Reviewers: mp+100783_code.launchpad.net,

Message:
Please take a look.

Description:
Security integration with state package.

Provides for principals created when corrresponding states are
created that have agents associated.

https://code.launchpad.net/~hazmat/juju/states-with-principals/+merge/100783

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/agents/tests/common.py
   M juju/lib/testing.py
   M juju/providers/local/__init__.py
   M juju/providers/local/tests/test_provider.py
   M juju/state/base.py
   M juju/state/initialize.py
   M juju/state/machine.py
   M juju/state/relation.py
   M juju/state/security.py
   A juju/state/securityrules.py
   M juju/state/service.py
   M juju/state/tests/common.py
   M juju/state/tests/test_agent.py
   M juju/state/tests/test_base.py
   M juju/state/tests/test_errors.py
   M juju/state/tests/test_initialize.py
   M juju/state/tests/test_machine.py
   M juju/state/tests/test_relation.py
   M juju/state/tests/test_security.py
   M juju/state/tests/test_service.py
   M juju/unit/lifecycle.py

[Kapil Thangavelu (hazmat)]

gustavo's original review, most of the naming and api conventions have been cleaned up. http://paste.ubuntu.com/914526/

[Gustavo Niemeyer (niemeyer)]

As we discussed before, this looks like a pretty significant change for
that point in time.

Either way, if you want to go with it, we need a written down
description of what is going on here in terms of changes to the public
interfaces, as usual.

https://codereview.appspot.com/5966076/

[Kapil Thangavelu (hazmat)]

On Wed, Apr 4, 2012 at 10:32 AM, <email address hidden> wrote:

> As we discussed before, this looks like a pretty significant change for
> that point in time.
>
> Either way, if you want to go with it, we need a written down
> description of what is going on here in terms of changes to the public
> interfaces, as usual.
>
> https://codereview.appspot.**com/5966076/<https://codereview.appspot.com/5966076/>

It seems to be the number one concern regarding the failed MIR.

The implementation here is per the security spec discussed last fall,
incorporating feedback from the initial reviews. The spec is out of date
(uses ensemble instead of juju), and per feedback the OTP agent was dropped
for interceptable OTP tokens.

There aren't any public interface changes, just the imposition of ACLs onto
existing nodes.

I can update the spec and send it around to the list if you'd like.. but as
is, the components can effectively be merged as the default security
policy is permissive, ie no functional delta till the policy is activated.

-k

[Gustavo Niemeyer (niemeyer)]

On 2012/04/04 15:53:23, hazmat wrote:
> It seems to be the number one concern regarding the failed MIR.

My understanding is that the MIR has been dropped.

> The implementation here is per the security spec discussed last fall,
> incorporating feedback from the initial reviews. The spec is out of
date
> (uses ensemble instead of juju), and per feedback the OTP agent was
dropped
> for interceptable OTP tokens.

> There aren't any public interface changes, just the imposition of ACLs
onto
> existing nodes.

Please see the mailing list conversation about what "public changes"
means. The message subject is "Code reviews and public API changes".

> I can update the spec and send it around to the list if you'd like..
but as
> is, the components can effectively be merged as the default security
> policy is permissive, ie no functional delta till the policy is
activated.

The management of the Python code base is under your control, as we
agreed. What I'm concerned about is with changes that are landing
without being debated for 12.04 (hint: we *are* in 04!).

https://codereview.appspot.com/5966076/

[Kapil Thangavelu (hazmat)]

> On 2012/04/04 15:53:23, hazmat wrote:
> > It seems to be the number one concern regarding the failed MIR.
>
> My understanding is that the MIR has been dropped.
>

hence the term 'failed'

> > The implementation here is per the security spec discussed last fall,
> > incorporating feedback from the initial reviews. The spec is out of
> date
> > (uses ensemble instead of juju), and per feedback the OTP agent was
> dropped
> > for interceptable OTP tokens.
>
> > There aren't any public interface changes, just the imposition of ACLs
> onto
> > existing nodes.
>
> Please see the mailing list conversation about what "public changes"
> means. The message subject is "Code reviews and public API changes".

>
> > I can update the spec and send it around to the list if you'd like..
> but as
> > is, the components can effectively be merged as the default security
> > policy is permissive, ie no functional delta till the policy is
> activated.
>
> The management of the Python code base is under your control, as we
> agreed. What I'm concerned about is with changes that are landing
> without being debated for 12.04 (hint: we *are* in 04!).

ic. we've publicly debated these changes previously last fall, i believe your saying that that process needs to be done again?

[Gustavo Niemeyer (niemeyer)]

On Wed, Apr 4, 2012 at 13:10, Kapil Thangavelu
<email address hidden> wrote:
>> On 2012/04/04 15:53:23, hazmat wrote:
>> > It seems to be the number one concern regarding the failed MIR.

>> My understanding is that the MIR has been dropped.

> hence the term 'failed'

If it's failed, it's not an argument to have changes going in now.

> ic. we've publicly debated these changes previously last fall,
> i believe your saying that that process needs to be done again?

Sorry, I don't want to frustrate you at all, but try to look from
another perspective: there's a release of Ubuntu *this month*, and we're
rushing with a Go implementation to catch up meanwhile. Then, there's
that massive change set that has seen zero debate in the mailing list. I
hope it's not a surprise that I'm asking more details.

Besides that, where's that previously reviewed document that you
mention? It's not linked from this proposal, and I can't find it
anywhere.

https://codereview.appspot.com/5966076/

[Kapil Thangavelu (hazmat)]

On Wed, Apr 4, 2012 at 12:43 PM, <email address hidden> wrote:

> On Wed, Apr 4, 2012 at 13:10, Kapil Thangavelu
> <kapil.thangavelu@canonical.**com <email address hidden>> wrote:
>
>> On 2012/04/04 15:53:23, hazmat wrote:
>>> > It seems to be the number one concern regarding the failed MIR.
>>>
>>
> My understanding is that the MIR has been dropped.
>>>
>>
>
> hence the term 'failed'
>>
>
> If it's failed, it's not an argument to have changes going in now.
>
> ic. we've publicly debated these changes previously last fall,
>> i believe your saying that that process needs to be done again?
>>
>
> Sorry, I don't want to frustrate you at all, but try to look from
> another perspective: there's a release of Ubuntu *this month*, and we're
> rushing with a Go implementation to catch up meanwhile. Then, there's
> that massive change set that has seen zero debate in the mailing list. I
> hope it's not a surprise that I'm asking more details.
>
> Besides that, where's that previously reviewed document that you
> mention? It's not linked from this proposal, and I can't find it
> anywhere.
>
> https://codereview.appspot.**com/5966076/<https://codereview.appspot.com/5966076/>

The mir failed in part because of lack of appropriate security, and the
security concerns remain an issue to adoption.

for reference this was the original spec
https://code.launchpad.net/~hazmat/juju/security-specification/+merge/63921

it was discussed in depth @ austin sprint
http://pad.ubuntu.com/cUU8nlRZ4U

i mispoke when i said it was previously discussed on list, while its
discussed in the context of several other threads, i don't see a dedicated
thread to it offhand.

what i'm asking for is direction wrt to your comments.. i've updated the
security branches and cleaned them up. If you'd like me to bring it up on
list, i'm happy to push the spec there. If its something that should be
dropped.. well then pls say that.. at this point clearly i'm not too
concerned about wasting time effort on dead code.

-k

[Gustavo Niemeyer (niemeyer)]

On 2012/04/04 17:43:36, hazmat wrote:
> The mir failed in part because of lack of appropriate security, and
the
> security concerns remain an issue to adoption.

A broken implementation is a much more serious concern for adoption.

> for reference this was the original spec

https://code.launchpad.net/%7Ehazmat/juju/security-specification/+merge/63921

"Needs Fixing on 2011-06-09"

> what i'm asking for is direction wrt to your comments.. i've updated
the
> security branches and cleaned them up. If you'd like me to bring it up
on
> list, i'm happy to push the spec there. If its something that should
be
> dropped.. well then pls say that.. at this point clearly i'm not too
> concerned about wasting time effort on dead code.

Do you want me to say one more time? Sure: I think it is a bad idea to
be merging massive changes for 12.04 at this stage. I would not do it. I
would not be merging all those significant changes right before a
release. It's supposed to be stable. Let's please not ship crack in
12.04.

https://codereview.appspot.com/5966076/

[Clint Byrum (clint-fewbar)]

Excerpts from Gustavo Niemeyer's message of Wed Apr 04 17:52:20 UTC 2012:
> On 2012/04/04 17:43:36, hazmat wrote:
> > The mir failed in part because of lack of appropriate security, and
> the
> > security concerns remain an issue to adoption.
>
> A broken implementation is a much more serious concern for adoption.
>

I think they are equally serious, and adoption will suffer on the backend
(as people get more serious) if the security is not handled. One side of
me says we should at least ship an insecure toy, so that people can try
it out. But the other hand says we already did that in 11.10, and doing
so again would only waste peoples' time who are actively looking to deploy
with juju. Perhaps its better that we ship secure with bugs than insecure
with less bugs, and push hard to fix those problems as they are found.

> > for reference this was the original spec
>
> https://code.launchpad.net/%7Ehazmat/juju/security-specification/+merge/63921
>
> "Needs Fixing on 2011-06-09"
>
> > what i'm asking for is direction wrt to your comments.. i've updated
> the
> > security branches and cleaned them up. If you'd like me to bring it up
> on
> > list, i'm happy to push the spec there. If its something that should
> be
> > dropped.. well then pls say that.. at this point clearly i'm not too
> > concerned about wasting time effort on dead code.
>
> Do you want me to say one more time? Sure: I think it is a bad idea to
> be merging massive changes for 12.04 at this stage. I would not do it. I
> would not be merging all those significant changes right before a
> release. It's supposed to be stable. Let's please not ship crack in
> 12.04.
>

I agree, I have been hoping we can take a mitigation strategy with some
lighter weight methods for the impending release. While its clear we're
not going to be in main, we should not ship something in universe or in
our PPA that will expose users to security problems in a non obvious way.

The problems in the late-landing features in the version we shipped for
11.10 should have taught us that we should really respect feature freeze
and spend that time testing rather than refactoring or adding on.

Perhaps we can defer the large impact that this work carries until
after 12.04 releases, and instead focus on just fixing the "wide open
zookeeper" problem with a minimal patch that just adds a basic ACL like
"anonymous can't do anything", and be able to pass generated credentials
to each node?

This would go a long way to making this version of juju safe for
production use, which will go a long way to people using and contributing
charms. Then we can look at shipping something either in an SRU if it
is appropriate for that, or if it breaks behavior, in a new "stable"
PPA for 12.04 users.

[Gustavo Niemeyer (niemeyer)]

On Wed, Apr 4, 2012 at 15:43, Clint Byrum <email address hidden> wrote:
> I think they are equally serious, and adoption will suffer on the
backend
> (as people get more serious) if the security is not handled. One side
of
> me says we should at least ship an insecure toy, so that people can
try
> it out. But the other hand says we already did that in 11.10, and
doing
> so again would only waste peoples' time who are actively looking to
deploy
> with juju. Perhaps its better that we ship secure with bugs than
insecure
> with less bugs, and push hard to fix those problems as they are found.

This was a great topic for us to have had at the last UDS. It's now
time to ship 12.04, which was supposed to be a stable release, in my
humble opinion.

> Perhaps we can defer the large impact that this work carries until
> after 12.04 releases, and instead focus on just fixing the "wide open
> zookeeper" problem with a minimal patch that just adds a basic ACL
like
> "anonymous can't do anything", and be able to pass generated
credentials
> to each node?

That sounds less complex and thus preferable as an entry point. It was
once the whole point of that never-used admin-secret setting, by the
way.

That said, I'd still postpone that to 12.04.1. It's time to stop
changing
juju and preparing for an actual release. I can use juju in EC2 knowing
zookeeper is open. I won't use juju anywhere if it breaks all the time.

https://codereview.appspot.com/5966076/

[Kapil Thangavelu (hazmat)]

On Wed, Apr 4, 2012 at 1:50 PM, <email address hidden> wrote:

> On 2012/04/04 17:43:36, hazmat wrote:
>
>> The mir failed in part because of lack of appropriate security, and
>>
> the
>
>> security concerns remain an issue to adoption.
>>
>
> A broken implementation is a much more serious concern for adoption.

its unclear how the implementation which has yet to land got classified as
broken.

[Gustavo Niemeyer (niemeyer)]

On 2012/04/04 19:02:10, hazmat wrote:
> its unclear how the implementation which has yet to land got
classified as
> broken.

Kapil, it's your take. I'm not going to be the bad guy again. If you
want to release this work in 12.04, just provide an up-to-date document
explaining in detail what's being suggested as far as the external
interactions go, and I'll review.

https://codereview.appspot.com/5966076/

[Kapil Thangavelu (hazmat)]

On 2012/04/04 19:13:54, niemeyer wrote:
> On 2012/04/04 19:02:10, hazmat wrote:
> > its unclear how the implementation which has yet to land got
classified as
> > broken.

> Kapil, it's your take. I'm not going to be the bad guy again. If you
want to
> release this work in 12.04, just provide an up-to-date document
explaining in
> detail what's being suggested as far as the external interactions go,
and I'll
> review.

all right given the effort and timeline it seems more important to work
on bug fixes for 12.04 instead of this. i'll bring it back up post
release for 12.04.1

http://codereview.appspot.com/5966076/

[lp:~hazmat/pyjuju/states-with-principals]

335. By Kapil Thangavelu on 2012-08-09

security test tweaks

336. By Kapil Thangavelu on 2012-08-12

merge trunk

337. By Kapil Thangavelu on 2012-08-27

switch over some remaining test uses of txzk deletetree for juju state async impl of the test helper

[Kapil Thangavelu (hazmat)]

Please take a look.

https://codereview.appspot.com/5966076/

Unmerged revisions

337. By Kapil Thangavelu on 2012-08-27

switch over some remaining test uses of txzk deletetree for juju state async impl of the test helper

336. By Kapil Thangavelu on 2012-08-12

merge trunk

335. By Kapil Thangavelu on 2012-08-09

security test tweaks

334. By Kapil Thangavelu on 2012-04-02

security api cleanup

333. By Kapil Thangavelu on 2012-04-02

remove extant refs to old node security adapter

332. By Kapil Thangavelu on 2012-04-02

replace security facade with usage of concrete classes

331. By Kapil Thangavelu on 2012-04-01

remove extraneous auth waits, attach handles it

330. By Kapil Thangavelu on 2012-04-01

refactor some names for clarity

329. By Kapil Thangavelu on 2012-04-01

fix local provider bootstrap

328. By Kapil Thangavelu on 2012-03-31

merge trunk

=== modified file 'juju/agents/tests/common.py'
--- juju/agents/tests/common.py	2012-03-27 23:22:28 +0000
+++ juju/agents/tests/common.py	2012-08-28 16:13:21 +0000
@@ -27,13 +27,11 @@
         self.agent.configure(self.options)
         self.agent.set_watch_enabled(False)
 
+    @inlineCallbacks
     def tearDown(self):
         if self.agent.client and self.agent.client.connected:
             self.agent.client.close()
-
-        if self.client.connected:
-            deleteTree("/", self.client.handle)
-            self.client.close()
+        yield super(AgentTestBase, self).tearDown()
 
     def get_agent_config(self):
         options = TwistedOptionNamespace()

=== modified file 'juju/charm/tests/test_publisher.py'
--- juju/charm/tests/test_publisher.py	2012-04-06 16:35:31 +0000
+++ juju/charm/tests/test_publisher.py	2012-08-28 16:13:21 +0000
@@ -6,8 +6,6 @@
 from twisted.internet.defer import inlineCallbacks, fail
 from twisted.python.failure import Failure
 
-from txzookeeper.tests.utils import deleteTree
-
 from juju.charm.bundle import CharmBundle
 from juju.charm.directory import CharmDirectory
 from juju.charm.publisher import CharmPublisher
@@ -19,6 +17,7 @@
 
 from juju.environment.tests.test_config import (
     EnvironmentsConfigTestBase, SAMPLE_ENV)
+from juju.state.tests.common import deleteTree
 from juju.lib.mocker import MATCH
 
 from .test_repository import RepositoryTestBase
@@ -60,8 +59,9 @@
         yield self.client.connect()
         yield self.client.create("/charms")
 
+    @inlineCallbacks
     def tearDown(self):
-        deleteTree("/", self.client.handle)
+        yield deleteTree(self.client)
         self.client.close()
         super(CharmPublisherTest, self).tearDown()
 

=== modified file 'juju/lib/testing.py'
--- juju/lib/testing.py	2012-08-24 23:00:50 +0000
+++ juju/lib/testing.py	2012-08-28 16:13:21 +0000
@@ -121,6 +121,24 @@
     def assertInstance(self, instance, type):
         self.assertTrue(isinstance(instance, type))
 
+    def assertACE(self, acl, token=None, name=None):
+        """Assert an ACE exists for the given identity token."""
+        found = False
+
+        assert token or name
+
+        for ace in acl:
+            if token and ace["id"] == token:
+                found = True
+                break
+            elif name and ace["id"].split(":")[0] == name:
+                found = True
+                break
+
+        if not found:
+            self.fail("Identity token %r not found in ACL %s" % (
+                token, acl))
+
     def assertLogLines(self, observed, expected):
         """Asserts that the lines of `expected` exist in order in the log."""
         logged = observed.split("\n")

=== modified file 'juju/providers/common/tests/test_connect.py'
--- juju/providers/common/tests/test_connect.py	2011-11-19 05:49:05 +0000
+++ juju/providers/common/tests/test_connect.py	2012-08-28 16:13:21 +0000
@@ -14,6 +14,8 @@
 from juju.machine import ProviderMachine
 from juju.providers.common.base import MachineProviderBase
 from juju.state.sshclient import SSHClient
+
+from juju.state.tests.common import deleteTree
 from juju.tests.common import get_test_zookeeper_address
 
 
@@ -195,7 +197,7 @@
             self.assertTrue(client_result, client_result)
             self.assertIdentical(client_result[0], client)
         finally:
-            deleteTree("/", client.handle)
+            yield deleteTree(client)
             client.close()
 
     @inlineCallbacks
@@ -235,5 +237,5 @@
                 "Connected to environment.\n",
                 log.getvalue())
         finally:
-            deleteTree("/", client.handle)
+            yield deleteTree(client)
             client.close()

=== modified file 'juju/providers/local/__init__.py'
--- juju/providers/local/__init__.py	2012-07-20 17:28:17 +0000
+++ juju/providers/local/__init__.py	2012-08-28 16:13:21 +0000
@@ -17,9 +17,9 @@
 from juju.providers.local.machine import LocalMachine
 from juju.providers.local.network import Network
 from juju.providers.local.pkg import check_packages
-from juju.state.auth import make_identity
 from juju.state.initialize import StateHierarchy
 from juju.state.placement import LOCAL_POLICY
+from juju.state.security import Principal
 from juju.state.utils import get_open_port
 
 
@@ -126,12 +126,14 @@
 
         # Initialize the zookeeper state
         log.debug("Initializing state...")
-        admin_identity = make_identity(
-            "admin:%s" % self.config["admin-secret"])
+
         client = ZookeeperClient(zookeeper.address)
         yield client.connect()
+        principal = Principal("admin", self.config['admin-secret'])
+        yield principal.attach(client)
+
         hierarchy = StateHierarchy(
-            client, admin_identity, "local", constraints.data, "local")
+            client, principal.get_token(), "local", constraints.data, "local")
         yield hierarchy.initialize()
         yield client.close()
 

=== modified file 'juju/providers/local/tests/test_provider.py'
--- juju/providers/local/tests/test_provider.py	2012-07-20 17:28:17 +0000
+++ juju/providers/local/tests/test_provider.py	2012-08-28 16:13:21 +0000
@@ -6,7 +6,6 @@
 
 from twisted.internet.defer import succeed, inlineCallbacks
 
-from txzookeeper.tests.utils import deleteTree
 
 from juju.errors import ProviderError, EnvironmentNotFound
 from juju.lib.lxc import LXCContainer
@@ -17,9 +16,12 @@
 from juju.providers.local.agent import ManagedMachineAgent
 from juju.providers.local.files import StorageServer
 from juju.providers.local.network import Network
+from juju.state.auth import make_ace
+from juju.state.security import Principal, OTPPrincipal
 from juju.lib import lxc as lxc_lib
 
 from juju.lib.testing import TestCase
+from juju.state.tests.common import deleteTree
 from juju.tests.common import get_test_zookeeper_address
 
 
@@ -27,18 +29,26 @@
 
     @inlineCallbacks
     def setUp(self):
+        yield super(LocalProviderTest, self).setUp()
         self.constraints = ConstraintSet("local").parse([]).with_series("foo")
         self.provider = MachineProvider(
             "local-test", {
-                "admin-secret": "admin:abc", "data-dir": self.makeDir(),
+                "admin-secret": "admin", "data-dir": self.makeDir(),
                 "authorized-keys": "fooabc123", "default-series": "oneiric"})
         self.output = self.capture_logging(
             "juju.local-dev", level=logging.DEBUG)
         zookeeper.set_debug_level(0)
         self.client = yield self.get_zookeeper_client().connect()
 
+        principal = Principal("admin", "admin")
+        OTPPrincipal.set_additional_otp_ace(
+            make_ace(principal.get_token(), all=True))
+        yield principal.attach(self.client)
+
+    @inlineCallbacks
     def tearDown(self):
-        deleteTree("/", self.client.handle)
+        self.assertTrue(self.client.connected)
+        yield deleteTree(self.client)
         self.client.close()
 
     @property
@@ -60,6 +70,7 @@
         self.assertTrue(os.path.isdir(path))
 
     def bootstrap_mock(self):
+        """Mock all the external process interactions."""
         self.patch(local, "REQUIRED_PACKAGES", [])
         mock_network = self.mocker.patch(Network)
         mock_network.start()
@@ -97,7 +108,7 @@
         self.assertEqual(
             sorted(['services', 'settings', 'charms', 'relations', 'zookeeper',
                     'initialized', 'topology', 'machines', 'units',
-                    'constraints']),
+                    'constraints', 'auth-tokens', 'otp']),
             sorted(children))
         output = self.output.getvalue()
         self.assertIn("Starting networking...", output)

=== modified file 'juju/state/base.py'
--- juju/state/base.py	2011-12-20 15:09:26 +0000
+++ juju/state/base.py	2012-08-28 16:13:21 +0000
@@ -1,6 +1,6 @@
 import logging
 
-from twisted.internet.defer import inlineCallbacks, returnValue
+from twisted.internet.defer import inlineCallbacks, returnValue, maybeDeferred
 
 from zookeeper import NoNodeException
 
@@ -64,13 +64,13 @@
         subclasses.
         """
 
-        @inlineCallbacks
         def change_content_function(content, stat):
             topology = InternalTopology()
             if content:
                 topology.parse(content)
-            yield change_topology_function(topology)
-            returnValue(topology.dump())
+            d = maybeDeferred(change_topology_function, topology)
+            return d.addCallback(lambda result: topology.dump())
+
         return retry_change(self._client, "/topology",
                             change_content_function)
 

=== modified file 'juju/state/initialize.py'
--- juju/state/initialize.py	2012-07-18 01:41:26 +0000
+++ juju/state/initialize.py	2012-08-28 16:13:21 +0000
@@ -8,7 +8,7 @@
 from .auth import make_ace
 from .environment import EnvironmentStateManager, GlobalSettingsStateManager
 from .machine import MachineStateManager
-
+from .security import Principal
 
 log = logging.getLogger("juju.state.init")
 
@@ -50,6 +50,10 @@
         yield self.client.create("/machines", acls=acls)
         yield self.client.create("/units", acls=acls)
         yield self.client.create("/relations", acls=acls)
+        yield self.client.create("/otp", acls=acls)
+
+        # Seed the admin identity
+        yield Principal(*(self.admin_identity.split(":"))).create(self.client)
 
         # In this very specific case, it's OK to create a Constraints object
         # with a non-provider-specific ConstraintSet, because *all* we need it
@@ -72,6 +76,7 @@
         yield settings.set_provider_type(self.provider_type)
 
         # This must come last, since clients will wait on it.
+
         yield self.client.create("/initialized", acls=acls)
 
         # DON'T WRITE ANYTHING HERE.  See line above.

=== modified file 'juju/state/machine.py'
--- juju/state/machine.py	2012-04-02 23:33:39 +0000
+++ juju/state/machine.py	2012-08-28 16:13:21 +0000
@@ -9,6 +9,8 @@
 from juju.state.errors import MachineStateNotFound, MachineStateInUse
 from juju.state.base import StateBase
 from juju.state.utils import remove_tree, YAMLStateNodeMixin
+from juju.state.security import (
+    AgentPrincipal, GroupPrincipal, apply_security_rules)
 
 
 class MachineStateManager(StateBase):
@@ -29,8 +31,15 @@
             flags=zookeeper.SEQUENCE)
         _, internal_id = path.rsplit("/", 1)
 
+        # Add an agent principal and store its otp ref on the machine state.
+        yield AgentPrincipal(self._client).create(internal_id, store=path)
+
         def add_machine(topology):
             topology.add_machine(internal_id)
+
+            # Update the ACL after topology modification, only once though.
+            return apply_security_rules(self._client, path, topology)
+
         yield self._retry_topology_change(add_machine)
 
         returnValue(MachineState(self._client, internal_id))

=== modified file 'juju/state/relation.py'
--- juju/state/relation.py	2012-05-15 19:07:47 +0000
+++ juju/state/relation.py	2012-08-28 16:13:21 +0000
@@ -14,6 +14,7 @@
     DuplicateEndpoints, IncompatibleEndpoints, RelationAlreadyExists,
     RelationStateNotFound, StateChanged, UnitRelationStateNotFound,
     UnknownRelationRole)
+from juju.state.security import apply_security_rules
 
 
 class RelationStateManager(StateBase):
@@ -82,6 +83,7 @@
                                                  endpoint.relation_role,
                                                  endpoint.relation_name))
 
+        @inlineCallbacks
         def add_relation(topology):
             if topology.has_relation_between_endpoints(endpoints):
                 raise RelationAlreadyExists(endpoints)
@@ -97,6 +99,13 @@
                     service_relation.internal_service_id,
                     service_relation.relation_name,
                     service_relation.relation_role)
+
+            # Apply security rules with topology identity.
+            yield apply_security_rules(self._client,
+                                       "/relations/%s" % relation_id,
+                                       topology,
+                                       recurse=True)
+
         yield self._retry_topology_change(add_relation)
 
         returnValue((RelationState(self._client, relation_id), services))
@@ -123,11 +132,11 @@
         # /relations/relation_id/optional_container_id/relation_role/...
         # how far down the path we can create at this point depends on
         # what type of relation we have.
-        if relation_scope == "global":
-            # Add service container in relation.
-            path = "/relations/%s/%s" % (relation_id, endpoint.relation_role)
-        else:
-            path = "/relations/%s" % (relation_id,)
+        if relation_scope != "global":
+            return
+        # Add service container in relation.
+        path = "/relations/%s/%s" % (relation_id, endpoint.relation_role)
+
         try:
             yield self._client.create(path)
         except zookeeper.NodeExistsException:

=== modified file 'juju/state/security.py'
--- juju/state/security.py	2011-09-15 18:50:23 +0000
+++ juju/state/security.py	2012-08-28 16:13:21 +0000
@@ -12,11 +12,26 @@
 
 from juju.state.auth import make_identity, make_ace
 from juju.state.errors import (
-    StateNotFound, PrincipalNotFound)
+    StateChanged, StateNotFound, PrincipalNotFound)
 from juju.state.utils import YAMLState
-
-
-ZOO_OPEN_AUTH_ACL_UNSAFE = make_ace("auth", "world", all=True)
+from juju.state import securityrules
+
+ZOO_OPEN_AUTH_ACL_UNSAFE = ZOO_OPEN_ACL_UNSAFE
+OTP_IDENTITY_KEY = "otp-identity"
+
+
+def _generate_string(size=16):
+    return "".join(random.sample(string.letters, size))
+
+
+@inlineCallbacks
+def apply_security_rules(client, path, topology=None, recurse=False):
+    """Apply security policy ACL to the given path."""
+    token_db = TokenDatabase(client)
+    policy = SecurityPolicy(
+        client, token_db, securityrules.get_default_rules())
+    acl = yield policy(path, topology)
+    yield client.set_acl(path, acl)
 
 
 class Principal(object):
@@ -41,8 +56,16 @@
 
     def attach(self, connection):
         """A principal can be attached to a connection."""
-        return connection.add_auth(
+        auth_deferred = connection.add_auth(
             "digest", "%s:%s" % (self._name, self._password))
+        # Work around for fast auth, remove post ZOOKEEPER-770
+        #connection.exists("/")
+        return auth_deferred
+
+    def create(self, client):
+        """ Stores the user token into the token database.
+        """
+        return TokenDatabase(client).add_token(self.get_token())
 
 
 class GroupPrincipal(object):
@@ -53,19 +76,19 @@
     these credentials.
     """
 
-    def __init__(self, client, path):
+    def __init__(self, client, path, name=None):
         self._client = client
         self._path = path
-        self._name = None
+        self._name = name
         self._password = None
 
     @inlineCallbacks
     def initialize(self):
-        """Initialize the group.
+        """Initialize the group. Reads the group information from zk.
 
         The group must always be initialized before attribute
         use. Groups are persistent principals. If a group is being used
-        as a principal, it must first be loladed via initialize before
+        as a principal, it must first be loaded via initialize before
         any access to principal attributes or principal methods.
         """
 
@@ -99,12 +122,18 @@
     @inlineCallbacks
     def attach(self, connection):
         self._check()
-        yield connection.add_auth(
+        auth_deferred = connection.add_auth(
             "digest", "%s:%s" % (self._name, self._password))
+        # Work around for fast auth, remove post ZOOKEEPER-770
+        #connection.exists("/")
+        yield auth_deferred
 
     @inlineCallbacks
-    def create(self, name, password):
+    def create(self, name, password=None):
         """Create the group with the given name and password."""
+        if password is None:
+            password = _generate_string()
+
         try:
             yield self._client.create(
                 self._path,
@@ -113,43 +142,31 @@
             raise RuntimeError("Group already exists at %r" % self._path)
         self._name = name
         self._password = password
+        yield TokenDatabase(self._client).add_token(self.get_token())
 
-    @inlineCallbacks
     def add_member(self, principal):
         """Add a principal as a member of the group.
 
         A member of the group can use the group as an additional principal
         attached to the connection.
         """
-        self._check()
-        acl, stat = yield self._client.get_acl(self._path)
-        token = principal.get_token()
-
-        for ace in acl:
-            if ace["id"] == token:
-                return
-
-        acl.append(make_ace(token, read=True))
-
-        yield self._client.set_acl(self._path, acl)
-
-    @inlineCallbacks
+        acl = ACL(self._client, self._path)
+        return acl.grant_token(principal.get_token(), read=True).addErrback(
+            lambda error: error.trap(StateChanged) and None)
+
     def remove_member(self, name):
         """Remove a principal from a group by principal name"""
-        self._check()
+        acl = ACL(self._client, self._path)
+        return acl.prohibit(name)
+
+    @inlineCallbacks
+    def get_members(self):
+        """Return the names of the principals that are members of the group."""
         acl, stat = yield self._client.get_acl(self._path)
-
-        found = False
-        for ace in acl:
-            if ace["id"].split(":")[0] == name:
-                acl.remove(ace)
-                found = True
-                break
-
-        if not found:
-            return
-
-        yield self._client.set_acl(self._path, acl)
+        results = []
+        for entry in acl:
+            results.append(entry["id"].split(":", 1)[0])
+        returnValue(results)
 
 
 class OTPPrincipal(object):
@@ -174,9 +191,6 @@
         self._otp = None
         self._path = path
 
-    def _generate_string(self, size=16):
-        return "".join(random.sample(string.letters, size))
-
     def _check(self):
         if not self._name:
             raise RuntimeError("OTPPrincipal must be created before use")
@@ -201,35 +215,53 @@
 
     @classmethod
     @inlineCallbacks
-    def consume(cls, client, otp_data):
+    def consume(cls, client, otp_data=None, path=None):
         """Consume an OTP serialization to retrieve the actual credentials.
 
         returns a username, password tuple, and destroys the OTP node.
         """
+        assert path or otp_data
+        if otp_data is None:
+            content, stat = yield client.get(path)
+            otp_data = yaml.load(content)[OTP_IDENTITY_KEY]
+
         # Decode the data to get the path and otp credentials
         path, credentials = base64.b64decode(otp_data).split(":", 1)
-        yield client.add_auth("digest", credentials)
-        # Load the otp principal data
+
+        # Auth with otp token creds to read the final principal data.
+        p = Principal(*(credentials.split(":")))
+        yield p.attach(client)
+
+        # Load the principal data
         data, stat = yield client.get(path)
         principal_data = yaml.load(data)
+
         # Consume the otp node
         yield client.delete(path)
-        returnValue((principal_data["name"], principal_data["password"]))
+
+        returnValue("%s:%s" % (
+            principal_data["name"], principal_data["password"]))
 
     @inlineCallbacks
-    def create(self, name, password=None, otp_name=None, otp=None):
+    def create(self, name, password=None, otp_name=None, otp=None, store=None):
         """Create an OTP for a principal.
         """
         if self._name:
             raise ValueError("OTPPrincipal has already been created.")
 
         self._name = name
-        self._password = password or self._generate_string()
-        self._otp_name = otp_name or self._generate_string()
-        self._otp = otp or self._generate_string()
+        self._password = password or _generate_string()
+        self._otp_name = otp_name or _generate_string()
+        self._otp = otp or _generate_string()
 
-        acl = [make_ace(
-            make_identity("%s:%s" % (self._otp_name, self._otp)), read=True)]
+        acl = [
+            make_ace(
+                make_identity(
+                    "%s:%s" % (self._otp_name, self._otp)), read=True),
+            make_ace(
+                make_identity(
+                    "%s:%s" % (self._name, self._password)), all=True),
+               ]
 
         # Optional additional ACL entry for unit test teardown.
         if self._extra_otp_ace:
@@ -237,10 +269,20 @@
 
         self._path = yield self._client.create(
             self._path,
-            yaml.safe_dump(dict(name=name, password=password)),
+            yaml.safe_dump(dict(name=self._name, password=self._password)),
             acls=acl,
             flags=SEQUENCE)
 
+        # Store the final principal's name.
+        yield TokenDatabase(self._client).add_token(self.get_token())
+
+        # Optionally serialize the otp path/reference token
+        if store is not None:
+            state = YAMLState(self._client, store)
+            yield state.read()
+            state[OTP_IDENTITY_KEY] = self.serialize()
+            yield state.write()
+
         returnValue(self)
 
     def serialize(self):
@@ -262,6 +304,10 @@
         cls._extra_otp_ace = ace
 
 
+class AgentPrincipal(OTPPrincipal):
+    pass
+
+
 class TokenDatabase(object):
     """A hash map of principal names to their identity tokens.
 
@@ -277,6 +323,17 @@
         yield self._state.read()
         self._state[principal.name] = principal.get_token()
         yield self._state.write()
+        returnValue(principal)
+
+    @inlineCallbacks
+    def add_token(self, token):
+        """Add a principal to the token database.
+        """
+        yield self._state.read()
+        name, checksum = token.split(":")
+        self._state[name] = token
+        yield self._state.write()
+        returnValue(token)
 
     @inlineCallbacks
     def get(self, name):
@@ -323,7 +380,7 @@
         self._rules.append(rule)
 
     @inlineCallbacks
-    def __call__(self, path):
+    def __call__(self, path, topology=None):
         """Given a node path, determine the ACL.
         """
         acl_entries = []
@@ -388,14 +445,25 @@
         """Grant permissions on node to the given principal name."""
 
         token = yield self._token_db.get(principal_name)
+        yield self.grant_token(token, **perms)
+
+    @inlineCallbacks
+    def grant_token(self, token, **perms):
+        """Grant permissions on node to the given identity token.
+
+        Multiple permission grant to the same principal are additive in nature.
+         """
         ace = make_ace(token, **perms)
+        principal_name = token.split(":")[0]
 
         def add(acl):
+
             index = self._principal_index(acl, principal_name)
             if index is not None:
                 acl_ace = acl[index]
                 acl_ace["perms"] = ace["perms"] | acl_ace["perms"]
                 return acl
+
             acl.append(ace)
             return acl
 
@@ -426,6 +494,7 @@
                 acl, stat = yield self._client.get_acl(self._path)
             except (BadArgumentsException, NoNodeException):
                 raise StateNotFound(self._path)
+
             acl = change_func(acl)
             try:
                 yield self._client.set_acl(

=== added file 'juju/state/securityrules.py'
--- juju/state/securityrules.py	1970-01-01 00:00:00 +0000
+++ juju/state/securityrules.py	2012-08-28 16:13:21 +0000
@@ -0,0 +1,4 @@
+
+
+def get_default_rules():
+    return []

=== modified file 'juju/state/service.py'
--- juju/state/service.py	2012-04-05 22:27:11 +0000
+++ juju/state/service.py	2012-08-28 16:13:21 +0000
@@ -23,6 +23,8 @@
 from juju.state.charm import CharmStateManager
 from juju.state.relation import ServiceRelationState, RelationStateManager
 from juju.state.machine import _public_machine_id, MachineState
+from juju.state.security import (
+    AgentPrincipal, GroupPrincipal, apply_security_rules)
 from juju.state.topology import InternalTopologyError
 from juju.state.utils import (
     remove_tree, dict_merge, YAMLState, YAMLStateNodeMixin)
@@ -56,18 +58,24 @@
         # charm metadata is always decoded into unicode, ensure any
         # serialized state references strings to avoid tying to py runtime.
         node_data = yaml.safe_dump(service_details)
-        path = yield self._client.create("/services/service-", node_data,
-                                         flags=zookeeper.SEQUENCE)
+        path = yield self._client.create(
+            "/services/service-", node_data, flags=zookeeper.SEQUENCE)
         internal_id = path.rsplit("/", 1)[1]
 
         # create a child node for configuration options
         yield self._client.create("%s/config" % path, yaml.dump({}))
 
+        # Create a group principal for service units
+        yield GroupPrincipal(self._client, path + '/group').create(internal_id)
+
         def add_service(topology):
             if topology.find_service_with_name(service_name):
                 raise ServiceStateNameInUse(service_name)
             topology.add_service(internal_id, service_name)
 
+            # Update the tree ACL with the modified topology.
+            return apply_security_rules(self._client, path, topology)
+
         yield self._retry_topology_change(add_service)
 
         returnValue(ServiceState(self._client, internal_id, service_name))
@@ -335,8 +343,8 @@
     def get_charm_state(self):
         """Return the CharmState for this service."""
         charm_id = yield self.get_charm_id()
-        formula_state_manager = CharmStateManager(self._client)
-        charm = yield formula_state_manager.get_charm_state(charm_id)
+        charm_state_manager = CharmStateManager(self._client)
+        charm = yield charm_state_manager.get_charm_state(charm_id)
         returnValue(charm)
 
     @inlineCallbacks
@@ -412,32 +420,45 @@
         a notable change to env/service, users would potentially see units
         apparently deployed on wildly inappropriate machines)).
         """
+
         constraints = yield self.get_constraints()
         charm_id = yield self.get_charm_id()
         unit_data = {"charm": charm_id, "constraints": constraints.data}
         path = yield self._client.create(
             "/units/unit-", yaml.dump(unit_data), flags=zookeeper.SEQUENCE)
-
         internal_unit_id = path.rsplit("/", 1)[1]
 
-        sequence = [None]
         if container is not None:
             yield self._validate_principal_container(container)
             container_id = container.internal_id
         else:
             container_id = None
 
+        # Add an agent principal and store its otp ref on the machine state.
+        unit_principal = yield AgentPrincipal(
+            self._client).create(internal_unit_id, store=path)
+
+        # Add the unit to the service security group
+        service_group = yield GroupPrincipal(
+            self._client, self._zk_path + "/group")
+        yield service_group.add_member(unit_principal)
+        # XXX what access should the principal have to the subordinate.
+
+        scope_escape = []
+
         def add_unit(topology):
             if not topology.has_service(self._internal_id):
                 raise StateChanged()
-            sequence[0] = topology.add_service_unit(self._internal_id,
-                                                    internal_unit_id,
-                                                    container_id)
+            scope_escape.append(
+                topology.add_service_unit(
+                    self._internal_id, internal_unit_id, container_id))
+
+            # Update the ACL after topology modification, only once though.
+            return apply_security_rules(self._client, path, topology)
 
         yield self._retry_topology_change(add_unit)
-
         returnValue(ServiceUnitState(self._client, self._internal_id,
-                                     self._service_name, sequence[0],
+                                     self._service_name, scope_escape.pop(),
                                      internal_unit_id))
 
     @inlineCallbacks
@@ -452,7 +473,7 @@
         for unit_id in unit_ids:
             unit_names.append(
                 topology.get_service_unit_name(self._internal_id, unit_id))
-        returnValue(unit_names)
+        returnValue(sorted(unit_names))
 
     @inlineCallbacks
     def remove_unit_state(self, service_unit):
@@ -473,6 +494,10 @@
 
         yield self._retry_topology_change(remove_unit)
 
+        # Update the service security group.
+        service_group = GroupPrincipal(self._client, self._zk_path + '/group')
+        yield service_group.remove_member(service_unit.internal_id)
+
         # Remove any local settings.
         yield remove_tree(
             self._client, "/units/%s" % service_unit.internal_id)
@@ -524,7 +549,6 @@
 
         if not topology.has_service(self._internal_id):
             raise StateChanged()
-
         internal_unit_id = \
             topology.find_service_unit_with_sequence(self._internal_id,
                                                      sequence)

=== modified file 'juju/state/tests/common.py'
--- juju/state/tests/common.py	2012-05-31 23:24:19 +0000
+++ juju/state/tests/common.py	2012-08-28 16:13:21 +0000
@@ -2,12 +2,12 @@
 
 import zookeeper
 
-from txzookeeper.tests.utils import deleteTree
-
 from juju.charm.directory import CharmDirectory
 from juju.charm.tests.test_directory import sample_directory
 from juju.environment.tests.test_config import EnvironmentsConfigTestBase
 from juju.state.topology import InternalTopology
+from juju.state.auth import make_ace
+from juju.state.security import Principal, OTPPrincipal
 
 
 class StateTestBase(EnvironmentsConfigTestBase):
@@ -25,6 +25,8 @@
         yield self.client.create("/services")
         yield self.client.create("/units")
         yield self.client.create("/relations")
+        yield self.client.create("/otp")
+        yield self.setup_security()
 
     @inlineCallbacks
     def tearDown(self):
@@ -33,7 +35,8 @@
         self.client.close()
         client = self.get_zookeeper_client()
         yield client.connect()
-        deleteTree(handle=client.handle)
+        yield Principal("testadmin", "testadmin").attach(client)
+        yield deleteTree(client)
         client.close()
         yield super(StateTestBase, self).tearDown()
 
@@ -53,3 +56,28 @@
             yield self.client.set("/topology", content)
         except zookeeper.NoNodeException:
             yield self.client.create("/topology", content)
+
+    @inlineCallbacks
+    def setup_security(self):
+        """Additional test setup to allow for OTP and security rules.
+        """
+        # Setup additional test access to OTP nodes
+        principal = Principal("testadmin", "testadmin")
+        token = principal.get_token()
+        OTPPrincipal.set_additional_otp_ace(make_ace(token, all=True))
+
+        # Rules application expects 'admin' principal
+        yield Principal("admin", "admin").create(self.client)
+
+        # Remove the test otp ace, post test
+        self.addCleanup(lambda: OTPPrincipal.set_additional_otp_ace(None))
+
+
+@inlineCallbacks
+def deleteTree(client, path="/"):
+    for child in (yield client.get_children(path)):
+        if child == "zookeeper":
+            continue
+        child_path = "/" + ("%s/%s" % (path, child)).strip("/")
+        yield deleteTree(client, child_path)
+        yield client.delete(child_path)

=== modified file 'juju/state/tests/test_agent.py'
--- juju/state/tests/test_agent.py	2011-10-05 00:05:44 +0000
+++ juju/state/tests/test_agent.py	2012-08-28 16:13:21 +0000
@@ -1,12 +1,10 @@
-import zookeeper
-
 from twisted.internet.defer import inlineCallbacks
-from txzookeeper.tests.utils import deleteTree
 
-from juju.lib.testing import TestCase
 from juju.state.base import StateBase
 from juju.state.agent import AgentStateMixin
 
+from juju.state.tests.common import StateTestBase
+
 
 class DomainObject(StateBase, AgentStateMixin):
 
@@ -14,24 +12,15 @@
         return "/agent"
 
 
-class AgentDomainTest(TestCase):
+class AgentDomainTest(StateTestBase):
 
     @inlineCallbacks
     def setUp(self):
-        zookeeper.set_debug_level(0)
-        yield super(TestCase, self).setUp()
+        yield super(AgentDomainTest, self).setUp()
         self.client = self.get_zookeeper_client()
         yield self.client.connect()
 
     @inlineCallbacks
-    def tearDown(self):
-        yield self.client.close()
-        client = self.get_zookeeper_client()
-        yield client.connect()
-        deleteTree("/", client.handle)
-        yield client.close()
-
-    @inlineCallbacks
     def test_has_agent(self):
         domain = DomainObject(self.client)
         exists = yield domain.has_agent()

=== modified file 'juju/state/tests/test_auth.py'
--- juju/state/tests/test_auth.py	2011-09-15 18:50:23 +0000
+++ juju/state/tests/test_auth.py	2012-08-28 16:13:21 +0000
@@ -14,7 +14,7 @@
 
         credentials = "%s:%s" % (username, password)
 
-        identity = "%s:%s" %(
+        identity = "%s:%s" % (
             username,
             base64.b64encode(hashlib.new("sha1", credentials).digest()))
         self.assertEqual(identity, make_identity(credentials))
@@ -25,7 +25,7 @@
 
         credentials = "%s:%s" % (username, password)
 
-        identity = "%s:%s" %(
+        identity = "%s:%s" % (
             username,
             base64.b64encode(hashlib.new("sha1", credentials).digest()))
         self.assertEqual(identity, make_identity(credentials))
@@ -41,7 +41,7 @@
         self.assertEqual(ace["id"], identity)
         self.assertEqual(ace["scheme"], "digest")
         self.assertEqual(
-            ace["perms"], zookeeper.PERM_WRITE|zookeeper.PERM_CREATE)
+            ace["perms"], zookeeper.PERM_WRITE | zookeeper.PERM_CREATE)
 
     def test_make_ace_with_unknown_perm(self):
         identity = "admin:moss"

=== modified file 'juju/state/tests/test_base.py'
--- juju/state/tests/test_base.py	2012-04-06 14:42:06 +0000
+++ juju/state/tests/test_base.py	2012-08-28 16:13:21 +0000
@@ -50,6 +50,21 @@
         self.assertEquals(topology.dump(), topology_dump)
 
     @inlineCallbacks
+    def test_change_topology_waits(self):
+        """Verify yielding topology change functions are respected."""
+        @inlineCallbacks
+        def change_topology(topology):
+            yield self.client.exists("/")
+            yield self.sleep(0.1)
+            topology.add_machine("m-0")
+
+        yield self.base._retry_topology_change(change_topology)
+
+        content, stat = yield self.client.get("/topology")
+        topology = self.parse_topology(content)
+        self.assertTrue(topology.has_machine("m-0"))
+
+    @inlineCallbacks
     def test_change_empty_topology(self):
         """
         Attempting to change a non-existing topology should create
@@ -60,7 +75,6 @@
             topology.add_machine("m-0")
 
         yield self.base._retry_topology_change(change_topology)
-
         content, stat = yield self.client.get("/topology")
         topology = self.parse_topology(content)
         self.assertTrue(topology.has_machine("m-0"))
@@ -98,7 +112,7 @@
 
         def watch_topology(old_topology, new_topology):
             calls.append((old_topology, new_topology))
-            wait_callback[len(calls)-1].callback(True)
+            wait_callback[len(calls) - 1].callback(True)
 
         # Start watching.
         self.base._watch_topology(watch_topology)
@@ -155,7 +169,7 @@
 
         def watch_topology(old_topology, new_topology):
             calls.append((old_topology, new_topology))
-            wait_callback[len(calls)-1].callback(True)
+            wait_callback[len(calls) - 1].callback(True)
 
         # Start watching, and wait on callback immediately.
         self.base._watch_topology(watch_topology)
@@ -205,7 +219,7 @@
 
         def watch_topology(old_topology, new_topology):
             calls.append((old_topology, new_topology))
-            wait_callback[len(calls)-1].callback(True)
+            wait_callback[len(calls) - 1].callback(True)
 
         # Start watching, and wait on callback immediately.
         self.base._watch_topology(watch_topology)
@@ -251,8 +265,8 @@
 
         def watch_topology(old_topology, new_topology):
             calls.append((old_topology, new_topology))
-            wait_callback[len(calls)-1].callback(True)
-            return finish_callback[len(calls)-1]
+            wait_callback[len(calls) - 1].callback(True)
+            return finish_callback[len(calls) - 1]
 
         # Start watching.
         self.base._watch_topology(watch_topology)
@@ -299,7 +313,7 @@
 
         def watcher(old_topology, new_topology):
             calls.append((old_topology, new_topology))
-            wait_callback[len(calls)-1].callback(True)
+            wait_callback[len(calls) - 1].callback(True)
             if len(calls) == 2:
                 raise StopWatcher()
 
@@ -357,7 +371,7 @@
         topology = InternalTopology()
         topology.add_machine("m-0")
         yield self.set_topology(topology)
-        
+
         # Hold off until callback is started.
         yield wait_callback
 

=== modified file 'juju/state/tests/test_environment.py'
--- juju/state/tests/test_environment.py	2012-03-29 01:37:57 +0000
+++ juju/state/tests/test_environment.py	2012-08-28 16:13:21 +0000
@@ -24,10 +24,6 @@
         self.config.load()
 
     @inlineCallbacks
-    def tearDown(self):
-        yield super(EnvironmentStateManagerTest, self).tearDown()
-
-    @inlineCallbacks
     def test_set_config_state(self):
         """
         The simplest thing the manager can do is serialize a given

=== modified file 'juju/state/tests/test_errors.py'
--- juju/state/tests/test_errors.py	2012-04-11 19:38:41 +0000
+++ juju/state/tests/test_errors.py	2012-08-28 16:13:21 +0000
@@ -10,7 +10,7 @@
     ServiceUnitStateMachineAlreadyAssigned, ServiceStateNameInUse,
     BadServiceStateName, EnvironmentStateNotFound,
     RelationAlreadyExists, RelationStateNotFound, UnitRelationStateNotFound,
-    UnitRelationStateAlreadyAssigned, UnknownRelationRole,
+    UnitRelationStateAlreadyAssigned, UnknownRelationRole, PrincipalNotFound,
     BadDescriptor, DuplicateEndpoints, IncompatibleEndpoints,
     NoMatchingEndpoints, AmbiguousRelation,
     ServiceUnitStateMachineNotAssigned, ServiceUnitDebugAlreadyEnabled,

=== modified file 'juju/state/tests/test_firewall.py'
--- juju/state/tests/test_firewall.py	2012-07-09 22:49:36 +0000
+++ juju/state/tests/test_firewall.py	2012-08-28 16:13:21 +0000
@@ -272,7 +272,7 @@
             machine_provider, machine.id)
         returnValue(provider_ports)
 
-    def test_open_close_ports_on_machine(self):
+    def xtest_open_close_ports_on_machine(self):
         """Verify opening/closing ports on a machine works properly.
 
         In particular this is done without watch support."""

=== modified file 'juju/state/tests/test_initialize.py'
--- juju/state/tests/test_initialize.py	2012-04-06 14:42:06 +0000
+++ juju/state/tests/test_initialize.py	2012-08-28 16:13:21 +0000
@@ -1,15 +1,17 @@
 import zookeeper
 
 from twisted.internet.defer import inlineCallbacks
-from txzookeeper.tests.utils import deleteTree
 
 from juju.environment.tests.test_config import EnvironmentsConfigTestBase
-from juju.state.auth import  make_identity
+from juju.state.auth import make_identity, make_ace
 from juju.state.environment import (
     GlobalSettingsStateManager, EnvironmentStateManager)
 from juju.state.initialize import StateHierarchy
+from juju.state.security import OTPPrincipal, Principal
 from juju.state.machine import MachineStateManager
 
+from juju.state.tests.common import deleteTree
+
 
 class LayoutTest(EnvironmentsConfigTestBase):
 
@@ -25,12 +27,22 @@
             "cpu": None,
             "ubuntu-series": "cranky",
             "provider-type": "dummy"}
+        yield self.client.connect()
+
+        self.test_admin = Principal("admin", "secret")
+        yield self.test_admin.attach(self.client)
+        self.identity = self.test_admin.get_token()
+
+        # Add test user to all acl of all otp principal tokens for teardown.
+        OTPPrincipal.set_additional_otp_ace(make_ace(self.identity, all=True))
+        self.addCleanup(lambda: OTPPrincipal.set_additional_otp_ace(None))
+
         self.layout = StateHierarchy(
             self.client, self.identity, "i-abcdef", constraints_data, "dummy")
-        yield self.client.connect()
 
+    @inlineCallbacks
     def tearDown(self):
-        deleteTree(handle=self.client.handle)
+        yield deleteTree(self.client)
         self.client.close()
 
     @inlineCallbacks

=== modified file 'juju/state/tests/test_machine.py'
--- juju/state/tests/test_machine.py	2012-03-28 00:38:16 +0000
+++ juju/state/tests/test_machine.py	2012-08-28 16:13:21 +0000
@@ -8,10 +8,13 @@
 from juju.errors import ConstraintError
 from juju.machine.tests.test_constraints import (
     dummy_constraints, series_constraints)
+
 from juju.state.charm import CharmStateManager
+from juju.state.auth import make_identity
+from juju.state.errors import MachineStateNotFound, MachineStateInUse
 from juju.state.machine import MachineStateManager
 from juju.state.service import ServiceStateManager
-from juju.state.errors import MachineStateNotFound, MachineStateInUse
+from juju.state.security import TokenDatabase, OTPPrincipal
 from juju.state.utils import YAMLState
 
 from juju.state.tests.common import StateTestBase
@@ -66,6 +69,17 @@
         self.assertTrue(topology.has_machine("machine-0000000001"))
 
     @inlineCallbacks
+    def test_add_machine_security(self):
+        machine_state1 = yield self.add_machine_state()
+        creds = yield OTPPrincipal.consume(
+            self.client, path=machine_state1._zk_path)
+        # Verify the otp data is present, by consuming, and matches
+        # the expected token.
+        self.assertEqual(
+            (yield TokenDatabase(self.client).get(machine_state1.internal_id)),
+            make_identity(creds))
+
+    @inlineCallbacks
     def test_incomplete_constraints(self):
         e = yield self.assertFailure(
             self.add_machine_state(dummy_constraints), ConstraintError)

=== modified file 'juju/state/tests/test_relation.py'
--- juju/state/tests/test_relation.py	2012-06-01 22:50:44 +0000
+++ juju/state/tests/test_relation.py	2012-08-28 16:13:21 +0000
@@ -364,6 +364,9 @@
         exists = yield self.client.get(
             "/relations/%s/settings" % relation_state.internal_id)
         self.assertTrue(exists)
+        acl, stat = yield self.client.get_acl(
+            "/relations/%s" % relation_state.internal_id)
+        self.assertACE(acl, name="admin")
 
     @inlineCallbacks
     def test_add_relation_state_to_missing_service(self):

=== modified file 'juju/state/tests/test_security.py'
--- juju/state/tests/test_security.py	2012-02-22 09:23:16 +0000
+++ juju/state/tests/test_security.py	2012-08-28 16:13:21 +0000
@@ -5,19 +5,19 @@
 from twisted.internet.defer import inlineCallbacks, succeed
 
 from juju.state.auth import make_identity, make_ace
-from juju.state.errors import StateNotFound, PrincipalNotFound
+from juju.state.errors import (
+    StateNotFound, PrincipalNotFound)
 
 from juju.state.security import (
     ACL, Principal, GroupPrincipal, OTPPrincipal, TokenDatabase,
-    SecurityPolicy, SecurityPolicyConnection)
+    SecurityPolicy, SecurityPolicyConnection, apply_security_rules)
 
 from juju.lib.testing import TestCase
+from juju.state.tests.common import StateTestBase, deleteTree
 from juju.tests.common import get_test_zookeeper_address
 
 from txzookeeper.client import ZOO_OPEN_ACL_UNSAFE
 
-from txzookeeper.tests.utils import deleteTree
-
 
 class PrincipalTests(TestCase):
 
@@ -26,8 +26,9 @@
         zookeeper.set_debug_level(0)
         self.client = yield self.get_zookeeper_client().connect()
 
+    @inlineCallbacks
     def tearDown(self):
-        deleteTree(handle=self.client.handle)
+        yield deleteTree(self.client)
         self.client.close()
 
     def test_name(self):
@@ -48,7 +49,7 @@
         self.addCleanup(lambda: client.close())
         admin_credentials = "admin:admin"
         test_credentials = "test:test"
-        yield self.client.add_auth("digest", admin_credentials)
+        auth_deferred = self.client.add_auth("digest", admin_credentials)
 
         acl = [make_ace(make_identity(admin_credentials), all=True),
                make_ace(make_identity(
@@ -56,12 +57,16 @@
 
         yield client.create("/acl-test", "content", acls=acl)
 
+        # Wait on the auth after the create due to 770
+        yield auth_deferred
+
         # Verify the acl is active
         yield self.assertFailure(
             client.get("/acl-test"), zookeeper.NoAuthException)
 
         # Attach the principal to the connection
         principal = Principal("test", "test")
+
         yield principal.attach(client)
         content, stat = yield client.get("/acl-test")
         self.assertEqual(content, "content")
@@ -74,8 +79,9 @@
         zookeeper.set_debug_level(0)
         self.client = yield self.get_zookeeper_client().connect()
 
+    @inlineCallbacks
     def tearDown(self):
-        deleteTree(handle=self.client.handle)
+        yield deleteTree(self.client)
         self.client.close()
 
     def test_uninitialized_usage(self):
@@ -129,20 +135,40 @@
         self.assertEqual(
             acl[1:],
             [make_ace(principal.get_token(), read=True)])
-        # Adding a member again is fine
+        # Adding a member again is no-op, same goal state.
         yield group.add_member(principal)
 
     @inlineCallbacks
+    def test_get_members(self):
+        group = GroupPrincipal(self.client, "/group-a")
+        yield group.create("group/a", "zebra")
+
+        principals = [
+            Principal("aladdin", "genie"),
+            Principal("babar", "elephant")]
+
+        for p in principals:
+            yield group.add_member(p)
+
+        # Policy isn't active by default
+        principals.insert(0, Principal("anyone", ""))
+
+        members = yield group.get_members()
+        self.assertEqual(
+            [p.name for p in principals],
+            members)
+
+    @inlineCallbacks
     def test_remove_member(self):
         group = GroupPrincipal(self.client, "/group-a")
         yield group.create("group/a", "zebra")
 
         principal = Principal("aladdin", "genie")
+
         # Removing a member that doesn't exist is a no-op
-        yield group.remove_member(principal)
+        yield group.remove_member(principal.name)
         yield group.add_member(principal)
         yield group.remove_member(principal.name)
-
         acl, stat = yield self.client.get_acl("/group-a")
         self.assertEqual(acl[1:], [])
 
@@ -154,7 +180,7 @@
 
         admin_credentials = "admin:admin"
         test_credentials = "test:test"
-        yield self.client.add_auth("digest", admin_credentials)
+        self.client.add_auth("digest", admin_credentials)
 
         acl = [make_ace(make_identity(admin_credentials), all=True),
                make_ace(make_identity(
@@ -182,8 +208,9 @@
         self.client = yield self.get_zookeeper_client().connect()
         self.client.create("/otp")
 
+    @inlineCallbacks
     def tearDown(self):
-        deleteTree(handle=self.client.handle)
+        yield deleteTree(self.client)
         self.client.close()
 
     def set_otp_test_ace(self, test_ace=ZOO_OPEN_ACL_UNSAFE):
@@ -235,7 +262,10 @@
         self.assertEqual(credentials["password"], "secret")
 
         acl, stat = yield self.client.get_acl(otp_path)
-        self.assertEqual(len(acl), 2)
+        if OTPPrincipal._extra_otp_ace:
+            self.assertEqual(len(acl), 3)
+        else:
+            self.assertEqual(len(acl), 2)
 
     @inlineCallbacks
     def test_serialize(self):
@@ -267,12 +297,34 @@
         yield self.assertFailure(
             self.client.get(path), zookeeper.NoAuthException)
 
-        name, password = yield OTPPrincipal.consume(self.client, otp_data)
+        credentials = yield OTPPrincipal.consume(self.client, otp_data)
+        name, password = credentials.split(":")
         self.assertEqual(name, "foobar")
         self.assertEqual(password, "secret")
         children = yield self.client.get_children("/otp")
         self.assertFalse(children)
 
+    @inlineCallbacks
+    def test_create_and_store(self):
+        """Creates an otp principal and stores it serialization at path.
+        """
+        yield OTPPrincipal(self.client).create(
+            "aladdin", store="/zoo")
+
+        # Verify the otp serialization on the node
+        contents, stat = yield self.client.get("/zoo")
+        data = yaml.load(contents)
+        self.assertIn("otp-identity", data)
+
+        # Compare the persitsent user generated token to that in the tokendb
+        creds = yield OTPPrincipal.consume(self.client, path="/zoo")
+
+        token = yield TokenDatabase(self.client).get("aladdin")
+        self.assertEqual(
+            token, make_identity(creds))
+
+        self.assertTrue((yield self.client.exists("/zoo")))
+
 
 class TokenDatabaseTest(TestCase):
 
@@ -282,8 +334,9 @@
         self.client = yield self.get_zookeeper_client().connect()
         self.db = TokenDatabase(self.client, "/token-test")
 
+    @inlineCallbacks
     def tearDown(self):
-        deleteTree(handle=self.client.handle)
+        yield deleteTree(self.client)
         self.client.close()
 
     @inlineCallbacks
@@ -328,8 +381,9 @@
         yield self.tokens.add(Principal("admin", "admin"))
         self.policy = SecurityPolicy(self.client, self.tokens)
 
+    @inlineCallbacks
     def tearDown(self):
-        deleteTree(handle=self.client.handle)
+        yield deleteTree(self.client)
         self.client.close()
 
     @inlineCallbacks
@@ -349,7 +403,7 @@
         solution.
         """
         acl = yield self.policy("/random")
-        self.assertIn(make_ace("auth", "world", all=True), acl)
+        self.assertIn(make_ace("anyone", "world", all=True), acl)
 
     @inlineCallbacks
     def test_rule_match_suppress_open_access(self):
@@ -361,7 +415,7 @@
         # Check for matched rule ACL
         self.assertIn(make_ace(principal.get_token(), all=True), acl)
         # Verify no default access
-        self.assertNotIn(make_ace("auth", "world", all=True), acl)
+        self.assertNotIn(make_ace("anyone", "world", all=True), acl)
 
     @inlineCallbacks
     def test_rule_that_returns_deferred(self):
@@ -374,7 +428,7 @@
         # Check for matched rule ACL
         self.assertIn(make_ace(principal.get_token(), all=True), acl)
         # Verify no default access
-        self.assertNotIn(make_ace("auth", "world", all=True), acl)
+        self.assertNotIn(make_ace("anyone", "world", all=True), acl)
 
     @inlineCallbacks
     def test_owner_ace(self):
@@ -399,13 +453,11 @@
         self.token_db = TokenDatabase(self.client)
         yield self.token_db.add(admin)
         self.policy = SecurityPolicy(self.client, self.token_db, owner=admin)
-        attach_defer = admin.attach(self.client)
-        # Trick to speed up the auth response processing (fixed in ZK trunk)
-        self.client.exists("/")
-        yield attach_defer
+        yield admin.attach(self.client)
 
+    @inlineCallbacks
     def tearDown(self):
-        deleteTree(handle=self.client.handle)
+        yield deleteTree(self.client)
         self.client.close()
 
     @inlineCallbacks
@@ -448,13 +500,11 @@
         self.admin = Principal("admin", "admin")
         yield self.tokens.add(self.admin)
         self.policy = SecurityPolicy(self.client, self.tokens)
-        attach_deferred = self.admin.attach(self.client)
-
-        self.client.exists("/")
-        yield attach_deferred
-
+        yield self.admin.attach(self.client)
+
+    @inlineCallbacks
     def tearDown(self):
-        deleteTree(handle=self.client.handle)
+        yield deleteTree(self.client)
         self.client.close()
 
     @inlineCallbacks
@@ -469,11 +519,10 @@
         client = yield self.get_zookeeper_client().connect()
         principal = Principal("zebra", "stripes")
         yield self.tokens.add(principal)
-        attach_deferred = principal.attach(client)
+        yield principal.attach(client)
         yield self.client.create(
             "/abc",
             acls=[make_ace(self.admin.get_token(), all=True)])
-        yield attach_deferred
 
         acl = ACL(client, "/abc")
         yield self.assertFailure(
@@ -508,6 +557,27 @@
         yield self.assertFailure(acl.grant("zebra"), PrincipalNotFound)
 
     @inlineCallbacks
+    def test_multi_grant(self):
+        """Grants can be made multiple times against the same node version.
+        """
+        path = yield self.client.create("/abc")
+
+        alice = Principal("alice", "")
+        bob = Principal("bob", "")
+
+        yield self.tokens.add(alice)
+        yield self.tokens.add(bob)
+
+        acl = ACL(self.client, path)
+        yield acl.grant("alice", all=True)
+        yield acl.grant("bob", all=True)
+
+        acl, stat = yield self.client.get_acl(path)
+
+        self.assertACE(acl, alice.get_token())
+        self.assertACE(acl, bob.get_token())
+
+    @inlineCallbacks
     def test_prohibit(self):
         principal = Principal("zebra", "stripes")
         yield self.tokens.add(principal)
@@ -544,3 +614,38 @@
         acl, stat = yield self.client.get_acl(path)
         self.assertEqual(
             acl, [make_ace(self.admin.get_token(), all=True)])
+
+
+class SecurityModuleAPITest(StateTestBase):
+    """
+    Test of the top level conviencce functions in the security module.
+    """
+
+    @inlineCallbacks
+    def test_apply_rules(self):
+        """Calculates and sets an acl using the security rules.
+        """
+        cli_group = Principal("admin", "admin")
+        yield cli_group.attach(self.client)
+        yield TokenDatabase(self.client).add(cli_group)
+
+        principal = Principal("testadmin", "testadmin")
+        token = principal.get_token()
+
+        def get_test_rules():
+            return [lambda x, y: [make_ace(token, all=True)]]
+
+        get_default_rules = self.mocker.replace(
+            "juju.state.securityrules.get_default_rules")
+
+        get_default_rules()
+        self.mocker.call(get_test_rules)
+        self.mocker.replay()
+
+        path = yield self.client.create("/moon")
+        yield apply_security_rules(self.client, path)
+        acl, stat = yield self.client.get_acl(path)
+        self.assertEqual(
+            acl,
+            [make_ace(token, all=True),
+             make_ace(cli_group.get_token(), all=True)])

=== modified file 'juju/state/tests/test_service.py'
--- juju/state/tests/test_service.py	2012-07-03 07:30:53 +0000
+++ juju/state/tests/test_service.py	2012-08-28 16:13:21 +0000
@@ -234,24 +234,23 @@
         else:
             self.fail("Error not raised")
 
+    @inlineCallbacks
     def test_get_unit_state(self):
         """A unit state can be retrieved by name from the service manager."""
-        self.assertFailure(self.service_state_manager.get_unit_state(
-                "wordpress/1"), ServiceStateNotFound)
 
-        self.assertFailure(self.service_state_manager.get_unit_state(
-                "wordpress1"), ServiceUnitStateNotFound)
+        yield self.assertFailure(self.service_state_manager.get_unit_state(
+            "wordpress/1"), ServiceStateNotFound)
 
         wordpress_state = yield self.service_state_manager.add_service_state(
                 "wordpress", self.charm_state, dummy_constraints)
 
-        self.assertFailure(self.service_state_manager.get_unit_state(
-                "wordpress/1"), ServiceUnitStateNotFound)
+        yield self.assertFailure(self.service_state_manager.get_unit_state(
+            "wordpress/0"), ServiceUnitStateNotFound)
 
-        wordpress_unit = wordpress_state.add_unit_state()
+        wordpress_unit = yield wordpress_state.add_unit_state()
 
         unit_state = yield self.service_state_manager.get_unit_state(
-                        "wordpress/1")
+                "wordpress/0")
 
         self.assertEqual(unit_state.internal_id, wordpress_unit.internal_id)
 
@@ -588,7 +587,6 @@
         self.assertEquals((yield unit_state2.get_container()), unit_state3)
         self.assertEquals((yield unit_state3.get_container()), None)
 
-
     @inlineCallbacks
     def test_add_service_unit_with_changing_state(self):
         """
@@ -646,6 +644,7 @@
         exists = yield self.client.exists("/units/%s" % unit_state.internal_id)
         self.assertFalse(exists)
 
+    @inlineCallbacks
     def test_remove_service_unit_nonexistant(self):
         """Removing a non existant service unit, is fine."""
 
@@ -653,7 +652,9 @@
                 "wordpress", self.charm_state, dummy_constraints)
         unit_state = yield service_state.add_unit_state()
         yield service_state.remove_unit_state(unit_state)
-        yield service_state.remove_unit_state(unit_state)
+        yield self.assertFailure(
+            service_state.remove_unit_state(unit_state),
+            StateChanged)
 
     @inlineCallbacks
     def test_get_all_service_states(self):
@@ -1601,7 +1602,7 @@
         event = yield watch_d
         self.assertEqual(event.type_name, "created")
         self.assertEqual(event.path,
-                                         "/units/%s/agent" % unit_state.internal_id)
+                         "/units/%s/agent" % unit_state.internal_id)
 
     @inlineCallbacks
     def test_get_charm_id(self):
@@ -1760,7 +1761,8 @@
                 mysql_service_state)
         other_constraints = dummy_cs.parse(["arch=arm"])
         wordpress_service_state = yield self.add_service_from_charm(
-                "wordpress", constraints=other_constraints.with_series("series"))
+            "wordpress",
+            constraints=other_constraints.with_series("series"))
         wordpress_unit_state = yield wordpress_service_state.add_unit_state()
         yield self.assertFailure(
                 wordpress_unit_state.assign_to_unused_machine(),
@@ -1965,7 +1967,8 @@
                 [RelationEndpoint("wordpress", "varnish", "cache", "client"),
                 RelationEndpoint("wordpress", "mysql", "db", "client"),
                 RelationEndpoint("wordpress", "http", "url", "server"),
-                RelationEndpoint("wordpress", "juju-info", "juju-info", "server")])
+                RelationEndpoint(
+                     "wordpress", "juju-info", "juju-info", "server")])
         yield self.add_service_from_charm("riak")
         self.assertEqual(
                 (yield self.service_state_manager.get_relation_endpoints(
@@ -2056,7 +2059,8 @@
                 (yield self.service_state_manager.join_descriptors(
                         "wordpress", "varnish")),
                 [(RelationEndpoint("wordpress", "varnish", "cache", "client"),
-                  RelationEndpoint("varnish", "varnish", "webcache", "server"))])
+                  RelationEndpoint(
+                      "varnish", "varnish", "webcache", "server"))])
 
     @inlineCallbacks
     def test_join_descriptors_service_name_relation_name(self):
@@ -2084,12 +2088,14 @@
                 (yield self.service_state_manager.join_descriptors(
                         "wordpress:cache", "varnish")),
                 [(RelationEndpoint("wordpress", "varnish", "cache", "client"),
-                  RelationEndpoint("varnish", "varnish", "webcache", "server"))])
+                  RelationEndpoint(
+                      "varnish", "varnish", "webcache", "server"))])
         self.assertEqual(
                 (yield self.service_state_manager.join_descriptors(
                         "wordpress:cache", "varnish:webcache")),
                 [(RelationEndpoint("wordpress", "varnish", "cache", "client"),
-                  RelationEndpoint("varnish", "varnish", "webcache", "server"))])
+                  RelationEndpoint(
+                      "varnish", "varnish", "webcache", "server"))])
 
     @inlineCallbacks
     def test_join_peer_descriptors(self):

=== modified file 'juju/unit/lifecycle.py'
--- juju/unit/lifecycle.py	2012-05-01 00:26:25 +0000
+++ juju/unit/lifecycle.py	2012-08-28 16:13:21 +0000
@@ -535,7 +535,8 @@
                     relation_info["relation_scope"])
                 self._relations[relation_id] = workflow
 
-    def _reconstruct_workflow(self, relation_id, relation_ident, relation_scope):
+    def _reconstruct_workflow(
+        self, relation_id, relation_ident, relation_scope):
         """Create a RelationWorkflowState which may refer to outdated state.
 
         This means that *if* this service has already departed the relevant