Merge lp:~cjwatson/launchpad/codeimport-git-auth into lp:launchpad

[Colin Watson]

Allow pushing to Git repositories associated with running code import jobs using macaroon credentials.

Allow pushing to Git repositories associated with running code import jobs using macaroon credentials.

We don't actually issue suitable macaroons as yet outside the test suite; that will come in a later branch.

I've tried to design the IMacaroonIssuer interface so that it can be useful for some other similar applications (e.g. granting access tokens to builders), but it's provisional and I entirely expect it to need to change when it's actually battle-tested for that kind of thing.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2016-10-03 17:00:56 +0000
+++ lib/lp/code/configure.zcml	2016-10-12 12:42:25 +0000
@@ -729,6 +729,15 @@
                     reclaimJob"/>
   </securedutility>
 
+  <!-- CodeImportJobMacaroonIssuer -->
+
+  <securedutility
+      class="lp.code.model.codeimportjob.CodeImportJobMacaroonIssuer"
+      provides="lp.services.macaroons.interfaces.IMacaroonIssuer"
+      name="code-import-job">
+    <allow interface="lp.services.macaroons.interfaces.IMacaroonIssuerPublic"/>
+  </securedutility>
+
   <!-- CodeImportEvent -->
 
   <class class="lp.code.model.codeimportevent.CodeImportEvent">

=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py	2016-10-03 17:00:56 +0000
+++ lib/lp/code/model/codeimportjob.py	2016-10-12 12:42:25 +0000
@@ -12,6 +12,10 @@
 
 import datetime
 
+from pymacaroons import (
+    Macaroon,
+    Verifier,
+    )
 from sqlobject import (
     ForeignKey,
     IntCol,
@@ -27,7 +31,9 @@
     CodeImportMachineState,
     CodeImportResultStatus,
     CodeImportReviewStatus,
+    GitRepositoryType,
     )
+from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.code.interfaces.codeimportevent import ICodeImportEventSet
 from lp.code.interfaces.codeimportjob import (
     ICodeImportJob,
@@ -37,6 +43,7 @@
     )
 from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
 from lp.code.interfaces.codeimportresult import ICodeImportResultSet
+from lp.code.interfaces.gitrepository import IGitRepository
 from lp.code.model.codeimportresult import CodeImportResult
 from lp.registry.interfaces.person import validate_public_person
 from lp.services.config import config
@@ -48,6 +55,7 @@
     SQLBase,
     sqlvalues,
     )
+from lp.services.macaroons.interfaces import IMacaroonIssuer
 
 
 @implementer(ICodeImportJob)
@@ -340,3 +348,49 @@
         # 4)
         getUtility(ICodeImportEventSet).newReclaim(
             code_import, machine, job_id)
+
+
+@implementer(IMacaroonIssuer)
+class CodeImportJobMacaroonIssuer:
+
+    @property
+    def _root_secret(self):
+        secret = config.codeimport.macaroon_secret_key
+        if not secret:
+            raise RuntimeError(
+                "codeimport.macaroon_secret_key not configured.")
+        return secret
+
+    def issueMacaroon(self, context):
+        """See `IMacaroonIssuer`."""
+        assert context.code_import.git_repository is not None
+        macaroon = Macaroon(
+            location=config.vhost.mainsite.hostname,
+            identifier="code-import-job", key=self._root_secret)
+        macaroon.add_first_party_caveat("code-import-job %s" % context.id)
+        return macaroon
+
+    def checkMacaroonIssuer(self, macaroon):
+        """See `IMacaroonIssuer`."""
+        if macaroon.location != config.vhost.mainsite.hostname:
+            return False
+        try:
+            verifier = Verifier()
+            verifier.satisfy_general(
+                lambda caveat: caveat.startswith("code-import-job "))
+            return verifier.verify(macaroon, self._root_secret)
+        except Exception:
+            return False
+
+    def verifyMacaroon(self, macaroon, context):
+        """See `IMacaroonIssuer`."""
+        if not self.checkMacaroonIssuer(macaroon):
+            return False
+        try:
+            verifier = Verifier()
+            verifier.satisfy_exact("code-import-job %s" % context.id)
+            return (
+                verifier.verify(macaroon, self._root_secret) and
+                context.state == CodeImportJobState.RUNNING)
+        except Exception:
+            return False

=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py	2015-10-19 10:56:16 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py	2016-10-12 12:42:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for CodeImportJob and CodeImportJobWorkflow."""
@@ -13,7 +13,12 @@
 import StringIO
 import unittest
 
+from pymacaroons import Macaroon
 from pytz import UTC
+from testtools.matchers import (
+    MatchesListwise,
+    MatchesStructure,
+    )
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -23,6 +28,7 @@
     CodeImportJobState,
     CodeImportResultStatus,
     CodeImportReviewStatus,
+    TargetRevisionControlSystems,
     )
 from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.code.interfaces.codeimportevent import ICodeImportEventSet
@@ -41,6 +47,7 @@
 from lp.services.database.constants import UTC_NOW
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.librarian.interfaces.client import ILibrarianClient
+from lp.services.macaroons.interfaces import IMacaroonIssuer
 from lp.services.webapp import canonical_url
 from lp.testing import (
     ANONYMOUS,
@@ -1144,5 +1151,97 @@
             get_feedback_messages(user_browser.contents))
 
 
+class TestCodeImportJobMacaroonIssuer(TestCaseWithFactory):
+    """Test CodeImportJob macaroon issuing and verification."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestCodeImportJobMacaroonIssuer, self).setUp()
+        login_for_code_imports()
+        self.pushConfig("codeimport", macaroon_secret_key="some-secret")
+
+    def makeJob(self, target_rcs_type=TargetRevisionControlSystems.GIT):
+        code_import = self.factory.makeCodeImport(
+            target_rcs_type=target_rcs_type)
+        return self.factory.makeCodeImportJob(code_import=code_import)
+
+    def test_issueMacaroon_refuses_branch(self):
+        job = self.makeJob(target_rcs_type=TargetRevisionControlSystems.BZR)
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        self.assertRaises(
+            AssertionError, removeSecurityProxy(issuer).issueMacaroon, job)
+
+    def test_issueMacaroon_good(self):
+        job = self.makeJob()
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+        self.assertEqual("launchpad.dev", macaroon.location)
+        self.assertEqual("code-import-job", macaroon.identifier)
+        self.assertThat(macaroon.caveats, MatchesListwise([
+            MatchesStructure.byEquality(
+                caveat_id="code-import-job %s" % job.id),
+            ]))
+
+    def test_checkMacaroonIssuer_good(self):
+        job = self.makeJob()
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+        self.assertTrue(issuer.checkMacaroonIssuer(macaroon))
+
+    def test_checkMacaroonIssuer_wrong_location(self):
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroon = Macaroon(
+            location="another-location",
+            key=removeSecurityProxy(issuer)._root_secret)
+        self.assertFalse(issuer.checkMacaroonIssuer(macaroon))
+
+    def test_checkMacaroonIssuer_wrong_key(self):
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroon = Macaroon(
+            location=config.vhost.mainsite.hostname, key="another-secret")
+        self.assertFalse(issuer.checkMacaroonIssuer(macaroon))
+
+    def test_verifyMacaroon_good(self):
+        machine = self.factory.makeCodeImportMachine(set_online=True)
+        job = self.makeJob()
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        getUtility(ICodeImportJobWorkflow).startJob(job, machine)
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+        self.assertTrue(issuer.verifyMacaroon(macaroon, job))
+
+    def test_verifyMacaroon_wrong_location(self):
+        machine = self.factory.makeCodeImportMachine(set_online=True)
+        job = self.makeJob()
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        getUtility(ICodeImportJobWorkflow).startJob(job, machine)
+        macaroon = Macaroon(
+            location="another-location",
+            key=removeSecurityProxy(issuer)._root_secret)
+        self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+
+    def test_verifyMacaroon_wrong_key(self):
+        machine = self.factory.makeCodeImportMachine(set_online=True)
+        job = self.makeJob()
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        getUtility(ICodeImportJobWorkflow).startJob(job, machine)
+        macaroon = Macaroon(
+            location=config.vhost.mainsite.hostname, key="another-secret")
+        self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+
+    def test_verifyMacaroon_not_running(self):
+        job = self.makeJob()
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+        self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+
+    def test_verifyMacaroon_wrong_job(self):
+        job = self.makeJob()
+        other_job = self.factory.makeCodeImportJob(code_import=job.code_import)
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(other_job)
+        self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2016-10-06 20:37:39 +0000
+++ lib/lp/code/xmlrpc/git.py	2016-10-12 12:42:25 +0000
@@ -10,9 +10,13 @@
 
 import sys
 
+from pymacaroons import Macaroon
 from storm.store import Store
 import transaction
-from zope.component import getUtility
+from zope.component import (
+    ComponentLookupError,
+    getUtility,
+    )
 from zope.error.interfaces import IErrorReportingUtility
 from zope.interface import implementer
 from zope.security.interfaces import Unauthorized
@@ -30,6 +34,7 @@
     InvalidNamespace,
     )
 from lp.code.interfaces.codehosting import LAUNCHPAD_ANONYMOUS
+from lp.code.interfaces.codeimport import ICodeImportSet
 from lp.code.interfaces.gitapi import IGitAPI
 from lp.code.interfaces.githosting import IGitHostingClient
 from lp.code.interfaces.gitjob import IGitRefScanJobSource
@@ -53,6 +58,7 @@
     NoSuchProduct,
     )
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
+from lp.services.macaroons.interfaces import IMacaroonIssuer
 from lp.services.webapp import LaunchpadXMLRPCView
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.errorlog import ScriptRequest
@@ -68,22 +74,58 @@
         super(GitAPI, self).__init__(*args, **kwargs)
         self.repository_set = getUtility(IGitRepositorySet)
 
-    def _performLookup(self, path):
+    def _verifyMacaroon(self, macaroon_raw, repository=None):
+        try:
+            macaroon = Macaroon.deserialize(macaroon_raw)
+        except Exception:
+            return False
+        try:
+            issuer = getUtility(IMacaroonIssuer, macaroon.identifier)
+        except ComponentLookupError:
+            return False
+        if repository is not None:
+            if repository.repository_type != GitRepositoryType.IMPORTED:
+                return False
+            code_import = getUtility(ICodeImportSet).getByGitRepository(
+                repository)
+            if code_import is None:
+                return False
+            job = code_import.import_job
+            if job is None:
+                return False
+            return issuer.verifyMacaroon(macaroon, job)
+        else:
+            return issuer.checkMacaroonIssuer(macaroon)
+
+    def _performLookup(self, path, auth_params):
         repository, extra_path = getUtility(IGitLookup).getByPath(path)
         if repository is None:
             return None
-        try:
-            hosting_path = repository.getInternalPath()
-        except Unauthorized:
-            return None
-        writable = (
-            repository.repository_type == GitRepositoryType.HOSTED and
-            check_permission("launchpad.Edit", repository))
+        macaroon_raw = auth_params.get("macaroon")
+        naked_repository = removeSecurityProxy(repository)
+        if (macaroon_raw is not None and
+                self._verifyMacaroon(macaroon_raw, naked_repository)):
+            # The authentication parameters specifically grant access to
+            # this repository, so we can bypass other checks.
+            # For the time being, this only works for code imports.
+            assert repository.repository_type == GitRepositoryType.IMPORTED
+            hosting_path = naked_repository.getInternalPath()
+            writable = True
+            private = naked_repository.private
+        else:
+            try:
+                hosting_path = repository.getInternalPath()
+            except Unauthorized:
+                return None
+            writable = (
+                repository.repository_type == GitRepositoryType.HOSTED and
+                check_permission("launchpad.Edit", repository))
+            private = repository.private
         return {
             "path": hosting_path,
             "writable": writable,
             "trailing": extra_path,
-            "private": repository.private,
+            "private": private,
             }
 
     def _getGitNamespaceExtras(self, path, requester):
@@ -233,11 +275,11 @@
         if requester == LAUNCHPAD_ANONYMOUS:
             requester = None
         try:
-            result = self._performLookup(path)
+            result = self._performLookup(path, auth_params)
             if (result is None and requester is not None and
                 permission == "write"):
                 self._createRepository(requester, path)
-                result = self._performLookup(path)
+                result = self._performLookup(path, auth_params)
             if result is None:
                 raise faults.GitRepositoryNotFound(path)
             if permission != "read" and not result["writable"]:
@@ -286,5 +328,10 @@
 
     def authenticateWithPassword(self, username, password):
         """See `IGitAPI`."""
-        # Password authentication isn't supported yet.
-        return faults.Unauthorized()
+        # XXX cjwatson 2016-10-06: We only support free-floating macaroons
+        # at the moment, not ones bound to a user.
+        if not username and self._verifyMacaroon(password):
+            return {"macaroon": password}
+        else:
+            # Only macaroons are supported for password authentication.
+            return faults.Unauthorized()

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2016-10-06 11:17:05 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2016-10-12 12:42:25 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+from pymacaroons import Macaroon
 from testscenarios import (
     load_tests_apply_scenarios,
     WithScenarios,
@@ -13,8 +14,12 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
-from lp.code.enums import GitRepositoryType
+from lp.code.enums import (
+    GitRepositoryType,
+    TargetRevisionControlSystems,
+    )
 from lp.code.errors import GitRepositoryCreationFault
+from lp.code.interfaces.codeimportjob import ICodeImportJobWorkflow
 from lp.code.interfaces.gitcollection import IAllGitRepositories
 from lp.code.interfaces.gitjob import IGitRefScanJobSource
 from lp.code.interfaces.gitrepository import (
@@ -24,10 +29,13 @@
 from lp.code.tests.helpers import GitHostingFixture
 from lp.code.xmlrpc.git import GitAPI
 from lp.registry.enums import TeamMembershipPolicy
+from lp.services.config import config
+from lp.services.macaroons.interfaces import IMacaroonIssuer
 from lp.services.webapp.escaping import html_escape
 from lp.testing import (
     admin_logged_in,
     ANONYMOUS,
+    celebrity_logged_in,
     login,
     person_logged_in,
     TestCaseWithFactory,
@@ -74,13 +82,15 @@
 
     def assertPermissionDenied(self, requester, path,
                                message="Permission denied.",
-                               permission="read", can_authenticate=False):
+                               permission="read", can_authenticate=False,
+                               macaroon_raw=None):
         """Assert that looking at the given path returns PermissionDenied."""
         if requester is not None:
             requester = requester.id
-        fault = self._translatePath(
-            path, permission,
-            {"uid": requester, "can-authenticate": can_authenticate})
+        auth_params = {"uid": requester, "can-authenticate": can_authenticate}
+        if macaroon_raw is not None:
+            auth_params["macaroon"] = macaroon_raw
+        fault = self._translatePath(path, permission, auth_params)
         self.assertEqual(faults.PermissionDenied(message), fault)
 
     def assertUnauthorized(self, requester, path,
@@ -143,12 +153,13 @@
 
     def assertTranslates(self, requester, path, repository, writable,
                          permission="read", can_authenticate=False,
-                         trailing="", private=False):
+                         macaroon_raw=None, trailing="", private=False):
         if requester is not None:
             requester = requester.id
-        translation = self._translatePath(
-            path, permission,
-            {"uid": requester, "can-authenticate": can_authenticate})
+        auth_params = {"uid": requester, "can-authenticate": can_authenticate}
+        if macaroon_raw is not None:
+            auth_params["macaroon"] = macaroon_raw
+        translation = self._translatePath(path, permission, auth_params)
         login(ANONYMOUS)
         self.assertEqual(
             {"path": repository.getInternalPath(), "writable": writable,
@@ -634,6 +645,47 @@
             "GitRepositoryCreationFault: nothing here",
             self.oopses[0]["tb_text"])
 
+    def test_translatePath_code_import(self):
+        # A code import worker with a suitable macaroon can write to a
+        # repository associated with a running code import job.
+        self.pushConfig("codeimport", macaroon_secret_key="some-secret")
+        machine = self.factory.makeCodeImportMachine(set_online=True)
+        code_imports = [
+            self.factory.makeCodeImport(
+                target_rcs_type=TargetRevisionControlSystems.GIT)
+            for _ in range(2)]
+        with celebrity_logged_in("vcs_imports"):
+            jobs = [
+                self.factory.makeCodeImportJob(code_import=code_import)
+                for code_import in code_imports]
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroons = [
+            removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs]
+        path = u"/%s" % code_imports[0].git_repository.unique_name
+        self.assertPermissionDenied(
+            None, path, permission="write", macaroon_raw=macaroons[0])
+        with celebrity_logged_in("vcs_imports"):
+            getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
+        # This only works with new-style passing of authentication parameters.
+        if self.auth_params_dict:
+            self.assertTranslates(
+                None, path, code_imports[0].git_repository, True,
+                permission="write", macaroon_raw=macaroons[0].serialize())
+        else:
+            self.assertPermissionDenied(
+                None, path, permission="write",
+                macaroon_raw=macaroons[0].serialize())
+        self.assertPermissionDenied(
+            None, path, permission="write",
+            macaroon_raw=macaroons[1].serialize())
+        self.assertPermissionDenied(
+            None, path, permission="write",
+            macaroon_raw=Macaroon(
+                location=config.vhost.mainsite.hostname, identifier="another",
+                key="another-secret").serialize())
+        self.assertPermissionDenied(
+            None, path, permission="write", macaroon_raw="nonsense")
+
     def test_notify(self):
         # The notify call creates a GitRefScanJob.
         repository = self.factory.makeGitRepository()
@@ -666,6 +718,26 @@
             self.git_api.authenticateWithPassword('foo', 'bar'),
             faults.Unauthorized)
 
+    def test_authenticateWithPassword_code_import(self):
+        self.pushConfig("codeimport", macaroon_secret_key="some-secret")
+        code_import = self.factory.makeCodeImport(
+            target_rcs_type=TargetRevisionControlSystems.GIT)
+        with celebrity_logged_in("vcs_imports"):
+            job = self.factory.makeCodeImportJob(code_import=code_import)
+        issuer = getUtility(IMacaroonIssuer, "code-import-job")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
+        self.assertEqual(
+            {"macaroon": macaroon.serialize()},
+            self.git_api.authenticateWithPassword("", macaroon.serialize()))
+        other_macaroon = Macaroon(identifier="another", key="another-secret")
+        self.assertIsInstance(
+            self.git_api.authenticateWithPassword(
+                "", other_macaroon.serialize()),
+            faults.Unauthorized)
+        self.assertIsInstance(
+            self.git_api.authenticateWithPassword("", "nonsense"),
+            faults.Unauthorized)
+
 
 class TestGitAPISecurity(TestGitAPIMixin, TestCaseWithFactory):
     """Slow tests for `IGitAPI`.

=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf	2016-10-04 11:19:07 +0000
+++ lib/lp/services/config/schema-lazr.conf	2016-10-12 12:42:25 +0000
@@ -416,6 +416,9 @@
 # Import only this many revisions from svn (via bzr-svn) at once.
 svn_revisions_import_limit: 500
 
+# Secret key for macaroons used to grant git push permission to workers.
+macaroon_secret_key:
+
 [codeimportdispatcher]
 # The directory where the code import worker should be directed to
 # store its logs.

=== added directory 'lib/lp/services/macaroons'
=== added file 'lib/lp/services/macaroons/__init__.py'
=== added file 'lib/lp/services/macaroons/interfaces.py'
--- lib/lp/services/macaroons/interfaces.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/macaroons/interfaces.py	2016-10-12 12:42:25 +0000
@@ -0,0 +1,45 @@
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Interface to a policy for issuing and verifying macaroons."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'IMacaroonIssuer',
+    ]
+
+from zope.interface import Interface
+
+
+class IMacaroonIssuerPublic(Interface):
+    """Public interface to a policy for verifying macaroons."""
+
+    def checkMacaroonIssuer(macaroon):
+        """Check that `macaroon` was issued by this issuer.
+
+        This does not verify that the macaroon is valid for a given context,
+        only that it could be valid for some context.  Use this in the
+        authentication part of an authentication/authorisation API.
+        """
+
+    def verifyMacaroon(macaroon, context):
+        """Verify that `macaroon` is valid for `context`.
+
+        :param macaroon: A `Macaroon`.
+        :param context: The context to check.
+        :return: True if `macaroon` is valid for `context`, otherwise False.
+        """
+
+
+class IMacaroonIssuer(IMacaroonIssuerPublic):
+    """Interface to a policy for issuing and verifying macaroons."""
+
+    def issueMacaroon(context):
+        """Issue a macaroon for `context`.
+
+        :param context: The context that the returned macaroon should relate
+            to.
+        :return: A macaroon.
+        """