launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

I think the basic problem here was that we were using (what turns into) 403/404 as authz failures in the first place.  RFC 2616 suggests repeating 401 in the case where authz fails, and that seems to make sense, so I've just changed this to uniformly return an Unauthorized fault.

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

Ah.  In fact, I think this is a security vulnerability, not just a non-obvious structure.  If a user has write access to a repository, gets an access token, and then their access is revoked, they could still use that token until it expires simply by changing to a empty username.  I think it would also allow a user to write to any repository that they can view by requesting an access token for it and then using an empty username.  Not good.

How about if I were to make verifyMacaroon return either an object encapsulating bits of information about the macaroon or None?  That way the natural "if not issuer.verifyMacaroon" type of construction would still work, but call sites such as this one that need to do more checks could still do so; we could then explicitly check that an empty username can only be used with a macaroon that isn't bound to a particular user.

Another part of the confusion here is that we're overloading None.  We might switch to using a celebrity username instead rather than the empty username (perhaps "vcs-imports"), and treat that differently: it would make sense to continue to constrain that to imported repositories, we could require an appropriate issuer type, and it could be forbidden from doing anything unless it presents a valid macaroon.  Having specialised behaviour makes sense for code imports, but it would be easier to understand if that specialised behaviour were associated with a celebrity rather than with None.

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

Would the discussion above about using the vcs-imports celebrity help?  We could then require a user.  It's true that the parameters would still technically be subtractive, but it would be much less of a problem.

We could encode the uid and macaroon into a single "principal" parameter, but I'm not sure that that actually makes it any clearer.

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