You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@allura.apache.org by br...@apache.org on 2019/05/14 22:18:08 UTC
[allura] branch db/8279 created (now 0f40dfe)
This is an automated email from the ASF dual-hosted git repository.
brondsem pushed a change to branch db/8279
in repository https://gitbox.apache.org/repos/asf/allura.git.
at 0f40dfe [#8279] option to require password resets if listed on HIBP
This branch includes the following new commits:
new 0f40dfe [#8279] option to require password resets if listed on HIBP
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[allura] 01/01: [#8279] option to require password resets if listed
on HIBP
Posted by br...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
brondsem pushed a commit to branch db/8279
in repository https://gitbox.apache.org/repos/asf/allura.git
commit 0f40dfeb06a5b46ebcc847ed4c1e9794ee89450c
Author: Dave Brondsema <da...@brondsema.net>
AuthorDate: Mon May 13 17:48:34 2019 -0400
[#8279] option to require password resets if listed on HIBP
---
Allura/allura/controllers/auth.py | 22 ++---
Allura/allura/lib/plugin.py | 57 ++++++++++++-
Allura/allura/lib/utils.py | 4 +
Allura/allura/model/auth.py | 18 ++++
Allura/allura/nf/allura/css/site_style.css | 7 +-
Allura/allura/templates/mail/forgot_password.txt | 2 +-
Allura/allura/templates/pwd_expired.html | 25 +++++-
Allura/allura/templates/widgets/forge_form.html | 4 +-
.../templates_responsive/widgets/forge_form.html | 4 +-
Allura/allura/tests/functional/test_auth.py | 95 +++++++++++++++++++++-
Allura/allura/tests/functional/test_site_admin.py | 2 +-
Allura/allura/tests/test_utils.py | 6 ++
Allura/development.ini | 7 ++
13 files changed, 218 insertions(+), 35 deletions(-)
diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py
index 9273147..299664d 100644
--- a/Allura/allura/controllers/auth.py
+++ b/Allura/allura/controllers/auth.py
@@ -211,33 +211,20 @@ class AuthController(BaseController):
allow_non_primary_email_reset = asbool(config.get('auth.allow_non_primary_email_password_reset', True))
if not re.match(r"[^@]+@[^@]+\.[^@]+", email):
- flash('Enter email in correct format!','error')
+ flash('Enter email in correct format!', 'error')
redirect('/auth/forgotten_password')
if not allow_non_primary_email_reset:
message = 'If the given email address is on record, '\
'a password reset email has been sent to the account\'s primary email address.'
email_record = M.EmailAddress.get(email=provider.get_primary_email_address(user_record=user_record),
- confirmed=True)
+ confirmed=True)
else:
message = 'A password reset email has been sent, if the given email address is on record in our system.'
email_record = M.EmailAddress.get(email=email, confirmed=True)
if user_record and email_record and email_record.confirmed:
- hash = h.nonce(42)
- user_record.set_tool_data('AuthPasswordReset',
- hash=hash,
- hash_expiry=datetime.utcnow() +
- timedelta(seconds=int(config.get('auth.recovery_hash_expiry_period', 600))))
-
- log.info('Sending password recovery link to %s', email_record.email)
- subject = '%s Password recovery' % config['site_name']
- text = g.jinja2_env.get_template('allura:templates/mail/forgot_password.txt').render(dict(
- user=user_record,
- config=config,
- hash=hash,
- ))
- send_system_mail_to_user(email_record.email, subject, text)
+ user_record.send_password_reset_email(email_record.email)
h.auditlog_user('Password recovery link sent to: %s', email, user=user_record)
flash(message)
redirect('/')
@@ -522,9 +509,10 @@ class AuthController(BaseController):
session.pop('pwd-expired', None)
session['username'] = session.get('expired-username')
session.pop('expired-username', None)
+ expired_reason = session.pop('expired-reason', None)
session.save()
- h.auditlog_user('Password reset (via expiration process)')
+ h.auditlog_user('Password reset ({})'.format(expired_reason))
if return_to and return_to != request.url:
redirect(return_to)
else:
diff --git a/Allura/allura/lib/plugin.py b/Allura/allura/lib/plugin.py
index 9156f79..c52453f 100644
--- a/Allura/allura/lib/plugin.py
+++ b/Allura/allura/lib/plugin.py
@@ -41,7 +41,7 @@ except ImportError:
ldap = modlist = None
import pkg_resources
import tg
-from tg import config, request, redirect, response
+from tg import config, request, redirect, response, flash
from tg import tmpl_context as c, app_globals as g
from webob import exc, Request
from paste.deploy.converters import asbool, asint
@@ -187,10 +187,20 @@ class AuthenticationProvider(object):
else:
self.session.pop('multifactor-username', None)
+ login_details = self.get_login_detail(self.request)
+
+ expire_reason = None
if self.is_password_expired(user):
+ h.auditlog_user('Successful login; Password expired', user=user)
+ expire_reason = 'via expiration process'
+ if not expire_reason and 'password' in self.request.params:
+ # password not present with multifactor token; or if login directly after registering is enabled
+ expire_reason = self.login_check_password_change_needed(user, self.request.params['password'],
+ login_details)
+ if expire_reason:
self.session['pwd-expired'] = True
self.session['expired-username'] = user.username
- h.auditlog_user('Successful login; Password expired', user=user)
+ self.session['expired-reason'] = expire_reason
else:
self.session['username'] = user.username
h.auditlog_user('Successful login', user=user)
@@ -204,7 +214,7 @@ class AuthenticationProvider(object):
self.session['login_expires'] = True
self.session.save()
g.statsUpdater.addUserLogin(user)
- user.add_login_detail(self.get_login_detail(self.request))
+ user.add_login_detail(login_details)
user.track_login(self.request)
# set a non-secure cookie with same expiration as session,
# so an http request can know if there is a related session on https
@@ -213,6 +223,36 @@ class AuthenticationProvider(object):
secure=False, httponly=True)
return user
+ def login_check_password_change_needed(self, user, password, login_details):
+ if not self.hibp_password_check_enabled() \
+ or not asbool(tg.config.get('auth.hibp_failure_force_pwd_change', False)):
+ return
+
+ try:
+ security.HIBPClient.check_breached_password(password)
+ except security.HIBPClientError as ex:
+ log.error("Error invoking HIBP API", exc_info=ex)
+ except security.HIBPCompromisedCredentials:
+ trusted = False
+ try:
+ trusted = self.trusted_login_source(user, login_details)
+ except Exception:
+ log.exception('Error checking if login is trusted: %s %s', user.username, login_details)
+
+ if trusted:
+ # current user must change password
+ h.auditlog_user(u'Successful login with password in HIBP breach database, '
+ u'from trusted source (reason: {})'.format(trusted), user=user)
+ return 'hibp' # reason
+ else:
+ # current user may not continue, must reset password via email
+ h.auditlog_user('Attempted login from untrusted location with password in HIBP breach database',
+ user=user)
+ user.send_password_reset_email(subject_tmpl=u'Update your {site_name} password')
+ raise exc.HTTPBadRequest('To ensure account security, you must reset your password via email.'
+ '\n'
+ 'Please check your email to continue.')
+
def logout(self):
self.session.invalidate()
self.session.save()
@@ -416,6 +456,17 @@ class AuthenticationProvider(object):
ua=request.headers.get('User-Agent'),
)
+ def trusted_login_source(self, user, login_details):
+ # TODO: could also factor in User-Agent but hard to know what parts of the UA are meaningful to check here
+ for prev_login in user.previous_login_details:
+ if prev_login['ip'] == login_details['ip']:
+ return 'exact ip'
+ if asbool(tg.config.get('auth.trust_ip_3_octets_match', False)) and \
+ utils.close_ipv4_addrs(prev_login['ip'], login_details['ip']):
+ return 'close ip'
+
+ return False
+
class LocalAuthenticationProvider(AuthenticationProvider):
diff --git a/Allura/allura/lib/utils.py b/Allura/allura/lib/utils.py
index 450bade..91faac1 100644
--- a/Allura/allura/lib/utils.py
+++ b/Allura/allura/lib/utils.py
@@ -846,3 +846,7 @@ def urlencode(params):
then encoded as per normal.
"""
return urllib.urlencode([i for i in generate_smart_str(params)])
+
+
+def close_ipv4_addrs(ip1, ip2):
+ return ip1.split('.')[0:3] == ip2.split('.')[0:3]
diff --git a/Allura/allura/model/auth.py b/Allura/allura/model/auth.py
index 44ed66d..2a15a45 100644
--- a/Allura/allura/model/auth.py
+++ b/Allura/allura/model/auth.py
@@ -385,6 +385,24 @@ class User(MappedClass, ActivityNode, ActivityObject, SearchIndexable):
if login_detail:
self.add_login_detail(login_detail)
+ def send_password_reset_email(self, email_address=None, subject_tmpl=u'{site_name} Password recovery'):
+ if email_address is None:
+ email_address = self.get_pref('email_address')
+ hash = h.nonce(42)
+ self.set_tool_data('AuthPasswordReset',
+ hash=hash,
+ hash_expiry=datetime.utcnow() +
+ timedelta(seconds=int(config.get('auth.recovery_hash_expiry_period', 600))))
+
+ log.info('Sending password recovery link to %s', email_address)
+ subject = subject_tmpl.format(site_name=config['site_name'])
+ text = g.jinja2_env.get_template('allura:templates/mail/forgot_password.txt').render(dict(
+ user=self,
+ config=config,
+ hash=hash,
+ ))
+ allura.tasks.mail_tasks.send_system_mail_to_user(email_address, subject, text)
+
def can_send_user_message(self):
"""Return true if User is permitted to send a mesage to another user.
diff --git a/Allura/allura/nf/allura/css/site_style.css b/Allura/allura/nf/allura/css/site_style.css
index 69d0b5f..040336d 100644
--- a/Allura/allura/nf/allura/css/site_style.css
+++ b/Allura/allura/nf/allura/css/site_style.css
@@ -682,7 +682,7 @@ img.nav-logo {
-o-box-shadow: none;
box-shadow: none;
display: block;
- cursor: default;
+ cursor: auto;
margin-bottom: 1em;
padding: 0;
}
@@ -3109,8 +3109,9 @@ h1.title .viewer:hover {
font-style: italic;
}
-div.message.scm-learn-basics, div.message.scm-ssh-key, div.message.scm-empty-repo {
- cursor: default;
+/* .message default styles are hidden and controlled by jquery.notify.js. These are to be displayed as regular blocks */
+div.message.show-message, div.message.scm-learn-basics, div.message.scm-ssh-key, div.message.scm-empty-repo {
+ cursor: auto;
display: block;
width: auto;
margin: 0 1em 1em;
diff --git a/Allura/allura/templates/mail/forgot_password.txt b/Allura/allura/templates/mail/forgot_password.txt
index 5f5f71a..0e23312 100644
--- a/Allura/allura/templates/mail/forgot_password.txt
+++ b/Allura/allura/templates/mail/forgot_password.txt
@@ -19,6 +19,6 @@
Your username is {{ user.username }}
-To reset your password on {{ config['site_name'] }}, please visit the following URL:
+To update your password on {{ config['site_name'] }}, please visit the following URL:
{{ config['base_url'] }}/auth/forgotten_password/{{hash}}
diff --git a/Allura/allura/templates/pwd_expired.html b/Allura/allura/templates/pwd_expired.html
index 2af886d..476bc89 100644
--- a/Allura/allura/templates/pwd_expired.html
+++ b/Allura/allura/templates/pwd_expired.html
@@ -20,13 +20,30 @@
{% set hide_left_bar = True %}
{% extends g.theme.master %}
-{% block title %}{{c.user.username}} / Update expired password{% endblock %}
+{% block title %}{{c.user.username}} / Update password{% endblock %}
-{% block header %}Update expired password for {{c.user.username}}{% endblock %}
+{% block header %}Update password for {{c.user.username}}{% endblock %}
{% block content %}
- <div class='grid-23'>
- <p>Your {{config['site_name']}} password has expired - You must now choose a new password before logging into the site</p>
+ <div class='grid-15'>
+ <p>Your {{config['site_name']}} password
+ {% if session.get('expired-reason') == 'hibp' %}
+ must be updated to be more secure.
+ {% else %}
+ has expired.
+ {% endif %}
+ You must now choose a new password before logging into the site.</p>
+ </div>
+ <div class="grid-23">
{{ c.form.display(action='/auth/pwd_expired_change', value=dict(return_to=return_to)) }}
</div>
+ {% if h.asbool(config.get('auth.multifactor.totp')) %}
+ <div class='grid-8' style="position: absolute">
+ <div class="message info show-message">
+ Did you know? You can also enable multifactor authentication on your account.
+ <br><br>
+ After changing your password, go to your account settings to set it up.
+ </div>
+ </div>
+ {% endif %}
{% endblock content %}
diff --git a/Allura/allura/templates/widgets/forge_form.html b/Allura/allura/templates/widgets/forge_form.html
index c62404e..1ff6702 100644
--- a/Allura/allura/templates/widgets/forge_form.html
+++ b/Allura/allura/templates/widgets/forge_form.html
@@ -26,13 +26,13 @@
{% set extra_width = 4 %}
{% endif %}
{% if errors and not errors.iteritems and show_errors %}
- <div class="grid-{{19 + extra_width}}"><span {{widget.j2_attrs({'class':error_class})}}>{{errors}}</span></div>
+ <div class="grid-{{19 + extra_width}}"><span {{widget.j2_attrs({'class':error_class})}}>{{errors|nl2br}}</span></div>
{% endif %}
{% for field in widget.fields %}
{% set ctx=widget.context_for(field) %}
{% if field.field_type != 'hidden' %}
{% if ctx.errors and field.show_errors -%}
- <div class="grid-{{19 + extra_width}}"><span {{widget.j2_attrs({'class':error_class})}}>{{ctx.errors}}</span></div>
+ <div class="grid-{{19 + extra_width}}"><span {{widget.j2_attrs({'class':error_class})}}>{{ctx.errors|nl2br}}</span></div>
{%- endif %}
{% if field.show_label and field.label %}
<label for="{{ctx.id}}" class="grid-4">{{field.label}}:</label>
diff --git a/Allura/allura/templates_responsive/widgets/forge_form.html b/Allura/allura/templates_responsive/widgets/forge_form.html
index 29ef744..43b72e7 100644
--- a/Allura/allura/templates_responsive/widgets/forge_form.html
+++ b/Allura/allura/templates_responsive/widgets/forge_form.html
@@ -30,13 +30,13 @@
{% if target %}target="{{target}}"{% endif %}
action="{{action}}">
{% if errors and not errors.iteritems and show_errors %}
- <div class=""><span {{widget.j2_attrs({'class':error_class})}}>{{errors}}</span></div>
+ <div class=""><span {{widget.j2_attrs({'class':error_class})}}>{{errors|nl2br}}</span></div>
{% endif %}
{% for field in widget.fields %}
{% set ctx=widget.context_for(field) %}
{% if field.field_type != 'hidden' %}
{% if ctx.errors and field.show_errors -%}
- <div class="column small-12 large-6"><span {{widget.j2_attrs({'class':error_class})}}>{{ctx.errors}}</span></div>
+ <div class="column small-12 large-6"><span {{widget.j2_attrs({'class':error_class})}}>{{ctx.errors|nl2br}}</span></div>
{%- endif %}
{% if field.show_label and field.label %}
<label for="{{ctx.id}}" class="column small-12 large-3">{{field.label}}:</label>
diff --git a/Allura/allura/tests/functional/test_auth.py b/Allura/allura/tests/functional/test_auth.py
index a0419ff..09af206 100644
--- a/Allura/allura/tests/functional/test_auth.py
+++ b/Allura/allura/tests/functional/test_auth.py
@@ -128,6 +128,63 @@ class TestAuth(TestController):
assert_equal(wf['status'], 'error')
assert_equal(wf['message'], 'Spambot protection engaged')
+ @patch('allura.lib.plugin.AuthenticationProvider.hibp_password_check_enabled', Mock(return_value=True))
+ @patch('allura.tasks.mail_tasks.sendsimplemail')
+ def test_login_hibp_compromised_password_untrusted_client(self, sendsimplemail):
+ # first & only login by this user, so won't have any trusted previous logins
+ self.app.extra_environ = {'disable_auth_magic': 'True'}
+ r = self.app.get('/auth/')
+ f = r.forms[0]
+ encoded = self.app.antispam_field_names(f)
+ f[encoded['username']] = 'test-user'
+ f[encoded['password']] = 'foo'
+
+ with audits('Attempted login from untrusted location with password in HIBP breach database', user=True):
+ r = f.submit(status=200)
+
+ r.mustcontain('reset your password via email.')
+ r.mustcontain('reset your password via email.<br>\nPlease check your email')
+
+ args, kwargs = sendsimplemail.post.call_args
+ assert_equal(sendsimplemail.post.call_count, 1)
+ assert_equal(kwargs['subject'], u'Update your %s password' % config['site_name'])
+ assert_in('/auth/forgotten_password/', kwargs['text'])
+
+ @patch('allura.tasks.mail_tasks.sendsimplemail')
+ def test_login_hibp_compromised_password_trusted_client(self, sendsimplemail):
+ self.app.extra_environ = {'disable_auth_magic': 'True'}
+
+ # regular login first, so IP address will be recorded and then trusted
+ r = self.app.get('/auth/')
+ f = r.forms[0]
+ encoded = self.app.antispam_field_names(f)
+ f[encoded['username']] = 'test-user'
+ f[encoded['password']] = 'foo'
+ with audits('Successful login', user=True):
+ f.submit(status=302)
+ self.app.get('/auth/logout')
+
+ # this login will get caught by HIBP check, but trusted due to IP address being same
+ with patch('allura.lib.plugin.AuthenticationProvider.hibp_password_check_enabled', Mock(return_value=True)):
+ r = self.app.get('/auth/')
+ f = r.forms[0]
+ encoded = self.app.antispam_field_names(f)
+ f[encoded['username']] = 'test-user'
+ f[encoded['password']] = 'foo'
+
+ with audits(r'Successful login with password in HIBP breach database, from trusted source '
+ r'\(reason: exact ip\)', user=True):
+ r = f.submit(status=302)
+
+ assert r.session.get('pwd-expired')
+ assert_equal(r.session.get('expired-reason'), 'hibp')
+ assert_equal(r.location, 'http://localhost/auth/pwd_expired')
+
+ r = r.follow()
+ r.mustcontain('must be updated to be more secure')
+
+ # changing password covered in TestPasswordExpire
+
def test_logout(self):
self.app.extra_environ = {'disable_auth_magic': 'True'}
nav_pattern = ('nav', {'class': 'nav-main'})
@@ -1488,7 +1545,7 @@ class TestPasswordReset(TestController):
# confirm email sent
text = '''Your username is test-admin
-To reset your password on %s, please visit the following URL:
+To update your password on %s, please visit the following URL:
%s/auth/forgotten_password/%s''' % (config['site_name'], config['base_url'], hash)
sendmail.post.assert_called_once_with(
@@ -1578,7 +1635,7 @@ To reset your password on %s, please visit the following URL:
@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):
+ def test_pwd_reset_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()
@@ -2533,6 +2590,40 @@ class TestTwoFactor(TestController):
# confirm code used up
assert_not_in(recovery_code, RecoveryCodeService().get().get_codes(user))
+ @patch('allura.lib.plugin.AuthenticationProvider.hibp_password_check_enabled', Mock(return_value=True))
+ def test_login_totp_with_hibp(self):
+ # this is essentially the same as regular TOTP test, just making sure that HIBP doesn't get in the way
+ # or cause any problems. It shouldn't even run since a password isn't present when the final login happens
+
+ self._init_totp()
+
+ # so test-admin isn't automatically logged in for all requests
+ self.app.extra_environ = {'disable_auth_magic': 'True'}
+
+ # regular login
+ r = self.app.get('/auth/?return_to=/p/foo')
+ encoded = self.app.antispam_field_names(r.form)
+ r.form[encoded['username']] = 'test-admin'
+ r.form[encoded['password']] = 'foo'
+ with audits('Multifactor login - password ok, code not entered yet', user=True):
+ r = r.form.submit()
+
+ # check results
+ assert r.location.endswith('/auth/multifactor?return_to=%2Fp%2Ffoo'), r
+ r = r.follow()
+ assert not r.session.get('username')
+
+ # use a valid code
+ totp = TotpService().Totp(self.sample_key)
+ code = totp.generate(time_time())
+ r.form['code'] = code
+ with audits('Successful login', user=True):
+ r = r.form.submit()
+
+ # confirm login and final page
+ assert_equal(r.session['username'], 'test-admin')
+ assert r.location.endswith('/p/foo'), r
+
def test_view_key(self):
self._init_totp()
diff --git a/Allura/allura/tests/functional/test_site_admin.py b/Allura/allura/tests/functional/test_site_admin.py
index cb61e58..0831926 100644
--- a/Allura/allura/tests/functional/test_site_admin.py
+++ b/Allura/allura/tests/functional/test_site_admin.py
@@ -705,7 +705,7 @@ class TestUserDetails(TestController):
hash = user.get_tool_data('AuthPasswordReset', 'hash')
text = '''Your username is test-user
-To reset your password on %s, please visit the following URL:
+To update your password on %s, please visit the following URL:
%s/auth/forgotten_password/%s''' % (config['site_name'], config['base_url'], hash)
sendmail.post.assert_called_once_with(
diff --git a/Allura/allura/tests/test_utils.py b/Allura/allura/tests/test_utils.py
index 5f642a7..199b4e5 100644
--- a/Allura/allura/tests/test_utils.py
+++ b/Allura/allura/tests/test_utils.py
@@ -398,3 +398,9 @@ def test_is_nofollow_url():
assert not utils.is_nofollow_url('http://foo.com/')
assert not utils.is_nofollow_url('http://bar.io/')
assert utils.is_nofollow_url('http://bar.iot/')
+
+
+def test_close_ipv4_addrs():
+ assert utils.close_ipv4_addrs('1.2.3.4', '1.2.3.4')
+ assert utils.close_ipv4_addrs('1.2.3.4', '1.2.3.255')
+ assert not utils.close_ipv4_addrs('1.2.3.4', '1.2.4.4')
\ No newline at end of file
diff --git a/Allura/development.ini b/Allura/development.ini
index 640ab73..ed2cf50 100644
--- a/Allura/development.ini
+++ b/Allura/development.ini
@@ -227,6 +227,13 @@ auth.multifactor.recovery_code.length = 8
; that are known to be compromised
auth.hibp_password_check = false
+; if auth.hibp_password_check is true and this is also set to true, then a password reset will be forced
+; either via the current session if the login ip is "trusted", or via a password reset email if not trusted
+auth.hibp_failure_force_pwd_change = true
+; if auth.hibp_password_check and auth.hibp_failure_force_pwd_change are true, then this is used to determine if a
+; login can be trusted to reset their own ip address. If set to false, then only the exact same IP can reset a
+; HIBP-listed password without going through email
+auth.auth.trust_ip_3_octets_match = true
user_prefs_storage.method = local
; user_prefs_storage.method = ldap