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.

git-svn-id: http://svn.roundup-tracker.org/svnroot/roundup/roundup/trunk@4593 57a73879-2fb5-44c3-a270-3262357dd7e2

diff --git a/CHANGES.txt b/CHANGES.txt
index 8e167541bd84a7a71e30ed1f3e1967fb710927f2..4fe82cfcbe7d2afca9d908983087054c920534d1 100644 (file)
--- a/CHANGES.txt
+++ b/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)
 

diff --git a/doc/upgrading.txt b/doc/upgrading.txt
index 43051a169a2149b2fbdcefcd22fe299a1ea30ad6..a1bd77f9f18ad0cc0b9017c2ac9a51b780178edd 100644 (file)
--- a/doc/upgrading.txt
+++ b/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

diff --git a/roundup/cgi/actions.py b/roundup/cgi/actions.py
index 40bbfdabc63020b19f56fafca82620a6b49c6fd5..41f5979467dee8b5dceed43bdc0cd8298dd08fcf 100755 (executable)
--- a/roundup/cgi/actions.py
+++ b/roundup/cgi/actions.py
@@ -1005,12 +1005,18 @@ class LoginAction(Action):
             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
 

diff --git a/roundup/configuration.py b/roundup/configuration.py
index 8b862627d1d9f5a84192c11b9689f78a9013bc21..9a8d165c60e549f76b15b56852078705082f89f2 100644 (file)
--- a/roundup/configuration.py
+++ b/roundup/configuration.py
@@ -579,6 +579,10 @@ SETTINGS = (
             "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',

diff --git a/roundup/password.py b/roundup/password.py
index 545baeb66690603808961c02c6617cc8ea84e03b..92ada54a1fa8756e2d62ad6e9d5580efcbcad3ad 100644 (file)
--- a/roundup/password.py
+++ b/roundup/password.py
@@ -116,6 +116,25 @@ 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.
     """
@@ -123,20 +142,7 @@ def encodePassword(plaintext, scheme, other=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 @@ class Password(JournalPassword):
             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__)

diff --git a/test/test_cgi.py b/test/test_cgi.py
index b8371bd3e2defd985843fec86b9764057838a442..17e2f376d35534a2ee4bbfa19ddb3f9e1c2dd056 100644 (file)
--- a/test/test_cgi.py
+++ b/test/test_cgi.py
@@ -425,6 +425,30 @@ class FormTestCase(unittest.TestCase):
             ':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
     #