This is an automated email from the ASF dual-hosted git repository. brondsem pushed a commit to branch db/8566 in repository https://gitbox.apache.org/repos/asf/allura.git
commit 6d78f55700ed4388de36a447a4f1fff1d5ad1c52 Author: Dave Brondsema <dbronds...@slashdotmedia.com> AuthorDate: Tue Jul 2 18:08:27 2024 -0400 [#8566] support more password hashing algorithms - replace crypt with passlib - use recommended rounds & salts by default - automatically rehash user passwords to new algorithm upon login - some AuthProvider method changes - speed up bootstrap.py, since password hashing takes longer now --- Allura/allura/lib/plugin.py | 189 ++++++++++++++------- Allura/allura/model/auth.py | 3 +- Allura/allura/tests/decorators.py | 13 +- Allura/allura/tests/functional/test_auth.py | 16 +- Allura/allura/tests/functional/test_home.py | 6 +- Allura/allura/tests/model/test_auth.py | 8 +- Allura/allura/tests/test_plugin.py | 67 +++++++- .../allura/tests/unit/test_ldap_auth_provider.py | 38 +++-- Allura/allura/websetup/bootstrap.py | 18 +- Allura/development.ini | 26 ++- pytest.ini | 4 + requirements-optional.txt | 4 + requirements.in | 1 + requirements.txt | 6 +- 14 files changed, 288 insertions(+), 111 deletions(-) diff --git a/Allura/allura/lib/plugin.py b/Allura/allura/lib/plugin.py index 0dea14d29..85076c6ce 100644 --- a/Allura/allura/lib/plugin.py +++ b/Allura/allura/lib/plugin.py @@ -24,8 +24,9 @@ import os import logging import subprocess import string -import crypt import random +import sys +import warnings from contextlib import contextmanager from urllib.parse import urlparse from io import BytesIO @@ -36,8 +37,10 @@ from datetime import datetime, timedelta import typing import calendar +import passlib.ifc import requests import six +from passlib.context import CryptContext try: import ldap @@ -49,8 +52,9 @@ import tg 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 +from paste.deploy.converters import asbool, asint, aslist from formencode import validators as fev +from passlib.registry import get_crypt_handler from ming.utils import LazyProperty from ming.odm import state @@ -65,6 +69,7 @@ from allura.tasks.index_tasks import solr_del_project_artifacts if typing.TYPE_CHECKING: from allura.app import SitemapEntry + from allura import model as M log = logging.getLogger(__name__) @@ -93,6 +98,8 @@ class AuthenticationProvider: '/auth/multifactor', '/auth/do_multifactor', ] + cfg_prefix_pwds = 'auth.password.' + default_pwd_algo = 'scrypt' # noqa: S105 def __init__(self, request): self.request = request @@ -264,12 +271,23 @@ class AuthenticationProvider: self.session.save() response.set_cookie('memorable_forget', '/', secure=request.environ['beaker.session'].secure) - def validate_password(self, user, password): + def validate_password(self, user: M.User, password: str) -> bool: + ok = self._validate_password(user, password) + if ok: + self.rehash_password_if_needed(user, password) + return ok + + def rehash_password_if_needed(self, user: M.User, password: str) -> None: + if user.password_algorithm != self._password_algorithm(): + self.set_password(user, None, password, set_timestamp=False) + h.auditlog_user('Rehashed password automatically', user=user) + + def _validate_password(self, user: M.User, password: str) -> bool: '''Check that provided password matches actual user password :rtype: bool ''' - raise NotImplementedError('validate_password') + raise NotImplementedError('_validate_password') def disable_user(self, user, **kw): '''Disable user account''' @@ -295,13 +313,15 @@ class AuthenticationProvider: ''' raise NotImplementedError('by_username') - def set_password(self, user, old_password, new_password): + def set_password(self, user: M.User, old_password: str | None, new_password: str, set_timestamp=True): ''' Set a user's password. - A provider implementing this method should store the timestamp of this change, either + A provider implementing this method should store the timestamp of this change when set_timestamp=True, either on ``user.last_password_updated`` or somewhere else that a custom ``get_last_password_updated`` method uses. + Also should set user.password_algorithm to the password algorithm used. + :param user: a :class:`User <allura.model.auth.User>` :rtype: None :raises: HTTPUnauthorized if old_password is not valid @@ -504,12 +524,54 @@ class AuthenticationProvider: validator._messages['invalid'] = 'Usernames only include small letters, numbers, and dashes' return validator + def _passlib_crypt_by_id(self, algorithm: str) -> passlib.ifc.PasswordHash: + return get_crypt_handler(algorithm) + + def _encode_password_legacy_sha256(self, password: str, salt: str) -> str: + from allura import model as M + + if salt is None: + salt = ''.join(chr(randint(1, 0x7f)) + for i in range(M.User.SALT_LEN)) + hashpass = sha256((salt + password).encode('utf-8')).digest() + return 'sha256' + salt + six.ensure_text(b64encode(hashpass)) + + def _password_algorithm(self) -> str: + return config.get(self.cfg_prefix_pwds + 'algorithm', self.default_pwd_algo) + + def _encode_password(self, password: str, salt=None) -> tuple[str, str]: + algorithm = self._password_algorithm() + + if algorithm == 'allura_sha256': + return self._encode_password_legacy_sha256(password, salt), algorithm + + crypt = self._passlib_crypt_by_id(algorithm) + + passlib_kwargs = dict() + if crypt.name.startswith('bcrypt'): + passlib_kwargs['truncate_error'] = True + + rounds = config.get(self.cfg_prefix_pwds + 'rounds') + if rounds: + if int(rounds) < getattr(crypt, 'default_rounds', 0): + warnings.warn(f'rounds for {crypt.name} should be at least {crypt.default_rounds}', stacklevel=1) + passlib_kwargs['rounds'] = rounds + + salt_len = config.get(self.cfg_prefix_pwds + 'salt_len') + if salt_len: + passlib_kwargs['salt_size'] = int(salt_len) + if salt: + if crypt._salt_is_bytes: + salt = salt.encode() # must be bytes + passlib_kwargs['salt'] = salt + encrypted = crypt.using(**passlib_kwargs).hash(password) + return encrypted, algorithm + class LocalAuthenticationProvider(AuthenticationProvider): ''' - Stores user passwords on the User model, in mongo. Uses per-user salt and - SHA-256 encryption. + Stores user password hashes on the User model, in mongo. ''' forgotten_password_process = True @@ -525,7 +587,7 @@ class LocalAuthenticationProvider(AuthenticationProvider): def _login(self): user = self.by_username(self.request.params['username']) - if not self._validate_password(user, self.request.params['password']): + if not self.validate_password(user, self.request.params['password']): raise exc.HTTPUnauthorized() return user @@ -553,19 +615,27 @@ class LocalAuthenticationProvider(AuthenticationProvider): if kw.get('audit', True): h.auditlog_user('Account changed to pending', user=user) - def validate_password(self, user, password): - return self._validate_password(user, password) - - def _validate_password(self, user, password): + def _validate_password(self, user: M.User, password) -> bool: if user is None: return False if not user.password: return False - salt = str(user.password[6:6 + user.SALT_LEN]) - check = self._encode_password(password, salt) - if check != user.password: - return False - return True + + algorithm = config.get('auth.password.algorithm', self.default_pwd_algo) + old_algos = aslist(config.get('auth.password.algorithm.old', 'allura_sha256')) + + if user.password.startswith('sha256') and 'allura_sha256' in old_algos: + # legacy. 'allura_sha256' is a custom name for our old logic + salt = str(user.password[6:6 + user.SALT_LEN]) + check = self._encode_password_legacy_sha256(password, salt) + return check == user.password + else: + # https://passlib.readthedocs.io/en/stable/narr/context-tutorial.html#deprecation-hash-migration + crypt = CryptContext( + schemes=[self._passlib_crypt_by_id(algorithm)], + deprecated=[self._passlib_crypt_by_id(algo) for algo in old_algos if algo != 'allura_sha256'], + ) + return crypt.verify(password, user.password) def by_username(self, username): from allura import model as M @@ -576,22 +646,16 @@ class LocalAuthenticationProvider(AuthenticationProvider): rex = r'^' + un + '$' return M.User.query.get(username={'$regex': rex}, disabled=False, pending=False) - def set_password(self, user, old_password, new_password): + def set_password(self, user, old_password, new_password, set_timestamp=True): if old_password is not None and not self.validate_password(user, old_password): raise exc.HTTPUnauthorized() else: - user.password = self._encode_password(new_password) - user.last_password_updated = datetime.utcnow() + user.password, algorithm = self._encode_password(new_password) + user.password_algorithm = algorithm + if set_timestamp: + user.last_password_updated = datetime.utcnow() session(user).flush(user) - def _encode_password(self, password, salt=None): - from allura import model as M - if salt is None: - salt = ''.join(chr(randint(1, 0x7f)) - for i in range(M.User.SALT_LEN)) - hashpass = sha256((salt + password).encode('utf-8')).digest() - return 'sha256' + salt + six.ensure_text(b64encode(hashpass)) - def user_project_shortname(self, user): # "_" isn't valid for subdomains (which project names are used with) # so if a username has a "_" we change it to "-" @@ -671,9 +735,13 @@ def ldap_user_dn(username): class LdapAuthenticationProvider(AuthenticationProvider): forgotten_password_process = True + cfg_prefix_pwds = 'auth.ldap.password.' + default_pwd_algo = 'ldap_pbkdf2_sha512' # noqa: S105 def register_user(self, user_doc): from allura import model as M + pwd_hash, algorithm = self._encode_password(user_doc['password']) + user_doc['password_algorithm'] = algorithm result = M.User(**user_doc) if asbool(config.get('auth.ldap.autoregister', True)): if asbool(config.get('auth.allow_user_registration', True)): @@ -690,7 +758,7 @@ class LdapAuthenticationProvider(AuthenticationProvider): display_name = user_doc['display_name'].encode('utf-8') ldif_u = modlist.addModlist(dict( uid=uname, - userPassword=self._encode_password(user_doc['password']), + userPassword=pwd_hash, objectClass=[b'account', b'posixAccount'], cn=display_name, uidNumber=uid, @@ -732,39 +800,44 @@ class LdapAuthenticationProvider(AuthenticationProvider): username, errmsg) raise AssertionError(errmsg) - def _get_salt(self, length): - def random_char(): - return random.choice(string.ascii_uppercase + string.digits) - return ''.join(random_char() for i in range(length)) - - def _encode_password(self, password, salt=None): - cfg_prefix = 'auth.ldap.password.' - salt_len = asint(config.get(cfg_prefix + 'salt_len', 16)) - algorithm = config.get(cfg_prefix + 'algorithm', 6) - rounds = asint(config.get(cfg_prefix + 'rounds', 6000)) - salt = self._get_salt(salt_len) if salt is None else salt - encrypted = crypt.crypt( - six.ensure_str(password), - f'${algorithm}$rounds={rounds}${salt}') - return b'{CRYPT}%s' % encrypted.encode('utf-8') - def by_username(self, username): from allura import model as M return M.User.query.get(username=username, disabled=False, pending=False) - def set_password(self, user, old_password, new_password): + def _passlib_crypt_by_id(self, algorithm: str) -> passlib.ifc.PasswordHash: + try: + crypt = get_crypt_handler(algorithm) + except KeyError: + # handle a few old system "crypt" ids. https://passlib.readthedocs.io/en/stable/modular_crypt_format.html#mcf-identifiers + crypt = get_crypt_handler( + { + # '1': 'ldap_md5_crypt', # could support this, but it is super weak + '2b': 'ldap_bcrypt', + '5': 'ldap_sha256_crypt', + '6': 'ldap_sha512_crypt', + }[algorithm] + ) + else: + if not algorithm.startswith('ldap_'): + raise ValueError(f'LDAP algorithms should always start with ldap_ (got {algorithm})') + return crypt + + + def set_password(self, user, old_password, new_password, set_timestamp=True): dn = ldap_user_dn(user.username) - if old_password: + if old_password is not None: ldap_ident = dn ldap_pass = old_password.encode('utf-8') else: ldap_ident = ldap_pass = None try: - new_password = self._encode_password(new_password) + new_password, algorithm = self._encode_password(new_password) with ldap_conn(ldap_ident, ldap_pass) as con: con.modify_s( - dn, [(ldap.MOD_REPLACE, 'userPassword', new_password)]) - user.last_password_updated = datetime.utcnow() + dn, [(ldap.MOD_REPLACE, 'userPassword', new_password.encode('utf-8'))]) + if set_timestamp: + user.last_password_updated = datetime.utcnow() + user.password_algorithm = algorithm session(user).flush(user) except ldap.INVALID_CREDENTIALS: raise exc.HTTPUnauthorized() @@ -778,7 +851,7 @@ class LdapAuthenticationProvider(AuthenticationProvider): username = str(self.request.params['username']) except UnicodeEncodeError: raise exc.HTTPBadRequest('Unicode is not allowed in usernames') - if not self._validate_password(username, self.request.params['password']): + if not self._validate_password_username(username, self.request.params['password']): raise exc.HTTPUnauthorized() user = M.User.query.get(username=username) if user is None: @@ -792,17 +865,17 @@ class LdapAuthenticationProvider(AuthenticationProvider): else: log.debug(f'LdapAuth: no user {username} found in local mongo') raise exc.HTTPUnauthorized() - elif user.disabled or user.pending: + else: + self.rehash_password_if_needed(user, self.request.params['password']) + if user.disabled or user.pending: log.debug(f'LdapAuth: user {username} is disabled or pending in Allura') raise exc.HTTPUnauthorized() return user - def validate_password(self, user, password): - '''by user''' - return self._validate_password(user.username, password) + def _validate_password(self, user: M.User, password: str) -> bool: + return self._validate_password_username(user.username, password) - def _validate_password(self, username, password): - '''by username''' + def _validate_password_username(self, username: str, password: str) -> bool: password = h.really_unicode(password).encode('utf-8') try: ldap_user = ldap_user_dn(username) diff --git a/Allura/allura/model/auth.py b/Allura/allura/model/auth.py index 0b73f1b34..23ed932ba 100644 --- a/Allura/allura/model/auth.py +++ b/Allura/allura/model/auth.py @@ -259,8 +259,9 @@ class User(MappedClass, ActivityNode, ActivityObject, SearchIndexable): sfx_userid = FieldProperty(S.Deprecated) username = FieldProperty(str) email_addresses = FieldProperty([str]) - password = FieldProperty(str) + password = FieldProperty(str) # hashed last_password_updated = FieldProperty(datetime) # to access, use AuthProvider's get_last_password_updated + password_algorithm = FieldProperty(str) reg_date = FieldProperty(datetime) # to access, use user.registration_date() projects = FieldProperty(S.Deprecated) # full mount point: prefs dict diff --git a/Allura/allura/tests/decorators.py b/Allura/allura/tests/decorators.py index 7311f58be..f72c15648 100644 --- a/Allura/allura/tests/decorators.py +++ b/Allura/allura/tests/decorators.py @@ -151,7 +151,7 @@ class patch_middleware_config: @contextlib.contextmanager -def audits(*messages, **kwargs): +def audits(*messages, user=False, actor=r'.*', ip_addr=r'.*', user_agent=r'.*'): """ Asserts all the messages exist in audit log @@ -161,10 +161,7 @@ def audits(*messages, **kwargs): """ M.AuditLog.query.remove() yield - if kwargs.get('user'): - actor = kwargs.get('actor', '.*') - ip_addr = kwargs.get('ip_addr', '.*') - user_agent = kwargs.get('user_agent', '.*') + if user: preamble = f'(Done by user: {actor}\n)?IP Address: {ip_addr}\nUser-Agent: {user_agent}\n' else: preamble = '' @@ -182,7 +179,7 @@ def audits(*messages, **kwargs): @contextlib.contextmanager -def out_audits(*messages, **kwargs): +def out_audits(*messages, user=False, actor=r'.*', ip_addr=r'.*'): """ Asserts none the messages exist in audit log. "without audits" @@ -192,9 +189,7 @@ def out_audits(*messages, **kwargs): """ M.AuditLog.query.remove() yield - if kwargs.get('user'): - actor = kwargs.get('actor', '.*') - ip_addr = kwargs.get('ip_addr', '.*') + if user: preamble = f'(Done by user: {actor}\n)?IP Address: {ip_addr}\n' else: preamble = '' diff --git a/Allura/allura/tests/functional/test_auth.py b/Allura/allura/tests/functional/test_auth.py index be33727bb..868dbc26f 100644 --- a/Allura/allura/tests/functional/test_auth.py +++ b/Allura/allura/tests/functional/test_auth.py @@ -213,6 +213,18 @@ class TestAuth(TestController): with audits('Failed login', user=True): r = f.submit(extra_environ={'username': '*anonymous'}) + @pytest.mark.parametrize('old_algo', ['allura_sha256', 'sha512_crypt']) + def test_login_old_password_hash(self, old_algo): + u = M.User.query.get(username='test-user') + u.password_algorithm = old_algo + r = self.app.get('/auth/', extra_environ={'username': '*anonymous'}) + f = r.forms[0] + encoded = self.app.antispam_field_names(f) + f[encoded['username']] = 'test-user' + f[encoded['password']] = 'foo' + with audits('Successful login', 'Rehashed password automatically', user=True): + r = f.submit(extra_environ={'username': '*anonymous'}) + def test_login_overlay(self): r = self.app.get('/auth/login_fragment/', extra_environ={'username': '*anonymous'}) f = r.forms[0] @@ -1745,7 +1757,7 @@ To update your password on %s, please visit the following URL: user = M.User.query.get(username='test-admin') assert old_pw_hash != user.password provider = plugin.LocalAuthenticationProvider(None) - assert provider._validate_password(user, new_password) + assert provider.validate_password(user, new_password) # confirm reset fields cleared user = M.User.query.get(username='test-admin') @@ -1860,7 +1872,7 @@ To update your password on %s, please visit the following URL: # confirm password changed and works user = M.User.query.get(username='test-admin') provider = plugin.LocalAuthenticationProvider(None) - assert provider._validate_password(user, new_password) + assert provider.validate_password(user, new_password) # confirm can log in now in same session r = r.follow() diff --git a/Allura/allura/tests/functional/test_home.py b/Allura/allura/tests/functional/test_home.py index 1e668e219..24601cd3e 100644 --- a/Allura/allura/tests/functional/test_home.py +++ b/Allura/allura/tests/functional/test_home.py @@ -199,10 +199,9 @@ class TestProjectHome(TestController): test_project.add_user(M.User.by_username('test-user-1'), ['B_role']) test_project.add_user(M.User.by_username('test-user'), ['Developer']) test_project.add_user(M.User.by_username('test-user-0'), ['Member']) - test_project.add_user(M.User.by_username('test-user-2'), ['Member']) + test_project.add_user(M.User.by_username('test-user-2'), ['Admin']) test_project.add_user(M.User.by_username('test-user-3'), ['Member']) test_project.add_user(M.User.by_username('test-user-3'), ['Developer']) - test_project.add_user(M.User.by_username('test-user-4'), ['Admin']) ThreadLocalODMSession.flush_all() r = self.app.get('/p/test/_members/') @@ -211,12 +210,11 @@ class TestProjectHome(TestController): assert '<td>Admin</td>' in r tr = r.html.findAll('tr') assert "<td>Test Admin</td>" in str(tr[1]) - assert "<td>Test User 4</td>" in str(tr[2]) + assert "<td>Test User 2</td>" in str(tr[2]) assert "<td>Test User</td>" in str(tr[3]) assert "<td>Test User 3</td>" in str(tr[4]) assert "<td>Test User 0</td>" in str(tr[5]) assert "<td>Test User 1</td>" in str(tr[6]) - assert "<td>Test User 2</td>" in str(tr[7]) def test_members_anonymous(self): r = self.app.get('/p/test/_members/', diff --git a/Allura/allura/tests/model/test_auth.py b/Allura/allura/tests/model/test_auth.py index 11cd00cb4..4f6e708e4 100644 --- a/Allura/allura/tests/model/test_auth.py +++ b/Allura/allura/tests/model/test_auth.py @@ -135,11 +135,11 @@ class TestAuth: assert len(roles) == 3, roles u.set_password('foo') provider = plugin.LocalAuthenticationProvider(Request.blank('/')) - assert provider._validate_password(u, 'foo') - assert not provider._validate_password(u, 'foobar') + assert provider.validate_password(u, 'foo') + assert not provider.validate_password(u, 'foobar') u.set_password('foobar') - assert provider._validate_password(u, 'foobar') - assert not provider._validate_password(u, 'foo') + assert provider.validate_password(u, 'foobar') + assert not provider.validate_password(u, 'foo') def test_user_project_creates_on_demand(self): u = M.User.register(dict(username='foobar123'), make_project=False) diff --git a/Allura/allura/tests/test_plugin.py b/Allura/allura/tests/test_plugin.py index 085e3a143..a75e444f5 100644 --- a/Allura/allura/tests/test_plugin.py +++ b/Allura/allura/tests/test_plugin.py @@ -14,12 +14,15 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. - +import contextlib import datetime as dt import calendar +from unittest import SkipTest +import passlib import tg from tg import tmpl_context as c +from tg import config from webob import Request, exc from bson import ObjectId from ming.odm.odmsession import ThreadLocalODMSession @@ -30,7 +33,7 @@ from allura import model as M from allura.lib import plugin from allura.lib import phone from allura.lib import helpers as h -from allura.lib.plugin import ProjectRegistrationProvider +from allura.lib.plugin import ProjectRegistrationProvider, AuthenticationProvider from allura.lib.plugin import ThemeProvider from allura.lib.exceptions import ProjectConflict, ProjectShortnameInvalid from allura.tests.decorators import audits @@ -624,21 +627,69 @@ class TestLocalAuthenticationProvider: setup_global_objects() self.provider = plugin.LocalAuthenticationProvider(Request.blank('/')) - def test_password_encoder(self): + @patch.dict(tg.config, {'auth.password.algorithm': 'allura_sha256'}) + def test_password_encoder_legacy(self): # Verify salt ep = self.provider._encode_password assert ep('test_pass') != ep('test_pass') assert ep('test_pass', '0000') == ep('test_pass', '0000') - assert ep('test_pass', '0000') == 'sha2560000j7pRjKKZ5L8G0jScZKja9ECmYF2zBV82Mi+E3wkop30=' + assert ep('test_pass', '0000') == ('sha2560000j7pRjKKZ5L8G0jScZKja9ECmYF2zBV82Mi+E3wkop30=', 'allura_sha256') + + @pytest.mark.parametrize('algorithm,rounds,specific_salt,salt_len,expected_config', [ + ('sha512_crypt', None,None, None, '$6$rounds=656000$'), + ('pbkdf2_sha256', None,None, None, '$pbkdf2-sha256$29000$'), + ('pbkdf2_sha512', None,None, None, '$pbkdf2-sha512$25000$'), + # test with explicit # of rounds & salt_len + ('pbkdf2_sha512', 26789, None, 50, '$pbkdf2-sha512$26789$'), + ('bcrypt_sha256', None, 'O'*22, None, '$bcrypt-sha256$v=2,t=2b,r=12$'), + ('scrypt', None, None, None, '$scrypt$ln=16,r=8,p=1$'), + ('argon2', None, '0'*8, None, '$argon2id$v=19$m=65536,t=3,p=4$'), + ]) + def test_password_encoder(self, algorithm: str, rounds, specific_salt, salt_len, expected_config): + self._test_password_encoder( + 'auth.password.', + self.provider, + algorithm, rounds, specific_salt, salt_len, expected_config, + ) + + # shared by test_ldap_auth_provider.py + @staticmethod + def _test_password_encoder(cfg_prefix: str, provider: AuthenticationProvider, + algorithm, rounds, specific_salt, salt_len, expected_config): + if specific_salt is None: + specific_salt = '0000' + if rounds is not None: + rounds_patch = patch.dict(config, {f'{cfg_prefix}rounds': str(rounds)}) + else: + rounds_patch = contextlib.nullcontext() + if salt_len is not None: + salt_len_patch = patch.dict(config, {f'{cfg_prefix}salt_len': str(salt_len)}) + else: + salt_len_patch = contextlib.nullcontext() + with patch.dict(config, {f'{cfg_prefix}algorithm': algorithm}), rounds_patch, salt_len_patch: + ep = provider._encode_password + try: + # different values because of salting + assert ep('test_pass') != ep('test_pass') + except passlib.exc.MissingBackendError as e: + if 'bcrypt' in str(e): + raise SkipTest('bcrypt not available') + if 'argon2' in str(e): + raise SkipTest('argon2 not available') + + # same value when same salt + assert ep('test_pass', specific_salt) == ep('test_pass', specific_salt) + + # Test password format + assert ep('pwd')[0].startswith(expected_config) def test_set_password_with_old_password(self): user = Mock() user.__ming__ = Mock() self.provider.validate_password = lambda u, p: False - self.provider._encode_password = Mock() - pytest.raises( - exc.HTTPUnauthorized, - self.provider.set_password, user, 'old', 'new') + self.provider._encode_password = Mock(return_value=['asdfasdf', 'allura_sha256']) + with pytest.raises(exc.HTTPUnauthorized): + self.provider.set_password(user, 'old', 'new') assert self.provider._encode_password.call_count == 0 self.provider.validate_password = lambda u, p: True diff --git a/Allura/allura/tests/unit/test_ldap_auth_provider.py b/Allura/allura/tests/unit/test_ldap_auth_provider.py index 818b09e17..b1c4d1f04 100644 --- a/Allura/allura/tests/unit/test_ldap_auth_provider.py +++ b/Allura/allura/tests/unit/test_ldap_auth_provider.py @@ -16,13 +16,14 @@ # under the License. import calendar -import platform from datetime import datetime +import pytest from bson import ObjectId from mock import patch, Mock -from unittest import SkipTest from webob import Request + +from allura.tests.test_plugin import TestLocalAuthenticationProvider from ming.odm.odmsession import ThreadLocalODMSession from tg import config @@ -30,7 +31,6 @@ from alluratest.controller import setup_basic_test from allura.lib import plugin from allura.lib import helpers as h from allura import model as M -import six class TestLdapAuthenticationProvider: @@ -39,22 +39,26 @@ class TestLdapAuthenticationProvider: setup_basic_test() self.provider = plugin.LdapAuthenticationProvider(Request.blank('/')) - def test_password_encoder(self): - # Verify salt - ep = self.provider._encode_password - # Note: OSX uses a crypt library with a known issue relating the hashing algorithms. - if b'$6$rounds=' not in ep('pwd') and platform.system() == 'Darwin': - raise SkipTest - assert ep('test_pass') != ep('test_pass') - assert ep('test_pass', '0000') == ep('test_pass', '0000') - # Test password format - assert ep('pwd').startswith(b'{CRYPT}$6$rounds=6000$') + @pytest.mark.parametrize('algorithm,rounds,specific_salt,salt_len,expected_config', [ + ('2b', None, 'O'*22, None, '{CRYPT}$2b$12'), + ('5', None, None, None, '{CRYPT}$5$rounds=535000$'), + ('6', None, None, None, '{CRYPT}$6$rounds=656000$'), + ('ldap_pbkdf2_sha256', None, None, None, '{PBKDF2-SHA256}29000$'), + ('ldap_pbkdf2_sha512', None, None, None, '{PBKDF2-SHA512}25000$'), + ('ldap_bcrypt', None, 'O'*22, None, '{CRYPT}$2b$12$'), + ]) + def test_password_encoder(self, algorithm: str, rounds, specific_salt, salt_len, expected_config): + TestLocalAuthenticationProvider._test_password_encoder( + 'auth.ldap.password.', + self.provider, + algorithm, rounds, specific_salt, salt_len, expected_config, + ) @patch('allura.lib.plugin.ldap') def test_set_password(self, ldap): user = Mock(username='test-user') user.__ming__ = Mock() - self.provider._encode_password = Mock(return_value=b'new-pass-hash') + self.provider._encode_password = Mock(return_value=('new-pass-hash', 'somealgo')) ldap.dn.escape_dn_chars = lambda x: x dn = 'uid=%s,ou=people,dc=localdomain' % user.username @@ -75,11 +79,13 @@ class TestLdapAuthenticationProvider: self.provider.request.method = 'POST' self.provider.request.body = '&'.join([f'{k}={v}' for k, v in params.items()]).encode('utf-8') ldap.dn.escape_dn_chars = lambda x: x + user = M.User.query.get(username=params['username']) + user.password_algorithm = self.provider._password_algorithm() # default is non-ldap algo so would cause rehash self.provider._login() dn = 'uid=%s,ou=people,dc=localdomain' % params['username'] - ldap.initialize.assert_called_once_with('ldaps://localhost/') + ldap.initialize.assert_called_with('ldaps://localhost/') connection = ldap.initialize.return_value connection.simple_bind_s.assert_called_once_with(dn, b'test-password') assert connection.unbind_s.call_count == 1 @@ -113,7 +119,7 @@ class TestLdapAuthenticationProvider: 'password': 'new-password', } ldap.dn.escape_dn_chars = lambda x: x - self.provider._encode_password = Mock(return_value=b'new-password-hash') + self.provider._encode_password = Mock(return_value=('new-password-hash', 'somealgo')) assert M.User.query.get(username=user_doc['username']) is None with h.push_config(config, **{'auth.ldap.autoregister': 'false'}): diff --git a/Allura/allura/websetup/bootstrap.py b/Allura/allura/websetup/bootstrap.py index 21cfa6781..dba5f136f 100644 --- a/Allura/allura/websetup/bootstrap.py +++ b/Allura/allura/websetup/bootstrap.py @@ -20,6 +20,7 @@ import os import sys import logging import shutil +from datetime import datetime from textwrap import dedent import tg @@ -27,6 +28,7 @@ from tg import tmpl_context as c, app_globals as g from paste.deploy.converters import asbool import ew +from allura.lib.decorators import memoize from allura.model.oauth import dummy_oauths from ming import Session, mim from ming.odm import state, session @@ -178,7 +180,7 @@ def bootstrap(command, conf, vars): if create_test_data: # Add some test users - for unum in range(10): + for unum in range(4): make_user('Test User %d' % unum) log.info('Creating basic project categories') @@ -205,7 +207,7 @@ def bootstrap(command, conf, vars): u_admin = make_user('Test Admin') u_admin.preferences = dict(email_address='test-admin@users.localhost') u_admin.email_addresses = ['test-admin@users.localhost'] - u_admin.set_password('foo') + fast_set_pwd(u_admin, 'foo') u_admin.claim_address('test-admin@users.localhost') ThreadLocalODMSession.flush_all() @@ -306,6 +308,16 @@ def clear_all_database_tables(): continue db.drop_collection(coll) +# this re-uses hashes for the same pwd, which gives a huge speedup during tests. Not good for real usage, since salting is the same. +user_pwd_hash_speedup_cache = {} + +def fast_set_pwd(user: M.User, password: str): + if password in user_pwd_hash_speedup_cache: + user.password, user.password_algorithm = user_pwd_hash_speedup_cache[password] + user.last_password_updated = datetime.utcnow() + else: + user.set_password(password) + user_pwd_hash_speedup_cache[password] = (user.password, user.password_algorithm) def create_user(display_name, username=None, password='foo', make_project=False): # noqa: S107 if not username: @@ -321,7 +333,7 @@ def create_user(display_name, username=None, password='foo', make_project=False) em.confirmed = True em.set_nonce_hash() user.set_pref('email_address',email) - user.set_password(password) + fast_set_pwd(user, password) return user diff --git a/Allura/development.ini b/Allura/development.ini index 25384c647..b91356088 100644 --- a/Allura/development.ini +++ b/Allura/development.ini @@ -164,6 +164,18 @@ auth.post_logout_url = / auth.min_password_len = 6 auth.max_password_len = 30 +; supported values include argon2, scrypt, bcrypt_sha256, pbkdf2_sha512, sha512_crypt https://passlib.readthedocs.io/en/stable/lib/passlib.hash.html#active-hashes +; argon2 and bcrypt need additional packages, see requirements-optional.txt +; https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#password-hashing-algorithms +; https://passlib.readthedocs.io/en/stable/narr/quickstart.html#choosing-a-hash +auth.password.algorithm = scrypt +; list of previously-used algorithms to support when verifying passwords. Upon successful login, the password will be rehashed with the current algorithm +auth.password.algorithm.old = allura_sha256 +; By default passlib's recommended values will be used for rounds & salts, but they may be too low. +; Override them if needed, to match OWASP.org recommendations for example +; auth.password.rounds = +; auth.password.salt_len = + ; password expiration options (disabled if neither is set) ;auth.pwdexpire.days = 1 ; unix timestamp: @@ -177,9 +189,17 @@ auth.ldap.admin_dn = cn=admin,dc=localdomain auth.ldap.admin_password = secret auth.ldap.schroot_name = scm auth.ldap.use_schroot = false -auth.ldap.password.algorithm = 6 -auth.ldap.password.rounds = 6000 -auth.ldap.password.salt_len = 16 +; supported values include ldap_bcrypt, ldap_pbkdf2_sha512, ldap_pbkdf2_sha256, ldap_sha512_crypt https://passlib.readthedocs.io/en/stable/lib/passlib.hash.html#standard-ldap-hashes +; bcrypt needs additional packages, see requirements-optional.txt +; Legacy "2b" and "5" and "6" mean ldap_bcrypt, ldap_sha256_crypt and ldap_sha512_crypt respectively +; https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#password-hashing-algorithms +; https://passlib.readthedocs.io/en/stable/narr/quickstart.html#choosing-a-hash +auth.ldap.password.algorithm = ldap_pbkdf2_sha512 +; By default passlib's recommended values will be used for rounds & salts, but they may be too low. +; Override them if needed, to match OWASP.org recommendations for example +; auth.ldap.password.rounds = 6000 +; auth.ldap.password.salt_len = 16 + ; "autoregister" allows users to log in to Allura with an existing LDAP account ; If using ldap, with autoregister, you should also set "allow_user_registration" ; to false below. diff --git a/pytest.ini b/pytest.ini index ffc5fc09b..f5490508c 100644 --- a/pytest.ini +++ b/pytest.ini @@ -37,9 +37,13 @@ filterwarnings = ignore:pkg_resources is deprecated as an API:DeprecationWarning:formencode # supporting py3.8 still then can revert https://sourceforge.net/p/activitystream/code/ci/c0884668ac0f4445acb423edb25d18b7bd368be7/ ignore:SelectableGroups dict interface is deprecated. Use select.:DeprecationWarning:activitystream + # optional import within passlib + ignore:'crypt' is deprecated:DeprecationWarning:passlib.utils + ignore:Accessing argon2.__version__ is deprecated:DeprecationWarning:passlib.handlers.argon2 # py3.12 ignore::DeprecationWarning:smtpd + ignore:the imp module is deprecated:DeprecationWarning:mercurial.utils.resourceutil # py3.13 ignore:'cgi' is deprecated:DeprecationWarning:webob.compat diff --git a/requirements-optional.txt b/requirements-optional.txt index f68ec31a2..44d140829 100644 --- a/requirements-optional.txt +++ b/requirements-optional.txt @@ -9,3 +9,7 @@ mediawiki2html # GPL # for spam checking akismet>=1.3 + +# for this type of password hashing +bcrypt +argon2-cffi diff --git a/requirements.in b/requirements.in index a8c041986..98d20f933 100644 --- a/requirements.in +++ b/requirements.in @@ -20,6 +20,7 @@ MarkupSafe Ming oauthlib paginate +passlib Paste PasteDeploy PasteScript diff --git a/requirements.txt b/requirements.txt index be44e8c04..b762cfd7a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -17,9 +17,7 @@ beautifulsoup4==4.12.3 # -r requirements.in # webtest bleach[css]==6.1.0 - # via - # bleach - # pypeline + # via pypeline certifi==2024.7.4 # via requests cffi==1.16.0 @@ -125,6 +123,8 @@ packaging==24.0 # pytest-sugar paginate==0.5.6 # via -r requirements.in +passlib==1.7.4 + # via -r requirements.in paste==3.9.0 # via # -r requirements.in