launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === 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-11 13:39:53 +0000
> @@ -0,0 +1,125 @@
> +# 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 lp.services.config import config
> +
> +
> +class MacaroonIssuerBase:
> +    """See `IMacaroonIssuer`."""
> +
> +    @property
> +    def identifier(self):
> +        """An identifying name for this issuer."""
> +        raise NotImplementedError
> +
> +    @property
> +    def primary_caveat_name(self):

This feels just as private as _root_secret, but lacks the leading _.

> +        """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 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 ValueError: if the context is unsuitable.
> +        :return: The context to pass to individual caveat checkers.
> +        """
> +        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 ValueError: 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 ValueError:
> +                return False

This is probably going to hide some unintended conversion error from deep in model code at some point.

> +
> +        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)
> +        except Exception:
> +            return False

Same here. It would be nice if this were a more explicit verification failure exception.

> 
> === 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-11 13:39:53 +0000
> @@ -584,3 +588,52 @@
>              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 ValueError("Cannot handle context %r." % context)
> +        if not removeSecurityProxy(context).is_private:
> +            raise ValueError("Refusing to issue macaroon for public build.")
> +        return removeSecurityProxy(context).id
> +
> +    def checkVerificationContext(self, context):
> +        """See `MacaroonIssuerBase`."""
> +        if not IGitRepository.providedBy(context):
> +            raise ValueError("Cannot handle context %r." % 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

This is so backwards, but already seems to be the case so huh.

> +
> +        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/soyuz/model/binarypackagebuild.py'
> --- lib/lp/soyuz/model/binarypackagebuild.py	2019-04-11 13:39:52 +0000
> +++ lib/lp/soyuz/model/binarypackagebuild.py	2019-04-11 13:39:53 +0000
> @@ -1373,44 +1370,27 @@
>  
>  
>  @implementer(IMacaroonIssuer)
> -class BinaryPackageBuildMacaroonIssuer:
> -
> -    @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`.
> +class BinaryPackageBuildMacaroonIssuer(MacaroonIssuerBase):
> +
> +    identifier = "binary-package-build"
> +
> +    def checkIssuingContext(self, context):
> +        """See `MacaroonIssuerBase`.
>  
>          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)
> -        macaroon.add_first_party_caveat(
> -            "lp.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.binary-package-build "))
> -            return verifier.verify(macaroon, self._root_secret)
> -        except Exception:
> -            return False
> -
> -    def verifyMacaroon(self, macaroon, context):
> -        """See `IMacaroonIssuer`.
> +        return removeSecurityProxy(context).id
> +
> +    def checkVerificationContext(self, context):
> +        """See `MacaroonIssuerBase`."""
> +        if not isinstance(context, int):
> +            raise ValueError("Cannot handle context %r." % context)
> +        return context

As mentioned in the previous MP, it's extremely obscure and completely unobvious that this is an LFA ID rather than, say, a BPB ID.

> +
> +    def verifyPrimaryCaveat(self, caveat_value, context):
> +        """See `MacaroonIssuerBase`.
>  
>          For verification, the context is a `LibraryFileAlias` ID.  We check
>          that the file is one of those required to build the


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-build-macaroon/+merge/364333
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.