launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/refactor-git-code-import-authz-2 into lp:launchpad.

Commit message:
Improve code import authorisation a bit more.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/refactor-git-code-import-authz-2/+merge/367202

This adds another safety catch to forbid real users from using internal macaroons (this doesn't provide any real security protection in itself because the distinction between internal services and real users is made based on the username; it's just to avoid mistakes), and updates checkRefPermissions to the new code layout.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-git-code-import-authz-2 into lp:launchpad.

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2019-05-08 10:39:43 +0000
+++ lib/lp/code/xmlrpc/git.py	2019-05-09 15:59:18 +0000
@@ -95,6 +95,17 @@
         return LAUNCHPAD_ANONYMOUS
 
 
+def _is_issuer_internal(verified):
+    """Was the authorising macaroon issued by an internal-only issuer?
+
+    These macaroons are privileged in various ways, and are used by internal
+    services.
+
+    :param verified: An `IMacaroonVerificationResult`.
+    """
+    return verified.issuer_name == "code-import-job"
+
+
 @implementer(IGitAPI)
 class GitAPI(LaunchpadXMLRPCView):
     """See `IGitAPI`."""
@@ -137,24 +148,22 @@
                 # credentials.
                 raise faults.Unauthorized()
 
-            # Internal services use macaroons to authenticate.  In this
-            # case, we know that the authentication parameters specifically
-            # grant access to this repository because we were able to verify
-            # the macaroon using the repository as its context, so we can
-            # bypass other checks.  This is only permitted for selected
-            # macaroon issuers, currently only code import jobs.
-            # XXX cjwatson 2019-05-07: Remove None once
-            # authenticateWithPassword returns LAUNCHPAD_SERVICES for code
-            # import jobs on production.
-            if requester in (None, LAUNCHPAD_SERVICES):
-                repository_type = naked_repository.repository_type
-                if (verified.issuer_name == "code-import-job" and
-                        repository_type == GitRepositoryType.IMPORTED):
-                    hosting_path = naked_repository.getInternalPath()
-                    writable = True
-                    private = naked_repository.private
-                else:
-                    raise faults.Unauthorized()
+            # Internal macaroons may only be used by internal services, and
+            # user macaroons may only be used by real users.  Forbid
+            # potential confusion.
+            internal = _is_issuer_internal(verified)
+            if (requester == LAUNCHPAD_SERVICES) != internal:
+                raise faults.Unauthorized()
+
+            if internal:
+                # We know that the authentication parameters specifically
+                # grant access to this repository because we were able to
+                # verify the macaroon using the repository as its context,
+                # so we can bypass other checks.  This is only permitted for
+                # selected macaroon issuers used by internal services.
+                hosting_path = naked_repository.getInternalPath()
+                writable = True
+                private = naked_repository.private
 
             # In any other case, the macaroon constrains the permissions of
             # the principal, so fall through to doing normal user
@@ -405,14 +414,43 @@
         if repository is None:
             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
+        try:
+            macaroon_raw = auth_params.get("macaroon")
+            if macaroon_raw is not None:
+                verified = self._verifyMacaroon(macaroon_raw, repository)
+                if not verified:
+                    # Macaroon authentication failed.  Don't fall back to
+                    # the requester's permissions, since macaroons typically
+                    # have additional constraints.
+                    raise faults.Unauthorized()
+
+                # Internal macaroons may only be used by internal services,
+                # and user macaroons may only be used by real users.  Forbid
+                # potential confusion.
+                internal = _is_issuer_internal(verified)
+                if (requester == LAUNCHPAD_SERVICES) != internal:
+                    raise faults.Unauthorized()
+
+                if internal:
+                    # We know that the authentication parameters
+                    # specifically grant access to this repository because
+                    # we were able to verify the macaroon using the
+                    # repository as its context, so we can bypass other
+                    # checks and grant access as an anonymous repository
+                    # owner.  This is only permitted for selected macaroon
+                    # issuers used by internal services.
+                    requester = GitGranteeType.REPOSITORY_OWNER
+            elif requester == LAUNCHPAD_SERVICES:
+                # Internal services must authenticate using a macaroon.
+                raise faults.Unauthorized()
+        except faults.Unauthorized:
+            # XXX cjwatson 2019-05-09: It would be simpler to just raise
+            # this directly, but turnip won't handle it very gracefully at
+            # the moment.  It's possible to reach this by being very unlucky
+            # about the timing of a push.
+            return [
+                (xmlrpc_client.Binary(ref_path.data), [])
+                for ref_path in ref_paths]
 
         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-05-08 10:39:43 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2019-05-09 15:59:18 +0000
@@ -975,11 +975,6 @@
         self.assertTranslates(
             LAUNCHPAD_SERVICES, path, code_imports[0].git_repository, True,
             permission="write", macaroon_raw=macaroons[0].serialize())
-        # XXX cjwatson 2019-05-07: Remove this once we remove the
-        # corresponding compatibility code.
-        self.assertTranslates(
-            None, path, code_imports[0].git_repository, True,
-            permission="write", macaroon_raw=macaroons[0].serialize())
         self.assertUnauthorized(
             LAUNCHPAD_SERVICES, path, permission="write",
             macaroon_raw=macaroons[1].serialize())
@@ -991,6 +986,9 @@
         self.assertUnauthorized(
             LAUNCHPAD_SERVICES, path, permission="write",
             macaroon_raw="nonsense")
+        self.assertUnauthorized(
+            code_imports[0].registrant, path, permission="write",
+            macaroon_raw=macaroons[0].serialize())
 
     def test_translatePath_private_code_import(self):
         # A code import worker with a suitable macaroon can write to a
@@ -1022,12 +1020,6 @@
             LAUNCHPAD_SERVICES, path, code_imports[0].git_repository, True,
             permission="write", macaroon_raw=macaroons[0].serialize(),
             private=True)
-        # XXX cjwatson 2019-05-07: Remove this once we remove the
-        # corresponding compatibility code.
-        self.assertTranslates(
-            None, path, code_imports[0].git_repository, True,
-            permission="write", macaroon_raw=macaroons[0].serialize(),
-            private=True)
         self.assertUnauthorized(
             LAUNCHPAD_SERVICES, path, permission="write",
             macaroon_raw=macaroons[1].serialize())
@@ -1039,6 +1031,9 @@
         self.assertUnauthorized(
             LAUNCHPAD_SERVICES, path, permission="write",
             macaroon_raw="nonsense")
+        self.assertUnauthorized(
+            code_imports[0].registrant, path, permission="write",
+            macaroon_raw=macaroons[0].serialize())
 
     def test_notify(self):
         # The notify call creates a GitRefScanJob.
@@ -1114,25 +1109,28 @@
         repository = code_imports[0].git_repository
         ref_path = "refs/heads/master"
         self.assertHasRefPermissions(
-            None, repository, [ref_path], {ref_path: []},
+            LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []},
             macaroon_raw=macaroons[0].serialize())
         with celebrity_logged_in("vcs_imports"):
             getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
         self.assertHasRefPermissions(
-            None, repository, [ref_path],
+            LAUNCHPAD_SERVICES, repository, [ref_path],
             {ref_path: ["create", "push", "force_push"]},
             macaroon_raw=macaroons[0].serialize())
         self.assertHasRefPermissions(
-            None, repository, [ref_path], {ref_path: []},
+            LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []},
             macaroon_raw=macaroons[1].serialize())
         self.assertHasRefPermissions(
-            None, repository, [ref_path], {ref_path: []},
+            LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []},
             macaroon_raw=Macaroon(
                 location=config.vhost.mainsite.hostname, identifier="another",
                 key="another-secret").serialize())
         self.assertHasRefPermissions(
-            None, repository, [ref_path], {ref_path: []},
+            LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []},
             macaroon_raw="nonsense")
+        self.assertHasRefPermissions(
+            code_imports[0].registrant, repository, [ref_path], {ref_path: []},
+            macaroon_raw=macaroons[0].serialize())
 
     def test_checkRefPermissions_private_code_import(self):
         # A code import worker with a suitable macaroon has repository owner
@@ -1158,25 +1156,28 @@
         repository = code_imports[0].git_repository
         ref_path = "refs/heads/master"
         self.assertHasRefPermissions(
-            None, repository, [ref_path], {ref_path: []},
+            LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []},
             macaroon_raw=macaroons[0])
         with celebrity_logged_in("vcs_imports"):
             getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
         self.assertHasRefPermissions(
-            None, repository, [ref_path],
+            LAUNCHPAD_SERVICES, repository, [ref_path],
             {ref_path: ["create", "push", "force_push"]},
             macaroon_raw=macaroons[0].serialize())
         self.assertHasRefPermissions(
-            None, repository, [ref_path], {ref_path: []},
+            LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []},
             macaroon_raw=macaroons[1].serialize())
         self.assertHasRefPermissions(
-            None, repository, [ref_path], {ref_path: []},
+            LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []},
             macaroon_raw=Macaroon(
                 location=config.vhost.mainsite.hostname, identifier="another",
                 key="another-secret").serialize())
         self.assertHasRefPermissions(
-            None, repository, [ref_path], {ref_path: []},
+            LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []},
             macaroon_raw="nonsense")
+        self.assertHasRefPermissions(
+            code_imports[0].registrant, repository, [ref_path], {ref_path: []},
+            macaroon_raw=macaroons[0].serialize())
 
 
 class TestGitAPISecurity(TestGitAPIMixin, TestCaseWithFactory):