launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

A couple of bits are potentially confusing and would ideally be reworded, otherwise looks good.

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='/~+')

This could move into the TLT block.

> -            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:

Can we be any more specific here?

> +                    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)

With lp.code-import-job it was more obvious that it was effectively naming the principal, not the accessible object. That's less true for lp.binary-package-build, though BinaryPackageBuildJob no longer exists so it's not obvious what would be better.

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

BinaryPackageBuildMacaroonIssuer.verifyMacaroon's context being an LFA ID is extremely obscure and probably dangerous. Can we make it more obvious that the current implementation only does LFA IDs and that's what's being passed in?

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