Merge lp:~hazmat/pyjuju/security-node-policy-def into lp:pyjuju

[Kapil Thangavelu]

Zookeeper clients need to use ACLs for nodes created by them in order to leverage zookeeper security. The security policy class implemented in this branch enable these policy choices via a path based lookup for the corresponding ACLs. The policy defers to rules matched by the path to provide an ACL for the path.

[Gustavo Niemeyer (niemeyer)]

This looks very nice.. tiny, organized, clear. Thanks!

[1]

33 +class SecurityPolicy(object):
34 + """The security policy generates ACLs for new nodes based on their path.
35 + """

This should be able to handle existing nodes as well, I suppose? Imagine a
relation.. what happens when we add a new service unit to it?

[2]

83 +def owner_ace(principal):
84 + return make_ace(principal.get_token(), all=True)

This function doesn't feel worth it. Besides being called a single time, it's
a single liner, so it's just obscuring logic at the call site.

[3]

132 + """If no rules match, the default acl gives authenticated users access.
133 + """

Might be nice to add an equivalent comment here saying we don't actually want this for real.

review: Approve

[Benjamin Saller (bcsaller)]

LGTM as well.

[4] Want to verify that Principals don't need a detach method because the connection is implicitly scoped to the secured session?

[5] The way `owner` is used I'm not sure you really need to track it at all. It only maps to an `all` token once. Similar to point [2]

[6] add_rule should make more clear the signature of rules given that they don't have a base class or imply an interface. docstring change.

review: Approve

[Kapil Thangavelu (hazmat)]

> This looks very nice.. tiny, organized, clear. Thanks!
>
> [1]
>
> 33 +class SecurityPolicy(object):
> 34 + """The security policy generates ACLs for new nodes based on
> their path.
> 35 + """
>
> This should be able to handle existing nodes as well, I suppose? Imagine a
> relation.. what happens when we add a new service unit to it?

I'm going to utilize group principals for services, so giving access to the group for the acl will suffice, else there are too many possible extant acls on nodes that will need cleanup/modification when a principal (like a unit agent) dis/appears.

[Kapil Thangavelu (hazmat)]

> LGTM as well.
>
> [4] Want to verify that Principals don't need a detach method because the
> connection is implicitly scoped to the secured session?

There is no api to remove them from a connection/session available at the extension level.

> [5] The way `owner` is used I'm not sure you really need to track it at all.
> It only maps to an `all` token once. Similar to point [2]

sounds good, i'm still playing with with how this integrates with the connection, but your right, just tracking the owner ace value would suffice here.

>
> [6] add_rule should make more clear the signature of rules given that they
> don't have a base class or imply an interface. docstring change.

sounds good.

[Kapil Thangavelu (hazmat)]

> > This looks very nice.. tiny, organized, clear. Thanks!
> >
> > [1]
> >
> > 33 +class SecurityPolicy(object):
> > 34 + """The security policy generates ACLs for new nodes based on
> > their path.
> > 35 + """
> >
> > This should be able to handle existing nodes as well, I suppose? Imagine a
> > relation.. what happens when we add a new service unit to it?
>
> I'm going to utilize group principals for services, so giving access to the
> group for the acl will suffice, else there are too many possible extant acls
> on nodes that will need cleanup/modification when a principal (like a unit
> agent) dis/appears.

Just to clarify some more re updates, afaics with group principals, they will be a rare enough case, that the code that needs them can handle them inline. The only case i've come across so far is the provisioning agent updating a machine state to grant access to the machine agent principal it just created.

[lp:~hazmat/pyjuju/security-node-policy-def]

275. By Kapil Thangavelu on 2011-07-19

Merged trunk-merge into security-node-policy-def.

276. By Kapil Thangavelu on 2011-07-19

address review comments, inline owner_ace func, additional doc comments on rule signature.

=== modified file 'ensemble/state/security.py'
--- ensemble/state/security.py	2011-07-11 18:16:42 +0000
+++ ensemble/state/security.py	2011-07-19 15:43:29 +0000
@@ -1,8 +1,12 @@
 from twisted.internet.defer import inlineCallbacks, returnValue
-from ensemble.state.auth import make_identity
+
+from ensemble.state.auth import make_identity, make_ace
 from ensemble.state.utils import YAMLState
 
 
+ZOO_OPEN_AUTH_ACL_UNSAFE = make_ace("auth", "world", all=True)
+
+
 class Principal(object):
     """An ensemble/zookeeper principal.
     """
@@ -24,7 +28,7 @@
         return make_identity("%s:%s" % (self._name, self._password))
 
     def attach(self, connection):
-        """A princpal can be attached to a connection."""
+        """A principal can be attached to a connection."""
         return connection.add_auth(
             "digest", "%s:%s" % (self._name, self._password))
 
@@ -60,3 +64,55 @@
         if name in self._state:
             del self._state[name]
             yield self._state.write()
+
+
+class SecurityPolicy(object):
+    """The security policy generates ACLs for new nodes based on their path.
+    """
+    def __init__(self, client, token_db, rules=(), owner=None):
+        self._client = client
+        self._rules = list(rules)
+        self._token_db = token_db
+        self._owner = None
+
+    def set_owner(self, principal):
+        """If an owner is set all nodes ACLs will grant access to the owner.
+        """
+        assert not self._owner, "Owner already assigned"
+        self._owner = principal
+
+    def add_rule(self, rule):
+        """Add a security rule to the policy.
+
+        A rule is a callable object accepting the policy and the path as
+        arguments. The rule should return a list of ACL entries that apply
+        to the node at the given path. Rules may return deferred values.
+        """
+        self._rules.append(rule)
+
+    @inlineCallbacks
+    def __call__(self, path):
+        """Given a node path, determine the ACL.
+        """
+        acl_entries = []
+
+        for rule in self._rules:
+            entries = yield rule(self, path)
+            if entries:
+                acl_entries.extend(entries)
+
+        # XXX/TODO - Remove post security-integration
+        # Allow incremental integration
+        if not acl_entries:
+            acl_entries.append(ZOO_OPEN_AUTH_ACL_UNSAFE)
+
+        # Give cli admin access by default
+        admin_token = yield self._token_db.get("admin")
+        acl_entries.append(make_ace(admin_token, all=True))
+
+        # Give owner access by default
+        if self._owner:
+            acl_entries.append(
+                make_ace(self._owner.get_token(), all=True))
+
+        returnValue(acl_entries)

=== modified file 'ensemble/state/tests/test_security.py'
--- ensemble/state/tests/test_security.py	2011-07-11 18:16:42 +0000
+++ ensemble/state/tests/test_security.py	2011-07-19 15:43:29 +0000
@@ -1,10 +1,10 @@
 import yaml
 import zookeeper
 
-from twisted.internet.defer import inlineCallbacks
+from twisted.internet.defer import inlineCallbacks, succeed
 
 from ensemble.state.auth import make_identity, make_ace
-from ensemble.state.security import Principal, TokenDatabase
+from ensemble.state.security import Principal, TokenDatabase, SecurityPolicy
 
 from ensemble.lib.testing import TestCase
 from txzookeeper.tests.utils import deleteTree
@@ -93,3 +93,71 @@
         yield self.db.add(principal)
         token = yield self.db.get(principal.name)
         self.assertEqual(token, principal.get_token())
+
+
+class PolicyTest(TestCase):
+
+    @inlineCallbacks
+    def setUp(self):
+        zookeeper.set_debug_level(0)
+        self.client = yield self.get_zookeeper_client().connect()
+        self.tokens = TokenDatabase(self.client)
+        yield self.tokens.add(Principal("admin", "admin"))
+        self.policy = SecurityPolicy(self.client, self.tokens)
+
+    def tearDown(self):
+        deleteTree(handle=self.client.handle)
+        self.client.close()
+
+    @inlineCallbacks
+    def test_default_no_owner_no_rules_gives_admin_access(self):
+        """By default the policy setups a global access for the cli admins.
+        """
+        acl = yield self.policy("/random")
+        self.assertIn(
+            make_ace(Principal("admin", "admin").get_token(), all=True), acl)
+
+    @inlineCallbacks
+    def test_default_no_rules_gives_global_authenticated_access(self):
+        """If no rules match, the default acl gives authenticated users access.
+
+        XXX/TODO: This is intended as a temporary crutch for
+        integration of the security machinery, not a long term
+        solution.
+        """
+        acl = yield self.policy("/random")
+        self.assertIn(make_ace("auth", "world", all=True), acl)
+
+    @inlineCallbacks
+    def test_rule_match_suppress_open_access(self):
+        """If a rule returns an acl, then no default access is given."""
+        principal = Principal("foobar", "foobar")
+        self.policy.add_rule(lambda policy, path: [
+            make_ace(principal.get_token(), all=True)])
+        acl = yield self.policy("/random")
+        # 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)
+
+    @inlineCallbacks
+    def test_rule_that_returns_deferred(self):
+        """If a rule may do additional lookups, resulting in deferred values.
+        """
+        principal = Principal("foobar", "foobar")
+        self.policy.add_rule(lambda policy, path: succeed([
+            make_ace(principal.get_token(), all=True)]))
+        acl = yield self.policy("/random")
+        # 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)
+
+    @inlineCallbacks
+    def test_owner_ace(self):
+        """If an owner is set, all nodes ACLs will have an owner ACE.
+        """
+        owner = Principal("john", "doe")
+        self.policy.set_owner(owner)
+        acl = yield self.policy("/random")
+        self.assertIn(make_ace(owner.get_token(), all=True), acl)