Merge lp:~cjwatson/launchpad/snap-build-macaroon into lp:launchpad

[Colin Watson]

Add a snap-build macaroon issuer.

I factored out some common code into a base class, since there are now three rather similar implementations.

Nothing uses this yet, but I'm planning for SnapBuildBehaviour to do so via an extension to the authserver.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py	2018-11-01 18:03:06 +0000
+++ lib/lp/code/model/codeimportjob.py	2019-04-24 16:19:42 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Database classes for the CodeImportJob table."""
@@ -12,10 +12,6 @@
 
 import datetime
 
-from pymacaroons import (
-    Macaroon,
-    Verifier,
-    )
 from sqlobject import (
     ForeignKey,
     IntCol,
@@ -58,7 +54,11 @@
     SQLBase,
     sqlvalues,
     )
-from lp.services.macaroons.interfaces import IMacaroonIssuer
+from lp.services.macaroons.interfaces import (
+    BadMacaroonContext,
+    IMacaroonIssuer,
+    )
+from lp.services.macaroons.model import MacaroonIssuerBase
 
 
 @implementer(ICodeImportJob)
@@ -409,7 +409,9 @@
 
 
 @implementer(IMacaroonIssuer)
-class CodeImportJobMacaroonIssuer:
+class CodeImportJobMacaroonIssuer(MacaroonIssuerBase):
+
+    identifier = "code-import-job"
 
     @property
     def _root_secret(self):
@@ -423,38 +425,24 @@
                 "launchpad.internal_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("lp.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("lp.code-import-job "))
-            return verifier.verify(macaroon, self._root_secret)
-        except Exception:
-            return False
-
-    def verifyMacaroon(self, macaroon, context):
-        """See `IMacaroonIssuer`."""
-        if not ICodeImportJob.providedBy(context):
-            return False
-        if not self.checkMacaroonIssuer(macaroon):
-            return False
-        try:
-            verifier = Verifier()
-            verifier.satisfy_exact("lp.code-import-job %s" % context.id)
-            return (
-                verifier.verify(macaroon, self._root_secret) and
-                context.state == CodeImportJobState.RUNNING)
-        except Exception:
-            return False
+    def checkIssuingContext(self, context):
+        """See `MacaroonIssuerBase`."""
+        if context.code_import.git_repository is None:
+            raise BadMacaroonContext(
+                context, "context.code_import.git_repository is None")
+        return context.id
+
+    def checkVerificationContext(self, context):
+        """See `MacaroonIssuerBase`."""
+        if (not ICodeImportJob.providedBy(context) or
+                context.state != CodeImportJobState.RUNNING):
+            raise BadMacaroonContext(context)
+        return context
+
+    def verifyPrimaryCaveat(self, caveat_value, context):
+        """See `MacaroonIssuerBase`."""
+        if context is None:
+            # We're only verifying that the macaroon could be valid for some
+            # context.
+            return True
+        return caveat_value == str(context.id)

=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py	2018-11-01 18:03:06 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py	2019-04-24 16:19:42 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 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."""
@@ -59,7 +59,10 @@
 from lp.services.database.sqlbase import get_transaction_timestamp
 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.macaroons.interfaces import (
+    BadMacaroonContext,
+    IMacaroonIssuer,
+    )
 from lp.services.webapp import canonical_url
 from lp.testing import (
     ANONYMOUS,
@@ -1285,7 +1288,7 @@
         job = self.makeJob(target_rcs_type=TargetRevisionControlSystems.BZR)
         issuer = getUtility(IMacaroonIssuer, "code-import-job")
         self.assertRaises(
-            AssertionError, removeSecurityProxy(issuer).issueMacaroon, job)
+            BadMacaroonContext, removeSecurityProxy(issuer).issueMacaroon, job)
 
     def test_issueMacaroon_good(self):
         job = self.makeJob()
@@ -1303,25 +1306,6 @@
         self.pushConfig("codeimport", macaroon_secret_key="some-secret")
         self.test_issueMacaroon_good()
 
-    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()
@@ -1330,6 +1314,23 @@
         macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
         self.assertTrue(issuer.verifyMacaroon(macaroon, job))
 
+    def test_verifyMacaroon_good_no_context(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, None, require_context=False))
+
+    def test_verifyMacaroon_no_context_but_require_context(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.assertFalse(issuer.verifyMacaroon(macaroon, None))
+
     def test_verifyMacaroon_wrong_location(self):
         machine = self.factory.makeCodeImportMachine(set_online=True)
         job = self.makeJob()
@@ -1339,6 +1340,8 @@
             location="another-location",
             key=removeSecurityProxy(issuer)._root_secret)
         self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+        self.assertFalse(
+            issuer.verifyMacaroon(macaroon, None, require_context=False))
 
     def test_verifyMacaroon_wrong_key(self):
         machine = self.factory.makeCodeImportMachine(set_online=True)
@@ -1348,6 +1351,8 @@
         macaroon = Macaroon(
             location=config.vhost.mainsite.hostname, key="another-secret")
         self.assertFalse(issuer.verifyMacaroon(macaroon, job))
+        self.assertFalse(
+            issuer.verifyMacaroon(macaroon, None, require_context=False))
 
     def test_verifyMacaroon_not_running(self):
         job = self.makeJob()

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2019-04-23 12:30:16 +0000
+++ lib/lp/code/xmlrpc/git.py	2019-04-24 16:19:42 +0000
@@ -101,9 +101,9 @@
             job = code_import.import_job
             if job is None:
                 return False
-            return issuer.verifyMacaroon(macaroon, job)
         else:
-            return issuer.checkMacaroonIssuer(macaroon)
+            job = None
+        return issuer.verifyMacaroon(macaroon, job, require_context=False)
 
     def _performLookup(self, requester, path, auth_params):
         repository, extra_path = getUtility(IGitLookup).getByPath(path)

=== modified file 'lib/lp/services/authserver/tests/test_authserver.py'
--- lib/lp/services/authserver/tests/test_authserver.py	2019-04-23 13:50:35 +0000
+++ lib/lp/services/authserver/tests/test_authserver.py	2019-04-24 16:19:42 +0000
@@ -5,10 +5,7 @@
 
 __metaclass__ = type
 
-from pymacaroons import (
-    Macaroon,
-    Verifier,
-    )
+from pymacaroons import Macaroon
 from storm.sqlobject import SQLObjectNotFound
 from testtools.matchers import Is
 from zope.component import getUtility
@@ -21,7 +18,11 @@
     ILibraryFileAlias,
     ILibraryFileAliasSet,
     )
-from lp.services.macaroons.interfaces import IMacaroonIssuer
+from lp.services.macaroons.interfaces import (
+    BadMacaroonContext,
+    IMacaroonIssuer,
+    )
+from lp.services.macaroons.model import MacaroonIssuerBase
 from lp.testing import (
     person_logged_in,
     TestCase,
@@ -78,42 +79,26 @@
 
 
 @implementer(IMacaroonIssuer)
-class DummyMacaroonIssuer:
+class DummyMacaroonIssuer(MacaroonIssuerBase):
 
+    identifier = 'test'
     _root_secret = 'test'
 
-    def issueMacaroon(self, context):
-        """See `IMacaroonIssuer`."""
-        macaroon = Macaroon(
-            location=config.vhost.mainsite.hostname, identifier='test',
-            key=self._root_secret)
-        macaroon.add_first_party_caveat('test %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('test '))
-            return verifier.verify(macaroon, self._root_secret)
-        except Exception:
-            return False
-
-    def verifyMacaroon(self, macaroon, context):
-        """See `IMacaroonIssuer`."""
-        if not ILibraryFileAlias.providedBy(context):
-            return False
-        if not self.checkMacaroonIssuer(macaroon):
-            return False
-        try:
-            verifier = Verifier()
-            verifier.satisfy_exact('test %s' % context.id)
-            return verifier.verify(macaroon, self._root_secret)
-        except Exception:
-            return False
+    def checkIssuingContext(self, context):
+        """See `MacaroonIssuerBase`."""
+        if not ILibraryFileAlias.providedBy(context):
+            raise BadMacaroonContext(context)
+        return context.id
+
+    def checkVerificationContext(self, context):
+        """See `IMacaroonIssuerBase`."""
+        if not ILibraryFileAlias.providedBy(context):
+            raise BadMacaroonContext(context)
+        return context
+
+    def verifyPrimaryCaveat(self, caveat_value, context):
+        """See `MacaroonIssuerBase`."""
+        return caveat_value == str(context.id)
 
 
 class VerifyMacaroonTests(TestCase):

=== modified file 'lib/lp/services/macaroons/interfaces.py'
--- lib/lp/services/macaroons/interfaces.py	2016-10-07 15:50:10 +0000
+++ lib/lp/services/macaroons/interfaces.py	2019-04-24 16:19:42 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2019 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."""
@@ -7,28 +7,36 @@
 
 __metaclass__ = type
 __all__ = [
+    'BadMacaroonContext',
     'IMacaroonIssuer',
     ]
 
 from zope.interface import Interface
 
 
+class BadMacaroonContext(Exception):
+    """The requested context is unsuitable."""
+
+    def __init__(self, context, message=None):
+        if message is None:
+            message = "Cannot handle context %r." % context
+        super(BadMacaroonContext, self).__init__(message)
+        self.context = context
+
+
 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):
+    def verifyMacaroon(macaroon, context, require_context=True):
         """Verify that `macaroon` is valid for `context`.
 
         :param macaroon: A `Macaroon`.
         :param context: The context to check.
+        :param require_context: If True (the default), fail verification if
+            the context is None.  If False and the context is None, only
+            verify that the macaroon could be valid for some context.  Use
+            this in the authentication part of an
+            authentication/authorisation API.
         :return: True if `macaroon` is valid for `context`, otherwise False.
         """
 
@@ -41,5 +49,6 @@
 
         :param context: The context that the returned macaroon should relate
             to.
+        :raises ValueError: if the context is unsuitable.
         :return: A macaroon.
         """

=== added file 'lib/lp/services/macaroons/model.py'
--- lib/lp/services/macaroons/model.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/macaroons/model.py	2019-04-24 16:19:42 +0000
@@ -0,0 +1,132 @@
+# Copyright 2016-2019 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Policies for issuing and verifying macaroons."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    "MacaroonIssuerBase",
+    ]
+
+from pymacaroons import (
+    Macaroon,
+    Verifier,
+    )
+from pymacaroons.exceptions import MacaroonVerificationFailedException
+
+from lp.services.config import config
+from lp.services.macaroons.interfaces import BadMacaroonContext
+
+
+class MacaroonIssuerBase:
+    """See `IMacaroonIssuer`."""
+
+    @property
+    def identifier(self):
+        """An identifying name for this issuer."""
+        raise NotImplementedError
+
+    @property
+    def _primary_caveat_name(self):
+        """The name of the primary context caveat issued by this issuer."""
+        return "lp.%s" % self.identifier
+
+    @property
+    def _root_secret(self):
+        secret = config.launchpad.internal_macaroon_secret_key
+        if not secret:
+            raise RuntimeError(
+                "launchpad.internal_macaroon_secret_key not configured.")
+        return secret
+
+    def checkIssuingContext(self, context):
+        """Check that the issuing context is suitable.
+
+        Concrete implementations may implement this method to check that the
+        context of a macaroon issuance is suitable.  The returned context is
+        used to create the primary caveat, and may be the same context that
+        was passed in or an adapted one.
+
+        :param context: The context to check.
+        :raises BadMacaroonContext: if the context is unsuitable.
+        :return: The context to use to create the primary caveat.
+        """
+        return context
+
+    def issueMacaroon(self, context):
+        """See `IMacaroonIssuer`.
+
+        Concrete implementations should normally wrap this with some
+        additional checks of and/or changes to the context.
+        """
+        context = self.checkIssuingContext(context)
+        macaroon = Macaroon(
+            location=config.vhost.mainsite.hostname,
+            identifier=self.identifier, key=self._root_secret)
+        macaroon.add_first_party_caveat(
+            "%s %s" % (self._primary_caveat_name, context))
+        return macaroon
+
+    def checkVerificationContext(self, context):
+        """Check that the verification context is suitable.
+
+        Concrete implementations may implement this method to check that the
+        context of a macaroon verification is suitable.  The returned
+        context is passed to individual caveat checkers, and may be the same
+        context that was passed in or an adapted one.
+
+        :param context: The context to check.
+        :raises BadMacaroonContext: if the context is unsuitable.
+        :return: The context to pass to individual caveat checkers.
+        """
+        return context
+
+    def verifyPrimaryCaveat(self, caveat_value, context):
+        """Verify the primary context caveat on one of this issuer's macaroons.
+
+        :param caveat_value: The text of the caveat, with this issuer's
+            prefix removed.
+        :param context: The context to check.
+        :return: True if this caveat is allowed, otherwise False.
+        """
+        raise NotImplementedError
+
+    def verifyMacaroon(self, macaroon, context, require_context=True):
+        """See `IMacaroonIssuer`."""
+        if macaroon.location != config.vhost.mainsite.hostname:
+            return False
+        if require_context and context is None:
+            return False
+        if context is not None:
+            try:
+                context = self.checkVerificationContext(context)
+            except BadMacaroonContext:
+                return False
+
+        def verify(caveat):
+            try:
+                caveat_name, caveat_value = caveat.split(" ", 1)
+            except ValueError:
+                return False
+            if caveat_name == self._primary_caveat_name:
+                checker = self.verifyPrimaryCaveat
+            else:
+                # XXX cjwatson 2019-04-09: For now we just fail closed if
+                # there are any other caveats, which is good enough for
+                # internal use.
+                return False
+            return checker(caveat_value, context)
+
+        try:
+            verifier = Verifier()
+            verifier.satisfy_general(verify)
+            return verifier.verify(macaroon, self._root_secret)
+        # XXX cjwatson 2019-04-24: This can currently raise a number of
+        # other exceptions in the presence of non-well-formed input data,
+        # but most of them are too broad to reasonably catch so we let them
+        # turn into OOPSes for now.  Revisit this once
+        # https://github.com/ecordell/pymacaroons/issues/51 is fixed.
+        except MacaroonVerificationFailedException:
+            return False

=== modified file 'lib/lp/snappy/configure.zcml'
--- lib/lp/snappy/configure.zcml	2019-02-11 13:23:34 +0000
+++ lib/lp/snappy/configure.zcml	2019-04-24 16:19:42 +0000
@@ -83,6 +83,14 @@
         <allow interface="lp.buildmaster.interfaces.buildfarmjob.ISpecificBuildFarmJobSource" />
     </securedutility>
 
+    <!-- SnapBuildMacaroonIssuer -->
+    <securedutility
+        class="lp.snappy.model.snapbuild.SnapBuildMacaroonIssuer"
+        provides="lp.services.macaroons.interfaces.IMacaroonIssuer"
+        name="snap-build">
+        <allow interface="lp.services.macaroons.interfaces.IMacaroonIssuerPublic"/>
+    </securedutility>
+
     <!-- SnapBuildBehaviour -->
     <adapter
         for="lp.snappy.interfaces.snapbuild.ISnapBuild"

=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2018-12-18 18:14:37 +0000
+++ lib/lp/snappy/model/snapbuild.py	2019-04-24 16:19:42 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -35,6 +35,7 @@
 from zope.component.interfaces import ObjectEvent
 from zope.event import notify
 from zope.interface import implementer
+from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import (
@@ -45,6 +46,7 @@
 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource
 from lp.buildmaster.model.buildfarmjob import SpecificBuildFarmJobSourceMixin
 from lp.buildmaster.model.packagebuild import PackageBuildMixin
+from lp.code.interfaces.gitrepository import IGitRepository
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.distroseries import DistroSeries
@@ -65,6 +67,11 @@
     LibraryFileAlias,
     LibraryFileContent,
     )
+from lp.services.macaroons.interfaces import (
+    BadMacaroonContext,
+    IMacaroonIssuer,
+    )
+from lp.services.macaroons.model import MacaroonIssuerBase
 from lp.services.propertycache import (
     cachedproperty,
     get_property_cache,
@@ -584,3 +591,53 @@
             SnapBuild, SnapBuild.build_farm_job_id.is_in(
                 bfj.id for bfj in build_farm_jobs))
         return DecoratedResultSet(rows, pre_iter_hook=self.preloadBuildsData)
+
+
+@implementer(IMacaroonIssuer)
+class SnapBuildMacaroonIssuer(MacaroonIssuerBase):
+
+    identifier = "snap-build"
+
+    def checkIssuingContext(self, context):
+        """See `MacaroonIssuerBase`.
+
+        For issuing, the context is an `ISnapBuild` or its ID.
+        """
+        if ISnapBuild.providedBy(context):
+            pass
+        elif isinstance(context, int):
+            context = getUtility(ISnapBuildSet).getByID(context)
+        else:
+            raise BadMacaroonContext(context)
+        if not removeSecurityProxy(context).is_private:
+            raise BadMacaroonContext(
+                context, "Refusing to issue macaroon for public build.")
+        return removeSecurityProxy(context).id
+
+    def checkVerificationContext(self, context):
+        """See `MacaroonIssuerBase`."""
+        if not IGitRepository.providedBy(context):
+            raise BadMacaroonContext(context)
+        return context
+
+    def verifyPrimaryCaveat(self, caveat_value, context):
+        """See `MacaroonIssuerBase`.
+
+        For verification, the context is an `IGitRepository`.  We check that
+        the repository is needed to build the `ISnapBuild` that is the
+        context of the macaroon, and that the context build is currently
+        building.
+        """
+        # Circular import.
+        from lp.snappy.model.snap import Snap
+
+        try:
+            build_id = int(caveat_value)
+        except ValueError:
+            return False
+        return not IStore(SnapBuild).find(
+            SnapBuild,
+            SnapBuild.id == build_id,
+            SnapBuild.snap_id == Snap.id,
+            Snap.git_repository == context,
+            SnapBuild.status == BuildStatus.BUILDING).is_empty()

=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py	2018-12-18 18:23:52 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py	2019-04-24 16:19:42 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test snap package build features."""
@@ -22,11 +22,13 @@
     Equals,
     Is,
     MatchesDict,
+    MatchesListwise,
     MatchesStructure,
     )
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import InformationType
 from lp.app.errors import NotFoundError
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.buildmaster.enums import BuildStatus
@@ -38,6 +40,10 @@
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
+from lp.services.macaroons.interfaces import (
+    BadMacaroonContext,
+    IMacaroonIssuer,
+    )
 from lp.services.propertycache import clear_property_cache
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.services.webapp.publisher import canonical_url
@@ -774,3 +780,117 @@
         browser = self.getNonRedirectingBrowser(user=self.person)
         for file_url in file_urls:
             self.assertCanOpenRedirectedUrl(browser, file_url)
+
+
+class TestSnapBuildMacaroonIssuer(TestCaseWithFactory):
+    """Test SnapBuild macaroon issuing and verification."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestSnapBuildMacaroonIssuer, self).setUp()
+        self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
+
+    def test_issueMacaroon_refuses_public_snap(self):
+        build = self.factory.makeSnapBuild()
+        issuer = getUtility(IMacaroonIssuer, "snap-build")
+        self.assertRaises(
+            BadMacaroonContext, removeSecurityProxy(issuer).issueMacaroon,
+            build)
+
+    def test_issueMacaroon_good(self):
+        build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(private=True))
+        issuer = getUtility(IMacaroonIssuer, "snap-build")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(build)
+        self.assertThat(macaroon, MatchesStructure(
+            location=Equals("launchpad.dev"),
+            identifier=Equals("snap-build"),
+            caveats=MatchesListwise([
+                MatchesStructure.byEquality(
+                    caveat_id="lp.snap-build %s" % build.id),
+                ])))
+
+    def test_verifyMacaroon_good(self):
+        [ref] = self.factory.makeGitRefs(
+            information_type=InformationType.USERDATA)
+        build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(git_ref=ref, private=True))
+        build.updateStatus(BuildStatus.BUILDING)
+        issuer = removeSecurityProxy(
+            getUtility(IMacaroonIssuer, "snap-build"))
+        macaroon = issuer.issueMacaroon(build)
+        self.assertTrue(issuer.verifyMacaroon(macaroon, ref.repository))
+
+    def test_verifyMacaroon_wrong_location(self):
+        [ref] = self.factory.makeGitRefs(
+            information_type=InformationType.USERDATA)
+        build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(git_ref=ref, private=True))
+        build.updateStatus(BuildStatus.BUILDING)
+        issuer = removeSecurityProxy(
+            getUtility(IMacaroonIssuer, "snap-build"))
+        macaroon = Macaroon(
+            location="another-location", key=issuer._root_secret)
+        self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+
+    def test_verifyMacaroon_wrong_key(self):
+        [ref] = self.factory.makeGitRefs(
+            information_type=InformationType.USERDATA)
+        build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(git_ref=ref, private=True))
+        build.updateStatus(BuildStatus.BUILDING)
+        issuer = removeSecurityProxy(
+            getUtility(IMacaroonIssuer, "snap-build"))
+        macaroon = Macaroon(
+            location=config.vhost.mainsite.hostname, key="another-secret")
+        self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+
+    def test_verifyMacaroon_refuses_branch(self):
+        branch = self.factory.makeAnyBranch(
+            information_type=InformationType.USERDATA)
+        build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(branch=branch, private=True))
+        build.updateStatus(BuildStatus.BUILDING)
+        issuer = removeSecurityProxy(
+            getUtility(IMacaroonIssuer, "snap-build"))
+        macaroon = issuer.issueMacaroon(build)
+        self.assertFalse(issuer.verifyMacaroon(macaroon, branch))
+
+    def test_verifyMacaroon_not_building(self):
+        [ref] = self.factory.makeGitRefs(
+            information_type=InformationType.USERDATA)
+        build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(git_ref=ref, private=True))
+        issuer = removeSecurityProxy(
+            getUtility(IMacaroonIssuer, "snap-build"))
+        macaroon = issuer.issueMacaroon(build)
+        self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+
+    def test_verifyMacaroon_wrong_build(self):
+        [ref] = self.factory.makeGitRefs(
+            information_type=InformationType.USERDATA)
+        build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(git_ref=ref, private=True))
+        build.updateStatus(BuildStatus.BUILDING)
+        other_build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(private=True))
+        other_build.updateStatus(BuildStatus.BUILDING)
+        issuer = removeSecurityProxy(
+            getUtility(IMacaroonIssuer, "snap-build"))
+        macaroon = issuer.issueMacaroon(other_build)
+        self.assertFalse(issuer.verifyMacaroon(macaroon, ref.repository))
+
+    def test_verifyMacaroon_wrong_repository(self):
+        [ref] = self.factory.makeGitRefs(
+            information_type=InformationType.USERDATA)
+        build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(git_ref=ref, private=True))
+        other_repository = self.factory.makeGitRepository()
+        build.updateStatus(BuildStatus.BUILDING)
+        issuer = removeSecurityProxy(
+            getUtility(IMacaroonIssuer, "snap-build"))
+        macaroon = issuer.issueMacaroon(build)
+        self.assertFalse(issuer.verifyMacaroon(macaroon, other_repository))

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2019-04-23 14:00:43 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2019-04-24 16:19:42 +0000
@@ -21,10 +21,6 @@
 
 import apt_pkg
 from debian.deb822 import PkgRelation
-from pymacaroons import (
-    Macaroon,
-    Verifier,
-    )
 import pytz
 from sqlobject import SQLObjectNotFound
 from storm.expr import (
@@ -86,7 +82,11 @@
     LibraryFileAlias,
     LibraryFileContent,
     )
-from lp.services.macaroons.interfaces import IMacaroonIssuer
+from lp.services.macaroons.interfaces import (
+    BadMacaroonContext,
+    IMacaroonIssuer,
+    )
+from lp.services.macaroons.model import MacaroonIssuerBase
 from lp.soyuz.adapters.buildarch import determine_architectures_to_build
 from lp.soyuz.enums import (
     ArchivePurpose,
@@ -1374,52 +1374,39 @@
 
 
 @implementer(IMacaroonIssuer)
-class BinaryPackageBuildMacaroonIssuer:
+class BinaryPackageBuildMacaroonIssuer(MacaroonIssuerBase):
+
+    identifier = "binary-package-build"
 
     @property
-    def _root_secret(self):
-        secret = config.launchpad.internal_macaroon_secret_key
-        if not secret:
-            raise RuntimeError(
-                "launchpad.internal_macaroon_secret_key not configured.")
-        return secret
-
-    def issueMacaroon(self, context):
-        """See `IMacaroonIssuer`.
-
-        For issuing, the context is an `IBinaryPackageBuild`.
-        """
-        if not removeSecurityProxy(context).archive.private:
-            raise ValueError("Refusing to issue macaroon for public build.")
-        macaroon = Macaroon(
-            location=config.vhost.mainsite.hostname,
-            identifier="binary-package-build", key=self._root_secret)
+    def _primary_caveat_name(self):
+        """See `MacaroonIssuerBase`."""
         # The "lp.principal" prefix indicates that this caveat constrains
         # the macaroon to access only resources that should be accessible
         # when acting on behalf of the named build, rather than to access
         # the named build directly.
-        macaroon.add_first_party_caveat(
-            "lp.principal.binary-package-build %s" %
-            removeSecurityProxy(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(
-                    "lp.principal.binary-package-build "))
-            return verifier.verify(macaroon, self._root_secret)
-        except Exception:
-            return False
-
-    def verifyMacaroon(self, macaroon, context):
-        """See `IMacaroonIssuer`.
-
-        For verification, the context is a `LibraryFileAlias`.  We check
+        return "lp.principal.binary-package-build"
+
+    def checkIssuingContext(self, context):
+        """See `MacaroonIssuerBase`.
+
+        For issuing, the context is an `IBinaryPackageBuild`.
+        """
+        if not removeSecurityProxy(context).archive.private:
+            raise BadMacaroonContext(
+                context, "Refusing to issue macaroon for public build.")
+        return removeSecurityProxy(context).id
+
+    def checkVerificationContext(self, context):
+        """See `MacaroonIssuerBase`."""
+        if not ILibraryFileAlias.providedBy(context):
+            raise BadMacaroonContext(context)
+        return context
+
+    def verifyPrimaryCaveat(self, caveat_value, context):
+        """See `MacaroonIssuerBase`.
+
+        For verification, the context is an `ILibraryFileAlias`.  We check
         that the file is one of those required to build the
         `IBinaryPackageBuild` that is the context of the macaroon, and that
         the context build is currently building.
@@ -1427,32 +1414,16 @@
         # Circular import.
         from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
 
-        if not ILibraryFileAlias.providedBy(context):
-            return False
-        if not self.checkMacaroonIssuer(macaroon):
-            return False
-
-        def verify_build(caveat):
-            prefix = "lp.principal.binary-package-build "
-            if not caveat.startswith(prefix):
-                return False
-            try:
-                build_id = int(caveat[len(prefix):])
-            except ValueError:
-                return False
-            return not IStore(BinaryPackageBuild).find(
-                BinaryPackageBuild,
-                BinaryPackageBuild.id == build_id,
-                BinaryPackageBuild.source_package_release_id ==
-                    SourcePackageRelease.id,
-                SourcePackageReleaseFile.sourcepackagereleaseID ==
-                    SourcePackageRelease.id,
-                SourcePackageReleaseFile.libraryfile == context,
-                BinaryPackageBuild.status == BuildStatus.BUILDING).is_empty()
-
         try:
-            verifier = Verifier()
-            verifier.satisfy_general(verify_build)
-            return verifier.verify(macaroon, self._root_secret)
-        except Exception:
+            build_id = int(caveat_value)
+        except ValueError:
             return False
+        return not IStore(BinaryPackageBuild).find(
+            BinaryPackageBuild,
+            BinaryPackageBuild.id == build_id,
+            BinaryPackageBuild.source_package_release_id ==
+                SourcePackageRelease.id,
+            SourcePackageReleaseFile.sourcepackagereleaseID ==
+                SourcePackageRelease.id,
+            SourcePackageReleaseFile.libraryfile == context,
+            BinaryPackageBuild.status == BuildStatus.BUILDING).is_empty()

=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
--- lib/lp/soyuz/tests/test_binarypackagebuild.py	2019-04-23 14:00:43 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2019-04-24 16:19:42 +0000
@@ -29,7 +29,10 @@
 from lp.registry.interfaces.sourcepackage import SourcePackageUrgency
 from lp.services.config import config
 from lp.services.log.logger import DevNullLogger
-from lp.services.macaroons.interfaces import IMacaroonIssuer
+from lp.services.macaroons.interfaces import (
+    BadMacaroonContext,
+    IMacaroonIssuer,
+    )
 from lp.services.webapp.interaction import ANONYMOUS
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.soyuz.enums import (
@@ -920,7 +923,8 @@
         build = self.factory.makeBinaryPackageBuild()
         issuer = getUtility(IMacaroonIssuer, "binary-package-build")
         self.assertRaises(
-            ValueError, removeSecurityProxy(issuer).issueMacaroon, build)
+            BadMacaroonContext, removeSecurityProxy(issuer).issueMacaroon,
+            build)
 
     def test_issueMacaroon_good(self):
         build = self.factory.makeBinaryPackageBuild(
@@ -934,26 +938,6 @@
                 caveat_id="lp.principal.binary-package-build %s" % build.id),
             ]))
 
-    def test_checkMacaroonIssuer_good(self):
-        build = self.factory.makeBinaryPackageBuild(
-            archive=self.factory.makeArchive(private=True))
-        issuer = getUtility(IMacaroonIssuer, "binary-package-build")
-        macaroon = removeSecurityProxy(issuer).issueMacaroon(build)
-        self.assertTrue(issuer.checkMacaroonIssuer(macaroon))
-
-    def test_checkMacaroonIssuer_wrong_location(self):
-        issuer = getUtility(IMacaroonIssuer, "binary-package-build")
-        macaroon = Macaroon(
-            location="another-location",
-            key=removeSecurityProxy(issuer)._root_secret)
-        self.assertFalse(issuer.checkMacaroonIssuer(macaroon))
-
-    def test_checkMacaroonIssuer_wrong_key(self):
-        issuer = getUtility(IMacaroonIssuer, "binary-package-build")
-        macaroon = Macaroon(
-            location=config.vhost.mainsite.hostname, key="another-secret")
-        self.assertFalse(issuer.checkMacaroonIssuer(macaroon))
-
     def test_verifyMacaroon_good(self):
         build = self.factory.makeBinaryPackageBuild(
             archive=self.factory.makeArchive(private=True))