launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-987327 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-987327/+merge/103235

This branch teaches SourcePackageRecipeBuilds about privacy, fixing a
number of critical leaks. Recipes were designed to not work with
private branches, but people have been using them with private PPAs, so
they can involve private information.

This is the code fix for
https://wiki.canonical.com/IncidentReports/2012-04-24-LP-p3a-builder-secret-leak

 - Logs were being uploaded to the public librarian at an easily
   guessable URL. This is fixed by making SPRB.is_private use the
   default implementation, which matches the archive's privacy.
   lp.buildmaster code shared with BinaryPackageBuilds ensures that
   uploaded files for private builds are stored in the restricted
   librarian.
   
 - Logs contained an unredacted builder secret. This is fixed by
   passing archive_private=True through to launchpad-buildd, which
   already handles log scrubbing for BPBs.
   
 - ViewBuildFarmJobOld (the launchpad.View security adapter for
   SourcePackageRecipeBuildJob) was broken by a refactoring in
   December. DelegatedAuthorization's methods now return a non-zero
   iterable which always evaluates to true, causing jobs in private
   archives to always be visible. This is fixed by rewriting the
   adapter as a much cleaner DelegatedAuthorization.
   
   This issue is the cause of the specific problem reported as bug
   #987327: buildqueue-current.pt shows logtail if launchpad.View is
   held on the SPRBJ.
   
   Fixing this one prevented Builder:+index from rendering at all when
   an invisible recipe build was present. This is because the "Building
   private job" text is rendered by a fmt:link on SPRB, so the TALES
   has to be able to obtain job.build. I copied the horribleness from
   BPB's BuildPackageJob: allowing the SPRBJ interface unconditionally
   rather restricting it to launchpad.View. SPRBJ just links a Job and
   an SPRB, and this approach works fine for BPBs, so it is secure.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-987327/+merge/103235
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-987327 into lp:launchpad.

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py	2012-01-15 13:32:27 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipebuild.py	2012-04-24 08:21:20 +0000
@@ -31,6 +31,8 @@
     extract_text,
     find_main_content,
     find_tags_by_class,
+    setupBrowser,
+    setupBrowserForUser,
     )
 from lp.testing.sampledata import ADMIN_EMAIL
 
@@ -263,3 +265,41 @@
              extract_text(find_main_content(browser.contents)))
         self.assertEqual(build_url,
                 browser.getLink('~chef/chocolate/cake recipe build').url)
+
+    def makeBuildingRecipe(self, archive=None):
+        builder = self.factory.makeBuilder()
+        build = self.factory.makeSourcePackageRecipeBuild(archive=archive)
+        naked_build = removeSecurityProxy(build)
+        naked_build.queueBuild()
+        naked_build.builder = builder
+        naked_build.buildqueue_record.builder = builder
+        naked_build.buildqueue_record.logtail = 'i am failing'
+        return build
+
+    def makeNonRedirectingBrowser(url, user=None):
+        browser = setupBrowserForUser(user) if user else setupBrowser()
+        browser.mech_browser.set_handle_equiv(False)
+        browser.open(url)
+        return browser
+
+    def test_builder_index_public(self):
+        build = self.makeBuildingRecipe()
+        url = canonical_url(build.builder)
+        logout()
+        browser = self.makeNonRedirectingBrowser(url)
+        self.assertIn('i am failing', browser.contents)
+
+    def test_builder_index_private(self):
+        archive = self.factory.makeArchive(private=True)
+        build = self.makeBuildingRecipe(archive=archive)
+        url = canonical_url(removeSecurityProxy(build).builder)
+        random_person = self.factory.makePerson()
+        logout()
+
+        # An unrelated user can't see the logtail of a private build.
+        browser = self.makeNonRedirectingBrowser(url, random_person)
+        self.assertNotIn('i am failing', browser.contents)
+
+        # But someone who can see the archive can.
+        browser = self.makeNonRedirectingBrowser(url, archive.owner)
+        self.assertIn('i am failing', browser.contents)

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2012-02-09 16:13:00 +0000
+++ lib/lp/code/configure.zcml	2012-04-24 08:21:20 +0000
@@ -914,7 +914,7 @@
 
   <class
      class="lp.code.model.sourcepackagerecipebuild.SourcePackageRecipeBuildJob">
-    <require permission="launchpad.View" interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/>
+    <allow interface="lp.code.interfaces.sourcepackagerecipebuild.ISourcePackageRecipeBuildJob"/>
   </class>
 
   <securedutility

=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/model/recipebuilder.py	2012-04-24 08:21:20 +0000
@@ -85,6 +85,7 @@
             None)
         args['archives'] = get_sources_list_for_building(self.build,
             distroarchseries, None)
+        args['archive_private'] = self.build.archive.private
 
         # config.builddmaster.bzr_builder_sources_list can contain a
         # sources.list entry for an archive that will contain a

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2012-03-15 08:36:44 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2012-04-24 08:21:20 +0000
@@ -90,8 +90,6 @@
 
     id = Int(primary=True)
 
-    is_private = False
-
     # The list of build status values for which email notifications are
     # allowed to be sent. It is up to each callback as to whether it will
     # consider sending a notification but it won't do so if the status is not

=== modified file 'lib/lp/code/model/tests/test_recipebuilder.py'
--- lib/lp/code/model/tests/test_recipebuilder.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/model/tests/test_recipebuilder.py	2012-04-24 08:21:20 +0000
@@ -49,7 +49,8 @@
 
     layer = LaunchpadZopelessLayer
 
-    def makeJob(self, recipe_registrant=None, recipe_owner=None):
+    def makeJob(self, recipe_registrant=None, recipe_owner=None,
+                archive=None):
         """Create a sample `ISourcePackageRecipeBuildJob`."""
         spn = self.factory.makeSourcePackageName("apackage")
         distro = self.factory.makeDistribution(name="distro")
@@ -72,7 +73,7 @@
             recipe_registrant, recipe_owner, distroseries, u"recept",
             u"Recipe description", branches=[somebranch])
         spb = self.factory.makeSourcePackageRecipeBuild(
-            sourcepackage=sourcepackage,
+            sourcepackage=sourcepackage, archive=archive,
             recipe=recipe, requester=recipe_owner, distroseries=distroseries)
         job = spb.makeJob()
         job_id = removeSecurityProxy(job.job).id
@@ -153,6 +154,7 @@
         expected_archives.append(
             "deb http://foo %s main" % job.build.distroseries.name)
         self.assertEqual({
+           'archive_private': False,
            'arch_tag': 'i386',
            'author_email': u'requester@xxxxxxxxxx',
            'suite': u'mydistro',
@@ -166,6 +168,18 @@
            'distroseries_name': job.build.distroseries.name,
             }, job._extraBuildArgs(distroarchseries))
 
+    def test_extraBuildArgs_private_archive(self):
+        # If the build archive is private, the archive_private flag is
+        # True. This tells launchpad-buildd to redact credentials from
+        # build logs.
+        self._setBuilderConfig()
+        archive = self.factory.makeArchive(private=True)
+        job = self.makeJob(archive=archive)
+        distroarchseries = job.build.distroseries.architectures[0]
+        extra_args = job._extraBuildArgs(distroarchseries)
+        self.assertEqual(
+            True, extra_args['archive_private'])
+
     def test_extraBuildArgs_team_owner_no_email(self):
         # If the owner of the recipe is a team without a preferred email, the
         # registrant is used.
@@ -224,6 +238,7 @@
             job.build, distroarchseries, None)
         logger = BufferLogger()
         self.assertEqual({
+           'archive_private': False,
            'arch_tag': 'i386',
            'author_email': u'requester@xxxxxxxxxx',
            'suite': u'mydistro',

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2012-02-09 23:09:36 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2012-04-24 08:21:20 +0000
@@ -66,7 +66,7 @@
 
     layer = LaunchpadFunctionalLayer
 
-    def makeSourcePackageRecipeBuild(self):
+    def makeSourcePackageRecipeBuild(self, archive=None):
         """Create a `SourcePackageRecipeBuild` for testing."""
         person = self.factory.makePerson()
         distroseries = self.factory.makeDistroSeries()
@@ -75,12 +75,14 @@
             supports_virtualized=True)
         removeSecurityProxy(distroseries).nominatedarchindep = (
             distroseries_i386)
+        if archive is None:
+            archive = self.factory.makeArchive()
 
         return getUtility(ISourcePackageRecipeBuildSource).new(
             distroseries=distroseries,
             recipe=self.factory.makeSourcePackageRecipe(
                 distroseries=distroseries),
-            archive=self.factory.makeArchive(),
+            archive=archive,
             requester=person)
 
     def test_providesInterfaces(self):
@@ -156,9 +158,13 @@
         self.assertEqual('main', spb.current_component.name)
 
     def test_is_private(self):
-        # A source package recipe build is currently always public.
+        # A source package recipe build's is private iff its archive is.
         spb = self.makeSourcePackageRecipeBuild()
         self.assertEqual(False, spb.is_private)
+        archive = self.factory.makeArchive(private=True)
+        with person_logged_in(archive.owner):
+            spb = self.makeSourcePackageRecipeBuild(archive=archive)
+            self.assertEqual(True, spb.is_private)
 
     def test_view_private_branch(self):
         """Recipebuilds with private branches are restricted."""
@@ -167,24 +173,32 @@
         with person_logged_in(owner):
             recipe = self.factory.makeSourcePackageRecipe(branches=[branch])
             build = self.factory.makeSourcePackageRecipeBuild(recipe=recipe)
+            job = build.makeJob()
             self.assertTrue(check_permission('launchpad.View', build))
+            self.assertTrue(check_permission('launchpad.View', job))
         removeSecurityProxy(branch).explicitly_private = True
         with person_logged_in(self.factory.makePerson()):
             self.assertFalse(check_permission('launchpad.View', build))
+            self.assertFalse(check_permission('launchpad.View', job))
         login(ANONYMOUS)
         self.assertFalse(check_permission('launchpad.View', build))
+        self.assertFalse(check_permission('launchpad.View', job))
 
     def test_view_private_archive(self):
         """Recipebuilds with private branches are restricted."""
         owner = self.factory.makePerson()
         archive = self.factory.makeArchive(owner=owner, private=True)
-        build = self.factory.makeSourcePackageRecipeBuild(archive=archive)
         with person_logged_in(owner):
+            build = self.factory.makeSourcePackageRecipeBuild(archive=archive)
+            job = build.makeJob()
             self.assertTrue(check_permission('launchpad.View', build))
+            self.assertTrue(check_permission('launchpad.View', job))
         with person_logged_in(self.factory.makePerson()):
             self.assertFalse(check_permission('launchpad.View', build))
+            self.assertFalse(check_permission('launchpad.View', job))
         login(ANONYMOUS)
         self.assertFalse(check_permission('launchpad.View', build))
+        self.assertFalse(check_permission('launchpad.View', job))
 
     def test_estimateDuration(self):
         # If there are no successful builds, estimate 10 minutes.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-03-26 06:02:37 +0000
+++ lib/lp/security.py	2012-04-24 08:21:20 +0000
@@ -1846,7 +1846,7 @@
         return auth_spr.checkUnauthenticated()
 
 
-class ViewBuildFarmJobOld(AuthorizationBase):
+class ViewBuildFarmJobOld(DelegatedAuthorization):
     """Permission to view an `IBuildFarmJobOld`.
 
     This permission is based entirely on permission to view the
@@ -1869,29 +1869,15 @@
         else:
             return None
 
-    def _checkBuildPermission(self, user=None):
-        """Check access to `IPackageBuild` for this job."""
-        permission = getAdapter(
-            self.obj.build, IAuthorization, self.permission)
-        if user is None:
-            return permission.checkUnauthenticated()
-        else:
-            return permission.checkAuthenticated(user)
-
-    def _checkAccess(self, user=None):
-        """Unified access check for anonymous and authenticated users."""
+    def iter_objects(self):
         branch = self._getBranch()
-        if branch is not None and not branch.visibleByUser(user):
-            return False
-
         build = self._getBuild()
-        if build is not None and not self._checkBuildPermission(user):
-            return False
-
-        return True
-
-    checkAuthenticated = _checkAccess
-    checkUnauthenticated = _checkAccess
+        objects = []
+        if branch:
+            objects.append(branch)
+        if build:
+            objects.append(build)
+        return objects
 
 
 class SetQuestionCommentVisibility(AuthorizationBase):