launchpad-reviewers team mailing list archive

[Colin Watson <mp+420296@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:git-ci-builds-in-private-distros into launchpad:master.

Commit message:
Fix git authorization for CI builds in private distributions

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/420296

CI builds in private distributions fail with `fatal: repository 'https://git.launchpad.net/...' not found`.  This is because they authenticate using the special `+launchpad-services` user and a macaroon, and in that mode methods of `GitAPI` run with an anonymous principal and are expected to use `removeSecurityProxy` rather than doing normal security checks (see the comment near the top of `run_with_login`).

The lookup infrastructure in `lp.code.model.gitlookup` mostly doesn't do much in the way of permission checks, normally relying on the returned repository's security adapter to check access grants.  However, there are some exceptions: if the repository is in a source package or an OCI project inside a private pillar, then the pillar is security-proxied during traversal and so it implicitly performs permission checks.

Allow passing `check_permissions=False` to the lookup infrastructure to suppress these checks, and pass this when performing a lookup as `+launchpad-services`.  This is safe because it's only possible to use that user in conjunction with a macaroon issued for a specific repository.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:git-ci-builds-in-private-distros into launchpad:master.

diff --git a/lib/lp/code/interfaces/gitlookup.py b/lib/lp/code/interfaces/gitlookup.py
index 81e6355..35e297e 100644
--- a/lib/lp/code/interfaces/gitlookup.py
+++ b/lib/lp/code/interfaces/gitlookup.py
@@ -15,7 +15,7 @@ from zope.interface import Interface
 class IGitTraversable(Interface):
     """A thing that can be traversed to find a thing with a Git repository."""
 
-    def traverse(owner, name, segments):
+    def traverse(owner, name, segments, check_permissions=True):
         """Return the object beneath this one that matches 'name'.
 
         :param owner: The current `IPerson` context, or None.
@@ -32,10 +32,14 @@ class IGitTraversable(Interface):
 class IGitTraverser(Interface):
     """Utility for traversing to an object that can have a Git repository."""
 
-    def traverse(segments, owner=None):
+    def traverse(segments, owner=None, check_permissions=True):
         """Traverse to the object referred to by a prefix of the 'segments'
         iterable, starting from 'owner' if given.
 
+        :param check_permissions: If False, skip authorization checks on
+            intermediate segments of the path.  This should only be used in
+            unusual situations where we are specifically granting access to
+            a repository using a token rather than using a normal principal.
         :raises InvalidNamespace: If the path cannot be parsed as a
             repository namespace.
         :raises InvalidProductName: If the project component of the path is
@@ -56,11 +60,15 @@ class IGitTraverser(Interface):
             * a trailing path segment, or None.
         """
 
-    def traverse_path(path):
+    def traverse_path(path, check_permissions=True):
         """Traverse to the object referred to by 'path'.
 
         All segments of 'path' must be consumed.
 
+        :param check_permissions: If False, skip authorization checks on
+            intermediate segments of the path.  This should only be used in
+            unusual situations where we are specifically granting access to
+            a repository using a token rather than using a normal principal.
         :raises InvalidNamespace: If the path cannot be parsed as a
             repository namespace.
         :raises InvalidProductName: If the project component of the path is
@@ -124,7 +132,7 @@ class IGitLookup(Interface):
         lp: URL (which relies on client-side configuration).
         """
 
-    def getByPath(path):
+    def getByPath(path, check_permissions=True):
         """Find a repository by its path.
 
         Any of these forms may be used, with or without a leading slash:
@@ -139,6 +147,10 @@ class IGitLookup(Interface):
                 PROJECT
                 DISTRO/+source/SOURCE
 
+        :param check_permissions: If False, skip authorization checks on
+            intermediate segments of the path.  This should only be used in
+            unusual situations where we are specifically granting access to
+            a repository using a token rather than using a normal principal.
         :return: A tuple of (`IGitRepository`, extra_path), or (None, _).
             'extra_path' may be used by applications that need to traverse a
             leading part of a path as a repository, such as external code
diff --git a/lib/lp/code/model/gitlookup.py b/lib/lp/code/model/gitlookup.py
index e09427b..7a37937 100644
--- a/lib/lp/code/model/gitlookup.py
+++ b/lib/lp/code/model/gitlookup.py
@@ -19,6 +19,7 @@ from zope.component import (
     )
 from zope.interface import implementer
 from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NameLookupFailed
 from lp.app.validators.name import valid_name
@@ -291,7 +292,7 @@ class SegmentIterator:
 class GitTraverser:
     """Utility for traversing to objects that can have Git repositories."""
 
-    def traverse(self, segments, owner=None):
+    def traverse(self, segments, owner=None, check_permissions=True):
         """See `IGitTraverser`."""
         repository = None
         if owner is None:
@@ -318,15 +319,18 @@ class GitTraverser:
                     break
             if repository is not None:
                 break
+            if not check_permissions:
+                target = removeSecurityProxy(target)
             traversable = adapt(target, IGitTraversable)
         if target is None or not IHasGitRepositories.providedBy(target):
             raise InvalidNamespace("/".join(segments_iter.traversed))
         return owner, target, repository, trailing
 
-    def traverse_path(self, path):
+    def traverse_path(self, path, check_permissions=True):
         """See `IGitTraverser`."""
         segments = iter(path.split("/"))
-        owner, target, repository, trailing = self.traverse(segments)
+        owner, target, repository, trailing = self.traverse(
+            segments, check_permissions=check_permissions)
         if trailing or list(segments):
             raise InvalidNamespace(path)
         return owner, target, repository
@@ -396,12 +400,13 @@ class GitLookup:
             pass
         return None
 
-    def getByPath(self, path):
+    def getByPath(self, path, check_permissions=True):
         """See `IGitLookup`."""
         traverser = getUtility(IGitTraverser)
         segments = iter(path.split("/"))
         try:
-            owner, target, repository, trailing = traverser.traverse(segments)
+            owner, target, repository, trailing = traverser.traverse(
+                segments, check_permissions=check_permissions)
         except (InvalidNamespace, InvalidProductName, NameLookupFailed):
             return None, None
         if repository is None:
diff --git a/lib/lp/code/model/tests/test_gitlookup.py b/lib/lp/code/model/tests/test_gitlookup.py
index 266c7b3..658c33a 100644
--- a/lib/lp/code/model/tests/test_gitlookup.py
+++ b/lib/lp/code/model/tests/test_gitlookup.py
@@ -313,9 +313,12 @@ class TestGitTraverser(TestCaseWithFactory):
         super().setUp()
         self.traverser = getUtility(IGitTraverser)
 
-    def assertTraverses(self, path, owner, target, repository=None):
+    def assertTraverses(self, path, owner, target, repository=None,
+                        check_permissions=True):
         self.assertEqual(
-            (owner, target, repository), self.traverser.traverse_path(path))
+            (owner, target, repository),
+            self.traverser.traverse_path(
+                path, check_permissions=check_permissions))
 
     def test_nonexistent_project(self):
         # `traverse_path` raises `NoSuchProduct` when resolving a path of
@@ -401,7 +404,7 @@ class TestGitTraverser(TestCaseWithFactory):
         # `traverse_path` raises `NoSuchSourcePackageName` (denying
         # existence in order to avoid disclosing whether the object exists)
         # if it cannot traverse to a package due to the distribution being
-        # private.
+        # private.  `check_permissions=False` overrides this.
         with person_logged_in(self.factory.makePerson()) as owner:
             distro = self.factory.makeDistribution(
                 owner=owner, information_type=InformationType.PROPRIETARY)
@@ -411,6 +414,7 @@ class TestGitTraverser(TestCaseWithFactory):
                 dsp.distribution.name, dsp.sourcepackagename.name)
         self.assertRaises(
             NoSuchSourcePackageName, self.traverser.traverse_path, path)
+        self.assertTraverses(path, None, dsp, check_permissions=False)
 
     def test_missing_ociprojectname(self):
         # `traverse_path` raises `InvalidNamespace` if there are no segments
@@ -468,7 +472,7 @@ class TestGitTraverser(TestCaseWithFactory):
         # `traverse_path` raises `NoSuchOCIProjectName` (denying existence
         # in order to avoid disclosing whether the object exists) if it
         # cannot traverse to an OCI project due to the distribution being
-        # private.
+        # private.  `check_permissions=False` overrides this.
         with person_logged_in(self.factory.makePerson()) as owner:
             distro = self.factory.makeDistribution(
                 owner=owner, information_type=InformationType.PROPRIETARY)
@@ -476,6 +480,7 @@ class TestGitTraverser(TestCaseWithFactory):
             path = "%s/+oci/%s" % (oci_project.pillar.name, oci_project.name)
         self.assertRaises(
             NoSuchOCIProjectName, self.traverser.traverse_path, path)
+        self.assertTraverses(path, None, oci_project, check_permissions=False)
 
     def test_visible_ociproject_in_private_project(self):
         # `traverse_path` resolves 'project/+oci/ociproject' to the OCI
@@ -491,7 +496,7 @@ class TestGitTraverser(TestCaseWithFactory):
         # `traverse_path` raises `NoSuchOCIProjectName` (denying existence
         # in order to avoid disclosing whether the object exists) if it
         # cannot traverse to an OCI project due to the project being
-        # private.
+        # private.  `check_permissions=False` overrides this.
         with person_logged_in(self.factory.makePerson()) as owner:
             project = self.factory.makeProduct(
                 owner=owner, information_type=InformationType.PROPRIETARY)
@@ -499,6 +504,7 @@ class TestGitTraverser(TestCaseWithFactory):
             path = "%s/+oci/%s" % (oci_project.pillar.name, oci_project.name)
         self.assertRaises(
             NoSuchOCIProjectName, self.traverser.traverse_path, path)
+        self.assertTraverses(path, None, oci_project, check_permissions=False)
 
     def test_nonexistent_person(self):
         # `traverse_path` raises `NoSuchPerson` when resolving a path of
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 83383cd..23c67a2 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -241,7 +241,14 @@ class GitAPI(LaunchpadXMLRPCView):
 
         :return: A tuple with the repository object and a dict with
                  translation information."""
-        repository, extra_path = getUtility(IGitLookup).getByPath(path)
+        # Skip permission checks on intermediate URL segments if
+        # internal-services authentication was requested, since in that case
+        # they'll never succeed for elements of private pillars.  This is
+        # safe because internal services can only authenticate using
+        # macaroons issued for a specific repository.
+        check_permissions = requester != LAUNCHPAD_SERVICES
+        repository, extra_path = getUtility(IGitLookup).getByPath(
+            path, check_permissions=check_permissions)
         if repository is None:
             return None, None
 
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index c280f81..1f2b52a 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -1823,9 +1823,14 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         self.pushConfig(
             "launchpad", internal_macaroon_secret_key="some-secret")
         with person_logged_in(self.factory.makePerson()) as owner:
+            distro = self.factory.makeDistribution(
+                owner=owner, information_type=InformationType.PROPRIETARY)
+            dsp = self.factory.makeDistributionSourcePackage(
+                distribution=distro)
             repositories = [
                 self.factory.makeGitRepository(
-                    owner=owner, information_type=InformationType.USERDATA)
+                    registrant=owner, owner=owner,
+                    information_type=InformationType.PROPRIETARY, target=dsp)
                 for _ in range(2)]
             builds = [
                 self.factory.makeCIBuild(git_repository=repository)
@@ -2404,8 +2409,13 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         self.pushConfig(
             "launchpad", internal_macaroon_secret_key="some-secret")
         with person_logged_in(self.factory.makePerson()) as owner:
+            distro = self.factory.makeDistribution(
+                owner=owner, information_type=InformationType.PROPRIETARY)
+            dsp = self.factory.makeDistributionSourcePackage(
+                distribution=distro)
             repository = self.factory.makeGitRepository(
-                owner=owner, information_type=InformationType.USERDATA)
+                registrant=owner, owner=owner,
+                information_type=InformationType.PROPRIETARY, target=dsp)
             build = self.factory.makeCIBuild(git_repository=repository)
             issuer = getUtility(IMacaroonIssuer, "ci-build")
             macaroon = removeSecurityProxy(issuer).issueMacaroon(build)