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-05-09 15:44:59 +0000
> +++ lib/lp/code/xmlrpc/git.py	2019-05-10 16:51:16 +0000
> @@ -390,17 +397,18 @@
>          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:
> -            verified = self._verifyMacaroon(password)
> -            if verified:
> -                auth_params = {"macaroon": password}
> -                if _is_issuer_internal(verified):
> -                    auth_params["user"] = LAUNCHPAD_SERVICES
> -                return auth_params
> +        user = getUtility(IPersonSet).getByName(username) if username else None
> +        verified = self._verifyMacaroon(password, user=user)
> +        if verified:
> +            auth_params = {"macaroon": password}
> +            if user is not None:
> +                auth_params["uid"] = user.id

Fair point on checking the user.  I've added the user to MacaroonVerificationResult, explicitly distinguishing "issuer forgot to do any checks" from "issuer positively verified the lack of a user", and arranged for both GitAPI and the authserver to double-check this against what they expect.  Please re-review.

I don't really understand your comment about an extra auth parameter.  This sort of thing might be a problem if there were some way to inject extra auth parameters after authentication, but there isn't.  Can you explain what attack vector you see here?  (Also, on a practical note, the only other way to authenticate at the moment is via an SSH key, and we can't change the auth parameters for that until we get the git redeployment done so that we can deploy a newer version of turnip.)

> +            elif _is_issuer_internal(verified):
> +                auth_params["user"] = LAUNCHPAD_SERVICES
> +            return auth_params
>          # 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 subscribed to branch lp:launchpad.