launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23505
[Merge] lp:~cjwatson/launchpad/git-honour-access-tokens into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-honour-access-tokens into lp:launchpad with lp:~cjwatson/launchpad/git-issue-access-tokens as a prerequisite.
Commit message:
Make the Git XML-RPC API honour user macaroons.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1824399 in Launchpad itself: "Add Git HTTPS push tokens for snapcraft experiment"
https://bugs.launchpad.net/launchpad/+bug/1824399
For more details, see:
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.
=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py 2019-04-15 11:42:00 +0000
+++ lib/lp/code/model/codeimportjob.py 2019-04-15 11:42:00 +0000
@@ -456,6 +456,10 @@
def verifyPrimaryCaveat(self, caveat_value, context, **kwargs):
"""See `MacaroonIssuerBase`."""
+ # Code import jobs only support free-floating macaroons for Git
+ # authentication, not ones bound to a user.
+ if kwargs.get("user"):
+ return False
if context is None:
# We're only verifying that the macaroon could be valid for some
# context.
=== 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
@@ -57,7 +57,10 @@
InvalidName,
NoSuchSourcePackageName,
)
-from lp.registry.interfaces.person import NoSuchPerson
+from lp.registry.interfaces.person import (
+ IPersonSet,
+ NoSuchPerson,
+ )
from lp.registry.interfaces.product import (
InvalidProductName,
NoSuchProduct,
@@ -79,7 +82,7 @@
super(GitAPI, self).__init__(*args, **kwargs)
self.repository_set = getUtility(IGitRepositorySet)
- def _verifyMacaroon(self, macaroon_raw, repository=None):
+ def _verifyMacaroon(self, macaroon_raw, repository=None, user=None):
try:
macaroon = Macaroon.deserialize(macaroon_raw)
except Exception:
@@ -89,7 +92,7 @@
except ComponentLookupError:
return False
return issuer.verifyMacaroon(
- macaroon, repository, require_context=False)
+ macaroon, repository, require_context=False, user=user)
def _performLookup(self, requester, path, auth_params):
repository, extra_path = getUtility(IGitLookup).getByPath(path)
@@ -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
+ # 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
+
+ if writable is None:
try:
hosting_path = repository.getInternalPath()
except Unauthorized:
@@ -123,6 +143,7 @@
if not grants.is_empty():
writable = True
private = repository.private
+
return {
"path": hosting_path,
"writable": writable,
@@ -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
+ return params
else:
# Only macaroons are supported for password authentication.
return faults.Unauthorized()
@@ -347,13 +371,19 @@
raise faults.GitRepositoryNotFound(translated_path)
macaroon_raw = auth_params.get("macaroon")
- if (macaroon_raw is not None and
- self._verifyMacaroon(macaroon_raw, repository)):
- # The authentication parameters grant access as an anonymous
- # repository owner.
- # For the time being, this only works for code imports.
- assert repository.repository_type == GitRepositoryType.IMPORTED
- requester = GitGranteeType.REPOSITORY_OWNER
+ if macaroon_raw is not None:
+ if not self._verifyMacaroon(
+ macaroon_raw, repository, user=requester):
+ # Macaroon authentication failed. Don't fall back to the
+ # requester's permissions, since macaroons typically have
+ # additional constraints.
+ return [
+ (xmlrpc_client.Binary(ref_path.data), [])
+ for ref_path in ref_paths]
+ # If we don't have a requester, then the authentication
+ # parameters grant access as an anonymous repository owner.
+ if requester is None:
+ requester = GitGranteeType.REPOSITORY_OWNER
if all(isinstance(ref_path, xmlrpc_client.Binary)
for ref_path in ref_paths):
=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py 2019-04-15 11:42:00 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py 2019-04-15 11:42:00 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for the internal Git API."""
@@ -90,13 +90,15 @@
def assertUnauthorized(self, requester, path,
message="Authorisation required.",
- permission="read", can_authenticate=False):
+ permission="read", can_authenticate=False,
+ macaroon_raw=None):
"""Assert that looking at the given path returns Unauthorized."""
if requester is not None:
requester = requester.id
- fault = self.git_api.translatePath(
- path, permission,
- {"uid": requester, "can-authenticate": can_authenticate})
+ auth_params = {"uid": requester, "can-authenticate": can_authenticate}
+ if macaroon_raw is not None:
+ auth_params["macaroon"] = macaroon_raw
+ fault = self.git_api.translatePath(path, permission, auth_params)
self.assertEqual(faults.Unauthorized(message), fault)
def assertNotFound(self, requester, path, message, permission="read",
@@ -1039,6 +1041,46 @@
self.assertGitRepositoryNotFound(
None, path, permission="write", macaroon_raw="nonsense")
+ def test_translatePath_user_macaroon(self):
+ # A user with a suitable macaroon can write to the corresponding
+ # repository, but not others, even if they own them.
+ self.pushConfig(
+ "launchpad", internal_macaroon_secret_key="some-secret")
+ requester = self.factory.makePerson()
+ repositories = [
+ self.factory.makeGitRepository(owner=requester) for _ in range(2)]
+ repositories.append(self.factory.makeGitRepository(
+ owner=requester, information_type=InformationType.PRIVATESECURITY))
+ issuer = getUtility(IMacaroonIssuer, "git-repository")
+ with person_logged_in(requester):
+ macaroons = [
+ removeSecurityProxy(issuer).issueMacaroon(repository)
+ for repository in repositories]
+ paths = [
+ u"/%s" % repository.unique_name for repository in repositories]
+ for i, repository in enumerate(repositories):
+ for j, macaroon in enumerate(macaroons):
+ login(ANONYMOUS)
+ if i == j:
+ self.assertTranslates(
+ requester, paths[i], repository, True,
+ permission="write", macaroon_raw=macaroon.serialize(),
+ private=(i == 2))
+ else:
+ self.assertPermissionDenied(
+ requester, paths[i], permission="write",
+ macaroon_raw=macaroon.serialize())
+ login(ANONYMOUS)
+ self.assertPermissionDenied(
+ requester, paths[i], permission="write",
+ macaroon_raw=Macaroon(
+ location=config.vhost.mainsite.hostname,
+ identifier="another", key="another-secret").serialize())
+ login(ANONYMOUS)
+ self.assertPermissionDenied(
+ requester, paths[i], permission="write",
+ macaroon_raw="nonsense")
+
def test_notify(self):
# The notify call creates a GitRefScanJob.
repository = self.factory.makeGitRepository()
@@ -1092,6 +1134,37 @@
self.git_api.authenticateWithPassword("", "nonsense"),
faults.Unauthorized)
+ def test_authenticateWithPassword_user_macaroon(self):
+ # A user with a suitable macaroon can authenticate using it, in
+ # which case we return both the macaroon and the uid for use by
+ # later calls.
+ self.pushConfig(
+ "launchpad", internal_macaroon_secret_key="some-secret")
+ requester = self.factory.makePerson()
+ issuer = getUtility(IMacaroonIssuer, "git-repository")
+ with person_logged_in(requester):
+ macaroon = removeSecurityProxy(issuer).issueMacaroon(
+ self.factory.makeGitRepository(owner=requester))
+ self.assertEqual(
+ {"macaroon": macaroon.serialize(), "uid": requester.id},
+ self.git_api.authenticateWithPassword(
+ requester.name, macaroon.serialize()))
+ self.assertIsInstance(
+ self.git_api.authenticateWithPassword("", macaroon.serialize()),
+ faults.Unauthorized)
+ self.assertIsInstance(
+ self.git_api.authenticateWithPassword(
+ "nonexistent", macaroon.serialize()),
+ faults.Unauthorized)
+ other_macaroon = Macaroon(identifier="another", key="another-secret")
+ self.assertIsInstance(
+ self.git_api.authenticateWithPassword(
+ requester.name, other_macaroon.serialize()),
+ faults.Unauthorized)
+ self.assertIsInstance(
+ self.git_api.authenticateWithPassword(requester.name, "nonsense"),
+ faults.Unauthorized)
+
def test_checkRefPermissions_code_import(self):
# A code import worker with a suitable macaroon has repository owner
# privileges on a repository associated with a running code import
@@ -1177,6 +1250,53 @@
None, repository, [ref_path], {ref_path: []},
macaroon_raw="nonsense")
+ def test_checkRefPermissions_user_macaroon(self):
+ # A user with a suitable macaroon has their ordinary privileges on
+ # the corresponding repository, but not others, even if they own
+ # them.
+ self.pushConfig(
+ "launchpad", internal_macaroon_secret_key="some-secret")
+ requester = self.factory.makePerson()
+ repositories = [
+ self.factory.makeGitRepository(owner=requester) for _ in range(2)]
+ repositories.append(self.factory.makeGitRepository(
+ owner=requester, information_type=InformationType.PRIVATESECURITY))
+ ref_path = b"refs/heads/master"
+ issuer = getUtility(IMacaroonIssuer, "git-repository")
+ with person_logged_in(requester):
+ macaroons = [
+ removeSecurityProxy(issuer).issueMacaroon(repository)
+ for repository in repositories]
+ for i, repository in enumerate(repositories):
+ for j, macaroon in enumerate(macaroons):
+ login(ANONYMOUS)
+ if i == j:
+ expected_permissions = ["create", "push", "force_push"]
+ else:
+ expected_permissions = []
+ self.assertHasRefPermissions(
+ requester, repository, [ref_path],
+ {ref_path: expected_permissions},
+ macaroon_raw=macaroon.serialize())
+ login(ANONYMOUS)
+ self.assertHasRefPermissions(
+ None, repository, [ref_path], {ref_path: []},
+ macaroon_raw=macaroon.serialize())
+ login(ANONYMOUS)
+ self.assertHasRefPermissions(
+ self.factory.makePerson(), repository, [ref_path],
+ {ref_path: []}, macaroon_raw=macaroon.serialize())
+ login(ANONYMOUS)
+ self.assertHasRefPermissions(
+ requester, repository, [ref_path], {ref_path: []},
+ macaroon_raw=Macaroon(
+ location=config.vhost.mainsite.hostname,
+ identifier="another", key="another-secret").serialize())
+ login(ANONYMOUS)
+ self.assertHasRefPermissions(
+ requester, repository, [ref_path], {ref_path: []},
+ macaroon_raw="nonsense")
+
class TestGitAPISecurity(TestGitAPIMixin, TestCaseWithFactory):
"""Slow tests for `IGitAPI`.
Follow ups
