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

This reverts commit a6071970a7.

This introduced some unexpected changes too soon to deploy.
Once we deploy, some questions are answered, and it's hidden behind a flag
we can re-introduce it.

apps/users/management/commands/strengthen_user_passwords.py

@@ -1,16 +0,0 @@
-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,4 +1,3 @@
-import base64
 from datetime import datetime
 import hashlib
 import os
@@ -6,9 +5,6 @@ import random
 import re
 import string
 import time
-import hmac
-
-import bcrypt
 
 from django import forms, dispatch
 from django.conf import settings
@@ -32,21 +28,6 @@ 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()
 
@@ -56,21 +37,9 @@ def rand_string(length):
 
 
 def create_password(algorithm, raw_password):
-    if algorithm == 'bcrypt':
+    salt = get_hexdigest(algorithm, rand_string(12), rand_string(12))[:64]
-        try:
+    hsh = get_hexdigest(algorithm, salt, raw_password)
-            latest_key_id = max(settings.HMAC_KEYS.keys())
+    return '$'.join([algorithm, salt, hsh])
-        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):
@@ -291,47 +260,21 @@ class UserProfile(amo.models.OnChangeMixin, amo.models.ModelBase):
             delete_user.delete()
 
     def check_password(self, raw_password):
-        def passwords_match(algo_and_hash, key_ver, hmac_input):
+        if '$' not in self.password:
-            shared_key = settings.HMAC_KEYS.get(key_ver, None)
+            valid = (get_hexdigest('md5', '', raw_password) == self.password)
-            if not shared_key:
+            if valid:
-                log.info('Invalid shared key version "{0}"'.format(key_ver))
+                # Upgrade an old password.
-                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 True
+            return valid
-        else:
-            algo, salt, hsh = self.password.split('$')
-            return hsh == get_hexdigest(algo, salt, raw_password)
 
-    def set_password(self, raw_password, algorithm=None):
+        algo, salt, hsh = self.password.split('$')
-        if algorithm is None:
+        return hsh == get_hexdigest(algo, salt, raw_password)
-            algorithm = settings.PWD_ALGORITHM
+
+    def set_password(self, raw_password, algorithm='sha512'):
         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,7 +6,6 @@ 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_
@@ -183,70 +182,28 @@ class TestUserProfile(amo.tests.TestCase):
 class TestPasswords(amo.tests.TestCase):
     utf = u'\u0627\u0644\u062a\u0637\u0628'
 
-    def test_invalid_sha512_password(self):
+    def test_invalid_old_password(self):
-        u = UserProfile()
+        u = UserProfile(password=self.utf)
-        u.set_password(self.utf, algorithm='sha512')
+        assert u.check_password(self.utf) is False
-        assert not u.check_password('wrong')
 
-    def test_valid_sha512_password(self):
+    def test_invalid_new_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.password.startswith('bcrypt')
+        assert u.check_password('wrong') is False
-        assert u.check_password(self.utf)
 
-    def test_invalid_bcrypt_password(self):
+    def test_valid_old_password(self):
-        u = UserProfile()
+        hsh = hashlib.md5(encoding.smart_str(self.utf)).hexdigest()
-        u.set_password(self.utf, algorithm='bcrypt')
+        u = UserProfile(password=hsh)
-        assert not u.check_password('wrong')
+        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_hh_password(self):
+    def test_valid_new_password(self):
         u = UserProfile()
-        u.set_password(self.utf, algorithm='sha512')
+        u.set_password(self.utf)
-        u.upgrade_password_to(algorithm='bcrypt')
+        assert u.check_password(self.utf) is True
-        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) }}" title="{{ file.hash }}">Recalc Hash</a></td>
+                <td><a href="{{ url('zadmin.recalc_hash', file.id)|urlparams(page=pager.number) }}" class="recalc" 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,12 +1319,26 @@ class TestAddonManagement(amo.tests.TestCase):
         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]),
+        r = self.client.post(reverse('zadmin.recalc_hash', args=[file.id]))
-                           follow=True)
+        eq_(json.loads(r.content)[u'success'], 1)
+
         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,6 +628,8 @@ 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)
@@ -638,8 +640,4 @@ 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/zamboni/admin.js

@@ -34,5 +34,21 @@
                 // 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,17 +1356,3 @@ 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',
-#             }