Merge ~cjwatson/launchpad:git-ignore-auth-on-notify into launchpad:master

[Colin Watson]

Ignore authentication for git push notifications

Back when Ioana was implementing sending pack statistics for git push notifications (which are emitted by turnip at the end of each push), one of my review comments was that, since this was directly updating properties of the repository, the call ought to be authenticated. I remember being slightly ambivalent about that even at the time, but we went along with it since it seemed a relatively harmless piece of caution.

However, recently I've been experimenting with getting snapcraft to use personal access tokens, and one thing I noticed was that authenticated push notifications make it difficult to use appropriate expiry times. `snapcraft remote-build` creates a repository, generates an access token for it, and then pushes to it; the token relieves the user of the need to set up an SSH key before this system can work. The time between generating the token and starting the push is very short, and the token is only used once, so it seems as though a short expiry time (perhaps a minute) should be more than enough. However, because `notify` is called at the end of the push and also checks authorization, we have to choose an expiry time long enough to push an entire potentially-large repository over a connection of unknown speed.

We already have to trust turnip to handle part of the git authorization process, so an authorization check that effectively just ensures that turnip hasn't processed a push without appropriate authorization doesn't add any interesting security. Being able to update pack statistics on a repository would be a very low-value attack in any case. On the other hand, being able to use short-expiry tokens effectively feels like a much clearer security advantage.

I'm therefore reversing my previous view: it seems acceptably safe to skip authentication for `notify`, given the other safeguards around that and the benefits for use of personal access tokens.

[Guruprasad (lgp171188)]

LGTM 👍

review: Approve

[Otto Co-Pilot (otto-copilot)]

Merging failed
https://jenkins.ols.canonical.com/online-services/job/launchpad/3323/

diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 9793646..3b97a85 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -589,11 +589,11 @@ class GitAPI(LaunchpadXMLRPCView):
             del result
 
     @return_fault
-    def _notify(self, requester, translated_path, statistics, auth_params):
+    def _notify(self, translated_path, statistics, auth_params):
         logger = self._getLogger()
-        if requester == LAUNCHPAD_ANONYMOUS:
-            requester = None
-        repository = getUtility(IGitLookup).getByHostingPath(translated_path)
+        repository = removeSecurityProxy(
+            getUtility(IGitLookup).getByHostingPath(translated_path)
+        )
         if repository is None:
             fault = faults.NotFound(
                 "No repository found for '%s'." % translated_path
@@ -602,22 +602,30 @@ class GitAPI(LaunchpadXMLRPCView):
             return fault
         if repository is None:
             raise faults.GitRepositoryNotFound(translated_path)
-        if auth_params is not None:
-            verified = self._verifyAuthParams(
-                requester, repository, auth_params
+        if statistics:
+            removeSecurityProxy(repository).setRepackData(
+                statistics.get("loose_object_count"),
+                statistics.get("pack_count"),
             )
-            writable = self._isWritable(requester, repository, verified)
-            if writable and statistics:
-                removeSecurityProxy(repository).setRepackData(
-                    statistics.get("loose_object_count"),
-                    statistics.get("pack_count"),
-                )
         getUtility(IGitRefScanJobSource).create(
             removeSecurityProxy(repository)
         )
 
     def notify(self, translated_path, statistics, auth_params):
         """See `IGitAPI`."""
+        # This receives authentication parameters for historical reasons,
+        # but ignores them.  We already checked authorization at the start
+        # of the operation whose completion we're now being notified about,
+        # so we don't do so again here, as it can have weird effects: for
+        # example, it should be possible to have a short-duration personal
+        # access token that expires between the start and the end of a long
+        # push operation.  We have to trust turnip anyway, and the worst
+        # thing that any of this can do is spuriously update statistics.
+        #
+        # If we feel the need to authorize notify calls in future, then it
+        # should be done by checking whether a previous operation was
+        # authorized, e.g. by generating a single-use token earlier.  At the
+        # moment this seems like overkill, though.
         logger = self._getLogger()
         logger.info(
             "Request received: notify('%s', '%d', '%d')",
@@ -626,14 +634,7 @@ class GitAPI(LaunchpadXMLRPCView):
             statistics.get("pack_count"),
         )
 
-        requester_id = _get_requester_id(auth_params)
-        result = run_with_login(
-            requester_id,
-            self._notify,
-            translated_path,
-            statistics,
-            auth_params,
-        )
+        result = self._notify(translated_path, statistics, auth_params)
         try:
             if isinstance(result, xmlrpc.client.Fault):
                 logger.error("notify failed: %r", result)
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index 35f25da..1f69b8e 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -3158,210 +3158,25 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
         [job] = list(job_source.iterReady())
         self.assertEqual(repository, job.repository)
 
-    def assertSetsRepackData(self, repo, auth_params):
+    def test_notify_set_repack_data(self):
+        # The notify call sets the repack indicators (loose_object_count,
+        # pack_count, date_last_scanned) when received from turnip.
+        requester_owner = self.factory.makePerson()
+        repository = self.factory.makeGitRepository(owner=requester_owner)
         start_time = datetime.now(timezone.utc)
         self.assertIsNone(
             self.assertDoesNotFault(
                 None,
                 "notify",
-                repo.getInternalPath(),
+                repository.getInternalPath(),
                 {"loose_object_count": 5, "pack_count": 2},
-                auth_params,
+                {"uid": requester_owner.id},
             )
         )
         end_time = datetime.now(timezone.utc)
-        naked_repo = removeSecurityProxy(repo)
-        self.assertEqual(5, naked_repo.loose_object_count)
-        self.assertEqual(2, naked_repo.pack_count)
-        self.assertBetween(start_time, naked_repo.date_last_scanned, end_time)
-
-    def test_notify_set_repack_data(self):
-        # The notify call sets the repack
-        # indicators (loose_object_count, pack_count, date_last_scanned)
-        # when received from Turnip for a user
-        # that has their ordinary privileges on the corresponding repository
-        requester_owner = self.factory.makePerson()
-        repository = self.factory.makeGitRepository(owner=requester_owner)
-
-        self.assertSetsRepackData(repository, {"uid": requester_owner.id})
-
-    def test_notify_set_repack_data_user_macaroon(self):
-        self.pushConfig("codehosting", git_macaroon_secret_key="some-secret")
-        requester = self.factory.makePerson()
-        repository = self.factory.makeGitRepository(owner=requester)
-        issuer = getUtility(IMacaroonIssuer, "git-repository")
-        with person_logged_in(requester):
-            macaroon = removeSecurityProxy(issuer).issueMacaroon(
-                repository, user=requester
-            )
-        auth_params = _make_auth_params(
-            requester, macaroon_raw=macaroon.serialize()
-        )
-        self.assertSetsRepackData(repository, auth_params)
-
-    def test_notify_set_repack_data_user_mismatch(self):
-        # notify refuses macaroons in the case where the
-        # user doesn't match what the issuer claims was verified.  (We use a
-        # fake issuer for this, since this is a stopgap check to defend
-        # against issuer bugs)
-
-        issuer = FakeMacaroonIssuer()
-        # Claim to be the code-import-job issuer.  This is a bit weird, but
-        # it gets us past the difficulty that only certain named issuers are
-        # allowed to confer write permissions.
-        issuer.identifier = "code-import-job"
-        self.useFixture(
-            ZopeUtilityFixture(issuer, IMacaroonIssuer, name="code-import-job")
-        )
-        requesters = [self.factory.makePerson() for _ in range(2)]
-        owner = self.factory.makeTeam(members=requesters)
-        repository = self.factory.makeGitRepository(owner=owner)
-        macaroon = issuer.issueMacaroon(repository)
-
-        for verified_user, authorized, unauthorized in (
-            (
-                requesters[0],
-                [requesters[0]],
-                [LAUNCHPAD_SERVICES, requesters[1], None],
-            ),
-            ([None, NO_USER], [], [LAUNCHPAD_SERVICES] + requesters + [None]),
-        ):
-            issuer._verified_user = verified_user
-            for requester in authorized:
-                login(ANONYMOUS)
-                auth_params = _make_auth_params(
-                    requester, macaroon_raw=macaroon.serialize()
-                )
-                self.assertSetsRepackData(repository, auth_params)
-
-            for requester in unauthorized:
-                login(ANONYMOUS)
-                auth_params = _make_auth_params(
-                    requester, macaroon_raw=macaroon.serialize()
-                )
-                self.assertFault(
-                    faults.Unauthorized,
-                    None,
-                    "notify",
-                    repository.getInternalPath(),
-                    {"loose_object_count": 5, "pack_count": 2},
-                    auth_params,
-                )
-
-    def test_notify_set_repack_data_user_access_token(self):
-        requester = self.factory.makePerson()
-        repository = self.factory.makeGitRepository(owner=requester)
-        _, token = self.factory.makeAccessToken(
-            owner=requester,
-            target=repository,
-            scopes=[AccessTokenScope.REPOSITORY_PUSH],
-        )
-        auth_params = _make_auth_params(
-            requester, access_token_id=removeSecurityProxy(token).id
-        )
-        self.assertSetsRepackData(repository, auth_params)
-
-    def test_notify_set_repack_data_user_access_token_nonexistent(self):
-        requester = self.factory.makePerson()
-        repository = self.factory.makeGitRepository(owner=requester)
-        auth_params = _make_auth_params(requester, access_token_id=0)
-        self.assertFault(
-            faults.Unauthorized,
-            None,
-            "notify",
-            repository.getInternalPath(),
-            {"loose_object_count": 5, "pack_count": 2},
-            auth_params,
-        )
-
-    def test_notify_set_repack_data_code_import(self):
-        # We set the repack data on a repository from a code import worker
-        # with a suitable macaroon.
-        self.pushConfig(
-            "launchpad", internal_macaroon_secret_key="some-secret"
-        )
-        machine = self.factory.makeCodeImportMachine(set_online=True)
-        code_imports = [
-            self.factory.makeCodeImport(
-                target_rcs_type=TargetRevisionControlSystems.GIT
-            )
-            for _ in range(2)
-        ]
-        with celebrity_logged_in("vcs_imports"):
-            jobs = [
-                self.factory.makeCodeImportJob(code_import=code_import)
-                for code_import in code_imports
-            ]
-        issuer = getUtility(IMacaroonIssuer, "code-import-job")
-        macaroons = [
-            removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs
-        ]
-
-        with celebrity_logged_in("vcs_imports"):
-            getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
-
-        auth_params = _make_auth_params(
-            LAUNCHPAD_SERVICES, macaroon_raw=macaroons[0].serialize()
-        )
-        self.assertSetsRepackData(code_imports[0].git_repository, auth_params)
-
-        auth_params = _make_auth_params(
-            LAUNCHPAD_SERVICES, macaroon_raw=macaroons[1].serialize()
-        )
-        self.assertFault(
-            faults.Unauthorized,
-            None,
-            "notify",
-            code_imports[0].git_repository.getInternalPath(),
-            {"loose_object_count": 5, "pack_count": 2},
-            auth_params,
-        )
-
-    def test_notify_set_repack_data_private_code_import(self):
-        self.pushConfig(
-            "launchpad", internal_macaroon_secret_key="some-secret"
-        )
-        machine = self.factory.makeCodeImportMachine(set_online=True)
-        code_imports = [
-            self.factory.makeCodeImport(
-                target_rcs_type=TargetRevisionControlSystems.GIT
-            )
-            for _ in range(2)
-        ]
-        private_repository = code_imports[0].git_repository
-        path = private_repository.getInternalPath()
-        removeSecurityProxy(private_repository).transitionToInformationType(
-            InformationType.PRIVATESECURITY, private_repository.owner
-        )
-        with celebrity_logged_in("vcs_imports"):
-            jobs = [
-                self.factory.makeCodeImportJob(code_import=code_import)
-                for code_import in code_imports
-            ]
-        issuer = getUtility(IMacaroonIssuer, "code-import-job")
-        macaroons = [
-            removeSecurityProxy(issuer).issueMacaroon(job) for job in jobs
-        ]
-
-        with celebrity_logged_in("vcs_imports"):
-            getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
-
-        auth_params = _make_auth_params(
-            LAUNCHPAD_SERVICES, macaroon_raw=macaroons[0].serialize()
-        )
-        self.assertSetsRepackData(code_imports[0].git_repository, auth_params)
-
-        auth_params = _make_auth_params(
-            LAUNCHPAD_SERVICES, macaroon_raw=macaroons[1].serialize()
-        )
-        self.assertFault(
-            faults.Unauthorized,
-            None,
-            "notify",
-            path,
-            {"loose_object_count": 5, "pack_count": 2},
-            auth_params,
-        )
+        self.assertEqual(5, repository.loose_object_count)
+        self.assertEqual(2, repository.pack_count)
+        self.assertBetween(start_time, repository.date_last_scanned, end_time)
 
     def test_authenticateWithPassword(self):
         self.assertFault(