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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
