Merge lp:~hazmat/pyjuju/rapi-login into lp:~hazmat/pyjuju/rapi-rollup

[Kapil Thangavelu]

Require login before api usage.

This update requires login before using the ws api and before the delta stream is active.

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

[Kapil Thangavelu (hazmat)]

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

[Nicola Larosa (teknico)]

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/

[Benjamin Saller (bcsaller)]

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/

[Kapil Thangavelu (hazmat)]

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

[Kapil Thangavelu (hazmat)]

Please take a look.

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

[Kapil Thangavelu (hazmat)]

Please take a look.

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

[Benjamin Saller (bcsaller)]

LGTM

I didn't see a test around the login call returning the added
unauthorized error though.

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

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/context.py#newcode85
juju/rapi/context.py:85: if not self.authenticated and not func.__name__
== "_login":
func.__name__ has a bad smell to it but fair enough

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/transport/ws.py
File juju/rapi/transport/ws.py (right):

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/transport/ws.py#newcode68
juju/rapi/transport/ws.py:68:
Is there an use case for a logout/inverse to this stopping the delta? I
suspect not at this time.

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

[Kapil Thangavelu (hazmat)]

thanks for the review

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

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/context.py#newcode85
juju/rapi/context.py:85: if not self.authenticated and not func.__name__
== "_login":
On 2013/01/18 14:04:23, bcsaller wrote:
> func.__name__ has a bad smell to it but fair enough

its already playing with func.. not many other options outside of
decorating every other function with another attribute for perm, which
still amounts to the same, ie func inspection.

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/transport/ws.py
File juju/rapi/transport/ws.py (right):

https://codereview.appspot.com/6935068/diff/12001/juju/rapi/transport/ws.py#newcode68
juju/rapi/transport/ws.py:68:
On 2013/01/18 14:04:23, bcsaller wrote:
> Is there an use case for a logout/inverse to this stopping the delta?
I suspect
> not at this time.

logout not possible with the underlying zk auth mechanism, wrt to use
not at the moment outside of disconnect.

ideally stream sub/unsub would be explicit api methods, but doing so
without gui support first would break the gui.

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

=== modified file 'juju/errors.py'
--- juju/errors.py	2012-08-28 16:09:47 +0000
+++ juju/errors.py	2013-01-17 23:19:20 +0000
@@ -12,6 +12,10 @@
     """
 
 
+class Unauthorized(JujuError):
+    """The user is not authorized to perform the operation."""
+
+
 class IncompatibleVersion(JujuError):
     """Raised when there is a mismatch in versions using the topology.
 

=== modified file 'juju/rapi/context.py'
--- juju/rapi/context.py	2012-12-20 04:43:20 +0000
+++ juju/rapi/context.py	2013-01-17 23:19:20 +0000
@@ -1,6 +1,8 @@
 from StringIO import StringIO
 from twisted.internet.defer import (
-    maybeDeferred, succeed, returnValue, inlineCallbacks)
+    fail, maybeDeferred, succeed, returnValue, inlineCallbacks)
+
+from juju.errors import Unauthorized
 
 from juju.rapi.cmd import add_relation
 from juju.rapi.cmd import add_unit
@@ -67,6 +69,7 @@
         self.client = client
         self.provider = provider
         self.log = LogBuffer()
+        self.authenticated = False
 
     def connect(self, *args, **kw):
         if self.client.connected:
@@ -80,7 +83,10 @@
             self.client.close()
 
     def _invoke(self, func, *args, **kw):
-        d = maybeDeferred(func, *args, **kw)
+        if not self.authenticated and not func.__name__ == "_login":
+            d = fail(Unauthorized("Login required."))
+        else:
+            d = maybeDeferred(func, *args, **kw)
         return d.addCallback(
             lambda r: {'result': r, 'log': self.log.reset()}
         ).addErrback(self._on_error)
@@ -233,6 +239,7 @@
             self.log.error("Invalid credentials")
             raise
         else:
+            self.authenticated = True
             self.log.info("Login success")
         returnValue(True)
 

=== modified file 'juju/rapi/tests/common.py'
--- juju/rapi/tests/common.py	2012-09-27 12:02:51 +0000
+++ juju/rapi/tests/common.py	2013-01-17 23:19:20 +0000
@@ -34,6 +34,7 @@
             "cloudy", {"default-series": "precise",
                        "storage-directory": self.makeDir()})
         self.context = APIContext(self.client, self.provider)
+        self.context.authenticated = True
         self.context.charm_cache_dir = self.makeDir()
         yield self.client.create("/environment", TEST_ENV)
 

=== modified file 'juju/rapi/tests/test_context.py'
--- juju/rapi/tests/test_context.py	2012-12-20 04:43:20 +0000
+++ juju/rapi/tests/test_context.py	2013-01-17 23:19:20 +0000
@@ -182,12 +182,14 @@
         principal = Principal(u, p)
         self.client.create(
             "/login", acls=[make_ace(principal.get_token(), all=True)])
+        self.context.authenticated = False
         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')])
+        self.assertTrue(self.context.authenticated)
         yield self.client.delete("/login")
 
     @inlineCallbacks
@@ -205,7 +207,7 @@
                                     'public-address': None}},
                               'charm': 'cs:precise/wordpress-3',
                               'relations': {}}},
-             'machines': {0: {'instance-id': 'pending'}}}
+                'machines': {0: {'instance-id': 'pending'}}}
         self.assertEqual(response['result'], stat)
 
         response = yield self.context.expose('wordpress')

=== modified file 'juju/rapi/transport/tests/test_ws.py'
--- juju/rapi/transport/tests/test_ws.py	2012-10-04 15:58:43 +0000
+++ juju/rapi/transport/tests/test_ws.py	2013-01-17 23:19:20 +0000
@@ -1,14 +1,16 @@
 import json
 import logging
 
-
 from twisted.internet.defer import inlineCallbacks, Deferred
 
 from juju.rapi.transport.ws import WebSocketAPIFactory
 from juju.providers.dummy import MachineProvider
 
+from juju.state.auth import make_ace
+from juju.state.topology import InternalTopology
+from juju.state.security import Principal
+
 from juju.rapi.tests.common import ContextTestBase
-from juju.state.topology import InternalTopology
 from juju.state.tests.common import StateTestBase
 from juju.tests.common import get_test_zookeeper_address
 
@@ -112,7 +114,10 @@
     @inlineCallbacks
     def test_status(self):
         d = self.transport.watch_kv('op', 'status')
+
         self.ws.connectionMade()
+        self.ws.context.authenticated = True
+
         self.ws.dataReceived(json.dumps(dict(
             op='status')))
         msg = yield d
@@ -139,6 +144,44 @@
         return super(DeltaSocketTest, self).tearDown()
 
     @inlineCallbacks
+    def test_delta_after_login(self):
+        """No delta events are sent till after login.
+        """
+        # Setup for login and service deploy
+        u, p = "admin", "cekret"
+        self.client.create("/login")
+        yield self.mock_store_charms(["precise/wordpress"])
+        self.mocker.replay()
+
+        self.ws.connectionMade()
+        yield self.context.deploy("precise/wordpress")
+
+        # Even if we pump the delta stream now, the connected client
+        # sees nothing till after login.
+        yield self.stream_manager.pump()
+
+        greeting = self.transport.stream[-1]
+        self.assertEqual(greeting,
+                         {u'state': u'login-required',
+                          u'version': 0,
+                          u'extensions': [],
+                          u'ready': True,
+                          u'default_series': u'precise',
+                          u'provider_type': u'dummy'})
+
+        login_d = self.transport.watch_kv('op', 'login')
+        delta_d = self.transport.watch_kv('op', 'delta')
+
+        self.ws.dataReceived(json.dumps(dict(
+            op='login', user=u, password=p)))
+
+        yield login_d
+        yield delta_d
+
+        delta = self.transport.stream[-1]
+        self.assertEqual(delta['op'], 'delta')
+
+    @inlineCallbacks
     def test_delta_stream(self):
         yield self.mock_store_charms(["precise/wordpress", "precise/mysql"])
         self.mocker.replay()
@@ -146,6 +189,7 @@
         # There's always an implicit initial pump of current state made
         # on connection. But no initial state for this test.
         self.ws.connectionMade()
+        self.ws.context.authenticated = True
 
         # Modify state and send events out.
         yield self.context.deploy("precise/mysql")
@@ -153,6 +197,7 @@
         yield self.stream_manager.pump()
 
         delta = self.transport.stream[-1]
+
         self.assertEqual(delta['op'], 'delta')
         self.assertEqual(delta['result'], [
             [u'service', u'add', {u'id': 'wordpress',

=== modified file 'juju/rapi/transport/ws.py'
--- juju/rapi/transport/ws.py	2012-10-04 15:58:43 +0000
+++ juju/rapi/transport/ws.py	2013-01-17 23:19:20 +0000
@@ -46,18 +46,26 @@
         """Process the next message off the websocket channel.
         """
         self.transport.write(json.dumps(
-            {"version": 0, "extensions": [], "ready": True,
+            {"version": 0,
+             "extensions": [],
+             "ready": True,
+             "state": "login-required",
              "provider_type": self.provider.provider_type,
              "default_series": self.provider.config.get("default-series")}))
 
         log.info("Sent greeting")
 
-        # Push the current state.
-        self.publish_changes()
-
+        subscribed = False
         while 1:
             if not self.active:
                 break
+
+            # Enable delta with initial push of current after login.
+            if self.context.authenticated and not subscribed:
+                subscribed = True
+                self.publish_changes()
+                self.factory.stream_manager.add(self)
+
             msg = yield self.queue.get()
             if msg is None:
                 break
@@ -123,6 +131,8 @@
 
     def handle_msg_error(self, err):
         # TODO: return back a request/op structure with error info.
+        # ... errors generally handled by context, if we get here the
+        # connection is hosed.
         log.warning("Error %s" % err)
         self.active = False
         self.transport.write(json.dumps(
@@ -173,8 +183,12 @@
         self.zk_hosts = zk_hosts
         self.provider = provider
         self.numClients = 0
-        self.stream_manager = DeltaStreamManager(
-            APIContext(ManagedClient(self.zk_hosts), self.provider))
+
+        # Setup an internal context soley for use by the stream manager.
+        stream_context = APIContext(
+            ManagedClient(self.zk_hosts), self.provider)
+        stream_context.authenticated = True
+        self.stream_manager = DeltaStreamManager(stream_context)
 
     # The common usage for twisted's native ws support is via protocol
     # wrapper mounted onto an http server. This usage prevents
@@ -193,5 +207,4 @@
     def buildProtocol(self, addr=None):
         p = self.protocol(self.zk_hosts, self.provider)
         p.factory = self
-        self.stream_manager.add(p)
         return p