Merge ~cjwatson/launchpad:git-auth-launchpad-services into launchpad:master

[Colin Watson]

Authenticate using +launchpad-services rather than empty username

For private snap and OCI recipe builds, we previously authenticated to git using the empty username and a non-user-bound macaroon. This is syntactically valid and works with current versions of git, but it's the sort of edge case in the URL specification that's easy to mishandle.

curl 7.58.0 (used by git's HTTP transport in Ubuntu 18.04) fails to send an Authorization header in this case, while curl 7.68.0 (used by git's HTTP transport in Ubuntu 20.04) gets it right. I bisected this to https://github.com/curl/curl/commit/46e164069d, which is a substantial refactoring of URL parsing that would be unreasonable to try to backport. Using a reserved username instead is safer.

[Cristian Gonzalez (cristiangsp)]

Looks good!

review: Approve

diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 62dad82..e890605 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -565,7 +565,10 @@ class GitAPI(LaunchpadXMLRPCView):
         otherwise Zope's XML-RPC publication machinery gets confused by the
         decorator and publishes a method that takes zero arguments.
         """
-        user = getUtility(IPersonSet).getByName(username) if username else None
+        user = (
+            getUtility(IPersonSet).getByName(username)
+            if username and username != LAUNCHPAD_SERVICES
+            else None)
         verified = self._verifyMacaroon(password, user=user)
         if verified:
             auth_params = {"macaroon": password}
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index efc1087..8a032b5 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -2302,17 +2302,22 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
             job = self.factory.makeCodeImportJob(code_import=code_import)
         issuer = getUtility(IMacaroonIssuer, "code-import-job")
         macaroon = removeSecurityProxy(issuer).issueMacaroon(job)
-        self.assertEqual(
-            {"macaroon": macaroon.serialize(), "user": "+launchpad-services"},
-            self.assertDoesNotFault(
-                None, "authenticateWithPassword", "", macaroon.serialize()))
-        other_macaroon = Macaroon(identifier="another", key="another-secret")
-        self.assertFault(
-            faults.Unauthorized, None,
-            "authenticateWithPassword", "", other_macaroon.serialize())
-        self.assertFault(
-            faults.Unauthorized, None,
-            "authenticateWithPassword", "", "nonsense")
+        for username in ("", "+launchpad-services"):
+            self.assertEqual(
+                {"macaroon": macaroon.serialize(),
+                 "user": "+launchpad-services"},
+                self.assertDoesNotFault(
+                    None, "authenticateWithPassword",
+                    username, macaroon.serialize()))
+            other_macaroon = Macaroon(
+                identifier="another", key="another-secret")
+            self.assertFault(
+                faults.Unauthorized, None,
+                "authenticateWithPassword",
+                username, other_macaroon.serialize())
+            self.assertFault(
+                faults.Unauthorized, None,
+                "authenticateWithPassword", username, "nonsense")
 
     def test_authenticateWithPassword_private_snap_build(self):
         self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
@@ -2325,17 +2330,22 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
                 requester=owner, owner=owner, git_ref=ref, private=True)
             issuer = getUtility(IMacaroonIssuer, "snap-build")
             macaroon = removeSecurityProxy(issuer).issueMacaroon(build)
-        self.assertEqual(
-            {"macaroon": macaroon.serialize(), "user": "+launchpad-services"},
-            self.assertDoesNotFault(
-                None, "authenticateWithPassword", "", macaroon.serialize()))
-        other_macaroon = Macaroon(identifier="another", key="another-secret")
-        self.assertFault(
-            faults.Unauthorized, None,
-            "authenticateWithPassword", "", other_macaroon.serialize())
-        self.assertFault(
-            faults.Unauthorized, None,
-            "authenticateWithPassword", "", "nonsense")
+        for username in ("", "+launchpad-services"):
+            self.assertEqual(
+                {"macaroon": macaroon.serialize(),
+                 "user": "+launchpad-services"},
+                self.assertDoesNotFault(
+                    None, "authenticateWithPassword",
+                    username, macaroon.serialize()))
+            other_macaroon = Macaroon(
+                identifier="another", key="another-secret")
+            self.assertFault(
+                faults.Unauthorized, None,
+                "authenticateWithPassword",
+                username, other_macaroon.serialize())
+            self.assertFault(
+                faults.Unauthorized, None,
+                "authenticateWithPassword", username, "nonsense")
 
     def test_authenticateWithPassword_user_macaroon(self):
         # A user with a suitable macaroon can authenticate using it, in
@@ -2351,12 +2361,10 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
             self.assertDoesNotFault(
                 None, "authenticateWithPassword",
                 requester.name, macaroon.serialize()))
-        self.assertFault(
-            faults.Unauthorized, None,
-            "authenticateWithPassword", "", macaroon.serialize())
-        self.assertFault(
-            faults.Unauthorized, None,
-            "authenticateWithPassword", "nonexistent", macaroon.serialize())
+        for username in ("", "+launchpad-services", "nonexistent"):
+            self.assertFault(
+                faults.Unauthorized, None,
+                "authenticateWithPassword", username, macaroon.serialize())
         other_macaroon = Macaroon(identifier="another", key="another-secret")
         self.assertFault(
             faults.Unauthorized, None,
diff --git a/lib/lp/oci/model/ocirecipebuildbehaviour.py b/lib/lp/oci/model/ocirecipebuildbehaviour.py
index 8147c77..6b82f36 100644
--- a/lib/lp/oci/model/ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/model/ocirecipebuildbehaviour.py
@@ -34,6 +34,7 @@ from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
 from lp.buildmaster.model.buildfarmjobbehaviour import (
     BuildFarmJobBehaviourBase,
     )
+from lp.code.interfaces.codehosting import LAUNCHPAD_SERVICES
 from lp.oci.interfaces.ocirecipebuild import IOCIFileSet
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
@@ -156,7 +157,7 @@ class OCIRecipeBuildBehaviour(BuilderProxyMixin, BuildFarmJobBehaviourBase):
             if build.recipe.git_repository.private:
                 macaroon_raw = yield self.issueMacaroon()
                 url = build.recipe.git_repository.getCodebrowseUrl(
-                    username=None, password=macaroon_raw)
+                    username=LAUNCHPAD_SERVICES, password=macaroon_raw)
                 args["git_repository"] = url
             else:
                 args["git_repository"] = (
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
index a29408b..d379a12 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -457,7 +457,7 @@ class TestAsyncOCIRecipeBuildBehaviour(
             "fast_cleanup": Is(True),
             "git_repository": AfterPreprocessing(urlsplit, MatchesStructure(
                 scheme=Equals(split_browse_root.scheme),
-                username=Equals(""),
+                username=Equals("+launchpad-services"),
                 password=AfterPreprocessing(
                     Macaroon.deserialize, MatchesStructure(
                         location=Equals(config.vhost.mainsite.hostname),
diff --git a/lib/lp/snappy/model/snapbuildbehaviour.py b/lib/lp/snappy/model/snapbuildbehaviour.py
index b6fd2ee..f74895e 100644
--- a/lib/lp/snappy/model/snapbuildbehaviour.py
+++ b/lib/lp/snappy/model/snapbuildbehaviour.py
@@ -25,6 +25,7 @@ from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
 from lp.buildmaster.model.buildfarmjobbehaviour import (
     BuildFarmJobBehaviourBase,
     )
+from lp.code.interfaces.codehosting import LAUNCHPAD_SERVICES
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
 from lp.services.features import getFeatureFlag
@@ -143,7 +144,7 @@ class SnapBuildBehaviour(BuilderProxyMixin, BuildFarmJobBehaviourBase):
             elif build.snap.git_repository.private:
                 macaroon_raw = yield self.issueMacaroon()
                 url = build.snap.git_repository.getCodebrowseUrl(
-                    username=None, password=macaroon_raw)
+                    username=LAUNCHPAD_SERVICES, password=macaroon_raw)
                 args["git_repository"] = url
             else:
                 args["git_repository"] = (
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index b5b1326..1d89091 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -461,7 +461,7 @@ class TestAsyncSnapBuildBehaviour(StatsMixin, TestSnapBuildBehaviourBase):
             "fast_cleanup": Is(True),
             "git_repository": AfterPreprocessing(urlsplit, MatchesStructure(
                 scheme=Equals(split_browse_root.scheme),
-                username=Equals(""),
+                username=Equals("+launchpad-services"),
                 password=AfterPreprocessing(
                     Macaroon.deserialize, MatchesStructure(
                         location=Equals(config.vhost.mainsite.hostname),