launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #27462
[Merge] ~cjwatson/launchpad:git-auth-launchpad-services into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:git-auth-launchpad-services into launchpad:master.
Commit message:
Authenticate using +launchpad-services rather than empty username
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1940804 in Launchpad itself: "Private core18 snap builds fail to clone git repositories"
https://bugs.launchpad.net/launchpad/+bug/1940804
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/407532
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:git-auth-launchpad-services into launchpad:master.
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),
