Add new config-option 'migrate_passwords' in section 'web'...

...to auto-migrate passwords at web-login time. Default for the new
option is "yes" so if you don't want that passwords are auto-migrated
to a more secure password scheme on user login, set this to "no"
before running your tracker(s) after the upgrade.

Author: Ralf Schlatterbeck

CHANGES.txt

@@ -79,6 +79,11 @@ Fixed:
 - Fix Password handling security issue2550688 (thanks Joseph Myers for
   reporting and Eli Collins for fixing) -- this fixes all observations
   by Joseph Myers except for auto-migration of existing passwords.
+- Add new config-option 'migrate_passwords' in section 'web' to
+  auto-migrate passwords at web-login time. Default for the new option
+  is "yes" so if you don't want that passwords are auto-migrated to a
+  more secure password scheme on user login, set this to "no" before
+  running your tracker(s) after the upgrade.
 
 2010-10-08 1.4.16 (r4541)
 

doc/upgrading.txt

@@ -16,6 +16,13 @@ steps.
 Migrating from 1.4.x to 1.4.17
 ==============================
 
+There is a new config-option 'migrate_passwords' in section 'web' to
+auto-migrate passwords at web-login time to a more secure storage
+scheme. Default for the new option is "yes" so if you don't want that
+passwords are auto-migrated to a more secure password scheme on user
+login, set this to "no" before running your tracker(s) after the
+upgrade.
+
 Searching now requires either read-permission without a check method, or
 you will have to add a "Search" permission for a class or a list of
 properties for a class (if you want to allow searching). For the classic

roundup/cgi/actions.py

@@ -1005,12 +1005,18 @@ def verifyLogin(self, username, password):
             raise exceptions.LoginError(self._(
                 "You do not have permission to login"))
 
-    def verifyPassword(self, userid, password):
-        '''Verify the password that the user has supplied'''
-        stored = self.db.user.get(userid, 'password')
-        if password == stored:
+    def verifyPassword(self, userid, givenpw):
+        '''Verify the password that the user has supplied.
+           Optionally migrate to new password scheme if configured
+        '''
+        db = self.db
+        stored = db.user.get(userid, 'password')
+        if givenpw == stored:
+            if db.config.WEB_MIGRATE_PASSWORDS and stored.needs_migration():
+                db.user.set(userid, password=password.Password(givenpw))
+                db.commit()
             return 1
-        if not password and not stored:
+        if not givenpw and not stored:
             return 1
         return 0
 

roundup/configuration.py

@@ -579,6 +579,10 @@ def str2value(self, value):
             "Setting this option makes Roundup display error tracebacks\n"
             "in the user's browser rather than emailing them to the\n"
             "tracker admin."),
+        (BooleanOption, "migrate_passwords", "yes",
+            "Setting this option makes Roundup migrate passwords with\n"
+            "an insecure password-scheme to a more secure scheme\n"
+            "when the user logs in via the web-interface."),
     )),
     ("rdbms", (
         (Option, 'name', 'roundup',

roundup/password.py

@@ -116,27 +116,33 @@ class PasswordValueError(ValueError):
     """ The password value is not valid """
     pass
 
+def pbkdf2_unpack(pbkdf2):
+    """ unpack pbkdf2 encrypted password into parts,
+        assume it has format "{rounds}${salt}${digest}
+    """
+    if isinstance(pbkdf2, unicode):
+        pbkdf2 = pbkdf2.encode("ascii")
+    try:
+        rounds, salt, digest = pbkdf2.split("$")
+    except ValueError:
+        raise PasswordValueError, "invalid PBKDF2 hash (wrong number of separators)"
+    if rounds.startswith("0"):
+        raise PasswordValueError, "invalid PBKDF2 hash (zero-padded rounds)"
+    try:
+        rounds = int(rounds)
+    except ValueError:
+        raise PasswordValueError, "invalid PBKDF2 hash (invalid rounds)"
+    raw_salt = h64decode(salt)
+    return rounds, salt, raw_salt, digest
+
 def encodePassword(plaintext, scheme, other=None):
     """Encrypt the plaintext password.
     """
     if plaintext is None:
         plaintext = ""
     if scheme == "PBKDF2":
         if other:
-            #assume it has format "{rounds}${salt}${digest}"
-            if isinstance(other, unicode):
-                other = other.encode("ascii")
-            try:
-                rounds, salt, digest = other.split("$")
-            except ValueError:
-                raise PasswordValueError, "invalid PBKDF2 hash (wrong number of separators)"
-            if rounds.startswith("0"):
-                raise PasswordValueError, "invalid PBKDF2 hash (zero-padded rounds)"
-            try:
-                rounds = int(rounds)
-            except ValueError:
-                raise PasswordValueError, "invalid PBKDF2 hash (invalid rounds)"
-            raw_salt = h64decode(salt)
+            rounds, salt, raw_salt, digest = pbkdf2_unpack(other)
         else:
             raw_salt = getrandbytes(20)
             salt = h64encode(raw_salt)
@@ -249,6 +255,17 @@ def __init__(self, plaintext=None, scheme=None, encrypted=None, strict=False):
             self.password = None
             self.plaintext = None
 
+    def needs_migration(self):
+        """ Password has insecure scheme or other insecure parameters
+            and needs migration to new password scheme
+        """
+        if self.scheme != 'PBKDF2':
+            return True
+        rounds, salt, raw_salt, digest = pbkdf2_unpack(self.password)
+        if rounds < 1000:
+            return True
+        return False
+
     def unpack(self, encrypted, scheme=None, strict=False):
         """Set the password info from the scheme:<encryted info> string
            (the inverse of __str__)

test/test_cgi.py

@@ -425,6 +425,30 @@ def testEmptyPasswordNotSet(self):
             ':confirm:password': ''}, 'user', nodeid),
             ({('user', nodeid): {}}, []))
 
+    def testPasswordMigration(self):
+        chef = self.db.user.lookup('Chef')
+        form = dict(__login_name='Chef', __login_password='foo')
+        cl = self._make_client(form)
+        # assume that the "best" algorithm is the first one and doesn't
+        # need migration, all others should be migrated.
+        for scheme in password.Password.known_schemes[1:]:
+            pw1 = password.Password('foo', scheme=scheme)
+            self.assertEqual(pw1.needs_migration(), True)
+            self.db.user.set(chef, password=pw1)
+            self.db.commit()
+            actions.LoginAction(cl).handle()
+            pw = self.db.user.get(chef, 'password')
+            self.assertEqual(pw, 'foo')
+            self.assertEqual(pw.needs_migration(), False)
+        pw1 = pw
+        self.assertEqual(pw1.needs_migration(), False)
+        scheme = password.Password.known_schemes[0]
+        self.assertEqual(scheme, pw1.scheme)
+        actions.LoginAction(cl).handle()
+        pw = self.db.user.get(chef, 'password')
+        self.assertEqual(pw, 'foo')
+        self.assertEqual(pw, pw1)
+
     #
     # Boolean
     #