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
1diff --git a/debian/changelog b/debian/changelog
2index aa2253a..405000b 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,6 +1,8 @@
6 launchpad-buildd (222) UNRELEASED; urgency=medium
7
8 * Remove use of six.
9+ * Fix handling of librarian macaroons, where the username is empty and we
10+ must use prior authentication.
11
12 -- Colin Watson <cjwatson@ubuntu.com> Mon, 12 Sep 2022 09:50:13 +0100
13
14diff --git a/lpbuildd/builder.py b/lpbuildd/builder.py
15index 2f256ad..332d060 100644
16--- a/lpbuildd/builder.py
17+++ b/lpbuildd/builder.py
18@@ -17,7 +17,7 @@ import tempfile
19 from urllib.request import (
20 build_opener,
21 HTTPBasicAuthHandler,
22- HTTPPasswordMgrWithDefaultRealm,
23+ HTTPPasswordMgrWithPriorAuth,
24 urlopen,
25 )
26 from xmlrpc.client import Binary
27@@ -430,8 +430,10 @@ class Builder:
28 This helper installs an HTTPBasicAuthHandler that will deal with any
29 HTTP basic authentication required when opening the URL.
30 """
31- password_mgr = HTTPPasswordMgrWithDefaultRealm()
32- password_mgr.add_password(None, url, username, password)
33+ password_mgr = HTTPPasswordMgrWithPriorAuth()
34+ password_mgr.add_password(
35+ None, url, username, password, is_authenticated=True
36+ )
37 handler = HTTPBasicAuthHandler(password_mgr)
38 opener = build_opener(handler)
39 return opener
40@@ -449,7 +451,7 @@ class Builder:
41 extra_info = 'Cache'
42 if not os.path.exists(cachefile):
43 self.log(f'Fetching {sha1sum} by url {url}')
44- if username:
45+ if username or password:
46 opener = self.setupAuthHandler(
47 url, username, password).open
48 else:
49diff --git a/lpbuildd/tests/test_buildd.py b/lpbuildd/tests/test_buildd.py
50index 9eb421d..f97f396 100644
51--- a/lpbuildd/tests/test_buildd.py
52+++ b/lpbuildd/tests/test_buildd.py
53@@ -63,6 +63,29 @@ class LaunchpadBuilddTests(BuilddTestCase):
54 self.assertEqual(user, stored_user)
55 self.assertEqual(password, stored_pass)
56
57+ def testBasicAuthEmptyUser(self):
58+ """An empty username is used in conjunction with macaroons."""
59+ url = "http://fakeurl/"
60+ user = ""
61+ password = "fakepassword"
62+
63+ opener = self.builder.setupAuthHandler(url, user, password)
64+
65+ # Inspect the openers and ensure the wanted handler is installed.
66+ basic_auth_handler = None
67+ for handler in opener.handlers:
68+ if isinstance(handler, HTTPBasicAuthHandler):
69+ basic_auth_handler = handler
70+ break
71+ self.assertIsNotNone(
72+ basic_auth_handler, "No basic auth handler installed.")
73+
74+ password_mgr = basic_auth_handler.passwd
75+ stored_user, stored_pass = password_mgr.find_user_password(None, url)
76+ self.assertEqual(user, stored_user)
77+ self.assertEqual(password, stored_pass)
78+ self.assertTrue(password_mgr.is_authenticated(url))
79+
80 def testBuildlogScrubbing(self):
81 """Tests the buildlog scrubbing (removal of passwords from URLs)."""
82 # This is where the buildlog file lives.

Subscribers

People subscribed via source and target branches