Support for bcrypt+hmac for password hashing (bug 639696)

Includes upgrade for existing sha512 passwords via
`./manage.py strengthen_user_passwords`.

Requires an HMAC_KEYS dict to be added to `settings_local.py`.

apps/users/management/commands/strengthen_user_passwords.py

@@ -0,0 +1,16 @@
+from django.conf import settings
+from django.core.management.base import NoArgsCommand
+from users.models import UserProfile
+
+
+class Command(NoArgsCommand):
+    requires_model_validation = False
+    output_transaction = True
+
+    def handle_noargs(self, **options):
+
+        if not settings.PWD_ALGORITHM == 'bcrypt':
+            return
+
+        for user in UserProfile.objects.all():
+            user.upgrade_password_to(algorithm='bcrypt')

apps/users/models.py

@@ -1,3 +1,4 @@
+import base64
 from datetime import datetime
 import hashlib
 import os
@@ -5,6 +6,9 @@ import random
 import re
 import string
 import time
+import hmac
+
+import bcrypt
 
 from django import forms, dispatch
 from django.conf import settings
@@ -28,6 +32,21 @@ from translations.query import order_by_translation
 log = commonware.log.getLogger('z.users')
 
 
+def _hmac_create(userpwd, shared_key):
+    """Create HMAC value based on pwd and system-local and per-user salt."""
+    hmac_value = base64.b64encode(hmac.new(
+        smart_str(shared_key), smart_str(userpwd), hashlib.sha512).digest())
+    return hmac_value
+
+
+def _bcrypt_create(hmac_value):
+    """Create bcrypt hash."""
+    rounds = getattr(settings, 'BCRYPT_ROUNDS', 12)
+    # No need for us to create a user salt, bcrypt creates its own.
+    bcrypt_value = bcrypt.hashpw(hmac_value, bcrypt.gensalt(int(rounds)))
+    return bcrypt_value
+
+
 def get_hexdigest(algorithm, salt, raw_password):
     return hashlib.new(algorithm, smart_str(salt + raw_password)).hexdigest()
 
@@ -37,9 +56,21 @@ def rand_string(length):
 
 
 def create_password(algorithm, raw_password):
-    salt = get_hexdigest(algorithm, rand_string(12), rand_string(12))[:64]
-    hsh = get_hexdigest(algorithm, salt, raw_password)
-    return '$'.join([algorithm, salt, hsh])
+    if algorithm == 'bcrypt':
+        try:
+            latest_key_id = max(settings.HMAC_KEYS.keys())
+        except (AttributeError, ValueError):
+            raise ValueError('settings.HMAC_KEYS must be set to a dict whose'
+                             'values are secret keys.')
+        shared_key = settings.HMAC_KEYS[latest_key_id]
+        hmac_value = _hmac_create(raw_password, shared_key)
+        return ''.join((
+                'bcrypt', _bcrypt_create(hmac_value),
+                '$', latest_key_id))
+    else:
+        salt = get_hexdigest(algorithm, rand_string(12), rand_string(12))[:64]
+        hsh = get_hexdigest(algorithm, salt, raw_password)
+        return '$'.join([algorithm, salt, hsh])
 
 
 class UserForeignKey(models.ForeignKey):
@@ -260,21 +291,47 @@ class UserProfile(amo.models.OnChangeMixin, amo.models.ModelBase):
             delete_user.delete()
 
     def check_password(self, raw_password):
-        if '$' not in self.password:
-            valid = (get_hexdigest('md5', '', raw_password) == self.password)
-            if valid:
-                # Upgrade an old password.
+        def passwords_match(algo_and_hash, key_ver, hmac_input):
+            shared_key = settings.HMAC_KEYS.get(key_ver, None)
+            if not shared_key:
+                log.info('Invalid shared key version "{0}"'.format(key_ver))
+                return False
+            bcrypt_value = algo_and_hash[6:]
+            hmac_value = _hmac_create(hmac_input, shared_key)
+            return bcrypt.hashpw(hmac_value, bcrypt_value) == bcrypt_value
+
+        if self.password.startswith('bcrypt'):
+            algo_and_hash, key_ver = self.password.rsplit('$', 1)
+            return passwords_match(algo_and_hash, key_ver, raw_password)
+        elif self.password.startswith('hh'):
+            alg, salt, bc_pwd = self.password.split('$', 3)[1:]
+            hsh = get_hexdigest(alg, salt, raw_password)
+            algo_and_hash, key_ver = bc_pwd.rsplit('$', 1)
+            if passwords_match(algo_and_hash, key_ver,
+                               '$'.join((alg, salt, hsh))):
+                #convert to bcrypt format
                 self.set_password(raw_password)
                 self.save()
-            return valid
+                return True
+        else:
+            algo, salt, hsh = self.password.split('$')
+            return hsh == get_hexdigest(algo, salt, raw_password)
 
-        algo, salt, hsh = self.password.split('$')
-        return hsh == get_hexdigest(algo, salt, raw_password)
-
-    def set_password(self, raw_password, algorithm='sha512'):
+    def set_password(self, raw_password, algorithm=None):
+        if algorithm is None:
+            algorithm = settings.PWD_ALGORITHM
         self.password = create_password(algorithm, raw_password)
         # Can't do CEF logging here because we don't have a request object.
 
+    def upgrade_password_to(self, algorithm):
+        if (not self.password or
+            self.password.startswith(('hh', 'bcrypt'))):
+            return
+        algo, salt, hsh = self.password.split('$')
+        bc_value = create_password('bcrypt', self.password)
+        self.password = u'$'.join(['hh', algo, salt, bc_value])
+        self.save()
+
     def email_confirmation_code(self):
         from amo.utils import send_mail
         log.debug("Sending account confirmation code for user (%s)", self)

apps/users/tests/test_models.py

@@ -6,6 +6,7 @@ from django import forms
 from django.contrib.auth.models import User
 from django.core import mail
 from django.utils import encoding
+from django.conf import settings
 
 from mock import patch
 from nose.tools import eq_
@@ -182,28 +183,70 @@ class TestUserProfile(amo.tests.TestCase):
 class TestPasswords(amo.tests.TestCase):
     utf = u'\u0627\u0644\u062a\u0637\u0628'
 
-    def test_invalid_old_password(self):
-        u = UserProfile(password=self.utf)
-        assert u.check_password(self.utf) is False
+    def test_invalid_sha512_password(self):
+        u = UserProfile()
+        u.set_password(self.utf, algorithm='sha512')
+        assert not u.check_password('wrong')
 
-    def test_invalid_new_password(self):
+    def test_valid_sha512_password(self):
+        u = UserProfile()
+        u.set_password(self.utf, algorithm='sha512')
+        assert u.check_password(self.utf)
+
+    def test_valid_bcrypt_password(self):
+        u = UserProfile()
+        u.set_password(self.utf, algorithm='bcrypt')
+        assert u.password.startswith('bcrypt')
+        assert u.check_password(self.utf)
+
+    def test_bcrypt_is_default(self):
         u = UserProfile()
         u.set_password(self.utf)
-        assert u.check_password('wrong') is False
+        assert u.password.startswith('bcrypt')
+        assert u.check_password(self.utf)
 
-    def test_valid_old_password(self):
-        hsh = hashlib.md5(encoding.smart_str(self.utf)).hexdigest()
-        u = UserProfile(password=hsh)
-        assert u.check_password(self.utf) is True
-        # Make sure we updated the old password.
-        algo, salt, hsh = u.password.split('$')
-        eq_(algo, 'sha512')
-        eq_(hsh, get_hexdigest(algo, salt, self.utf))
-
-    def test_valid_new_password(self):
+    def test_invalid_bcrypt_password(self):
         u = UserProfile()
-        u.set_password(self.utf)
-        assert u.check_password(self.utf) is True
+        u.set_password(self.utf, algorithm='bcrypt')
+        assert not u.check_password('wrong')
+
+    def test_valid_hh_password(self):
+        u = UserProfile()
+        u.set_password(self.utf, algorithm='sha512')
+        u.upgrade_password_to(algorithm='bcrypt')
+        assert u.password.startswith('hh$')
+        assert u.check_password(self.utf)
+
+    def test_already_bcrypt(self):
+        u = UserProfile()
+        u.set_password(self.utf, algorithm='bcrypt')
+        old_password = u.password
+        u.upgrade_password_to(algorithm='bcrypt')
+        eq_(old_password, u.password)
+
+    @patch.object(settings, 'HMAC_KEYS', {})
+    def test_no_hmac_key(self):
+        u = UserProfile()
+        with self.assertRaises(ValueError) as a:
+            u.set_password(self.utf, algorithm='bcrypt')
+        assert 'HMAC_KEYS' in a.exception.message
+
+    @patch.object(settings, 'HMAC_KEYS',
+                  {'2011-12-01': 'secret1'})
+    def test_stale_hmac_key_bcrypt(self):
+        u = UserProfile()
+        u.set_password(self.utf, algorithm='bcrypt')
+        settings.HMAC_KEYS = {'2011-12-02': 'secret2'}
+        assert not u.check_password(self.utf)
+
+    @patch.object(settings, 'HMAC_KEYS',
+                  {'2011-12-01': 'secret1'})
+    def test_stale_hmac_key_hh(self):
+        u = UserProfile()
+        u.set_password(self.utf, algorithm='sha512')
+        u.upgrade_password_to(algorithm='bcrypt')
+        settings.HMAC_KEYS = {'2011-12-02': 'secret2'}
+        assert not u.check_password(self.utf)
 
 
 class TestBlacklistedUsername(amo.tests.TestCase):

apps/zadmin/templates/zadmin/addon_manage.html

@@ -44,7 +44,7 @@
                     {{ hidden }}
                   {% endfor %}
                 </td>
-                <td><a href="{{ url('zadmin.recalc_hash', file.id)|urlparams(page=pager.number) }}" class="recalc" title="{{ file.hash }}">Recalc Hash</a></td>
+                <td><a href="{{ url('zadmin.recalc_hash', file.id)|urlparams(page=pager.number) }}" title="{{ file.hash }}">Recalc Hash</a></td>
                 <td>
                   {% if file.is_mirrorable() %}
                     {{ "Copied" if file.has_been_copied() else "Not Copied" }}

apps/zadmin/tests/test_views.py

@@ -1319,26 +1319,12 @@ class TestAddonManagement(amo.tests.TestCase):
         file = File.objects.create(
             filename='delicious_bookmarks-2.1.106-fx.xpi', version=version)
 
-        r = self.client.post(reverse('zadmin.recalc_hash', args=[file.id]))
-        eq_(json.loads(r.content)[u'success'], 1)
-
+        r = self.client.get(reverse('zadmin.recalc_hash', args=[file.id]),
+                           follow=True)
         file = File.objects.get(pk=file.id)
-
         assert file.size, 'File size should not be zero'
         assert file.hash, 'File hash should not be empty'
 
-    @mock.patch.object(File, 'file_path',
-                       amo.tests.AMOPaths().file_fixture_path(
-                           'delicious_bookmarks-2.1.106-fx.xpi'))
-    def test_regenerate_hash_get(self):
-        """ Don't allow GET """
-        version = Version.objects.create(addon_id=3615)
-        file = File.objects.create(
-            filename='delicious_bookmarks-2.1.106-fx.xpi', version=version)
-
-        r = self.client.get(reverse('zadmin.recalc_hash', args=[file.id]))
-        eq_(r.status_code, 405)  # GET out of here
-
 
 class TestJetpack(amo.tests.TestCase):
     fixtures = ['base/users']

apps/zadmin/views.py

@@ -628,8 +628,6 @@ def addon_manage(request, addon):
 
 
 @admin.site.admin_view
-@post_required
-@json_view
 def recalc_hash(request, file_id):
 
     file = get_object_or_404(File, pk=file_id)
@@ -640,4 +638,8 @@ def recalc_hash(request, file_id):
     log.info('Recalculated hash for file ID %d' % file.id)
     messages.success(request,
                      'File hash and size recalculated for file %d.' % file.id)
-    return {'success': 1}
+
+    redirect_url = reverse('zadmin.addon_manage',
+                           args=[file.version.addon.slug])
+    page = request.GET.get('page', 1)
+    return redirect(urlparams(redirect_url, page=page))

media/js/lib/jstestnet.js

@@ -28,17 +28,19 @@
     }
 
     window.onerror = function(error, url, lineNumber) {
+        var errorMsg = error.toString();
+        if (typeof error === 'object') {
+            // Trying to figure out what this mystery object is...
+            $.each(error, function(k, v) {
+                errorMsg += '; ' + k.toString() + '=' + v.toString();
+            });
+        }
         var msg = {
             action: 'log',
             result: false, // failure
-            message: 'Exception: ' + error.toString() + ' at ' + url + ':' + lineNumber,
+            message: 'Exception: ' + errorMsg + ' at ' + url + ':' + lineNumber,
             stacktrace: (typeof printStackTrace !== 'undefined') ? printStackTrace({e: error}): null
         };
-        if (typeof console !== 'undefined') {
-            // Get notified of exceptions during local development.
-            console.error('Exception:');
-            console.log(error);
-        }
         postMsg(msg);
     };
 

media/js/zamboni/admin.js

@@ -34,21 +34,5 @@
                 // to your page. TODO: do something clever with this.
             }));
         });
-
-        // Recalculate Hash
-        $('.recalc').click(_pd(function() {
-            var $this = $(this);
-            $this.html('Recalcing…');
-            $.post($this.attr('href'), function(d) {
-                if(d.success) {
-                    $this.text('Done!');
-                } else {
-                    $this.text('Error :(');
-                }
-                setTimeout(function() {
-                    $this.text('Recalc Hash');
-                }, 2000);
-            });
-        }));
     });
 })();

settings.py

@@ -1356,3 +1356,17 @@ IGNORE_NON_CRITICAL_CRONS = False
 # To enable new default to compatible checks in services/update.py set to True.
 # Set to False in case of emergency to switch back to old code.
 DEFAULT_TO_COMPATIBLE = True
+
+
+# Use bcrypt for new passwords, and enable upgrade of old ones.
+PWD_ALGORITHM = 'bcrypt'
+
+# Add an HMAC_KEYS dict in your local settings, containing identifiers
+# as keys and secrets as values. The largest key will be put as
+# plaintext into the hashed password field, and the corresponding
+# value will be used as the secret for HMAC hashing in passwords. For
+# example:
+
+# HMAC_KEYS = {'2011-12-01': 'ChristmasIsMagic',
+#              '2012-01-01': 'PotchHasAUnicornRanch',
+#             }