launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/maas-pserv-auth into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/maas-pserv-auth/+merge/101365

This adds username and password authentication to pserv. It supports only a single username and password, because this is meant to be used by maas only. The password will be set dynamically at package installation time so that maas can talk to pserv. In common with other username config settings, it defaults to the current user.

-- 
https://code.launchpad.net/~allenap/maas/maas-pserv-auth/+merge/101365
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/maas-pserv-auth into lp:maas.

=== modified file '.ctags'
--- .ctags	2012-03-12 11:06:46 +0000
+++ .ctags	2012-04-10 12:29:23 +0000
@@ -1,5 +1,4 @@
 --python-kinds=-iv
---exclude=*-min.js
---exclude=*-debug.js
+--exclude=*.js
 --extra=+f
---links=no
+--links=yes

=== modified file 'etc/pserv.yaml'
--- etc/pserv.yaml	2012-04-02 07:06:37 +0000
+++ etc/pserv.yaml	2012-04-10 12:29:23 +0000
@@ -6,6 +6,12 @@
 #
 # port: 5241
 
+## The credentials which the Provisioning API will require.
+#
+# username: <current user>
+# password:
+password: "test"
+
 ## Where to log. This log can be rotated by sending SIGUSR1 to the
 ## running server.
 #

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-04-02 07:06:37 +0000
+++ src/maas/settings.py	2012-04-10 12:29:23 +0000
@@ -10,6 +10,7 @@
 
 __metaclass__ = type
 
+from getpass import getuser
 import os
 from urlparse import urljoin
 
@@ -254,7 +255,7 @@
 
 # The location of the Provisioning API XML-RPC endpoint.  This should
 # match the setting in etc/pserv.yaml.
-PSERV_URL = "http://localhost:5241/api";
+PSERV_URL = "http://%s:test@localhost:5241/api"; % getuser()
 
 # Use a real provisioning server?  If yes, the URL for the provisioning
 # server's API should be set in PSERV_URL.  If this is set to False, for

=== modified file 'src/provisioningserver/plugin.py'
--- src/provisioningserver/plugin.py	2012-03-15 22:43:55 +0000
+++ src/provisioningserver/plugin.py	2012-04-10 12:29:23 +0000
@@ -35,15 +35,71 @@
     IServiceMaker,
     MultiService,
     )
+from twisted.cred.checkers import ICredentialsChecker
+from twisted.cred.credentials import IUsernamePassword
+from twisted.cred.error import UnauthorizedLogin
+from twisted.cred.portal import (
+    IRealm,
+    Portal,
+    )
+from twisted.internet.defer import (
+    inlineCallbacks,
+    maybeDeferred,
+    returnValue,
+    )
 from twisted.plugin import IPlugin
 from twisted.python import (
     log,
     usage,
     )
-from twisted.web.resource import Resource
+from twisted.web.guard import (
+    BasicCredentialFactory,
+    HTTPAuthSessionWrapper,
+    )
+from twisted.web.resource import (
+    IResource,
+    Resource,
+    )
 from twisted.web.server import Site
 import yaml
-from zope.interface import implements
+from zope.interface import implementer
+
+
+@implementer(ICredentialsChecker)
+class SingleUsernamePasswordChecker:
+    """An `ICredentialsChecker` for a single username and password."""
+
+    credentialInterfaces = [IUsernamePassword]
+
+    def __init__(self, username, password):
+        super(SingleUsernamePasswordChecker, self).__init__()
+        self.username = username
+        self.password = password
+
+    @inlineCallbacks
+    def requestAvatarId(self, credentials):
+        if credentials.username == self.username:
+            matched = yield maybeDeferred(
+                credentials.checkPassword, self.password)
+            if matched:
+                returnValue(credentials.username)
+        raise UnauthorizedLogin(credentials.username)
+
+
+@implementer(IRealm)
+class ProvisioningRealm:
+    """The `IRealm` for the Provisioning API."""
+
+    noop = staticmethod(lambda: None)
+
+    def __init__(self, resource):
+        super(ProvisioningRealm, self).__init__()
+        self.resource = resource
+
+    def requestAvatar(self, avatarId, mind, *interfaces):
+        if IResource in interfaces:
+            return (IResource, self.resource, self.noop)
+        raise NotImplementedError()
 
 
 class ConfigOops(Schema):
@@ -90,6 +146,8 @@
     if_key_missing = None
 
     port = Int(min=1, max=65535, if_missing=5241)
+    username = String(not_empty=True, if_missing=getuser())
+    password = String(not_empty=True)
     logfile = String(if_empty=b"pserv.log", if_missing=b"pserv.log")
     oops = ConfigOops
     broker = ConfigBroker
@@ -115,17 +173,27 @@
         ]
 
 
+@implementer(IServiceMaker, IPlugin)
 class ProvisioningServiceMaker(object):
     """Create a service for the Twisted plugin."""
 
-    implements(IServiceMaker, IPlugin)
-
     options = Options
 
     def __init__(self, name, description):
         self.tapname = name
         self.description = description
 
+    def _makeProvisioningAPI(self, config, cobbler_session):
+        """Construct an `IResource` for the Provisioning API."""
+        papi_xmlrpc = ProvisioningAPI_XMLRPC(cobbler_session)
+        papi_realm = ProvisioningRealm(papi_xmlrpc)
+        papi_checker = SingleUsernamePasswordChecker(
+            config["username"], config["password"])
+        papi_portal = Portal(papi_realm, [papi_checker])
+        papi_creds = BasicCredentialFactory(b"MAAS Provisioning API")
+        papi_root = HTTPAuthSessionWrapper(papi_portal, [papi_creds])
+        return papi_root
+
     def makeService(self, options):
         """Construct a service."""
         services = MultiService()
@@ -168,10 +236,11 @@
         cobbler_session = CobblerSession(
             cobbler_config["url"], cobbler_config["username"],
             cobbler_config["password"])
-        papi_xmlrpc = ProvisioningAPI_XMLRPC(cobbler_session)
+
+        papi_root = self._makeProvisioningAPI(config, cobbler_session)
 
         site_root = Resource()
-        site_root.putChild("api", papi_xmlrpc)
+        site_root.putChild("api", papi_root)
         site = Site(site_root)
         site_port = config["port"]
         site_service = TCPServer(site_port, site)

=== modified file 'src/provisioningserver/tests/test_plugin.py'
--- src/provisioningserver/tests/test_plugin.py	2012-03-21 12:18:48 +0000
+++ src/provisioningserver/tests/test_plugin.py	2012-04-10 12:29:23 +0000
@@ -20,15 +20,27 @@
 from provisioningserver.plugin import (
     Config,
     Options,
+    ProvisioningRealm,
     ProvisioningServiceMaker,
+    SingleUsernamePasswordChecker,
     )
 from testtools import TestCase
+from testtools.deferredruntest import (
+    assert_fails_with,
+    AsynchronousDeferredRunTest,
+    )
 from testtools.matchers import (
     MatchesException,
     Raises,
     )
+from twisted.application.internet import TCPServer
 from twisted.application.service import MultiService
+from twisted.cred.credentials import UsernamePassword
+from twisted.cred.error import UnauthorizedLogin
+from twisted.internet.defer import inlineCallbacks
 from twisted.python.usage import UsageError
+from twisted.web.guard import HTTPAuthSessionWrapper
+from twisted.web.resource import IResource
 import yaml
 
 
@@ -36,6 +48,9 @@
     """Tests for `provisioningserver.plugin.Config`."""
 
     def test_defaults(self):
+        mandatory = {
+            'password': 'killing_joke',
+            }
         expected = {
             'broker': {
                 'host': 'localhost',
@@ -55,13 +70,18 @@
                 'reporter': '',
                 },
             'port': 5241,
+            'username': getuser(),
             }
-        observed = Config.to_python({})
+        expected.update(mandatory)
+        observed = Config.to_python(mandatory)
         self.assertEqual(expected, observed)
 
     def test_parse(self):
         # Configuration can be parsed from a snippet of YAML.
-        observed = Config.parse(b'logfile: "/some/where.log"')
+        observed = Config.parse(
+            b'logfile: "/some/where.log"\n'
+            b'password: "black_sabbath"\n'
+            )
         self.assertEqual("/some/where.log", observed["logfile"])
 
     def test_load(self):
@@ -69,7 +89,8 @@
         filename = os.path.join(
             self.useFixture(TempDir()).path, "config.yaml")
         with open(filename, "wb") as stream:
-            stream.write(b'logfile: "/some/where.log"')
+            stream.write(b'logfile: "/some/where.log"\n')
+            stream.write(b'password: "megadeth"\n')
         observed = Config.load(filename)
         self.assertEqual("/some/where.log", observed["logfile"])
 
@@ -122,6 +143,7 @@
         self.tempdir = self.useFixture(TempDir()).path
 
     def write_config(self, config):
+        config.setdefault("password", "evile")
         config_filename = os.path.join(self.tempdir, "config.yaml")
         with open(config_filename, "wb") as stream:
             yaml.dump(config, stream)
@@ -165,3 +187,58 @@
         self.assertEqual(
             len(service.namedServices), len(service.services),
             "Not all services are named.")
+
+    def test_makeService_api_requires_credentials(self):
+        """
+        The site service's /api resource requires credentials from clients.
+        """
+        options = Options()
+        options["config-file"] = self.write_config({})
+        service_maker = ProvisioningServiceMaker("Harry", "Hill")
+        service = service_maker.makeService(options)
+        self.assertIsInstance(service, MultiService)
+        site_service = service.getServiceNamed("site")
+        self.assertIsInstance(site_service, TCPServer)
+        port, site = site_service.args
+        self.assertIn("api", site.resource.listStaticNames())
+        api = site.resource.getStaticEntity("api")
+        self.assertIsInstance(api, HTTPAuthSessionWrapper)
+
+
+class TestSingleUsernamePasswordChecker(TestCase):
+    """Tests for `SingleUsernamePasswordChecker`."""
+
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
+
+    @inlineCallbacks
+    def test_requestAvatarId_okay(self):
+        credentials = UsernamePassword("frank", "zappa")
+        checker = SingleUsernamePasswordChecker("frank", "zappa")
+        avatar = yield checker.requestAvatarId(credentials)
+        self.assertEqual("frank", avatar)
+
+    def test_requestAvatarId_bad(self):
+        credentials = UsernamePassword("frank", "zappa")
+        checker = SingleUsernamePasswordChecker("zap", "franka")
+        d = checker.requestAvatarId(credentials)
+        return assert_fails_with(d, UnauthorizedLogin)
+
+
+class TestProvisioningRealm(TestCase):
+    """Tests for `ProvisioningRealm`."""
+
+    def test_requestAvatar_okay(self):
+        resource = object()
+        realm = ProvisioningRealm(resource)
+        avatar = realm.requestAvatar(
+            "irrelevant", "also irrelevant", IResource)
+        self.assertEqual((IResource, resource, realm.noop), avatar)
+
+    def test_requestAvatar_bad(self):
+        # If IResource is not amongst the interfaces passed to requestAvatar,
+        # NotImplementedError is raised.
+        resource = object()
+        realm = ProvisioningRealm(resource)
+        self.assertRaises(
+            NotImplementedError, realm.requestAvatar,
+            "irrelevant", "also irrelevant")