Merge ~cjwatson/launchpad-buildd:librarian-macaroons into launchpad-buildd:master

Proposed by Colin Watson
Status: Merged
Approved by: Colin Watson
Approved revision: 107aeec724037de874fe9d6be68f8f067bfc350a
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad-buildd:librarian-macaroons
Merge into: launchpad-buildd:master
Diff against target: 82 lines (+31/-4)
3 files modified
debian/changelog (+2/-0)
lpbuildd/builder.py (+6/-4)
lpbuildd/tests/test_buildd.py (+23/-0)
Reviewer Review Type Date Requested Status
Jürgen Gmach Approve
Review via email: mp+430026@code.launchpad.net

Commit message

Fix handling of librarian macaroons

Description of the change

https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/429703 causes Launchpad to send authenticated librarian URLs to builders for private source packages. However, this needs a couple of fixes in launchpad-buildd before it works.

Firstly, these macaroons are sent with an empty username, so we need to send authentication if either the username or password is non-empty rather than only if the username is non-empty.

Secondly, the librarian just sends 404 on authorization failures to avoid revealing information, so we have to tell `urllib.request` to send authentication credentials up-front (which is faster anyway) rather than waiting for a 401.

To post a comment you must log in.
Revision history for this message
Jürgen Gmach (jugmac00) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index aa2253a..405000b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,6 +1,8 @@
1launchpad-buildd (222) UNRELEASED; urgency=medium1launchpad-buildd (222) UNRELEASED; urgency=medium
22
3 * Remove use of six.3 * Remove use of six.
4 * Fix handling of librarian macaroons, where the username is empty and we
5 must use prior authentication.
46
5 -- Colin Watson <cjwatson@ubuntu.com> Mon, 12 Sep 2022 09:50:13 +01007 -- Colin Watson <cjwatson@ubuntu.com> Mon, 12 Sep 2022 09:50:13 +0100
68
diff --git a/lpbuildd/builder.py b/lpbuildd/builder.py
index 2f256ad..332d060 100644
--- a/lpbuildd/builder.py
+++ b/lpbuildd/builder.py
@@ -17,7 +17,7 @@ import tempfile
17from urllib.request import (17from urllib.request import (
18 build_opener,18 build_opener,
19 HTTPBasicAuthHandler,19 HTTPBasicAuthHandler,
20 HTTPPasswordMgrWithDefaultRealm,20 HTTPPasswordMgrWithPriorAuth,
21 urlopen,21 urlopen,
22 )22 )
23from xmlrpc.client import Binary23from xmlrpc.client import Binary
@@ -430,8 +430,10 @@ class Builder:
430 This helper installs an HTTPBasicAuthHandler that will deal with any430 This helper installs an HTTPBasicAuthHandler that will deal with any
431 HTTP basic authentication required when opening the URL.431 HTTP basic authentication required when opening the URL.
432 """432 """
433 password_mgr = HTTPPasswordMgrWithDefaultRealm()433 password_mgr = HTTPPasswordMgrWithPriorAuth()
434 password_mgr.add_password(None, url, username, password)434 password_mgr.add_password(
435 None, url, username, password, is_authenticated=True
436 )
435 handler = HTTPBasicAuthHandler(password_mgr)437 handler = HTTPBasicAuthHandler(password_mgr)
436 opener = build_opener(handler)438 opener = build_opener(handler)
437 return opener439 return opener
@@ -449,7 +451,7 @@ class Builder:
449 extra_info = 'Cache'451 extra_info = 'Cache'
450 if not os.path.exists(cachefile):452 if not os.path.exists(cachefile):
451 self.log(f'Fetching {sha1sum} by url {url}')453 self.log(f'Fetching {sha1sum} by url {url}')
452 if username:454 if username or password:
453 opener = self.setupAuthHandler(455 opener = self.setupAuthHandler(
454 url, username, password).open456 url, username, password).open
455 else:457 else:
diff --git a/lpbuildd/tests/test_buildd.py b/lpbuildd/tests/test_buildd.py
index 9eb421d..f97f396 100644
--- a/lpbuildd/tests/test_buildd.py
+++ b/lpbuildd/tests/test_buildd.py
@@ -63,6 +63,29 @@ class LaunchpadBuilddTests(BuilddTestCase):
63 self.assertEqual(user, stored_user)63 self.assertEqual(user, stored_user)
64 self.assertEqual(password, stored_pass)64 self.assertEqual(password, stored_pass)
6565
66 def testBasicAuthEmptyUser(self):
67 """An empty username is used in conjunction with macaroons."""
68 url = "http://fakeurl/"
69 user = ""
70 password = "fakepassword"
71
72 opener = self.builder.setupAuthHandler(url, user, password)
73
74 # Inspect the openers and ensure the wanted handler is installed.
75 basic_auth_handler = None
76 for handler in opener.handlers:
77 if isinstance(handler, HTTPBasicAuthHandler):
78 basic_auth_handler = handler
79 break
80 self.assertIsNotNone(
81 basic_auth_handler, "No basic auth handler installed.")
82
83 password_mgr = basic_auth_handler.passwd
84 stored_user, stored_pass = password_mgr.find_user_password(None, url)
85 self.assertEqual(user, stored_user)
86 self.assertEqual(password, stored_pass)
87 self.assertTrue(password_mgr.is_authenticated(url))
88
66 def testBuildlogScrubbing(self):89 def testBuildlogScrubbing(self):
67 """Tests the buildlog scrubbing (removal of passwords from URLs)."""90 """Tests the buildlog scrubbing (removal of passwords from URLs)."""
68 # This is where the buildlog file lives.91 # This is where the buildlog file lives.

Subscribers

People subscribed via source and target branches