launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-build-behaviour-macaroon into lp:launchpad with lp:~cjwatson/launchpad/authserver-issue-macaroon as a prerequisite.

Commit message:
Issue an appropriate macaroon when dispatching a snap build that uses 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-build-behaviour-macaroon/+merge/364387
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-build-behaviour-macaroon into lp:launchpad.

=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf	2019-03-13 17:47:21 +0000
+++ configs/development/launchpad-lazr.conf	2019-03-13 17:47:21 +0000
@@ -12,6 +12,7 @@
 root: /var/tmp/builddmaster/
 uploader: scripts/process-upload.py -Mvv
 bzr_builder_sources_list: None
+authentication_endpoint: http://xmlrpc-private.launchpad.dev:8087/authserver
 
 [canonical]
 show_tracebacks: True

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehaviour.py'
--- lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2019-01-14 13:43:06 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2019-03-13 17:47:21 +0000
@@ -17,6 +17,7 @@
 
 import transaction
 from twisted.internet import defer
+from twisted.web import xmlrpc
 from zope.component import getUtility
 
 from lp.buildmaster.enums import (
@@ -52,6 +53,9 @@
         """Store a reference to the job_type with which we were created."""
         self.build = build
         self._builder = None
+        self._authserver = xmlrpc.Proxy(
+            config.builddmaster.authentication_endpoint,
+            connectTimeout=config.builddmaster.authentication_timeout)
 
     @property
     def archive(self):

=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf	2019-03-13 17:47:21 +0000
+++ lib/lp/services/config/schema-lazr.conf	2019-03-13 17:47:21 +0000
@@ -112,6 +112,16 @@
 # datatype: string
 bzr_builder_sources_list: none
 
+# The URL of the XML-RPC endpoint that handles issuing macaroons.  This
+# should implement IAuthServer.
+#
+# datatype: string
+authentication_endpoint: none
+
+# The time in seconds that the builddmaster will wait for a reply from the
+# authserver.
+authentication_timeout: 15
+
 
 [canonical]
 # datatype: boolean

=== modified file 'lib/lp/snappy/model/snapbuildbehaviour.py'
--- lib/lp/snappy/model/snapbuildbehaviour.py	2019-03-06 23:00:49 +0000
+++ lib/lp/snappy/model/snapbuildbehaviour.py	2019-03-13 17:47:21 +0000
@@ -14,6 +14,10 @@
 import base64
 import time
 
+from six.moves.urllib.parse import (
+    urlsplit,
+    urlunsplit,
+    )
 import treq
 from twisted.internet import defer
 from zope.component import adapter
@@ -31,6 +35,7 @@
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
 from lp.services.features import getFeatureFlag
+from lp.services.twistedsupport import cancel_on_timeout
 from lp.services.twistedsupport.treq import check_status
 from lp.snappy.interfaces.snap import (
     SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG,
@@ -128,6 +133,21 @@
         elif build.snap.git_ref is not None:
             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", build.id),
+                    config.builddmaster.authentication_timeout)
+                # XXX cjwatson 2019-03-07: This is ugly and needs
+                # refactoring once we support more general HTTPS
+                # authentication; see also comment in
+                # GitRepository.git_https_url.
+                split = urlsplit(build.snap.git_repository.getCodebrowseUrl())
+                netloc = ":%s@%s" % (macaroon_raw, split.hostname)
+                if split.port:
+                    netloc += ":%s" % split.port
+                args["git_repository"] = urlunsplit([
+                    split.scheme, netloc, split.path, "", ""])
             else:
                 args["git_repository"] = (
                     build.snap.git_repository.git_https_url)

=== modified file 'lib/lp/snappy/tests/test_snapbuildbehaviour.py'
--- lib/lp/snappy/tests/test_snapbuildbehaviour.py	2019-03-06 22:27:11 +0000
+++ lib/lp/snappy/tests/test_snapbuildbehaviour.py	2019-03-13 17:47:21 +0000
@@ -45,11 +45,14 @@
 from twisted.web import (
     resource,
     server,
+    xmlrpc,
     )
 from zope.component import getUtility
 from zope.proxy import isProxy
+from zope.publisher.xmlrpc import TestRequest
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import InformationType
 from lp.archivepublisher.interfaces.archivesigningkey import (
     IArchiveSigningKey,
     )
@@ -74,6 +77,7 @@
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
+from lp.services.authserver.xmlrpc import AuthServerAPIView
 from lp.services.config import config
 from lp.services.features.testing import FeatureFixture
 from lp.services.log.logger import (
@@ -97,6 +101,7 @@
 from lp.testing.gpgkeys import gpgkeysdir
 from lp.testing.keyserver import InProcessKeyServerFixture
 from lp.testing.layers import LaunchpadZopelessLayer
+from lp.xmlrpc.interfaces import IPrivateApplication
 
 
 class ProxyAuthAPITokensResource(resource.Resource):
@@ -162,6 +167,34 @@
         self.addCleanup(config.pop, "in-process-proxy-auth-api-fixture")
 
 
+class InProcessAuthServer(xmlrpc.XMLRPC):
+
+    def __init__(self, *args, **kwargs):
+        xmlrpc.XMLRPC.__init__(self, *args, **kwargs)
+        private_root = getUtility(IPrivateApplication)
+        self.authserver = AuthServerAPIView(
+            private_root.authserver, TestRequest())
+
+    def __getattr__(self, name):
+        if name.startswith("xmlrpc_"):
+            return getattr(self.authserver, name[len("xmlrpc_"):])
+        else:
+            raise AttributeError("%r has no attribute '%s'" % name)
+
+
+class InProcessAuthServerFixture(fixtures.Fixture, xmlrpc.XMLRPC):
+    """A fixture that runs an in-process authserver."""
+
+    def _setUp(self):
+        listener = reactor.listenTCP(0, server.Site(InProcessAuthServer()))
+        self.addCleanup(listener.stopListening)
+        config.push("in-process-auth-server-fixture", (dedent("""
+            [builddmaster]
+            authentication_endpoint: http://localhost:%d/
+            """) % listener.getHost().port).encode("UTF-8"))
+        self.addCleanup(config.pop, "in-process-auth-server-fixture")
+
+
 class TestSnapBuildBehaviourBase(TestCaseWithFactory):
     layer = LaunchpadZopelessLayer
 
@@ -459,6 +492,51 @@
             }))
 
     @defer.inlineCallbacks
+    def test_extraBuildArgs_git_private(self):
+        # extraBuildArgs returns appropriate arguments if asked to build a
+        # job for a private Git branch.
+        self.useFixture(FeatureFixture({SNAP_PRIVATE_FEATURE_FLAG: "on"}))
+        self.useFixture(InProcessAuthServerFixture())
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
+        [ref] = self.factory.makeGitRefs(
+            information_type=InformationType.USERDATA)
+        job = self.makeJob(git_ref=ref, private=True)
+        expected_archives, expected_trusted_keys = (
+            yield get_sources_list_for_building(
+                job.build, job.build.distro_arch_series, None))
+        args = yield job.extraBuildArgs()
+        split_browse_root = urlsplit(config.codehosting.git_browse_root)
+        self.assertThat(args, MatchesDict({
+            "archive_private": Is(False),
+            "archives": Equals(expected_archives),
+            "arch_tag": Equals("i386"),
+            "build_source_tarball": Is(False),
+            "build_url": Equals(canonical_url(job.build)),
+            "fast_cleanup": Is(True),
+            "git_repository": AfterPreprocessing(urlsplit, MatchesStructure(
+                scheme=Equals(split_browse_root.scheme),
+                username=Equals(""),
+                password=AfterPreprocessing(
+                    Macaroon.deserialize, MatchesStructure(
+                        location=Equals(config.vhost.mainsite.hostname),
+                        identifier=Equals("snap-build"),
+                        caveats=MatchesListwise([
+                            MatchesStructure.byEquality(
+                                caveat_id="lp.snap-build %s" % job.build.id),
+                            ]))),
+                hostname=Equals(split_browse_root.hostname),
+                port=Equals(split_browse_root.port))),
+            "git_path": Equals(ref.name),
+            "name": Equals("test-snap"),
+            "private": Is(True),
+            "proxy_url": self.getProxyURLMatcher(job),
+            "revocation_endpoint": self.getRevocationEndpointMatcher(job),
+            "series": Equals("unstable"),
+            "trusted_keys": Equals(expected_trusted_keys),
+            }))
+
+    @defer.inlineCallbacks
     def test_extraBuildArgs_git_url(self):
         # extraBuildArgs returns appropriate arguments if asked to build a
         # job for a Git branch backed by a URL for an external repository.