allura-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From brond...@apache.org
Subject [allura] branch master updated: [#8274] Add optional HaveIBeenPwned checks for password changes
Date Wed, 10 Apr 2019 16:55:41 GMT
This is an automated email from the ASF dual-hosted git repository.

brondsem pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/allura.git


The following commit(s) were added to refs/heads/master by this push:
     new d75190e  [#8274] Add optional HaveIBeenPwned checks for password changes
d75190e is described below

commit d75190e886aba6dd21dad171d4f7845a31270d81
Author: Kenton Taylor <ktaylor@slashdotmedia.com>
AuthorDate: Thu Apr 4 14:44:53 2019 +0000

    [#8274] Add optional HaveIBeenPwned checks for password changes
---
 Allura/allura/controllers/auth.py           | 25 ++++++++-
 Allura/allura/lib/plugin.py                 |  3 ++
 Allura/allura/lib/security.py               | 73 ++++++++++++++++++++++++++
 Allura/allura/tests/functional/test_auth.py | 80 +++++++++++++++++++++++++++++
 Allura/development.ini                      |  4 ++
 Allura/docker-dev.ini                       |  1 -
 6 files changed, 184 insertions(+), 2 deletions(-)

diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py
index 008de84..9273147 100644
--- a/Allura/allura/controllers/auth.py
+++ b/Allura/allura/controllers/auth.py
@@ -41,6 +41,7 @@ from allura.lib import plugin
 from allura.lib.decorators import require_post, reconfirm_auth
 from allura.lib.exceptions import InvalidRecoveryCode, MultifactorRateLimitError
 from allura.lib.repository import RepositoryApp
+from allura.lib.security import HIBPClient, HIBPCompromisedCredentials, HIBPClientError
 from allura.lib.widgets import (
     SubscriptionForm,
     OAuthApplicationForm,
@@ -85,6 +86,19 @@ class F(object):
     disable_account_form = DisableAccountForm()
 
 
+def enforce_hibp_password_check(provider, password, failure_redirect_url):
+    if provider.hibp_password_check_enabled():
+        try:
+            HIBPClient.check_breached_password(password)
+
+        except HIBPClientError as ex:
+            log.error("Error invoking HIBP API", exc_info=ex)
+
+        except HIBPCompromisedCredentials:
+            flash('Unsafe Password - Please use a strong, unique password', 'error')
+            redirect(failure_redirect_url)
+
+
 class AuthController(BaseController):
 
     def __init__(self):
@@ -175,6 +189,8 @@ class AuthController(BaseController):
         if not provider.forgotten_password_process:
             raise wexc.HTTPNotFound()
         user = self._validate_hash(hash)
+        enforce_hibp_password_check(provider, pw, '/auth/forgotten_password/{}'.format(hash))
+
         user.set_password(pw)
         user.set_tool_data('AuthPasswordReset', hash='', hash_expiry='')  # Clear password
reset token
         user.set_tool_data('allura', pwd_reset_preserve_session=session.id)
@@ -487,6 +503,10 @@ class AuthController(BaseController):
         require_authenticated()
         return_to = kw.get('return_to')
         ap = plugin.AuthenticationProvider.get(request)
+        failure_redirect_url = tg.url('/auth/pwd_expired', dict(return_to=return_to))
+
+        enforce_hibp_password_check(ap, kw['pw'], failure_redirect_url)
+
         try:
             expired_username = session.get('expired-username')
             expired_user = M.User.query.get(username=expired_username) if expired_username
else None
@@ -496,7 +516,8 @@ class AuthController(BaseController):
 
         except wexc.HTTPUnauthorized:
             flash('Incorrect password', 'error')
-            redirect(tg.url('/auth/pwd_expired', dict(return_to=return_to)))
+            redirect(failure_redirect_url)
+
         flash('Password changed')
         session.pop('pwd-expired', None)
         session['username'] = session.get('expired-username')
@@ -646,6 +667,8 @@ class PreferencesController(BaseController):
     def change_password(self, **kw):
         ap = plugin.AuthenticationProvider.get(request)
         try:
+            enforce_hibp_password_check(ap, kw['pw'], '.')
+
             ap.set_password(c.user, kw['oldpw'], kw['pw'])
             session['_id'] = _session_id()  # new one so even if this session had been intercepted
somehow, its invalid
             session.save()
diff --git a/Allura/allura/lib/plugin.py b/Allura/allura/lib/plugin.py
index f64c8ca..8d2434e 100644
--- a/Allura/allura/lib/plugin.py
+++ b/Allura/allura/lib/plugin.py
@@ -371,6 +371,9 @@ class AuthenticationProvider(object):
             ('/nf/admin/user/%s' % user.username, 'Details/Edit'),
         ]
 
+    def hibp_password_check_enabled(self):
+        return asbool(tg.config.get('auth.hibp_password_check', False))
+
 
 class LocalAuthenticationProvider(AuthenticationProvider):
 
diff --git a/Allura/allura/lib/security.py b/Allura/allura/lib/security.py
index 544bc76..785b1b9 100644
--- a/Allura/allura/lib/security.py
+++ b/Allura/allura/lib/security.py
@@ -18,14 +18,19 @@
 """
 This module provides the security predicates used in decorating various models.
 """
+from __future__ import absolute_import, division, print_function, unicode_literals
+
 import logging
 from collections import defaultdict
+import hashlib
+import requests
 
 from tg import tmpl_context as c
 from tg import request
 from webob import exc
 from itertools import chain
 from ming.utils import LazyProperty
+import tg
 
 from allura.lib.utils import TruthyCallable
 
@@ -505,3 +510,71 @@ def simple_revoke(acl, role_id, permission):
             remove.append(i)
     for i in reversed(remove):
         acl.pop(i)
+
+
+class HIBPClientError(Exception):
+    """
+    Represents an unexpected failure consuming the HIBP API
+    """
+    pass
+
+
+class HIBPCompromisedCredentials(Exception):
+    """
+    Raised when a sha1 hash of a password is found to be compromised according to HIBP
+    """
+
+    def __init__(self, count, partial_hash):
+        self.count = count
+        self.partial_hash = partial_hash
+
+
+class HIBPClient(object):
+
+    @classmethod
+    def check_breached_password(cls, password):
+        """
+        Checks the Have I Been Pwned API for a known compromised password.
+        Raises a named HIBPCompromisedCredentials exception if any found
+        :param password: user-supplied password
+        """
+        result = 0
+        try:
+            # sha1 it
+            sha_1 = hashlib.sha1(password).hexdigest()
+
+            # first 5 for HIBP API
+            sha_1_first_5 = sha_1[:5]
+
+            # hit HIBP API
+            headers = {'User-Agent': '{}-pwnage-checker'.format(tg.config.get('site_name',
'Allura'))}
+            resp = requests.get('https://api.pwnedpasswords.com/range/{}'.format(sha_1_first_5),
timeout=1,
+                                headers=headers)
+
+            # check results
+            result = cls.scan_response(resp, sha_1)
+
+        except Exception as ex:
+            raise HIBPClientError(ex)
+
+        if result:
+            raise HIBPCompromisedCredentials(result, sha_1_first_5)
+
+    @classmethod
+    def scan_response(self, resp, sha):
+        """
+        Scans an API result from HIBP matching the supplied SHA
+        :return: The entry count HIBP has of the password; 0 if not present
+        """
+        sha_remainder = sha[5:]
+
+        entries = resp.text.split('\r\n')
+        try:
+            entry = [e for e in entries if e.startswith(sha_remainder.upper())][0]
+            vals = entry.split(':')
+            result = vals[1]
+            result = int(result)
+        except IndexError:
+            result = 0
+
+        return result
diff --git a/Allura/allura/tests/functional/test_auth.py b/Allura/allura/tests/functional/test_auth.py
index 9ab4560..f28f032 100644
--- a/Allura/allura/tests/functional/test_auth.py
+++ b/Allura/allura/tests/functional/test_auth.py
@@ -523,6 +523,40 @@ class TestAuth(TestController):
         assert_equal(tasks[0].kwargs['subject'], 'Password Changed')
         assert_in('The password for your', tasks[0].kwargs['text'])
 
+    @patch('allura.lib.plugin.AuthenticationProvider.hibp_password_check_enabled', Mock(return_value=True))
+    @td.with_user_project('test-admin')
+    def test_change_password_hibp(self):
+        self.app.get('/').follow()  # establish session
+        # Get and assert user with password reset token.
+        user = self._create_password_reset_hash()
+        old_pass = user.get_pref('password')
+
+        # Attempt change password with weak pwd
+        r = self.app.post('/auth/preferences/change_password',
+                      extra_environ=dict(username='test-admin'),
+                      params={
+                          'oldpw': 'foo',
+                          'pw': 'password',
+                          'pw2': 'password',
+                          '_session_id': self.app.cookies['_session_id'],
+                      })
+
+        assert 'Unsafe' in str(r.headers)
+
+        r = self.app.post('/auth/preferences/change_password',
+                          extra_environ=dict(username='test-admin'),
+                          params={
+                              'oldpw': 'foo',
+                              'pw': '3j84rhoirwnoiwrnoiw',
+                              'pw2': '3j84rhoirwnoiwrnoiw',
+                              '_session_id': self.app.cookies['_session_id'],
+                          })
+        assert 'Unsafe' not in str(r.headers)
+
+        # Confirm password was changed.
+        user = M.User.by_username('test-admin')
+        assert_not_equal(old_pass, user.get_pref('password'))
+
     @td.with_user_project('test-admin')
     def test_prefs(self):
         r = self.app.get('/auth/preferences/',
@@ -1531,6 +1565,52 @@ To reset your password on %s, please visit the following URL:
                       {'email': 'foo', '_session_id': self.app.cookies['_session_id']},
                       status=404)
 
+    @patch('allura.lib.plugin.AuthenticationProvider.hibp_password_check_enabled', Mock(return_value=True))
+    @patch('allura.tasks.mail_tasks.sendsimplemail')
+    @patch('allura.lib.helpers.gen_message_id')
+    def test_hibp_check(self, gen_message_id, sendmail):
+        self.app.get('/').follow()  # establish session
+        user = M.User.query.get(username='test-admin')
+        email = M.EmailAddress.find({'claimed_by_user_id': user._id}).first()
+        email.confirmed = True
+        ThreadLocalORMSession.flush_all()
+
+        # request a reset
+        r = self.app.post('/auth/password_recovery_hash', {'email': email.email,
+                                                           '_session_id': self.app.cookies['_session_id'],
+                                                           })
+        hash = user.get_tool_data('AuthPasswordReset', 'hash')
+
+        # load reset form and fill it out with weak password
+        r = self.app.get('/auth/forgotten_password/%s' % hash)
+        form = r.forms[0]
+        form['pw'] = form['pw2'] = new_password = 'password'
+        r = form.submit()
+        assert 'Unsafe' in str(r.headers)
+
+        # fill it out again, with a stronger password
+        r = r.follow()
+        form = r.forms[0]
+        form['pw'] = form['pw2'] = new_password = 'oj35h9u34280j924hnuiw'  # something unlikely
to trip at hibp
+        r = form.submit()
+        assert 'Unsafe' not in str(r.headers)
+
+        # confirm password changed and works
+        user = M.User.query.get(username='test-admin')
+        provider = plugin.LocalAuthenticationProvider(None)
+        assert_true(provider._validate_password(user, new_password))
+
+        # confirm can log in now in same session
+        r = r.follow()
+        assert 'Log Out' not in r, r
+        form = r.forms[0]
+        encoded = self.app.antispam_field_names(r.form)
+        form[encoded['username']] = 'test-admin'
+        form[encoded['password']] = new_password
+        r = form.submit(status=302)
+        r = r.follow().follow()
+        assert 'Log Out' in r, r
+
 
 class TestOAuth(TestController):
     def test_register_deregister_app(self):
diff --git a/Allura/development.ini b/Allura/development.ini
index 8f86fca..640ab73 100644
--- a/Allura/development.ini
+++ b/Allura/development.ini
@@ -223,6 +223,10 @@ auth.multifactor.recovery_code.count = 10
 ; length of each code.  Must be 8 for compatibility with "filesystem-googleauth" files
 auth.multifactor.recovery_code.length = 8
 
+; Optionally enable password hash checks against haveibeenpwned.com during password changes,
and disallow any
+; that are known to be compromised
+auth.hibp_password_check = false
+
 
 user_prefs_storage.method = local
 ; user_prefs_storage.method = ldap
diff --git a/Allura/docker-dev.ini b/Allura/docker-dev.ini
index 3f008b4..9842d9b 100644
--- a/Allura/docker-dev.ini
+++ b/Allura/docker-dev.ini
@@ -64,7 +64,6 @@ smtp_port = 8826
 forgemail.host = 0.0.0.0
 forgemail.port = 8825
 
-
 [app:task]
 use = main
 override_root = task ; TurboGears will use controllers/task.py as root controller


Mime
View raw message