launchpad-reviewers team mailing list archive

[Colin Watson <mp+430026@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad-buildd:librarian-macaroons into launchpad-buildd:master.

Commit message:
Fix handling of librarian macaroons

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/430026

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-buildd:librarian-macaroons into launchpad-buildd:master.

diff --git a/debian/changelog b/debian/changelog
index aa2253a..405000b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,6 +1,8 @@
 launchpad-buildd (222) UNRELEASED; urgency=medium
 
   * Remove use of six.
+  * Fix handling of librarian macaroons, where the username is empty and we
+    must use prior authentication.
 
  -- Colin Watson <cjwatson@xxxxxxxxxx>  Mon, 12 Sep 2022 09:50:13 +0100
 
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
 from urllib.request import (
     build_opener,
     HTTPBasicAuthHandler,
-    HTTPPasswordMgrWithDefaultRealm,
+    HTTPPasswordMgrWithPriorAuth,
     urlopen,
     )
 from xmlrpc.client import Binary
@@ -430,8 +430,10 @@ class Builder:
         This helper installs an HTTPBasicAuthHandler that will deal with any
         HTTP basic authentication required when opening the URL.
         """
-        password_mgr = HTTPPasswordMgrWithDefaultRealm()
-        password_mgr.add_password(None, url, username, password)
+        password_mgr = HTTPPasswordMgrWithPriorAuth()
+        password_mgr.add_password(
+            None, url, username, password, is_authenticated=True
+        )
         handler = HTTPBasicAuthHandler(password_mgr)
         opener = build_opener(handler)
         return opener
@@ -449,7 +451,7 @@ class Builder:
             extra_info = 'Cache'
             if not os.path.exists(cachefile):
                 self.log(f'Fetching {sha1sum} by url {url}')
-                if username:
+                if username or password:
                     opener = self.setupAuthHandler(
                         url, username, password).open
                 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):
         self.assertEqual(user, stored_user)
         self.assertEqual(password, stored_pass)
 
+    def testBasicAuthEmptyUser(self):
+        """An empty username is used in conjunction with macaroons."""
+        url = "http://fakeurl/";
+        user = ""
+        password = "fakepassword"
+
+        opener = self.builder.setupAuthHandler(url, user, password)
+
+        # Inspect the openers and ensure the wanted handler is installed.
+        basic_auth_handler = None
+        for handler in opener.handlers:
+            if isinstance(handler, HTTPBasicAuthHandler):
+                basic_auth_handler = handler
+                break
+        self.assertIsNotNone(
+            basic_auth_handler, "No basic auth handler installed.")
+
+        password_mgr = basic_auth_handler.passwd
+        stored_user, stored_pass = password_mgr.find_user_password(None, url)
+        self.assertEqual(user, stored_user)
+        self.assertEqual(password, stored_pass)
+        self.assertTrue(password_mgr.is_authenticated(url))
+
     def testBuildlogScrubbing(self):
         """Tests the buildlog scrubbing (removal of passwords from URLs)."""
         # This is where the buildlog file lives.