Merge ~cjwatson/launchpad:remove-buildd-secret into launchpad:master

[Colin Watson]

Remove Archive.buildd_secret

Now that we issue build-scoped macaroons to allow builders to access private archives when necessary, we no longer need static buildd secrets.

DB MP: https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/402376

[Cristian Gonzalez (cristiangsp)]

Looks good!

review: Approve

diff --git a/lib/lp/archivepublisher/tests/test_config.py b/lib/lp/archivepublisher/tests/test_config.py
index 09850a9..55ebba9 100644
--- a/lib/lp/archivepublisher/tests/test_config.py
+++ b/lib/lp/archivepublisher/tests/test_config.py
@@ -162,7 +162,6 @@ class TestGetPubConfigPPA(TestCaseWithFactory):
             owner=self.ppa.owner, name="myprivateppa",
             distribution=self.ubuntutest, purpose=ArchivePurpose.PPA)
         p3a.private = True
-        p3a.buildd_secret = "secret"
         p3a_config = getPubConfig(p3a)
         self.assertEqual(
             config.personalpackagearchive.private_root, p3a_config.distroroot)
diff --git a/lib/lp/archiveuploader/tests/test_ppauploadprocessor.py b/lib/lp/archiveuploader/tests/test_ppauploadprocessor.py
index 899a85e..3c91839 100644
--- a/lib/lp/archiveuploader/tests/test_ppauploadprocessor.py
+++ b/lib/lp/archiveuploader/tests/test_ppauploadprocessor.py
@@ -652,7 +652,6 @@ class TestPPAUploadProcessor(TestPPAUploadProcessorBase):
 
         Make sure that the files are placed in the restricted librarian.
         """
-        self.name16.archive.buildd_secret = "secret"
         self.name16.archive.private = True
         queue_item = self.doCustomUploadToPPA()
         self.checkFilesRestrictedInLibrarian(queue_item, True)
@@ -662,7 +661,6 @@ class TestPPAUploadProcessor(TestPPAUploadProcessorBase):
 
         Make sure that the files are placed in the restricted librarian.
         """
-        self.name16.archive.buildd_secret = "secret"
         self.name16.archive.private = True
 
         upload_dir = self.queueUpload("bar_1.0-1", "~name16/ubuntu")
diff --git a/lib/lp/buildmaster/tests/test_packagebuild.py b/lib/lp/buildmaster/tests/test_packagebuild.py
index 77d85a6..9ff5f97 100644
--- a/lib/lp/buildmaster/tests/test_packagebuild.py
+++ b/lib/lp/buildmaster/tests/test_packagebuild.py
@@ -83,7 +83,6 @@ class TestPackageBuildMixin(TestCaseWithFactory):
         # A private package build will store the upload log on the
         # restricted librarian.
         login('[e-mail]')
-        self.package_build.archive.buildd_secret = 'sekrit'
         self.package_build.archive.private = True
         self.assertTrue(self.package_build.is_private)
         self.package_build.storeUploadLog("Some content")
diff --git a/lib/lp/registry/browser/tests/test_person.py b/lib/lp/registry/browser/tests/test_person.py
index 82dc89b..beb6b9e 100644
--- a/lib/lp/registry/browser/tests/test_person.py
+++ b/lib/lp/registry/browser/tests/test_person.py
@@ -658,7 +658,6 @@ class TestShouldShowPpaSection(TestCaseWithFactory):
         """Helper method to privatise a ppa."""
         login('[e-mail]')
         ppa.private = True
-        ppa.buildd_secret = "secret"
         login(ANONYMOUS)
 
     def test_viewing_person_with_public_ppa(self):
diff --git a/lib/lp/registry/model/distributionsourcepackage.py b/lib/lp/registry/model/distributionsourcepackage.py
index 3224251..863ae85 100644
--- a/lib/lp/registry/model/distributionsourcepackage.py
+++ b/lib/lp/registry/model/distributionsourcepackage.py
@@ -320,7 +320,7 @@ class DistributionSourcePackage(BugTargetBase,
             Archive,
             Archive.distribution == self.distribution,
             Archive._enabled == True,
-            Archive._private == False,
+            Archive.private == False,
             SourcePackagePublishingHistory.archive == Archive.id,
             (SourcePackagePublishingHistory.status ==
                 PackagePublishingStatus.PUBLISHED),
diff --git a/lib/lp/registry/vocabularies.py b/lib/lp/registry/vocabularies.py
index 315ab8f..f2154e3 100644
--- a/lib/lp/registry/vocabularies.py
+++ b/lib/lp/registry/vocabularies.py
@@ -1982,7 +1982,7 @@ class SourcePackageNameVocabulary(NamedStormHugeVocabulary):
             # can do is search for names that are present in public archives
             # of any distribution.
             where=Or(
-                Not(Archive._private),
+                Not(Archive.private),
                 DistributionSourcePackageCache.archive == None),
             tables=LeftJoin(
                 DistributionSourcePackageCache, Archive,
diff --git a/lib/lp/soyuz/browser/tests/archive-views.txt b/lib/lp/soyuz/browser/tests/archive-views.txt
index 859488d..7d90cf0 100644
--- a/lib/lp/soyuz/browser/tests/archive-views.txt
+++ b/lib/lp/soyuz/browser/tests/archive-views.txt
@@ -986,12 +986,13 @@ option to be selected.
 
 Dependencies on private PPAs can be only set if the user performing
 the action also has permission to view the private PPA and if the
-context PPA is also private.
-
-The latter guarantee that the P3A buildd_secret won't get exposed in
-the buildlogs. The remaining risk is to have untrusted people in the
-context PPA which would have a chance to expose the contents of the
-other P3As dependencies while their sources get built.
+context PPA is also private.  This reduces the risk of confidential
+information being leaked; it does not eliminate that risk, because it
+is still possible for other people to be able to see the context PPA
+who cannot see the dependencies directly, but who can see some of
+their contents via builds.  We currently assume that owners of private
+PPAs are aware of this risk when adding other private PPAs as
+dependencies.
 
 Before testing we will create a new team owned by Mark Shutteworth,
 with a private PPA attached to it.
@@ -1023,9 +1024,8 @@ PPA the form fails because he has no permission to view its contents.
     You don't have permission to use this dependency.
 
 When we grant access to Celso for viewing the private PPA, by making
-him a memeber of the new team, setting the private PPA as dependency
-still denied since Celso's PPA still public and thus the dependencies
-buildd_secret would leak through the public buildlogs.
+him a member of the new team, setting the private PPA as dependency is
+still denied since Celso's PPA is still public.
 
     >>> login('[e-mail]')
     >>> ignored = a_team.addMember(cprov, mark)
diff --git a/lib/lp/soyuz/browser/tests/test_archive_admin_view.py b/lib/lp/soyuz/browser/tests/test_archive_admin_view.py
index 485898f..f7243bb 100644
--- a/lib/lp/soyuz/browser/tests/test_archive_admin_view.py
+++ b/lib/lp/soyuz/browser/tests/test_archive_admin_view.py
@@ -53,12 +53,10 @@ class TestArchivePrivacySwitchingView(TestCaseWithFactory):
 
     def test_set_private_without_packages(self):
         # If a ppa does not have packages published, it is possible to
-        # update the private attribute. Marking the PPA private also
-        # generates a buildd secret.
+        # update the private attribute.
         view = self.initialize_admin_view(private=True)
         self.assertEqual(0, len(view.errors))
         self.assertTrue(view.context.private)
-        self.assertTrue(len(view.context.buildd_secret) > 4)
 
     def test_set_public_without_packages(self):
         # If a ppa does not have packages published, it is possible to
diff --git a/lib/lp/soyuz/configure.zcml b/lib/lp/soyuz/configure.zcml
index 9c05d32..5aec316 100644
--- a/lib/lp/soyuz/configure.zcml
+++ b/lib/lp/soyuz/configure.zcml
@@ -365,7 +365,7 @@
         <require
             permission="launchpad.Admin"
             interface="lp.soyuz.interfaces.archive.IArchiveAdmin"
-            set_attributes="authorized_size buildd_secret
+            set_attributes="authorized_size
                             enabled_restricted_processors
                             external_dependencies name
                             permit_obsolete_series_uploads
diff --git a/lib/lp/soyuz/doc/archive.txt b/lib/lp/soyuz/doc/archive.txt
index 87b2cd9..e3799c7 100644
--- a/lib/lp/soyuz/doc/archive.txt
+++ b/lib/lp/soyuz/doc/archive.txt
@@ -72,7 +72,6 @@ records to disk, including configuration and indexes.
     ...     owner=cprov, name='myprivateppa',
     ...     distribution=cprov_archive.distribution)
     >>> login("[e-mail]")
-    >>> cprov_private_ppa.buildd_secret = 'really secret'
     >>> cprov_private_ppa.private = True
     >>> login(ANONYMOUS)
 
@@ -1487,11 +1486,9 @@ method will by default only return public archives:
     >>> login("[e-mail]")
     >>> my_copy_archive = archive_set.getArchivesForDistribution(
     ...     ubuntu, name='my-copy-archive')[0]
-    >>> my_copy_archive.buildd_secret = 'really secret'
     >>> my_copy_archive.private = True
     >>> team_archive = archive_set.getArchivesForDistribution(
     ...     ubuntu, name='team-archive')[0]
-    >>> team_archive.buildd_secret = 'really secret'
     >>> team_archive.private = True
 
     >>> ubuntu_copy_archives = archive_set.getArchivesForDistribution(
@@ -1714,7 +1711,6 @@ So even if Joe's PPA suddenly becomes private, Carlos rights will be
 preserved.
 
     >>> login('[e-mail]')
-    >>> joes_ppa.buildd_secret = 'x'
     >>> joes_ppa.private = True
 
     >>> login("[e-mail]")
diff --git a/lib/lp/soyuz/interfaces/archive.py b/lib/lp/soyuz/interfaces/archive.py
index ed35643..e4929b7 100644
--- a/lib/lp/soyuz/interfaces/archive.py
+++ b/lib/lp/soyuz/interfaces/archive.py
@@ -1191,11 +1191,6 @@ class IArchiveView(IHasBuildRecords):
     def getOverridePolicy(distroseries, pocket, phased_update_percentage=None):
         """Returns an instantiated `IOverridePolicy` for the archive."""
 
-    buildd_secret = TextLine(
-        title=_("Build farm secret"), required=False,
-        description=_(
-            "The password used by the build farm to access the archive."))
-
     @call_with(eager_load=True)
     @rename_parameters_as(
         name="binary_name", distroarchseries="distro_arch_series")
diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
index 0fbca87..b549c13 100644
--- a/lib/lp/soyuz/model/archive.py
+++ b/lib/lp/soyuz/model/archive.py
@@ -312,7 +312,7 @@ class Archive(SQLBase):
 
     publish = BoolCol(dbName='publish', notNull=True, default=True)
 
-    _private = BoolCol(dbName='private', notNull=True, default=False,
+    private = BoolCol(dbName='private', notNull=True, default=False,
                       storm_validator=_validate_archive_privacy)
 
     require_virtualized = BoolCol(
@@ -337,8 +337,6 @@ class Archive(SQLBase):
     package_description_cache = StringCol(
         dbName='package_description_cache', notNull=False, default=None)
 
-    buildd_secret = StringCol(dbName='buildd_secret', default=None)
-
     total_count = IntCol(dbName='total_count', notNull=True, default=0)
 
     pending_count = IntCol(dbName='pending_count', notNull=True, default=0)
@@ -385,22 +383,6 @@ class Archive(SQLBase):
         else:
             alsoProvides(self, IDistributionArchive)
 
-    # Note: You may safely ignore lint when it complains about this
-    # declaration.  As of Python 2.6, this is a perfectly valid way
-    # of adding a setter
-    @property
-    def private(self):
-        return self._private
-
-    @private.setter  # pyflakes:ignore
-    def private(self, private):
-        self._private = private
-        if private:
-            if not self.buildd_secret:
-                self.buildd_secret = create_token(20)
-        else:
-            self.buildd_secret = None
-
     @property
     def title(self):
         """See `IArchive`."""
@@ -2739,7 +2721,6 @@ class ArchiveSet:
 
         # Private teams cannot have public PPAs.
         if owner.visibility == PersonVisibility.PRIVATE:
-            new_archive.buildd_secret = create_token(20)
             new_archive.private = True
         else:
             new_archive.private = private
@@ -2854,7 +2835,7 @@ class ArchiveSet:
             SourcePackagePublishingHistory,
             SourcePackagePublishingHistory.archive == Archive.id,
             SourcePackagePublishingHistory.distroseries == DistroSeries.id,
-            Archive._private == False,
+            Archive.private == False,
             Archive._enabled == True,
             DistroSeries.distribution == distribution,
             Archive.purpose == ArchivePurpose.PPA,
@@ -2893,7 +2874,7 @@ class ArchiveSet:
         """See `IArchiveSet`."""
         return IStore(Archive).find(
             Archive,
-            Archive._private == True, Archive.purpose == ArchivePurpose.PPA)
+            Archive.private == True, Archive.purpose == ArchivePurpose.PPA)
 
     def getArchivesForDistribution(self, distribution, name=None,
                                    purposes=None,
@@ -2914,7 +2895,7 @@ class ArchiveSet:
         if name is not None:
             extra_exprs.append(Archive.name == name)
 
-        public_archive = And(Archive._private == False,
+        public_archive = And(Archive.private == False,
                              Archive._enabled == True)
 
         if not check_permissions:
@@ -3019,12 +3000,12 @@ def get_archive_privacy_filter(user):
     Incorrect and deprecated. Use get_enabled_archive_filter instead.
     """
     if user is None:
-        privacy_filter = Not(Archive._private)
+        privacy_filter = Not(Archive.private)
     elif IPersonRoles(user).in_admin:
         privacy_filter = True
     else:
         privacy_filter = Or(
-            Not(Archive._private),
+            Not(Archive.private),
             Archive.ownerID.is_in(
                 Select(
                     TeamParticipation.teamID,
@@ -3044,7 +3025,7 @@ def get_enabled_archive_filter(user, purpose=None,
     if user is None:
         if include_public:
             terms = [
-                purpose_term, Archive._private == False,
+                purpose_term, Archive.private == False,
                 Archive._enabled == True]
             return And(*terms)
         else:
@@ -3089,5 +3070,5 @@ def get_enabled_archive_filter(user, purpose=None,
 
     if include_public:
         filter_terms.append(
-            And(Archive._enabled == True, Archive._private == False))
+            And(Archive._enabled == True, Archive.private == False))
     return And(purpose_term, Or(*filter_terms))
diff --git a/lib/lp/soyuz/scripts/expire_archive_files.py b/lib/lp/soyuz/scripts/expire_archive_files.py
index 7ae54e8..e871fce 100755
--- a/lib/lp/soyuz/scripts/expire_archive_files.py
+++ b/lib/lp/soyuz/scripts/expire_archive_files.py
@@ -131,7 +131,7 @@ class ArchiveExpirer(LaunchpadCronScript):
                             full_archive_name.is_in(self.blacklist)),
                         Archive.purpose == ArchivePurpose.PPA),
                     And(
-                        IsTrue(Archive._private),
+                        IsTrue(Archive.private),
                         Not(full_archive_name.is_in(self.whitelist))),
                     Not(Archive.purpose.is_in(archive_types)),
                     xPPH.dateremoved > UTC_NOW - stay_of_execution,
diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py
index 2d53207..425355b 100644
--- a/lib/lp/soyuz/tests/test_archive.py
+++ b/lib/lp/soyuz/tests/test_archive.py
@@ -1268,56 +1268,6 @@ class TestProcessors(TestCaseWithFactory):
         self.assertContentEqual([], self.archive.enabled_restricted_processors)
 
 
-class TestBuilddSecret(TestCaseWithFactory):
-    """Test buildd_secret security.
-
-    The buildd_secret is used by the slave scanner when generating a
-    sources.list entry for the builder to access a private archive.  It is
-    essentially the password to the archive for the builder.
-    """
-
-    layer = DatabaseFunctionalLayer
-
-    def setUp(self):
-        super(TestBuilddSecret, self).setUp()
-        self.archive = self.factory.makeArchive()
-
-    def test_anonymous_cannot_set_buildd_secret(self):
-        login(ANONYMOUS)
-        e = self.assertRaises(
-            Unauthorized, setattr, self.archive, "buildd_secret", "boing")
-        self.assertEqual("launchpad.Admin", e.args[2])
-
-    def test_commercial_admin_can_set_buildd_secret(self):
-        with celebrity_logged_in("commercial_admin"):
-            self.archive.buildd_secret = "not so secret at all"
-
-    def test_admin_can_set_buildd_secret(self):
-        with celebrity_logged_in("admin"):
-            self.archive.buildd_secret = "not so secret"
-
-    def test_public_archive_has_public_buildd_secret(self):
-        # In a public PPA, the buildd "secret" is visible to anyone.
-        with celebrity_logged_in("admin"):
-            self.archive.buildd_secret = "not so secret"
-        login(ANONYMOUS)
-        self.assertFalse(self.archive.private)
-        self.assertEqual("not so secret", self.archive.buildd_secret)
-
-    def test_private_archive_has_private_buildd_secret(self):
-        # In a private PPA, the buildd secret can only be read by users with
-        # launchpad.View on the archive.
-        with celebrity_logged_in("admin"):
-            self.archive.buildd_secret = "really secret"
-            self.archive.private = True
-        login(ANONYMOUS)
-        e = self.assertRaises(
-            Unauthorized, getattr, self.archive, "buildd_secret")
-        self.assertEqual("launchpad.View", e.args[2])
-        with person_logged_in(self.archive.owner):
-            self.assertEqual("really secret", self.archive.buildd_secret)
-
-
 class TestNamedAuthTokenFeatureFlag(TestCaseWithFactory):
     layer = LaunchpadZopelessLayer
 
diff --git a/lib/lp/soyuz/tests/test_archive_privacy.py b/lib/lp/soyuz/tests/test_archive_privacy.py
index c5b0605..57b1ce9 100644
--- a/lib/lp/soyuz/tests/test_archive_privacy.py
+++ b/lib/lp/soyuz/tests/test_archive_privacy.py
@@ -98,13 +98,3 @@ class TestPrivacySwitching(TestCaseWithFactory):
 
         self.assertRaises(
             CannotSwitchPrivacy, setattr, private_ppa, 'private', False)
-
-    def test_buildd_secret_was_generated(self):
-        public_ppa = self.factory.makeArchive()
-        public_ppa.private = True
-        self.assertNotEqual(public_ppa.buildd_secret, None)
-
-    def test_discard_buildd_was_discarded(self):
-        private_ppa = self.factory.makeArchive(private=True)
-        private_ppa.private = False
-        self.assertEqual(private_ppa.buildd_secret, None)
diff --git a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
index 0131ffb..2ab0429 100644
--- a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
@@ -709,7 +709,7 @@ class TestBinaryBuildPackageBehaviourBuildCollection(TestCaseWithFactory):
         # Archive.private prevents us from setting it to True with
         # existing published sources.
         Store.of(self.build).execute("""
-            UPDATE archive SET private=True,buildd_secret='foo'
+            UPDATE archive SET private=True
             WHERE archive.id = %s""" % self.build.archive.id)
         Store.of(self.build).invalidate()
 
diff --git a/lib/lp/soyuz/xmlrpc/archive.py b/lib/lp/soyuz/xmlrpc/archive.py
index 5acf7f3..8507873 100644
--- a/lib/lp/soyuz/xmlrpc/archive.py
+++ b/lib/lp/soyuz/xmlrpc/archive.py
@@ -66,26 +66,25 @@ class ArchiveAPI(LaunchpadXMLRPCView):
 
         # If the password is a serialized macaroon for the buildd user, then
         # try macaroon authentication.
-        if (username == BUILDD_USER_NAME and
-                self._verifyMacaroon(archive, password)):
-            # Success.
-            return
+        if username == BUILDD_USER_NAME:
+            if self._verifyMacaroon(archive, password):
+                # Success.
+                return
+            else:
+                raise faults.Unauthorized()
 
         # Fall back to checking archive auth tokens.
-        if username == BUILDD_USER_NAME:
-            secret = archive.buildd_secret
+        if username.startswith("+"):
+            token = token_set.getActiveNamedTokenForArchive(
+                archive, username[1:])
         else:
-            if username.startswith("+"):
-                token = token_set.getActiveNamedTokenForArchive(
-                    archive, username[1:])
-            else:
-                token = token_set.getActiveTokenForArchiveAndPersonName(
-                    archive, username)
-            if token is None:
-                raise faults.NotFound(
-                    message="No valid tokens for '%s' in '%s'." % (
-                        username, archive_reference))
-            secret = removeSecurityProxy(token).token
+            token = token_set.getActiveTokenForArchiveAndPersonName(
+                archive, username)
+        if token is None:
+            raise faults.NotFound(
+                message="No valid tokens for '%s' in '%s'." % (
+                    username, archive_reference))
+        secret = removeSecurityProxy(token).token
         if password != secret:
             raise faults.Unauthorized()
 
diff --git a/lib/lp/soyuz/xmlrpc/tests/test_archive.py b/lib/lp/soyuz/xmlrpc/tests/test_archive.py
index 3007668..86c6ffa 100644
--- a/lib/lp/soyuz/xmlrpc/tests/test_archive.py
+++ b/lib/lp/soyuz/xmlrpc/tests/test_archive.py
@@ -60,16 +60,6 @@ class TestArchiveAPI(TestCaseWithFactory):
             archive.reference, "+missing", "",
             "No valid tokens for '+missing' in '%s'." % archive.reference)
 
-    def test_checkArchiveAuthToken_buildd_wrong_password(self):
-        archive = removeSecurityProxy(self.factory.makeArchive(private=True))
-        self.assertUnauthorized(
-            archive.reference, "buildd", archive.buildd_secret + "-bad")
-
-    def test_checkArchiveAuthToken_buildd_correct_password(self):
-        archive = removeSecurityProxy(self.factory.makeArchive(private=True))
-        self.assertIsNone(self.archive_api.checkArchiveAuthToken(
-            archive.reference, "buildd", archive.buildd_secret))
-
     def test_checkArchiveAuthToken_buildd_macaroon_wrong_archive(self):
         archive = self.factory.makeArchive(private=True)
         build = self.factory.makeBinaryPackageBuild(archive=archive)
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index e95e83d..c74c7f6 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -2943,7 +2943,6 @@ class BareLaunchpadObjectFactory(ObjectFactory):
         if private:
             naked_archive = removeSecurityProxy(archive)
             naked_archive.private = True
-            naked_archive.buildd_secret = "sekrit"
 
         if suppress_subscription_notifications:
             naked_archive = removeSecurityProxy(archive)