launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> 
> === modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
> --- lib/lp/soyuz/model/binarypackagebuild.py	2017-03-29 09:28:09 +0000
> +++ lib/lp/soyuz/model/binarypackagebuild.py	2019-03-12 19:17:31 +0000
> @@ -1362,3 +1370,82 @@
>                      % (build.title, build.id, build.archive.displayname,
>                      build_queue.lastscore))
>          return new_builds
> +
> +
> +@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`.
> +
> +        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)

The ambiguity is that it sounds like it's accessing the named BPB, but actually it's accessing all resources that should be accessible to a job acting on behalf of the BPB. I don't know how to solve it, and it's not a blocker, but the inclarity is not ideal.

> +        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`.
> +
> +        For verification, the context is a `LibraryFileAlias` ID.  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.
> +        """

We can't send an LFA over XML-RPC, but we need to indicate that the value is an LFA ID by method name, parameter name, value prefix, or some equivalent mechanism.  Having the generic authserver verifyMacaroon method take an unmarked LFA ID is just asking for trouble when we start adding more object types, and it's going to be slightly annoying to fix later.

> +        # Circular import.
> +        from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
> +
> +        if not isinstance(context, int):
> +            return False
> +        if not self.checkMacaroonIssuer(macaroon):
> +            return False
> +
> +        def verify_build(caveat):
> +            prefix = "lp.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.libraryfileID == context,
> +                BinaryPackageBuild.status == BuildStatus.BUILDING).is_empty()
> +
> +        try:
> +            verifier = Verifier()
> +            verifier.satisfy_general(verify_build)
> +            return verifier.verify(macaroon, self._root_secret)
> +        except Exception:
> +            return False


-- 
https://code.launchpad.net/~cjwatson/launchpad/librarian-accept-macaroon/+merge/345079
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.