launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/no-private-base-images into lp:launchpad.

Commit message:
Forbid setting base images to the output of a private livefs build.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/no-private-base-images/+merge/370760

This can't work at the moment because builders don't have authorisation to fetch the relevant private files, so just forbid it.  If we need it in future (I can think of some edge cases where it might potentially be handy), then we could do something similar to https://code.launchpad.net/~cjwatson/launchpad/build-private-bpb-immediately/+merge/345104 to grant builders the necessary access (some extra work would be required in the librarian too), or we could do something like the thing where BPBs become public if their SPR is copied into a public archive.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/no-private-base-images into lp:launchpad.

=== modified file 'lib/lp/soyuz/browser/tests/test_distroarchseries_webservice.py'
--- lib/lp/soyuz/browser/tests/test_distroarchseries_webservice.py	2019-02-07 12:38:15 +0000
+++ lib/lp/soyuz/browser/tests/test_distroarchseries_webservice.py	2019-07-30 11:59:56 +0000
@@ -14,6 +14,7 @@
 from zope.security.management import endInteraction
 
 from lp.buildmaster.enums import BuildBaseImageType
+from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.features.testing import FeatureFixture
 from lp.soyuz.interfaces.livefs import LIVEFS_FEATURE_FLAG
@@ -93,8 +94,9 @@
         user = das.distroseries.distribution.main_archive.owner
         webservice = launchpadlib_for("testing", user)
         ws_das = ws_object(webservice, das)
-        self.assertRaises(
+        e = self.assertRaises(
             BadRequest, ws_das.setChroot, data='zyx', sha1sum='x')
+        self.assertEqual("Chroot upload checksums do not match", e.content)
 
     def test_setChroot_missing_trailing_cr(self):
         # Due to http://bugs.python.org/issue1349106 launchpadlib sends
@@ -230,6 +232,28 @@
             Unauthorized, ws_das.setChrootFromBuild,
             livefsbuild=build_url, filename="livecd.ubuntu-base.rootfs.tar.gz")
 
+    def test_setChrootFromBuild_private(self):
+        # Chroots may not be set to the output of a private livefs build.
+        self.useFixture(FeatureFixture({LIVEFS_FEATURE_FLAG: "on"}))
+        das = self.factory.makeDistroArchSeries()
+        owner = self.factory.makePerson()
+        private_team = self.factory.makeTeam(
+            owner=owner, visibility=PersonVisibility.PRIVATE)
+        login_as(owner)
+        build = self.factory.makeLiveFSBuild(
+            requester=owner, owner=private_team)
+        build_url = api_url(build)
+        build.addFile(self.factory.makeLibraryFileAlias(
+            filename="livecd.ubuntu-base.rootfs.tar.gz"))
+        user = das.distroseries.distribution.main_archive.owner
+        private_team.addMember(user, owner)
+        webservice = launchpadlib_for("testing", user)
+        ws_das = ws_object(webservice, das)
+        e = self.assertRaises(
+            BadRequest, ws_das.setChrootFromBuild,
+            livefsbuild=build_url, filename="livecd.ubuntu-base.rootfs.tar.gz")
+        self.assertEqual("Cannot set chroot from a private build.", e.content)
+
     def test_setChrootFromBuild_pocket(self):
         self.useFixture(FeatureFixture({LIVEFS_FEATURE_FLAG: "on"}))
         das = self.factory.makeDistroArchSeries()

=== modified file 'lib/lp/soyuz/interfaces/distroarchseries.py'
--- lib/lp/soyuz/interfaces/distroarchseries.py	2019-02-07 12:28:52 +0000
+++ lib/lp/soyuz/interfaces/distroarchseries.py	2019-07-30 11:59:56 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 __all__ = [
+    'ChrootNotPublic',
     'IDistroArchSeries',
     'InvalidChrootUploaded',
     'IPocketChroot',
@@ -55,6 +56,15 @@
     """Raised when the sha1sum of an uploaded chroot does not match."""
 
 
+@error_status(httplib.BAD_REQUEST)
+class ChrootNotPublic(Exception):
+    """Raised when trying to set a chroot from a private livefs build."""
+
+    def __init__(self):
+        super(Exception, self).__init__(
+            "Cannot set chroot from a private build.")
+
+
 class IDistroArchSeriesPublic(IHasBuildRecords, IHasOwner):
     """Public attributes for a DistroArchSeries."""
 

=== modified file 'lib/lp/soyuz/model/distroarchseries.py'
--- lib/lp/soyuz/model/distroarchseries.py	2019-02-07 12:38:15 +0000
+++ lib/lp/soyuz/model/distroarchseries.py	2019-07-30 11:59:56 +0000
@@ -54,6 +54,7 @@
 from lp.soyuz.interfaces.binarypackagename import IBinaryPackageName
 from lp.soyuz.interfaces.buildrecords import IHasBuildRecords
 from lp.soyuz.interfaces.distroarchseries import (
+    ChrootNotPublic,
     IDistroArchSeries,
     InvalidChrootUploaded,
     IPocketChroot,
@@ -236,6 +237,16 @@
     def setChrootFromBuild(self, livefsbuild, filename, pocket=None,
                            image_type=None):
         """See `IDistroArchSeries`."""
+        if livefsbuild.is_private:
+            # This is disallowed partly because files that act as base
+            # images for other builds (including public ones) ought to be
+            # public on principle, and partly because
+            # BuildFarmJobBehaviourBase.dispatchBuildToSlave doesn't
+            # currently support sending a token that would allow builders to
+            # fetch private URLs.  If we ever need to change this (perhaps
+            # for the sake of short-lived security fixes in base images?),
+            # then we need to fix the latter problem first.
+            raise ChrootNotPublic()
         self.addOrUpdateChroot(
             livefsbuild.getFileByName(filename), pocket=pocket,
             image_type=image_type)