launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> 
> === modified file 'lib/lp/services/librarianserver/db.py'
> --- lib/lp/services/librarianserver/db.py	2016-01-10 22:37:40 +0000
> +++ lib/lp/services/librarianserver/db.py	2019-03-12 19:17:31 +0000
> @@ -69,16 +108,24 @@
>              #
>              # This needs to match url_path_quote.
>              normalised_path = urllib.quote(urllib.unquote(path), safe='/~+')

Done.

> -            store = session_store()
> -            token_found = store.find(TimeLimitedToken,
> -                SQL("age(created) < interval '1 day'"),
> -                TimeLimitedToken.token == hashlib.sha256(token).hexdigest(),
> -                TimeLimitedToken.path == normalised_path).is_empty()
> -            store.reset()
> -            if token_found:
> +            if isinstance(token, Macaroon):
> +                # Macaroons have enough other constraints that they don't
> +                # need to be path-specific; it's simpler and faster to just
> +                # check the alias ID.
> +                token_ok = threads.blockingCallFromThread(
> +                    default_reactor, self._verifyMacaroon, token, aliasid)
> +            else:
> +                store = session_store()
> +                token_ok = not store.find(TimeLimitedToken,
> +                    SQL("age(created) < interval '1 day'"),
> +                    TimeLimitedToken.token ==
> +                        hashlib.sha256(token).hexdigest(),
> +                    TimeLimitedToken.path == normalised_path).is_empty()
> +                store.reset()
> +            if token_ok:
> +                restricted = True
> +            else:
>                  raise LookupError("Token stale/pruned/path mismatch")
> -            else:
> -                restricted = True
>          alias = LibraryFileAlias.selectOne(And(
>              LibraryFileAlias.id == aliasid,
>              LibraryFileAlias.contentID == LibraryFileContent.q.id,
> 
> === modified file 'lib/lp/services/librarianserver/web.py'
> --- lib/lp/services/librarianserver/web.py	2019-01-05 09:54:44 +0000
> +++ lib/lp/services/librarianserver/web.py	2019-03-12 19:17:31 +0000
> @@ -126,6 +127,12 @@
>                  return fourOhFour
>  
>          token = request.args.get('token', [None])[0]
> +        if token is None:
> +            if not request.getUser() and request.getPassword():
> +                try:
> +                    token = Macaroon.deserialize(request.getPassword())
> +                except Exception:

Unfortunately not with any degree of safety.  I've filed https://github.com/ecordell/pymacaroons/issues/50 to get pymacaroons to behave better here, and added XXX comments everywhere relevant linking to that.

> +                    pass
>          path = request.path
>          deferred = deferToThread(
>              self._getFileAlias, self.aliasID, token, path)
> 
> === 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)

Hm, it seems obvious enough to me that the object being named is described by the LHS of the caveat ID, and I don't know how to make that clearer.  There are multiple accessible objects so we clearly can't use those (at least not without a substantially more complicated construction).

> +        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.
> +        """

At the relatively minor cost of an extra query in the authserver, I've made this take an actual LFA rather than a bare LFA ID.  The authserver interface still has to take an ID because we can't marshal the LFA object over XML-RPC (and even if we came up with a way to do that it would probably be equivalent to sending the ID anyway), but this at least makes the issuer interface safer.

> +        # 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.