Merge ~cjwatson/launchpad:issue-private-archive-macaroons into launchpad:master

[Colin Watson]

Dispatch sources.list entries for private archives using macaroons

This allows snap base archive dependencies on private archives to work (for 16.04 ESM), and also takes a significant step towards abolishing `Archive.buildd_secret`. These tokens are only valid while the build is running, and so are much less vulnerable to accidental leakage.

The old buildd secret is still used in the file map passed to builders to fetch the source package for binary package builds in private archives, so it can't yet be removed entirely.

[Ioana Lasc (ilasc)]

LGTM

review: Approve

diff --git a/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py b/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
index 4d768d8..d210d64 100644
--- a/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
@@ -46,6 +46,15 @@ class IBuildFarmJobBehaviour(Interface):
             'password' (optional): password to authenticate with
         """
 
+    def issueMacaroon():
+        """Issue a macaroon to access private resources for this build.
+
+        :raises NotImplementedError: if the build type does not support
+            accessing private resources.
+        :return: A Deferred that calls back with a serialized macaroon or a
+            fault.
+        """
+
     def extraBuildArgs(logger=None):
         """Return extra arguments required by the builder for this build.
 
diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index f8b5034..e0d5380 100644
--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
@@ -89,6 +89,10 @@ class BuildFarmJobBehaviourBase:
         """The default behaviour is to send no files."""
         return {}
 
+    def issueMacaroon(self):
+        raise NotImplementedError(
+            "This build type does not support accessing private resources.")
+
     def extraBuildArgs(self, logger=None):
         """The default behaviour is to send only common extra arguments."""
         args = {}
diff --git a/lib/lp/oci/model/ocirecipebuildbehaviour.py b/lib/lp/oci/model/ocirecipebuildbehaviour.py
index e8a60f5..0ca766d 100644
--- a/lib/lp/oci/model/ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/model/ocirecipebuildbehaviour.py
@@ -80,6 +80,14 @@ class OCIRecipeBuildBehaviour(SnapProxyMixin, BuildFarmJobBehaviourBase):
             raise CannotBuild(
                 "Missing chroot for %s" % build.distro_arch_series.displayname)
 
+    def issueMacaroon(self):
+        """See `IBuildFarmJobBehaviour`."""
+        return cancel_on_timeout(
+            self._authserver.callRemote(
+                "issueMacaroon",
+                "oci-recipe-build", "OCIRecipeBuild", self.build.id),
+            config.builddmaster.authentication_timeout)
+
     def _getBuildInfoArgs(self):
         def format_user(user):
             if user is None:
@@ -142,11 +150,7 @@ class OCIRecipeBuildBehaviour(SnapProxyMixin, BuildFarmJobBehaviourBase):
 
         if build.recipe.git_ref is not None:
             if build.recipe.git_repository.private:
-                macaroon_raw = yield cancel_on_timeout(
-                    self._authserver.callRemote(
-                        "issueMacaroon",
-                        "oci-recipe-build", "OCIRecipeBuild", build.id),
-                    config.builddmaster.authentication_timeout)
+                macaroon_raw = yield self.issueMacaroon()
                 url = build.recipe.git_repository.getCodebrowseUrl(
                     username=None, password=macaroon_raw)
                 args["git_repository"] = url
diff --git a/lib/lp/snappy/model/snapbuildbehaviour.py b/lib/lp/snappy/model/snapbuildbehaviour.py
index dda949b..0057670 100644
--- a/lib/lp/snappy/model/snapbuildbehaviour.py
+++ b/lib/lp/snappy/model/snapbuildbehaviour.py
@@ -145,6 +145,13 @@ class SnapBuildBehaviour(SnapProxyMixin, BuildFarmJobBehaviourBase):
             raise CannotBuild(
                 "Missing chroot for %s" % build.distro_arch_series.displayname)
 
+    def issueMacaroon(self):
+        """See `IBuildFarmJobBehaviour`."""
+        return cancel_on_timeout(
+            self._authserver.callRemote(
+                "issueMacaroon", "snap-build", "SnapBuild", self.build.id),
+            config.builddmaster.authentication_timeout)
+
     @defer.inlineCallbacks
     def extraBuildArgs(self, logger=None):
         """
@@ -186,10 +193,7 @@ class SnapBuildBehaviour(SnapProxyMixin, BuildFarmJobBehaviourBase):
             if build.snap.git_ref.repository_url is not None:
                 args["git_repository"] = build.snap.git_ref.repository_url
             elif build.snap.git_repository.private:
-                macaroon_raw = yield cancel_on_timeout(
-                    self._authserver.callRemote(
-                        "issueMacaroon", "snap-build", "SnapBuild", build.id),
-                    config.builddmaster.authentication_timeout)
+                macaroon_raw = yield self.issueMacaroon()
                 url = build.snap.git_repository.getCodebrowseUrl(
                     username=None, password=macaroon_raw)
                 args["git_repository"] = url
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index 1d80326..0deebed 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -14,6 +14,7 @@ from textwrap import dedent
 import time
 import uuid
 
+from aptsources.sourceslist import SourceEntry
 import fixtures
 from pymacaroons import Macaroon
 import pytz
@@ -77,6 +78,7 @@ from lp.services.log.logger import (
     BufferLogger,
     DevNullLogger,
     )
+from lp.services.macaroons.testing import MacaroonVerifies
 from lp.services.statsd.tests import StatsMixin
 from lp.services.webapp import canonical_url
 from lp.snappy.interfaces.snap import (
@@ -790,6 +792,59 @@ class TestAsyncSnapBuildBehaviour(StatsMixin, TestSnapBuildBehaviourBase):
         self.assertEqual(expected_archives, args["archives"])
 
     @defer.inlineCallbacks
+    def test_extraBuildArgs_snap_base_with_private_archive_dependencies(self):
+        # If the build is using a snap base that has archive dependencies on
+        # private PPAs, extraBuildArgs sends them.
+        self.useFixture(InProcessAuthServerFixture())
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
+        snap_base = self.factory.makeSnapBase()
+        job = self.makeJob(snap_base=snap_base)
+        dependency = self.factory.makeArchive(
+            distribution=job.archive.distribution, private=True)
+        snap_base.addArchiveDependency(
+            dependency, PackagePublishingPocket.RELEASE,
+            getUtility(IComponentSet)["main"])
+        self.factory.makeBinaryPackagePublishingHistory(
+            archive=dependency, distroarchseries=job.build.distro_arch_series,
+            pocket=PackagePublishingPocket.RELEASE,
+            status=PackagePublishingStatus.PUBLISHED)
+        with dbuser(config.builddmaster.dbuser):
+            args = yield job.extraBuildArgs()
+        job.build.updateStatus(BuildStatus.BUILDING)
+        self.assertThat(
+            [SourceEntry(item) for item in args["archives"]],
+            MatchesListwise([
+                MatchesStructure(
+                    type=Equals("deb"),
+                    uri=AfterPreprocessing(urlsplit, MatchesStructure(
+                        scheme=Equals("http"),
+                        username=Equals("buildd"),
+                        password=MacaroonVerifies("snap-build", dependency),
+                        hostname=Equals("private-ppa.launchpad.test"),
+                        path=Equals("/%s/%s/%s" % (
+                            dependency.owner.name, dependency.name,
+                            dependency.distribution.name)))),
+                    dist=Equals(job.build.distro_series.name),
+                    comps=Equals(["main"])),
+                MatchesStructure.byEquality(
+                    type="deb",
+                    uri=job.archive.archive_url,
+                    dist=job.build.distro_series.name,
+                    comps=["main", "universe"]),
+                MatchesStructure.byEquality(
+                    type="deb",
+                    uri=job.archive.archive_url,
+                    dist="%s-security" % job.build.distro_series.name,
+                    comps=["main", "universe"]),
+                MatchesStructure.byEquality(
+                    type="deb",
+                    uri=job.archive.archive_url,
+                    dist="%s-updates" % job.build.distro_series.name,
+                    comps=["main", "universe"]),
+                ]))
+
+    @defer.inlineCallbacks
     def test_extraBuildArgs_ppa_and_snap_base_with_archive_dependencies(self):
         # If the build is using a PPA and a snap base that both have archive
         # dependencies, extraBuildArgs sends them all.
diff --git a/lib/lp/soyuz/adapters/archivedependencies.py b/lib/lp/soyuz/adapters/archivedependencies.py
index ff1d117..685a82e 100644
--- a/lib/lp/soyuz/adapters/archivedependencies.py
+++ b/lib/lp/soyuz/adapters/archivedependencies.py
@@ -290,7 +290,8 @@ def get_sources_list_for_building(behaviour, distroarchseries,
         tools_source=tools_source, tools_fingerprint=tools_fingerprint,
         logger=logger)
     sources_list_lines, trusted_keys = (
-        yield _get_sources_list_for_dependencies(deps, logger=logger))
+        yield _get_sources_list_for_dependencies(
+            behaviour, deps, logger=logger))
 
     external_dep_lines = []
     # Append external sources.list lines for this build if specified.  No
@@ -343,7 +344,8 @@ def _has_published_binaries(archive, distroarchseries, pocket):
     return not published_binaries.is_empty()
 
 
-def _get_binary_sources_list_line(archive, distroarchseries, pocket,
[e-mail]
+def _get_binary_sources_list_line(behaviour, archive, distroarchseries, pocket,
                                   components):
     """Return the correponding binary sources_list line."""
     # Encode the private PPA repository password in the
@@ -351,23 +353,24 @@ def _get_binary_sources_list_line(archive, distroarchseries, pocket,
     # sanitized to not expose it.
     if archive.private:
         uri = URI(archive.archive_url)
-        uri = uri.replace(
-            userinfo="buildd:%s" % archive.buildd_secret)
+        macaroon_raw = yield behaviour.issueMacaroon()
+        uri = uri.replace(userinfo="buildd:%s" % macaroon_raw)
         url = str(uri)
     else:
         url = archive.archive_url
 
     suite = distroarchseries.distroseries.name + pocketsuffix[pocket]
-    return 'deb %s %s %s' % (url, suite, ' '.join(components))
+    defer.returnValue('deb %s %s %s' % (url, suite, ' '.join(components)))
 
 
 @defer.inlineCallbacks
-def _get_sources_list_for_dependencies(dependencies, logger=None):
+def _get_sources_list_for_dependencies(behaviour, dependencies, logger=None):
     """Return sources.list entries and keys.
 
     Process the given list of dependency tuples for the given
     `DistroArchseries`.
 
+    :param behaviour: the build's `IBuildFarmJobBehaviour`.
     :param dependencies: list of 3 elements tuples as:
         (`IArchive`, `IDistroArchSeries`, `PackagePublishingPocket`,
          list of `IComponent` names)
@@ -399,8 +402,8 @@ def _get_sources_list_for_dependencies(dependencies, logger=None):
             components = [
                 component for component in components
                 if component in archive_components]
-            sources_list_line = _get_binary_sources_list_line(
-                archive, distro_arch_series, pocket, components)
+            sources_list_line = yield _get_binary_sources_list_line(
+                behaviour, archive, distro_arch_series, pocket, components)
             sources_list_lines.append(sources_list_line)
             fingerprint = archive.signing_key_fingerprint
         if fingerprint is not None and fingerprint not in trusted_keys:
diff --git a/lib/lp/soyuz/model/binarypackagebuildbehaviour.py b/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
index 6b74f46..2876ac0 100644
--- a/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
@@ -22,6 +22,8 @@ from lp.buildmaster.model.buildfarmjobbehaviour import (
     BuildFarmJobBehaviourBase,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.config import config
+from lp.services.twistedsupport import cancel_on_timeout
 from lp.services.webapp import urlappend
 from lp.soyuz.adapters.archivedependencies import (
     get_primary_current_component,
@@ -131,6 +133,14 @@ class BinaryPackageBuildBehaviour(BuildFarmJobBehaviourBase):
                     (build.title, build.id, build.pocket.name,
                      build.distro_series.name))
 
+    def issueMacaroon(self):
+        """See `IBuildFarmJobBehaviour`."""
+        return cancel_on_timeout(
+            self._authserver.callRemote(
+                "issueMacaroon", "binary-package-build", "BinaryPackageBuild",
+                self.build.id),
+            config.builddmaster.authentication_timeout)
+
     @defer.inlineCallbacks
     def extraBuildArgs(self, logger=None):
         """
diff --git a/lib/lp/soyuz/model/livefsbuildbehaviour.py b/lib/lp/soyuz/model/livefsbuildbehaviour.py
index c439559..0f93d39 100644
--- a/lib/lp/soyuz/model/livefsbuildbehaviour.py
+++ b/lib/lp/soyuz/model/livefsbuildbehaviour.py
@@ -25,6 +25,8 @@ from lp.buildmaster.model.buildfarmjobbehaviour import (
     BuildFarmJobBehaviourBase,
     )
 from lp.registry.interfaces.series import SeriesStatus
+from lp.services.config import config
+from lp.services.twistedsupport import cancel_on_timeout
 from lp.soyuz.adapters.archivedependencies import (
     get_sources_list_for_building,
     )
@@ -78,6 +80,13 @@ class LiveFSBuildBehaviour(BuildFarmJobBehaviourBase):
             raise CannotBuild(
                 "Missing chroot for %s" % build.distro_arch_series.displayname)
 
+    def issueMacaroon(self):
+        """See `IBuildFarmJobBehaviour`."""
+        return cancel_on_timeout(
+            self._authserver.callRemote(
+                "issueMacaroon", "livefs-build", "LiveFSBuild", self.build.id),
+            config.builddmaster.authentication_timeout)
+
     @defer.inlineCallbacks
     def extraBuildArgs(self, logger=None):
         """
diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py
index a49ccd3..2d53207 100644
--- a/lib/lp/soyuz/tests/test_archive.py
+++ b/lib/lp/soyuz/tests/test_archive.py
@@ -13,22 +13,28 @@ from datetime import (
 import doctest
 import os.path
 
+from aptsources.sourceslist import SourceEntry
 from pytz import UTC
 import responses
 import six
 from six.moves import http_client
+from six.moves.urllib.parse import urlsplit
 from storm.store import Store
 from testtools.matchers import (
+    AfterPreprocessing,
     AllMatch,
     DocTestMatches,
+    Equals,
     LessThan,
     MatchesListwise,
     MatchesPredicate,
-    MatchesRegex,
     MatchesStructure,
     )
 from testtools.testcase import ExpectedException
-from testtools.twistedsupport import AsynchronousDeferredRunTest
+from testtools.twistedsupport import (
+    AsynchronousDeferredRunTest,
+    AsynchronousDeferredRunTestForBrokenTwisted,
+    )
 import transaction
 from twisted.internet import defer
 from zope.component import getUtility
@@ -57,6 +63,7 @@ from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.teammembership import TeamMembershipStatus
+from lp.services.authserver.testing import InProcessAuthServerFixture
 from lp.services.database.interfaces import IStore
 from lp.services.database.sqlbase import sqlvalues
 from lp.services.features import getFeatureFlag
@@ -67,6 +74,7 @@ from lp.services.gpg.interfaces import (
     IGPGHandler,
     )
 from lp.services.job.interfaces.job import JobStatus
+from lp.services.macaroons.testing import MacaroonVerifies
 from lp.services.propertycache import (
     clear_property_cache,
     get_property_cache,
@@ -1866,11 +1874,15 @@ class TestAddArchiveDependencies(TestCaseWithFactory):
 class TestArchiveDependencies(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
-    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=30)
+    run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
+        timeout=30)
 
     @defer.inlineCallbacks
     def test_private_sources_list(self):
         """Entries for private dependencies include credentials."""
+        self.useFixture(InProcessAuthServerFixture())
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
         p3a = self.factory.makeArchive(name='p3a', private=True)
         yield self.useFixture(InProcessKeyServerFixture()).start()
         key_path = os.path.join(gpgkeysdir, '[e-mail]')
@@ -1890,10 +1902,20 @@ class TestArchiveDependencies(TestCaseWithFactory):
             sources_list, trusted_keys = yield get_sources_list_for_building(
                 behaviour, build.distro_arch_series,
                 build.source_package_release.name)
-            matches = MatchesRegex(
-                "deb http://buildd:[e-mail]/"
-                "person-name-.*/dependency/ubuntu distroseries-.* main")
-            self.assertThat(sources_list[0], matches)
+            # Mark the build as building so that we can verify its macaroons.
+            build.updateStatus(BuildStatus.BUILDING)
+            self.assertThat(SourceEntry(sources_list[0]), MatchesStructure(
+                type=Equals("deb"),
+                uri=AfterPreprocessing(urlsplit, MatchesStructure(
+                    scheme=Equals("http"),
+                    username=Equals("buildd"),
+                    password=MacaroonVerifies("binary-package-build", p3a),
+                    hostname=Equals("private-ppa.launchpad.test"),
+                    path=Equals("/%s/dependency/ubuntu" % p3a.owner.name),
+                    )),
+                dist=Equals(build.distro_series.getSuite(build.pocket)),
+                comps=Equals(["main"]),
+                ))
             self.assertThat(trusted_keys, MatchesListwise([
                 Base64KeyMatches("0D57E99656BEFB0897606EE9A022DD1F5001B46D"),
                 ]))
diff --git a/system-packages.txt b/system-packages.txt
index 84a8548..56a1291 100644
--- a/system-packages.txt
+++ b/system-packages.txt
@@ -19,6 +19,9 @@ apt
 apt_inst
 apt_pkg
 
+# Used by tests to parse sources.list entries.
+aptsources
+
 # utilities/js-deps
 convoy