launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/htpasswd-salt into lp:launchpad.

Commit message:
Ensure that PPA .htpasswd salts are drawn from the correct alphabet.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1722209 in Launchpad itself: "crypt.crypt() changed in Xenial causing incorrectly generated .htpasswd entries"
  https://bugs.launchpad.net/launchpad/+bug/1722209

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/htpasswd-salt/+merge/332013

This is all a horrible hack, but I think it'll do until we have a better authentication system.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/htpasswd-salt into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/htaccess.py'
--- lib/lp/archivepublisher/htaccess.py	2016-07-13 16:42:34 +0000
+++ lib/lp/archivepublisher/htaccess.py	2017-10-09 12:45:58 +0000
@@ -1,6 +1,6 @@
 #!/usr/bin/python
 #
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Writing of htaccess and htpasswd files."""
@@ -13,7 +13,7 @@
     'write_htpasswd',
     ]
 
-
+import base64
 import crypt
 from operator import itemgetter
 import os
@@ -65,6 +65,21 @@
         file.close()
 
 
+# XXX cjwatson 2017-10-09: This whole mechanism of writing password files to
+# disk (as opposed to e.g. using a WSGI authentication provider that checks
+# passwords against the database) is terrible, but as long as we're using it
+# we should use something like bcrypt rather than DES-based crypt.
+def make_salt(s):
+    """Produce a salt from an input string.
+
+    This ensures that salts are drawn from the correct alphabet
+    ([./a-zA-Z0-9]).
+    """
+    # As long as the input string is at least one character long, there will
+    # be no padding within the first two characters.
+    return base64.b64encode((s or " ").encode("UTF-8"), altchars=b"./")[:2]
+
+
 def htpasswd_credentials_for_archive(archive):
     """Return credentials for an archive for use with write_htpasswd.
 
@@ -94,10 +109,11 @@
     for person_id, token_name, token in tokens:
         if token_name:
             # A named auth token.
-            output.append(('+' + token_name, token, token_name[:2]))
+            output.append(('+' + token_name, token, make_salt(token_name)))
         else:
             # A subscription auth token.
-            output.append((names[person_id], token, names[person_id][:2]))
+            output.append(
+                (names[person_id], token, make_salt(names[person_id])))
 
     # The first .htpasswd entry is the buildd_secret.
     yield (BUILDD_USER_NAME, archive.buildd_secret, BUILDD_USER_NAME[:2])

=== modified file 'lib/lp/archivepublisher/tests/test_htaccess.py'
--- lib/lp/archivepublisher/tests/test_htaccess.py	2016-07-13 16:42:34 +0000
+++ lib/lp/archivepublisher/tests/test_htaccess.py	2017-10-09 12:45:58 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test htaccess/htpasswd file generation. """
@@ -111,23 +111,27 @@
         self.ppa.buildd_secret = "geheim"
         name12 = getUtility(IPersonSet).getByName("name12")
         name16 = getUtility(IPersonSet).getByName("name16")
+        hyphenated = self.factory.makePerson(name="a-b-c")
         self.ppa.newSubscription(name12, self.ppa.owner)
         self.ppa.newSubscription(name16, self.ppa.owner)
+        self.ppa.newSubscription(hyphenated, self.ppa.owner)
         first_created_token = self.ppa.newAuthToken(name16)
         second_created_token = self.ppa.newAuthToken(name12)
+        third_created_token = self.ppa.newAuthToken(hyphenated)
         named_token_20 = self.ppa.newNamedAuthToken(u"name20", as_dict=False)
         named_token_14 = self.ppa.newNamedAuthToken(u"name14", as_dict=False)
         named_token_99 = self.ppa.newNamedAuthToken(u"name99", as_dict=False)
         named_token_99.deactivate()
 
+        expected_credentials = [
+            ("buildd", "geheim", "bu"),
+            ("+name14", named_token_14.token, "bm"),
+            ("+name20", named_token_20.token, "bm"),
+            ("a-b-c", third_created_token.token, "YS"),
+            ("name12", second_created_token.token, "bm"),
+            ("name16", first_created_token.token, "bm"),
+            ]
         credentials = list(htpasswd_credentials_for_archive(self.ppa))
 
         # Use assertEqual instead of assertContentEqual to verify order.
-        self.assertEqual(
-            credentials, [
-                ("buildd", "geheim", "bu"),
-                ("+name14", named_token_14.token, "na"),
-                ("+name20", named_token_20.token, "na"),
-                ("name12", second_created_token.token, "na"),
-                ("name16", first_created_token.token, "na"),
-                ])
+        self.assertEqual(expected_credentials, credentials)