launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-check-request-private-git into lp:launchpad with lp:~cjwatson/launchpad/snap-build-behaviour-macaroon as a prerequisite.

Commit message:
Ensure that owners match exactly if building a snap from a private Git repository.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1639975 in Launchpad itself: "support for building private snaps"
  https://bugs.launchpad.net/launchpad/+bug/1639975

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-check-request-private-git/+merge/365071
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-check-request-private-git into lp:launchpad.

=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2019-02-07 10:30:03 +0000
+++ lib/lp/snappy/interfaces/snap.py	2019-03-25 21:01:52 +0000
@@ -30,6 +30,7 @@
     'SnapBuildAlreadyPending',
     'SnapBuildArchiveOwnerMismatch',
     'SnapBuildDisallowedArchitecture',
+    'SnapBuildGitRepositoryOwnerMismatch',
     'SnapBuildRequestStatus',
     'SnapNotOwner',
     'SnapPrivacyMismatch',
@@ -149,6 +150,26 @@
             "if the snap package owner and the archive owner are equal.")
 
 
+@error_status(httplib.FORBIDDEN)
+class SnapBuildGitRepositoryOwnerMismatch(Forbidden):
+    """Builds against private Git repositories require that owners match.
+
+    The snap package owner must be able to see the repository, so that a
+    malicious snap package build can't steal any secrets that its owner
+    didn't already have access to.  Furthermore, we want to make sure that
+    future changes to the team owning the snap package don't grant it
+    retrospective access to information about a private repository.  For
+    now, the simplest way to do this is to require that the owners match
+    exactly.
+    """
+
+    def __init__(self):
+        super(SnapBuildGitRepositoryOwnerMismatch, self).__init__(
+            "Snap package builds against private Git repositories are only "
+            "allowed if the snap package owner and the repository owner are "
+            "equal.")
+
+
 @error_status(httplib.BAD_REQUEST)
 class SnapBuildDisallowedArchitecture(Exception):
     """A build was requested for a disallowed architecture."""

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2019-02-26 06:47:45 +0000
+++ lib/lp/snappy/model/snap.py	2019-03-25 21:01:52 +0000
@@ -151,6 +151,7 @@
     SnapBuildAlreadyPending,
     SnapBuildArchiveOwnerMismatch,
     SnapBuildDisallowedArchitecture,
+    SnapBuildGitRepositoryOwnerMismatch,
     SnapBuildRequestStatus,
     SnapNotOwner,
     SnapPrivacyMismatch,
@@ -584,6 +585,11 @@
         if archive.private and self.owner != archive.owner:
             # See rationale in `SnapBuildArchiveOwnerMismatch` docstring.
             raise SnapBuildArchiveOwnerMismatch()
+        if (self.git_repository is not None and self.git_repository.private and
+                self.owner != self.git_repository.owner):
+            # See rationale in `SnapBuildGitRepositoryOwnerMismatch`
+            # docstring.
+            raise SnapBuildGitRepositoryOwnerMismatch()
 
     def requestBuild(self, requester, archive, distro_arch_series, pocket,
                      channels=None, build_request=None):

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2019-02-26 06:47:45 +0000
+++ lib/lp/snappy/tests/test_snap.py	2019-03-25 21:01:52 +0000
@@ -3102,6 +3102,66 @@
             "if the snap package owner and the archive owner are equal.",
             response.body)
 
+    def test_requestBuild_git_private_owners_match(self):
+        # Build requests against a private Git repository are allowed if the
+        # Snap and GitRepository owners match exactly.
+        distroseries = self.factory.makeDistroSeries(
+            distribution=getUtility(IDistributionSet)['ubuntu'],
+            registrant=self.person)
+        processor = self.factory.makeProcessor(supports_virtualized=True)
+        distroarchseries = self.makeBuildableDistroArchSeries(
+            distroseries=distroseries, processor=processor, owner=self.person)
+        distroarchseries_url = api_url(distroarchseries)
+        archive_url = api_url(distroseries.main_archive)
+        [git_ref] = self.factory.makeGitRefs(owner=self.person)
+        snap = self.makeSnap(
+            distroseries=distroseries, processors=[processor], git_ref=git_ref)
+        with person_logged_in(self.person):
+            git_ref.repository.transitionToInformationType(
+                InformationType.PRIVATESECURITY, self.person)
+        transaction.commit()
+        private_webservice = webservice_for_person(
+            self.person, permission=OAuthPermission.WRITE_PRIVATE)
+        private_webservice.default_api_version = "devel"
+        response = private_webservice.named_post(
+            snap["self_link"], "requestBuild", archive=archive_url,
+            distro_arch_series=distroarchseries_url, pocket="Updates")
+        self.assertEqual(201, response.status)
+
+    def test_requestBuild_git_private_owners_mismatch(self):
+        # Build requests against a private Git repository are rejected if
+        # the Snap and GitRepository owners do not match exactly.
+        distroseries = self.factory.makeDistroSeries(
+            distribution=getUtility(IDistributionSet)['ubuntu'],
+            registrant=self.person)
+        processor = self.factory.makeProcessor(supports_virtualized=True)
+        distroarchseries = self.makeBuildableDistroArchSeries(
+            distroseries=distroseries, processor=processor, owner=self.person)
+        distroarchseries_url = api_url(distroarchseries)
+        archive_url = api_url(distroseries.main_archive)
+        [git_ref] = self.factory.makeGitRefs(owner=self.person)
+        snap = self.makeSnap(
+            distroseries=distroseries, processors=[processor], git_ref=git_ref)
+        with admin_logged_in() as admin:
+            git_ref.repository.setOwner(self.factory.makePerson(), admin)
+            git_ref.repository.transitionToInformationType(
+                InformationType.PRIVATESECURITY, admin)
+            git_ref.repository.unsubscribe(self.person, admin)
+            self.assertFalse(git_ref.repository.visibleByUser(self.person))
+        transaction.commit()
+        private_webservice = webservice_for_person(
+            self.person, permission=OAuthPermission.WRITE_PRIVATE)
+        private_webservice.default_api_version = "devel"
+        response = private_webservice.named_post(
+            snap["self_link"], "requestBuild", archive=archive_url,
+            distro_arch_series=distroarchseries_url, pocket="Updates")
+        self.assertEqual(403, response.status)
+        self.assertEqual(
+            "Snap package builds against private Git repositories are only "
+            "allowed if the snap package owner and the repository owner are "
+            "equal.",
+            response.body)
+
     def test_requestBuilds(self):
         # Requests for builds for all relevant architectures can be
         # performed over the webservice, and the returned entry indicates