launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/restore-git-snap-build-macaroon into lp:launchpad with lp:~cjwatson/launchpad/refactor-git-code-import-authz-2 as a prerequisite.

Commit message:
Make GitAPI accept snap-build macaroons again.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/restore-git-snap-build-macaroon/+merge/367204

I added support for snap-build macaroons recently, and then promptly forgot about it when refactoring code import authorisation, largely because I hadn't explicitly written any tests for it.

In the process of writing these tests, I noticed that I really should have forced snap-build macaroons to be read-only, so I've done that.  The mechanism for doing this is pretty ad-hoc, but we can always improve it later.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/restore-git-snap-build-macaroon into lp:launchpad.

=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py	2019-04-26 13:13:37 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py	2019-05-09 16:04:44 +0000
@@ -1336,6 +1336,8 @@
         getUtility(ICodeImportJobWorkflow).startJob(job, machine)
         macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
         self.assertMacaroonVerifies(
+            issuer, macaroon, None, require_context=False)
+        self.assertMacaroonVerifies(
             issuer, macaroon, job, require_context=False)
         self.assertMacaroonVerifies(
             issuer, macaroon, job.code_import.target, require_context=False)

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2019-05-09 16:04:44 +0000
+++ lib/lp/code/xmlrpc/git.py	2019-05-09 16:04:44 +0000
@@ -103,6 +103,17 @@
 
     :param verified: An `IMacaroonVerificationResult`.
     """
+    return verified.issuer_name in ("code-import-job", "snap-build")
+
+
+def _can_internal_issuer_write(verified):
+    """Does this internal-only issuer have write access?
+
+    Some macaroons used by internal services are intended for writing to the
+    repository; others only allow read access.
+
+    :param verified: An `IMacaroonVerificationResult`.
+    """
     return verified.issuer_name == "code-import-job"
 
 
@@ -162,7 +173,7 @@
                 # 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
+                writable = _can_internal_issuer_write(verified)
                 private = naked_repository.private
 
             # In any other case, the macaroon constrains the permissions of
@@ -387,7 +398,7 @@
             verified = self._verifyMacaroon(password)
             if verified:
                 auth_params = {"macaroon": password}
-                if verified.issuer_name == "code-import-job":
+                if _is_issuer_internal(verified):
                     auth_params["user"] = LAUNCHPAD_SERVICES
                 return auth_params
         # Only macaroons are supported for password authentication.
@@ -432,6 +443,9 @@
                     raise faults.Unauthorized()
 
                 if internal:
+                    if not _can_internal_issuer_write(verified):
+                        raise faults.Unauthorized()
+
                     # We know that the authentication parameters
                     # specifically grant access to this repository because
                     # we were able to verify the macaroon using the

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2019-05-09 16:04:44 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2019-05-09 16:04:44 +0000
@@ -20,6 +20,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import InformationType
+from lp.buildmaster.enums import BuildStatus
 from lp.code.enums import (
     GitGranteeType,
     GitRepositoryType,
@@ -38,8 +39,10 @@
 from lp.code.xmlrpc.git import GitAPI
 from lp.registry.enums import TeamMembershipPolicy
 from lp.services.config import config
+from lp.services.features.testing import FeatureFixture
 from lp.services.macaroons.interfaces import IMacaroonIssuer
 from lp.services.webapp.escaping import html_escape
+from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
 from lp.testing import (
     admin_logged_in,
     ANONYMOUS,
@@ -1035,6 +1038,49 @@
             code_imports[0].registrant, path, permission="write",
             macaroon_raw=macaroons[0].serialize())
 
+    def test_translatePath_private_snap_build(self):
+        # A builder with a suitable macaroon can read from a repository
+        # associated with a running private snap build.
+        self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
+        with person_logged_in(self.factory.makePerson()) as owner:
+            refs = [
+                self.factory.makeGitRefs(
+                    owner=owner, information_type=InformationType.USERDATA)[0]
+                for _ in range(2)]
+            builds = [
+                self.factory.makeSnapBuild(
+                    requester=owner, owner=owner, git_ref=ref, private=True)
+                for ref in refs]
+            issuer = getUtility(IMacaroonIssuer, "snap-build")
+            macaroons = [
+                removeSecurityProxy(issuer).issueMacaroon(build)
+                for build in builds]
+            repository = refs[0].repository
+            path = u"/%s" % repository.unique_name
+        self.assertUnauthorized(
+            LAUNCHPAD_SERVICES, path, permission="write",
+            macaroon_raw=macaroons[0].serialize())
+        removeSecurityProxy(builds[0]).updateStatus(BuildStatus.BUILDING)
+        self.assertTranslates(
+            LAUNCHPAD_SERVICES, path, repository, False, permission="read",
+            macaroon_raw=macaroons[0].serialize(), private=True)
+        self.assertUnauthorized(
+            LAUNCHPAD_SERVICES, path, permission="read",
+            macaroon_raw=macaroons[1].serialize())
+        self.assertUnauthorized(
+            LAUNCHPAD_SERVICES, path, permission="read",
+            macaroon_raw=Macaroon(
+                location=config.vhost.mainsite.hostname, identifier="another",
+                key="another-secret").serialize())
+        self.assertUnauthorized(
+            LAUNCHPAD_SERVICES, path, permission="read",
+            macaroon_raw="nonsense")
+        self.assertUnauthorized(
+            repository.registrant, path, permission="read",
+            macaroon_raw=macaroons[0].serialize())
+
     def test_notify(self):
         # The notify call creates a GitRefScanJob.
         repository = self.factory.makeGitRepository()
@@ -1088,6 +1134,29 @@
             self.git_api.authenticateWithPassword("", "nonsense"),
             faults.Unauthorized)
 
+    def test_authenticateWithPassword_private_snap_build(self):
+        self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
+        with person_logged_in(self.factory.makePerson()) as owner:
+            [ref] = self.factory.makeGitRefs(
+                owner=owner, information_type=InformationType.USERDATA)
+            build = self.factory.makeSnapBuild(
+                requester=owner, owner=owner, git_ref=ref, private=True)
+            issuer = getUtility(IMacaroonIssuer, "snap-build")
+            macaroon = removeSecurityProxy(issuer).issueMacaroon(build)
+        self.assertEqual(
+            {"macaroon": macaroon.serialize(), "user": "+launchpad-services"},
+            self.git_api.authenticateWithPassword("", macaroon.serialize()))
+        other_macaroon = Macaroon(identifier="another", key="another-secret")
+        self.assertIsInstance(
+            self.git_api.authenticateWithPassword(
+                "", other_macaroon.serialize()),
+            faults.Unauthorized)
+        self.assertIsInstance(
+            self.git_api.authenticateWithPassword("", "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
@@ -1157,7 +1226,7 @@
         ref_path = "refs/heads/master"
         self.assertHasRefPermissions(
             LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []},
-            macaroon_raw=macaroons[0])
+            macaroon_raw=macaroons[0].serialize())
         with celebrity_logged_in("vcs_imports"):
             getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine)
         self.assertHasRefPermissions(
@@ -1179,6 +1248,25 @@
             code_imports[0].registrant, repository, [ref_path], {ref_path: []},
             macaroon_raw=macaroons[0].serialize())
 
+    def test_checkRefPermissions_private_snap_build(self):
+        # A builder with a suitable macaroon cannot write to a repository,
+        # even if it is associated with a running private snap build.
+        self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
+        with person_logged_in(self.factory.makePerson()) as owner:
+            [ref] = self.factory.makeGitRefs(
+                owner=owner, information_type=InformationType.USERDATA)
+            build = self.factory.makeSnapBuild(
+                requester=owner, owner=owner, git_ref=ref, private=True)
+            issuer = getUtility(IMacaroonIssuer, "snap-build")
+            macaroon = removeSecurityProxy(issuer).issueMacaroon(build)
+            build.updateStatus(BuildStatus.BUILDING)
+            path = ref.path.encode("UTF-8")
+        self.assertHasRefPermissions(
+            LAUNCHPAD_SERVICES, ref.repository, [path], {path: []},
+            macaroon_raw=macaroon.serialize())
+
 
 class TestGitAPISecurity(TestGitAPIMixin, TestCaseWithFactory):
     """Slow tests for `IGitAPI`.

=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2019-05-01 15:47:37 +0000
+++ lib/lp/snappy/model/snapbuild.py	2019-05-09 16:04:44 +0000
@@ -628,6 +628,11 @@
         # Circular import.
         from lp.snappy.model.snap import Snap
 
+        if context is None:
+            # We're only verifying that the macaroon could be valid for some
+            # context.
+            return True
+
         try:
             build_id = int(caveat_value)
         except ValueError:

=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py	2019-04-25 21:57:30 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py	2019-05-09 16:04:44 +0000
@@ -843,6 +843,33 @@
         macaroon = issuer.issueMacaroon(build)
         self.assertMacaroonVerifies(issuer, macaroon, ref.repository)
 
+    def test_verifyMacaroon_good_no_context(self):
+        [ref] = self.factory.makeGitRefs(
+            information_type=InformationType.USERDATA)
+        build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(git_ref=ref, private=True))
+        build.updateStatus(BuildStatus.BUILDING)
+        issuer = removeSecurityProxy(
+            getUtility(IMacaroonIssuer, "snap-build"))
+        macaroon = issuer.issueMacaroon(build)
+        self.assertMacaroonVerifies(
+            issuer, macaroon, None, require_context=False)
+        self.assertMacaroonVerifies(
+            issuer, macaroon, ref.repository, require_context=False)
+
+    def test_verifyMacaroon_no_context_but_require_context(self):
+        [ref] = self.factory.makeGitRefs(
+            information_type=InformationType.USERDATA)
+        build = self.factory.makeSnapBuild(
+            snap=self.factory.makeSnap(git_ref=ref, private=True))
+        build.updateStatus(BuildStatus.BUILDING)
+        issuer = removeSecurityProxy(
+            getUtility(IMacaroonIssuer, "snap-build"))
+        macaroon = issuer.issueMacaroon(build)
+        self.assertMacaroonDoesNotVerify(
+            ["Expected macaroon verification context but got None."],
+            issuer, macaroon, None)
+
     def test_verifyMacaroon_wrong_location(self):
         [ref] = self.factory.makeGitRefs(
             information_type=InformationType.USERDATA)
@@ -856,6 +883,9 @@
         self.assertMacaroonDoesNotVerify(
             ["Macaroon has unknown location 'another-location'."],
             issuer, macaroon, ref.repository)
+        self.assertMacaroonDoesNotVerify(
+            ["Macaroon has unknown location 'another-location'."],
+            issuer, macaroon, ref.repository, require_context=False)
 
     def test_verifyMacaroon_wrong_key(self):
         [ref] = self.factory.makeGitRefs(
@@ -869,6 +899,9 @@
             location=config.vhost.mainsite.hostname, key="another-secret")
         self.assertMacaroonDoesNotVerify(
             ["Signatures do not match"], issuer, macaroon, ref.repository)
+        self.assertMacaroonDoesNotVerify(
+            ["Signatures do not match"],
+            issuer, macaroon, ref.repository, require_context=False)
 
     def test_verifyMacaroon_refuses_branch(self):
         branch = self.factory.makeAnyBranch(