launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> 
> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py	2019-04-15 11:42:00 +0000
> +++ lib/lp/code/xmlrpc/git.py	2019-04-15 11:42:00 +0000
> @@ -97,17 +100,34 @@
>              return None
>          macaroon_raw = auth_params.get("macaroon")
>          naked_repository = removeSecurityProxy(repository)
> -        if (macaroon_raw is not None and
> -                self._verifyMacaroon(macaroon_raw, naked_repository)):
> -            # The authentication parameters specifically grant access to
> -            # this repository, so we can bypass other checks.
> -            # For the time being, this only works for code imports.
> -            assert (
> -                naked_repository.repository_type == GitRepositoryType.IMPORTED)
> -            hosting_path = naked_repository.getInternalPath()
> -            writable = True
> -            private = naked_repository.private
> -        else:
> +        writable = None
> +
> +        if macaroon_raw is not None:
> +            if not self._verifyMacaroon(
> +                    macaroon_raw, naked_repository, user=requester):
> +                # Macaroon authentication failed.  Don't fall back to the
> +                # requester's permissions, since macaroons typically have
> +                # additional constraints.  Instead, return "permission
> +                # denied" for visible repositories, and deny the existence
> +                # of invisible ones.
> +                if check_permission("launchpad.View", repository):
> +                    raise faults.PermissionDenied()
> +                else:
> +                    return None

This seems to allow probing of visible repositories even with a macaroon constrained to a single repository. Is that desirable? It's also less than obvious that this doesn't allow probing by simply knowing a username, since that's only guaranteed by the _verifyMacaroon call in authenticateWithPassword.

> +            # We have a macaroon that grants a subset of the requester's
> +            # permissions.  If the requester is None (the code import case),
> +            # then we ordinarily map that to the anonymous principal, but
> +            # that doesn't have any useful write access; in this case we're
> +            # really starting from a principal with all permissions and
> +            # constraining that, so just bypass other checks and say what we
> +            # mean directly.  If the requester is not None, then fall
> +            # through and do ordinary permission checks for them.
> +            if requester is None:
> +                hosting_path = naked_repository.getInternalPath()
> +                writable = True
> +                private = naked_repository.private

This structure seems weird and confusing, since we're not directly verifying that the macaroon didn't specify a user principal. But I don't really have a recommendation to make it obvious what's going on.

Hm, unless we split the _verifyMacaroon calls up into one for requester is None, and one without. But not sure that's better.

> +
> +        if writable is None:
>              try:
>                  hosting_path = repository.getInternalPath()
>              except Unauthorized:
> @@ -315,12 +336,15 @@
>          getUtility(IGitRefScanJobSource).create(
>              removeSecurityProxy(repository))
>  
> +    @return_fault
>      def authenticateWithPassword(self, username, password):
>          """See `IGitAPI`."""
> -        # XXX cjwatson 2016-10-06: We only support free-floating macaroons
> -        # at the moment, not ones bound to a user.
> -        if not username and self._verifyMacaroon(password):
> -            return {"macaroon": password}
> +        user = getUtility(IPersonSet).getByName(username) if username else None
> +        if self._verifyMacaroon(password, user=user):
> +            params = {"macaroon": password}
> +            if user is not None:
> +                params["uid"] = user.id

This is novel in that turnip authentication parameters have not previously been subtractive. Are we sure we want to go that direction?

> +            return params
>          else:
>              # Only macaroons are supported for password authentication.
>              return faults.Unauthorized()


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-honour-access-tokens/+merge/366053
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-honour-access-tokens into lp:launchpad.