Merge lp:~cjwatson/launchpad/git-honour-access-tokens into lp:launchpad

[Colin Watson]

Make the Git XML-RPC API honour user macaroons.

[Colin Watson (cjwatson)]

This should all be much clearer now, so please take another look.

[William Grant (wgrant)]

review: Needs Fixing (code)

[William Grant (wgrant)]

In _verifyAuthParams's "User authentication with no macaroon. We do no additional checks here." case it's very unclear that some other kind of authentication has already taken place, and verifyAuthParams isn't in fact the thing that's verifying the auth. Otherwise I think this is all safe enough -- thanks.

review: Approve (code)

=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py	2019-05-01 15:47:37 +0000
+++ lib/lp/code/model/codeimportjob.py	2019-05-10 16:51:16 +0000
@@ -463,6 +463,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-05-09 15:44:59 +0000
+++ lib/lp/code/xmlrpc/git.py	2019-05-10 16:51:16 +0000
@@ -60,7 +60,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,
@@ -125,7 +128,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)
         # XXX cjwatson 2019-04-23: Restrict exceptions once
@@ -137,7 +140,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)
@@ -149,7 +152,10 @@
         writable = None
 
         if macaroon_raw is not None:
-            verified = self._verifyMacaroon(macaroon_raw, naked_repository)
+            verify_user = (
+                None if requester == LAUNCHPAD_SERVICES else requester)
+            verified = self._verifyMacaroon(
+                macaroon_raw, naked_repository, user=verify_user)
             if not verified:
                 # Macaroon authentication failed.  Don't fall back to the
                 # requester's permissions, since macaroons typically have
@@ -201,6 +207,7 @@
                 if not grants.is_empty():
                     writable = True
             private = repository.private
+
         return {
             "path": hosting_path,
             "writable": writable,
@@ -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
+            elif _is_issuer_internal(verified):
+                auth_params["user"] = LAUNCHPAD_SERVICES
+            return auth_params
         # Only macaroons are supported for password authentication.
         return faults.Unauthorized()
 
@@ -428,7 +436,10 @@
         try:
             macaroon_raw = auth_params.get("macaroon")
             if macaroon_raw is not None:
-                verified = self._verifyMacaroon(macaroon_raw, repository)
+                verify_user = (
+                    None if requester == LAUNCHPAD_SERVICES else requester)
+                verified = self._verifyMacaroon(
+                    macaroon_raw, repository, user=verify_user)
                 if not verified:
                     # Macaroon authentication failed.  Don't fall back to
                     # the requester's permissions, since macaroons typically
 
=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2019-05-09 15:44:59 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2019-05-10 16:51:16 +0000
@@ -1081,6 +1081,46 @@
             repository.registrant, path, permission="read",
             macaroon_raw=macaroons[0].serialize())
 
+    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("codehosting", git_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, user=requester)
+                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.assertUnauthorized(
+                        requester, paths[i], permission="write",
+                        macaroon_raw=macaroon.serialize())
+            login(ANONYMOUS)
+            self.assertUnauthorized(
+                requester, paths[i], permission="write",
+                macaroon_raw=Macaroon(
+                    location=config.vhost.mainsite.hostname,
+                    identifier="another", key="another-secret").serialize())
+            login(ANONYMOUS)
+            self.assertUnauthorized(
+                requester, paths[i], permission="write",
+                macaroon_raw="nonsense")
+
     def test_notify(self):
         # The notify call creates a GitRefScanJob.
         repository = self.factory.makeGitRepository()
@@ -1157,6 +1197,35 @@
             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("codehosting", git_macaroon_secret_key="some-secret")
+        requester = self.factory.makePerson()
+        issuer = getUtility(IMacaroonIssuer, "git-repository")
+        macaroon = removeSecurityProxy(issuer).issueMacaroon(
+            self.factory.makeGitRepository(owner=requester), user=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
@@ -1267,6 +1336,53 @@
             LAUNCHPAD_SERVICES, ref.repository, [path], {path: []},
             macaroon_raw=macaroon.serialize())
 
+    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("codehosting", git_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, user=requester)
+                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`.