Merge into rapi-rollup : rapi-login : Code : pyjuju

Status:	Merged
Merged at revision:	624
Proposed branch:	lp://staging/~hazmat/pyjuju/rapi-login
Merge into:	lp://staging/~hazmat/pyjuju/rapi-rollup
Diff against target:	256 lines (+84/-12) 6 files modified juju/errors.py (+4/-0) juju/rapi/context.py (+9/-2) juju/rapi/tests/common.py (+1/-0) juju/rapi/tests/test_context.py (+3/-1) juju/rapi/transport/tests/test_ws.py (+47/-2) juju/rapi/transport/ws.py (+20/-7)
To merge this branch:	bzr merge lp://staging/~hazmat/pyjuju/rapi-login
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Kapil Thangavelu			Pending
Review via email: mp+140268@code.staging.launchpad.net

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2012-12-17:

#

Download full text (8.5 KiB)

Reviewers: mp+140268_code.launchpad.net,

Message:
Please take a look.

Description:
Add a login api

Login support using admin credentials/secret

https://code.launchpad.net/~hazmat/juju/rapi-login/+merge/140268

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/agents/api.py
   M juju/rapi/context.py
   M juju/rapi/tests/test_context.py
   M juju/state/initialize.py
   M juju/state/tests/test_initialize.py

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: juju/agents/api.py
=== modified file 'juju/agents/api.py'
--- juju/agents/api.py 2012-09-10 03:43:29 +0000
+++ juju/agents/api.py 2012-12-12 19:30:20 +0000
@@ -11,6 +11,7 @@
  from juju.errors import JujuError
  from juju.lib.websockets import WebSocketsResource
  from juju.rapi.transport.ws import WebSocketAPIFactory
+from juju.state.auth import make_ace

  from twisted.web.static import Data
  from twisted.web.server import Site
@@ -32,7 +33,7 @@
          log.debug("Received environment data.")
          root = Data("Juju API Server\n", "text/plain")
          self.ws_factory = WebSocketAPIFactory(
- self.config['zookeeper_servers'], self.provider)
+ self.config['zookeeper_servers'], self.provider)
          yield self.ws_factory.startFactory()

          root.putChild('ws', WebSocketsResource(self.ws_factory))
@@ -79,10 +80,30 @@
                  environment = yield watch_d
              returnValue(environment)

+ # Lazy initialize login nodes if not present.
+ yield self._initialize_login()
+
          config = EnvironmentsConfig()
          config.parse(environment_data)
          returnValue(config.get_default())

+ @inlineCallbacks
+ def _initialize_login(self):
+ children = yield self.client.get_children("/")
+ if "login" in children:
+ return
+
+ acls, stat = yield self.client.get_acl("/initialized")
+ admin_acl = None
+
+ for a in acls:
+ if a['id'].startswith('admin'):
+ admin_acl = a
+ break
+
+ yield self.client.create(
+ "/login", acls=[make_ace(admin_acl['id'], all=True)])
+
      def configure(self, options):
          super(APIEndpointAgent, self).configure(options)
          if not options.get("port"):

Index: juju/rapi/context.py
=== modified file 'juju/rapi/context.py'
--- juju/rapi/context.py 2012-10-12 20:49:45 +0000
+++ juju/rapi/context.py 2012-12-12 19:30:20 +0000
@@ -1,5 +1,6 @@
from StringIO import StringIO
-from twisted.internet.defer import maybeDeferred, succeed
+from twisted.internet.defer import (
+ maybeDeferred, succeed, returnValue, inlineCallbacks)

from juju.rapi.cmd import add_relation
from juju.rapi.cmd import add_unit
@@ -22,7 +23,10 @@

from juju.rapi import rest

+from juju.state.security import Principal
+
...

Reviewers: mp+140268_code.launchpad.net,

Message:
Please take a look.

Description:
Add a login api

Login support using admin credentials/secret

https://code.launchpad.net/~hazmat/juju/rapi-login/+merge/140268

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/agents/api.py
   M juju/rapi/context.py
   M juju/rapi/tests/test_context.py
   M juju/state/initialize.py
   M juju/state/tests/test_initialize.py

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: kapil@canonical.com-20121107125114-ff5oz0w1olti12po
+New revision: kapil@canonical.com-20121212193020-upkkru9lxwz9hmso

Index: juju/agents/api.py
=== modified file 'juju/agents/api.py'
--- juju/agents/api.py	2012-09-10 03:43:29 +0000
+++ juju/agents/api.py	2012-12-12 19:30:20 +0000
@@ -11,6 +11,7 @@
  from juju.errors import JujuError
  from juju.lib.websockets import WebSocketsResource
  from juju.rapi.transport.ws import WebSocketAPIFactory
+from juju.state.auth import make_ace

from twisted.web.static import Data
  from twisted.web.server import Site
@@ -32,7 +33,7 @@
          log.debug("Received environment data.")
          root = Data("Juju API Server\n", "text/plain")
          self.ws_factory = WebSocketAPIFactory(
-                self.config['zookeeper_servers'], self.provider)
+            self.config['zookeeper_servers'], self.provider)
          yield self.ws_factory.startFactory()

root.putChild('ws', WebSocketsResource(self.ws_factory))
@@ -79,10 +80,30 @@
                  environment = yield watch_d
              returnValue(environment)

+        # Lazy initialize login nodes if not present.
+        yield self._initialize_login()
+
          config = EnvironmentsConfig()
          config.parse(environment_data)
          returnValue(config.get_default())

+    @inlineCallbacks
+    def _initialize_login(self):
+        children = yield self.client.get_children("/")
+        if "login" in children:
+            return
+
+        acls, stat = yield self.client.get_acl("/initialized")
+        admin_acl = None
+
+        for a in acls:
+            if a['id'].startswith('admin'):
+                admin_acl = a
+                break
+
+        yield self.client.create(
+            "/login", acls=[make_ace(admin_acl['id'], all=True)])
+
      def configure(self, options):
          super(APIEndpointAgent, self).configure(options)
          if not options.get("port"):

Index: juju/rapi/context.py
=== modified file 'juju/rapi/context.py'
--- juju/rapi/context.py	2012-10-12 20:49:45 +0000
+++ juju/rapi/context.py	2012-12-12 19:30:20 +0000
@@ -1,5 +1,6 @@
  from StringIO import StringIO
-from twisted.internet.defer import maybeDeferred, succeed
+from twisted.internet.defer import (
+    maybeDeferred, succeed, returnValue, inlineCallbacks)

from juju.rapi.cmd import add_relation
  from juju.rapi.cmd import add_unit
@@ -22,7 +23,10 @@

from juju.rapi import rest

+from juju.state.security import Principal
+
  import logging
+import zookeeper

log = logging.getLogger('juju.rapi.context')

@@ -78,7 +82,7 @@
          d = maybeDeferred(func, *args, **kw)
          return d.addCallback(
              lambda r: {'result': r, 'log': self.log.reset()}
-            ).addErrback(self._on_error)
+        ).addErrback(self._on_error)

def _on_error(self, failure):
          io = StringIO()
@@ -134,8 +138,7 @@
          return self._invoke(
              constraints_get.constraints_get,
              self,
-            named_entities
-            )
+            named_entities)

def get_config(self, service_name):
          """Get configuration of a service.
@@ -163,8 +166,7 @@
              constraints,
              config,
              config_raw,
-            num_units
-            )
+            num_units)

def debug_hooks(self, unit_name, hook_names=()):
          """Enable hook debug on a unit.
@@ -206,6 +208,25 @@
              self,
              data)

+    def login(self, user, password):
+        return self._invoke(
+            self._login,
+            user,
+            password)
+
+    @inlineCallbacks
+    def _login(self, user, password):
+        principal = Principal(user, password)
+        yield principal.attach(self.client)
+        try:
+            yield self.client.get("/login")
+        except (zookeeper.NoAuthException, zookeeper.AuthFailedException):
+            self.log.error("Invalid credentials")
+            raise
+        else:
+            self.log.info("Login success")
+        returnValue(True)
+
      def remove_relation(self, endpoint_a, endpoint_b):
          """Remove a relation from a service(s).
          """

Index: juju/state/initialize.py
=== modified file 'juju/state/initialize.py'
--- juju/state/initialize.py	2012-07-18 01:41:26 +0000
+++ juju/state/initialize.py	2012-12-12 19:30:20 +0000
@@ -71,6 +71,10 @@
          settings = GlobalSettingsStateManager(self.client)
          yield settings.set_provider_type(self.provider_type)

+        # Create a node that only the admin can read.
+        yield self.client.create("/login", acls=[
+            make_ace(self.admin_identity, all=True)])
+
          # This must come last, since clients will wait on it.
          yield self.client.create("/initialized", acls=acls)

Index: juju/rapi/tests/test_context.py
=== modified file 'juju/rapi/tests/test_context.py'
--- juju/rapi/tests/test_context.py	2012-11-07 12:49:11 +0000
+++ juju/rapi/tests/test_context.py	2012-12-12 19:30:20 +0000
@@ -1,6 +1,9 @@
  from twisted.internet.defer import inlineCallbacks
  from juju.rapi.tests.common import ContextTestBase

+from juju.state.security import Principal
+from juju.state.auth import make_ace
+

class APIContextTest(ContextTestBase):

@@ -159,6 +162,20 @@
          self.assertEqual(response['result'], cs)

@inlineCallbacks
+    def test_login(self):
+        u, p = "admin", "cekret"
+        principal = Principal(u, p)
+        self.client.create(
+            "/login", acls=[make_ace(principal.get_token(), all=True)])
+        result = yield self.context.login(u, "hello")
+        self.assertEqual(result['log'], [('error', 'Invalid credentials')])
+        self.assertEqual(result['err'], True)
+        result = yield self.context.login(u, p)
+        self.assertEqual(result['result'], True)
+        self.assertEqual(result['log'], [('info', 'Login success')])
+        yield self.client.delete("/login")
+
+    @inlineCallbacks
      def test_expose_and_unexpose(self):
          yield self.mock_store_charms(["precise/wordpress"])
          self.mocker.replay()

Index: juju/state/tests/test_initialize.py
=== 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-12-12 19:30:20 +0000
@@ -4,11 +4,12 @@
  from txzookeeper.tests.utils import deleteTree

from juju.environment.tests.test_config import EnvironmentsConfigTestBase
-from juju.state.auth import  make_identity
+
  from juju.state.environment import (
      GlobalSettingsStateManager, EnvironmentStateManager)
  from juju.state.initialize import StateHierarchy
  from juju.state.machine import MachineStateManager
+from juju.state.security import Principal

class LayoutTest(EnvironmentsConfigTestBase):
@@ -19,7 +20,9 @@
          self.log = self.capture_logging("juju.state.init")
          zookeeper.set_debug_level(0)
          self.client = self.get_zookeeper_client()
-        self.identity = make_identity("admin:genie")
+        self.principal = Principal("admin", "genie")
+        self.identity = self.principal.get_token()
+
          constraints_data = {
              "arch": "arm",
              "cpu": None,
@@ -28,6 +31,7 @@
          self.layout = StateHierarchy(
              self.client, self.identity, "i-abcdef",  
constraints_data, "dummy")
          yield self.client.connect()
+        yield self.principal.attach(self.client)

def tearDown(self):
          deleteTree(handle=self.client.handle)
@@ -57,6 +61,7 @@
          yield self.assert_existence_and_acl("/machines")
          yield self.assert_existence_and_acl("/relations")
          yield self.assert_existence_and_acl("/initialized")
+        yield self.assert_existence_and_acl("/login")

# To check that the constraints landed correctly, we need the
          # environment config to have been sent, or we won't be able to

Revision history for this message

Nicola Larosa (teknico) wrote on 2012-12-17:

#

Code looks good, on the face of it (I'm not familiar with this
codebase).

I dislike one- or two-letter names, even in tests, but that's just
personal preference.

https://codereview.appspot.com/6935068/

Revision history for this message

Benjamin Saller (bcsaller) wrote on 2012-12-17:

#

I think I'm a little shy on context here given the comments below. If
the goals are in keeping with the comments then this seems ok, if we are
looking for real ACL access over the tree, it wasn't clear to make that
worked as expected.

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py
File juju/agents/api.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py#newcode102
juju/agents/api.py:102: break
Its not possible for someone to register another 'admin' like name here
or will the first added always appear first? This seems an odd way of
tracking this. Since this codebase only supports admin anyway wouldn't
this be acls[0] anyway or are there others?

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py#newcode105
juju/agents/api.py:105: "/login", acls=[make_ace(admin_acl['id'],
all=True)])
I don't recall the details of the security branch, but this copies it
from /initialized to /login basically? I only have vague guesses as to
why this is the process, you need to create it during init to set up the
agents and then add it to the login tree which will in the future
support additional logins?

https://codereview.appspot.com/6935068/diff/1/juju/rapi/context.py
File juju/rapi/context.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/rapi/context.py#newcode222
juju/rapi/context.py:222: yield self.client.get("/login")
This does the ACL check on the login node and and uses that as the
success boolean. Are the other nodes protected with the same ACL? Its
not clear where that's being managed here.

https://codereview.appspot.com/6935068/diff/1/juju/rapi/tests/test_context.py
File juju/rapi/tests/test_context.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/rapi/tests/test_context.py#newcode177
juju/rapi/tests/test_context.py:177:
Again it looks like the ACL check works here, but I'm not sure if the
way this is wired in provides any actual access control outside of that
node. Is that a non-goal with this iteration?

https://codereview.appspot.com/6935068/

I think I'm a little shy on context here given the comments below. If
the goals are in keeping with the comments then this seems ok, if we are
looking for real ACL access over the tree, it wasn't clear to make that
worked as expected.

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py
File juju/agents/api.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py#newcode102
juju/agents/api.py:102: break
Its not possible for someone to register another 'admin' like name here
or will the first added always appear first? This seems an odd way of
tracking this. Since this codebase only supports admin anyway wouldn't
this be acls[0] anyway or are there others?

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py#newcode105
juju/agents/api.py:105: "/login", acls=[make_ace(admin_acl['id'],
all=True)])
I don't recall the details of the security branch, but this copies it
from /initialized to /login basically? I only have vague guesses as to
why this is the process, you need to create it during init to set up the
agents and then add it to the login tree which will in the future
support additional logins?

https://codereview.appspot.com/6935068/diff/1/juju/rapi/context.py
File juju/rapi/context.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/rapi/context.py#newcode222
juju/rapi/context.py:222: yield self.client.get("/login")
This does the ACL check on the login node and and uses that as the
success boolean. Are the other nodes protected with the same ACL? Its
not clear where that's being managed here.

https://codereview.appspot.com/6935068/diff/1/juju/rapi/tests/test_context.py
File juju/rapi/tests/test_context.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/rapi/tests/test_context.py#newcode177
juju/rapi/tests/test_context.py:177:
Again it looks like the ACL check works here, but I'm not sure if the
way this is wired in provides any actual access control outside of that
node. Is that a non-goal with this iteration?

https://codereview.appspot.com/6935068/

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2012-12-17:

#

Download full text (3.5 KiB)

replies to ben's comments.

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py
File juju/agents/api.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py#newcode102
juju/agents/api.py:102: break
On 2012/12/17 18:36:59, benjamin.saller wrote:
> Its not possible for someone to register another 'admin' like name
here or will
> the first added always appear first? This seems an odd way of tracking
this.
> Since this codebase only supports admin anyway wouldn't this be
acls[0] anyway
> or are there others?

at the moment the only time this node is created is if it doesn't exist,
at the client side. there is no other client that will modify it once
its in place. the initialized node is the marker node for the rest of
the system to go forward once the environment details have been loaded.

ie. while its true that multiple 'admin' users could potentially be
created with different passwords, this node is WORM and thus suitable
for this use.

https://codereview.appspot.com/6935068/diff/1/juju/agents/api.py#newcode105
juju/agents/api.py:105: "/login", acls=[make_ace(admin_acl['id'],
all=True)])
On 2012/12/17 18:36:59, benjamin.saller wrote:
> I don't recall the details of the security branch, but this copies it
from
> /initialized to /login basically?

This copies the admin identity (which is a hash of the secret of the
user id) with a more restrictive acl grant to a newly created login
node. Effectively to read this node you must have authenticated with the
admin credentials.

This code also exists in initialized, but our current usage/deployment
mode is via charm so the initialize code from this branch will never be
called, hence this lazy init technique for the api agent startup.

> I only have vague guesses as to why this is
> the process, you need to create it during init to set up the agents
and then add
> it to the login tree which will in the future support additional
logins?

hopefully the previous comments make it clear.

https://codereview.appspot.com/6935068/diff/1/juju/rapi/context.py
File juju/rapi/context.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/rapi/context.py#newcode222
juju/rapi/context.py:222: yield self.client.get("/login")
On 2012/12/17 18:36:59, benjamin.saller wrote:
> This does the ACL check on the login node and and uses that as the
success
> boolean. Are the other nodes protected with the same ACL? Its not
clear where
> that's being managed here.

access to the node is only possible if the username and password are
correct, ie. match the admin identity used for the node acl.

https://codereview.appspot.com/6935068/diff/1/juju/rapi/tests/test_context.py
File juju/rapi/tests/test_context.py (right):

https://codereview.appspot.com/6935068/diff/1/juju/rapi/tests/test_context.py#newcode177
juju/rapi/tests/test_context.py:177:
On 2012/12/17 18:36:59, benjamin.saller wrote:
> Again it looks like the ACL check works here, but I'm not sure if the
way this
> is wired in provides any actual access control outside of that node.
Is that non-goal with this iteration?

this isn't about access control at the node tree level, that's the
security branch work which is *much* larger ...