From 58b2cdc4288854481e8914b3a1b4318708906e15 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Thu, 21 Jan 2021 20:01:57 +0100 Subject: [PATCH 01/17] Don't do more work than necessary --- core/admin/mailu/internal/nginx.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/core/admin/mailu/internal/nginx.py b/core/admin/mailu/internal/nginx.py index 1e0b16c2..de4248fa 100644 --- a/core/admin/mailu/internal/nginx.py +++ b/core/admin/mailu/internal/nginx.py @@ -49,11 +49,14 @@ def handle_authentication(headers): user = models.User.query.get(user_email) status = False if user: - for token in user.tokens: - if (token.check_password(password) and - (not token.ip or token.ip == ip)): - status = True - if user.check_password(password): + # All tokens are 32 characters hex lowercase + if len(password) == 32: + for token in user.tokens: + if (token.check_password(password) and + (not token.ip or token.ip == ip)): + status = True + break + if not status and user.check_password(password): status = True if status: if protocol == "imap" and not user.enable_imap: From eb7895bd1cf5ae41ccfda384f06480767ed75172 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Sun, 7 Feb 2021 17:08:52 +0100 Subject: [PATCH 02/17] Don't do more work than necessary (/webdav) This is also fixing tokens on /webdav/ --- core/admin/mailu/internal/views/auth.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/core/admin/mailu/internal/views/auth.py b/core/admin/mailu/internal/views/auth.py index 825dba56..26d57b3d 100644 --- a/core/admin/mailu/internal/views/auth.py +++ b/core/admin/mailu/internal/views/auth.py @@ -53,10 +53,22 @@ def basic_authentication(): encoded = authorization.replace("Basic ", "") user_email, password = base64.b64decode(encoded).split(b":") user = models.User.query.get(user_email.decode("utf8")) - if user and user.enabled and user.check_password(password.decode("utf8")): - response = flask.Response() - response.headers["X-User"] = user.email - return response + if user and user.enabled: + password = password.decode('utf-8') + status = False + # All tokens are 32 characters hex lowercase + if len(password) == 32: + for token in user.tokens: + if (token.check_password(password) and + (not token.ip or token.ip == ip)): + status = True + break + if not status and user.check_password(password): + status = True + if status: + response = flask.Response() + response.headers["X-User"] = user.email + return response response = flask.Response(status=401) response.headers["WWW-Authenticate"] = 'Basic realm="Login Required"' return response From 00b001f76b7818f2561b9212fbe22edad113a970 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Thu, 21 Jan 2021 20:37:25 +0100 Subject: [PATCH 03/17] Improve the token storage format shortcomings of the previous format included: - 1000x slower than it should be (no point in adding rounds since there is enough entropy: they are not bruteforceable) - vulnerable to DoS as explained in https://passlib.readthedocs.io/en/stable/lib/passlib.hash.sha256_crypt.html#security-issues --- core/admin/mailu/models.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/core/admin/mailu/models.py b/core/admin/mailu/models.py index bbc00f2d..164312ad 100644 --- a/core/admin/mailu/models.py +++ b/core/admin/mailu/models.py @@ -493,10 +493,18 @@ class Token(Base): ip = db.Column(db.String(255)) def check_password(self, password): - return hash.sha256_crypt.verify(password, self.password) + if self.password.startswith("$5$"): + if hash.sha256_crypt.verify(password, self.password): + self.set_password(password) + db.session.add(self) + db.session.commit() + return True + return False + return hash.pbkdf2_sha256.verify(password, self.password) def set_password(self, password): - self.password = hash.sha256_crypt.using(rounds=1000).hash(password) + # tokens have 128bits of entropy, they are not bruteforceable + self.password = hash.pbkdf2_sha256.using(rounds=1).hash(password) def __str__(self): return self.comment From 7137ba6ff18c87185e25b9913377d8e7ce3fa8b6 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Tue, 2 Feb 2021 20:10:18 +0100 Subject: [PATCH 04/17] Misc improvements to PASSWORD_SCHEME - remove PASSWORD_SCHEME altogether - introduce CREDENTIAL_ROUNDS - migrate all old hashes to the current format - auto-detect/enable all hash types that passlib supports - upgrade passlib to 1.7.4 (see #1706: ldap_salted_sha512 support) --- core/admin/mailu/configuration.py | 2 +- core/admin/mailu/manage.py | 24 +++++++-------------- core/admin/mailu/models.py | 35 ++++++++++++++++++++----------- core/admin/requirements-prod.txt | 2 +- docs/cli.rst | 1 - docs/compose/.env | 5 ++--- docs/configuration.rst | 4 +--- 7 files changed, 35 insertions(+), 38 deletions(-) diff --git a/core/admin/mailu/configuration.py b/core/admin/mailu/configuration.py index 3d4d8668..cdee1084 100644 --- a/core/admin/mailu/configuration.py +++ b/core/admin/mailu/configuration.py @@ -33,6 +33,7 @@ DEFAULT_CONFIG = { 'TLS_FLAVOR': 'cert', 'AUTH_RATELIMIT': '10/minute;1000/hour', 'AUTH_RATELIMIT_SUBNET': True, + 'CREDENTIAL_ROUNDS': 12, 'DISABLE_STATISTICS': False, # Mail settings 'DMARC_RUA': None, @@ -52,7 +53,6 @@ DEFAULT_CONFIG = { 'RECAPTCHA_PUBLIC_KEY': '', 'RECAPTCHA_PRIVATE_KEY': '', # Advanced settings - 'PASSWORD_SCHEME': 'PBKDF2', 'LOG_LEVEL': 'WARNING', 'SESSION_COOKIE_SECURE': True, # Host settings diff --git a/core/admin/mailu/manage.py b/core/admin/mailu/manage.py index 62f214d3..9c576404 100644 --- a/core/admin/mailu/manage.py +++ b/core/admin/mailu/manage.py @@ -86,13 +86,10 @@ def admin(localpart, domain_name, password, mode='create'): @click.argument('localpart') @click.argument('domain_name') @click.argument('password') -@click.argument('hash_scheme', required=False) @flask_cli.with_appcontext -def user(localpart, domain_name, password, hash_scheme=None): +def user(localpart, domain_name, password): """ Create a user """ - if hash_scheme is None: - hash_scheme = app.config['PASSWORD_SCHEME'] domain = models.Domain.query.get(domain_name) if not domain: domain = models.Domain(name=domain_name) @@ -102,7 +99,7 @@ def user(localpart, domain_name, password, hash_scheme=None): domain=domain, global_admin=False ) - user.set_password(password, hash_scheme=hash_scheme) + user.set_password(password) db.session.add(user) db.session.commit() @@ -111,17 +108,14 @@ def user(localpart, domain_name, password, hash_scheme=None): @click.argument('localpart') @click.argument('domain_name') @click.argument('password') -@click.argument('hash_scheme', required=False) @flask_cli.with_appcontext -def password(localpart, domain_name, password, hash_scheme=None): +def password(localpart, domain_name, password): """ Change the password of an user """ email = '{0}@{1}'.format(localpart, domain_name) user = models.User.query.get(email) - if hash_scheme is None: - hash_scheme = app.config['PASSWORD_SCHEME'] if user: - user.set_password(password, hash_scheme=hash_scheme) + user.set_password(password) else: print("User " + email + " not found.") db.session.commit() @@ -148,13 +142,10 @@ def domain(domain_name, max_users=-1, max_aliases=-1, max_quota_bytes=0): @click.argument('localpart') @click.argument('domain_name') @click.argument('password_hash') -@click.argument('hash_scheme') @flask_cli.with_appcontext -def user_import(localpart, domain_name, password_hash, hash_scheme = None): +def user_import(localpart, domain_name, password_hash): """ Import a user along with password hash. """ - if hash_scheme is None: - hash_scheme = app.config['PASSWORD_SCHEME'] domain = models.Domain.query.get(domain_name) if not domain: domain = models.Domain(name=domain_name) @@ -164,7 +155,7 @@ def user_import(localpart, domain_name, password_hash, hash_scheme = None): domain=domain, global_admin=False ) - user.set_password(password_hash, hash_scheme=hash_scheme, raw=True) + user.set_password(password_hash, raw=True) db.session.add(user) db.session.commit() @@ -217,7 +208,6 @@ def config_update(verbose=False, delete_objects=False): localpart = user_config['localpart'] domain_name = user_config['domain'] password_hash = user_config.get('password_hash', None) - hash_scheme = user_config.get('hash_scheme', None) domain = models.Domain.query.get(domain_name) email = '{0}@{1}'.format(localpart, domain_name) optional_params = {} @@ -239,7 +229,7 @@ def config_update(verbose=False, delete_objects=False): else: for k in optional_params: setattr(user, k, optional_params[k]) - user.set_password(password_hash, hash_scheme=hash_scheme, raw=True) + user.set_password(password_hash, raw=True) db.session.add(user) aliases = new_config.get('aliases', []) diff --git a/core/admin/mailu/models.py b/core/admin/mailu/models.py index 164312ad..905af4a2 100644 --- a/core/admin/mailu/models.py +++ b/core/admin/mailu/models.py @@ -1,7 +1,7 @@ from mailu import dkim from sqlalchemy.ext import declarative -from passlib import context, hash +from passlib import context, hash, registry from datetime import datetime, date from email.mime import text from flask import current_app as app @@ -370,17 +370,30 @@ class User(Base, Email): 'CRYPT': "des_crypt"} def get_password_context(self): + schemes = registry.list_crypt_handlers() + # scrypt throws a warning if the native wheels aren't found + schemes.remove('scrypt') + # we can't leave plaintext schemes as they will be misidentified + for scheme in schemes: + if scheme.endswith('plaintext'): + schemes.remove(scheme) return context.CryptContext( - schemes=self.scheme_dict.values(), - default=self.scheme_dict[app.config['PASSWORD_SCHEME']], + schemes=schemes, + default='bcrypt_sha256', + bcrypt_sha256__rounds=app.config['CREDENTIAL_ROUNDS'], + deprecated='auto' ) def check_password(self, password): context = self.get_password_context() - reference = re.match('({[^}]+})?(.*)', self.password).group(2) - result = context.verify(password, reference) - if result and context.identify(reference) != context.default_scheme(): - self.set_password(password) + # {scheme} will most likely be migrated on first use + reference = self.password + if self.password.startswith("{"): + reference = re.match('({[^}]+})?(.*)', reference).group(2) + + result, new_hash = context.verify_and_update(password, reference) + if new_hash: + self.password = new_hash db.session.add(self) db.session.commit() return result @@ -389,13 +402,11 @@ class User(Base, Email): """Set password for user with specified encryption scheme @password: plain text password to encrypt (if raw == True the hash itself) """ - if hash_scheme is None: - hash_scheme = app.config['PASSWORD_SCHEME'] - # for the list of hash schemes see https://wiki2.dovecot.org/Authentication/PasswordSchemes if raw: - self.password = '{'+hash_scheme+'}' + password + self.password = password else: - self.password = '{'+hash_scheme+'}' + self.get_password_context().encrypt(password, self.scheme_dict[hash_scheme]) + self.password = self.get_password_context().hash(password) + app.cache.delete(self.get_id()) def get_managed_domains(self): if self.global_admin: diff --git a/core/admin/requirements-prod.txt b/core/admin/requirements-prod.txt index a3c32855..f767f431 100644 --- a/core/admin/requirements-prod.txt +++ b/core/admin/requirements-prod.txt @@ -29,7 +29,7 @@ limits==1.3 Mako==1.0.9 MarkupSafe==1.1.1 mysqlclient==1.4.2.post1 -passlib==1.7.1 +passlib==1.7.4 psycopg2==2.8.2 pycparser==2.19 pyOpenSSL==19.0.0 diff --git a/docs/cli.rst b/docs/cli.rst index a9cff41c..8e94026b 100644 --- a/docs/cli.rst +++ b/docs/cli.rst @@ -85,7 +85,6 @@ where mail-config.yml looks like: - localpart: foo domain: example.com password_hash: klkjhumnzxcjkajahsdqweqqwr - hash_scheme: MD5-CRYPT aliases: - localpart: alias1 diff --git a/docs/compose/.env b/docs/compose/.env index 7f91c270..432b20b0 100644 --- a/docs/compose/.env +++ b/docs/compose/.env @@ -144,9 +144,8 @@ LOG_DRIVER=json-file # Docker-compose project name, this will prepended to containers names. COMPOSE_PROJECT_NAME=mailu -# Default password scheme used for newly created accounts and changed passwords -# (value: PBKDF2, BLF-CRYPT, SHA512-CRYPT, SHA256-CRYPT) -PASSWORD_SCHEME=PBKDF2 +# Number of rounds used by the password hashing scheme +CREDENTIAL_ROUNDS=12 # Header to take the real ip from REAL_IP_HEADER= diff --git a/docs/configuration.rst b/docs/configuration.rst index 9123054c..bc2027c6 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -138,9 +138,7 @@ Depending on your particular deployment you most probably will want to change th Advanced settings ----------------- -The ``PASSWORD_SCHEME`` is the password encryption scheme. You should use the -default value, unless you are importing password from a separate system and -want to keep using the old password encryption scheme. +The ``CREDENTIAL_ROUNDS`` (default: 12) is the number of rounds used by the password hashing scheme. You should use the default value. The ``SESSION_COOKIE_SECURE`` (default: True) setting controls the secure flag on the cookies of the administrative interface. It should only be turned off if you intend to access it over plain HTTP. From 57a6abaf50400802be5da48913c75cff00dce6f4 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Sun, 7 Feb 2021 09:31:07 +0100 Subject: [PATCH 05/17] Remove {scheme} from the DB if mailu has set it --- core/admin/mailu/models.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/core/admin/mailu/models.py b/core/admin/mailu/models.py index 905af4a2..9ab9088e 100644 --- a/core/admin/mailu/models.py +++ b/core/admin/mailu/models.py @@ -386,10 +386,14 @@ class User(Base, Email): def check_password(self, password): context = self.get_password_context() - # {scheme} will most likely be migrated on first use reference = self.password + # strip {scheme} if that's something mailu has added + # passlib will identify *crypt based hashes just fine + # on its own if self.password.startswith("{"): - reference = re.match('({[^}]+})?(.*)', reference).group(2) + scheme = self.password.split('}')[0][1:] + if scheme in scheme_dict: + reference = reference[len(scheme)+2:] result, new_hash = context.verify_and_update(password, reference) if new_hash: From 927bd2bd8ec8051e93609d3e8fd24706cc8dc8a2 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Sun, 7 Feb 2021 17:29:58 +0100 Subject: [PATCH 06/17] towncrier --- towncrier/newsfragments/1194.misc | 1 + towncrier/newsfragments/1662.feature | 1 + 2 files changed, 2 insertions(+) create mode 100644 towncrier/newsfragments/1194.misc create mode 100644 towncrier/newsfragments/1662.feature diff --git a/towncrier/newsfragments/1194.misc b/towncrier/newsfragments/1194.misc new file mode 100644 index 00000000..7cbf2b94 --- /dev/null +++ b/towncrier/newsfragments/1194.misc @@ -0,0 +1 @@ +Switch to bcrypt_sha256, remove PASSWORD_SCHEME and replace it with CREDENTIAL_ROUNDS diff --git a/towncrier/newsfragments/1662.feature b/towncrier/newsfragments/1662.feature new file mode 100644 index 00000000..4fc8b2fd --- /dev/null +++ b/towncrier/newsfragments/1662.feature @@ -0,0 +1 @@ +Enable support of all hash types passlib supports. Convert them to the default scheme on first use. From fda758e2b4ea19751369dd122797c5210e18dc15 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Sun, 7 Feb 2021 17:34:22 +0100 Subject: [PATCH 07/17] remove merge artifact --- core/admin/mailu/models.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/admin/mailu/models.py b/core/admin/mailu/models.py index 9ab9088e..b7a4d501 100644 --- a/core/admin/mailu/models.py +++ b/core/admin/mailu/models.py @@ -392,7 +392,7 @@ class User(Base, Email): # on its own if self.password.startswith("{"): scheme = self.password.split('}')[0][1:] - if scheme in scheme_dict: + if scheme in self.scheme_dict: reference = reference[len(scheme)+2:] result, new_hash = context.verify_and_update(password, reference) @@ -410,7 +410,6 @@ class User(Base, Email): self.password = password else: self.password = self.get_password_context().hash(password) - app.cache.delete(self.get_id()) def get_managed_domains(self): if self.global_admin: From 89d88e0c19476692abef0b0b13c4765f2db7bacb Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Mon, 8 Feb 2021 08:50:32 +0100 Subject: [PATCH 08/17] Fix the test --- tests/compose/core/00_create_users.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/compose/core/00_create_users.sh b/tests/compose/core/00_create_users.sh index 49d0511b..f5108302 100755 --- a/tests/compose/core/00_create_users.sh +++ b/tests/compose/core/00_create_users.sh @@ -6,6 +6,6 @@ echo "The above error was intended!" docker-compose -f tests/compose/core/docker-compose.yml exec -T admin flask mailu admin admin mailu.io 'FooBar' --mode=ifmissing || exit 1 # Should not fail and update the password; update mode docker-compose -f tests/compose/core/docker-compose.yml exec -T admin flask mailu admin admin mailu.io 'password' --mode=update || exit 1 -docker-compose -f tests/compose/core/docker-compose.yml exec -T admin flask mailu user user mailu.io 'password' 'SHA512-CRYPT' || exit 1 -docker-compose -f tests/compose/core/docker-compose.yml exec -T admin flask mailu user 'user/with/slash' mailu.io 'password' 'SHA512-CRYPT' || exit 1 +docker-compose -f tests/compose/core/docker-compose.yml exec -T admin flask mailu user user mailu.io 'password' || exit 1 +docker-compose -f tests/compose/core/docker-compose.yml exec -T admin flask mailu user 'user/with/slash' mailu.io 'password' || exit 1 echo "User testing succesfull!" From 29306d5abbd17ebd9b479d9438ccfa5cc00a3de6 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Mon, 8 Feb 2021 08:56:03 +0100 Subject: [PATCH 09/17] Fix the tests (again) --- tests/compose/core/02_forward_test.sh | 2 -- tests/compose/core/04_reply_test.sh | 2 -- 2 files changed, 4 deletions(-) diff --git a/tests/compose/core/02_forward_test.sh b/tests/compose/core/02_forward_test.sh index 595820cf..a53fa459 100755 --- a/tests/compose/core/02_forward_test.sh +++ b/tests/compose/core/02_forward_test.sh @@ -2,7 +2,6 @@ cat << EOF | docker-compose -f tests/compose/core/docker-compose.yml exec -T adm users: - localpart: forwardinguser password_hash: "\$1\$F2OStvi1\$Q8hBIHkdJpJkJn/TrMIZ9/" - hash_scheme: MD5-CRYPT domain: mailu.io forward_enabled: true forward_destination: ["user@mailu.io"] @@ -14,7 +13,6 @@ cat << EOF | docker-compose -f tests/compose/core/docker-compose.yml exec -T adm users: - localpart: forwardinguser password_hash: "\$1\$F2OStvi1\$Q8hBIHkdJpJkJn/TrMIZ9/" - hash_scheme: MD5-CRYPT domain: mailu.io forward_enabled: false forward_destination: [] diff --git a/tests/compose/core/04_reply_test.sh b/tests/compose/core/04_reply_test.sh index 83c114f6..e1479cf0 100755 --- a/tests/compose/core/04_reply_test.sh +++ b/tests/compose/core/04_reply_test.sh @@ -2,7 +2,6 @@ cat << EOF | docker-compose -f tests/compose/core/docker-compose.yml exec -T adm users: - localpart: replyuser password_hash: "\$1\$F2OStvi1\$Q8hBIHkdJpJkJn/TrMIZ9/" - hash_scheme: MD5-CRYPT domain: mailu.io reply_enabled: true reply_subject: This will not reach me @@ -15,7 +14,6 @@ cat << EOF | docker-compose -f tests/compose/core/docker-compose.yml exec -T adm users: - localpart: replyuser password_hash: "\$1\$F2OStvi1\$Q8hBIHkdJpJkJn/TrMIZ9/" - hash_scheme: MD5-CRYPT domain: mailu.io reply_enabled: false EOF From d0b34f8e240a1049ed5e1ccd3399af3ff18236e2 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Tue, 9 Feb 2021 08:56:06 +0100 Subject: [PATCH 10/17] Move CREDENTIAL_ROUNDS to advanced settings --- core/admin/mailu/configuration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/admin/mailu/configuration.py b/core/admin/mailu/configuration.py index cdee1084..429e778c 100644 --- a/core/admin/mailu/configuration.py +++ b/core/admin/mailu/configuration.py @@ -33,7 +33,6 @@ DEFAULT_CONFIG = { 'TLS_FLAVOR': 'cert', 'AUTH_RATELIMIT': '10/minute;1000/hour', 'AUTH_RATELIMIT_SUBNET': True, - 'CREDENTIAL_ROUNDS': 12, 'DISABLE_STATISTICS': False, # Mail settings 'DMARC_RUA': None, @@ -55,6 +54,7 @@ DEFAULT_CONFIG = { # Advanced settings 'LOG_LEVEL': 'WARNING', 'SESSION_COOKIE_SECURE': True, + 'CREDENTIAL_ROUNDS': 12, # Host settings 'HOST_IMAP': 'imap', 'HOST_LMTP': 'imap:2525', From f9ed517b394e3d267fcae8f960e2299df9bb389f Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Tue, 9 Feb 2021 09:02:09 +0100 Subject: [PATCH 11/17] Be specific token length --- core/admin/mailu/ui/views/tokens.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/admin/mailu/ui/views/tokens.py b/core/admin/mailu/ui/views/tokens.py index 069587e1..820dd405 100644 --- a/core/admin/mailu/ui/views/tokens.py +++ b/core/admin/mailu/ui/views/tokens.py @@ -26,7 +26,7 @@ def token_create(user_email): form = forms.TokenForm() wtforms_components.read_only(form.displayed_password) if not form.raw_password.data: - form.raw_password.data = pwd.genword(entropy=128, charset="hex") + form.raw_password.data = pwd.genword(entropy=128, length=32, charset="hex") form.displayed_password.data = form.raw_password.data if form.validate_on_submit(): token = models.Token(user=user) From df230cb482777e0b3c06e26174af203b5f3070b7 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Tue, 9 Feb 2021 09:20:02 +0100 Subject: [PATCH 12/17] Refactor auth under nginx.check_credentials() --- core/admin/mailu/internal/nginx.py | 32 ++++++++++++------------- core/admin/mailu/internal/views/auth.py | 20 ++++------------ 2 files changed, 19 insertions(+), 33 deletions(-) diff --git a/core/admin/mailu/internal/nginx.py b/core/admin/mailu/internal/nginx.py index de4248fa..f9ebbf13 100644 --- a/core/admin/mailu/internal/nginx.py +++ b/core/admin/mailu/internal/nginx.py @@ -19,6 +19,20 @@ STATUSES = { }), } +def check_credentials(user, password, ip, protocol=None): + if not user or not user.enabled or (protocol == "imap" and not user.enable_imap) or (protocol == "pop3" and not user.enable_pop): + return False + is_ok = False + # All tokens are 32 characters hex lowercase + if len(password) == 32: + for token in user.tokens: + if (token.check_password(password) and + (not token.ip or token.ip == ip)): + is_ok = True + break + if not is_ok and user.check_password(password): + is_ok = True + return is_ok def handle_authentication(headers): """ Handle an HTTP nginx authentication request @@ -47,23 +61,7 @@ def handle_authentication(headers): password = raw_password.encode("iso8859-1").decode("utf8") ip = urllib.parse.unquote(headers["Client-Ip"]) user = models.User.query.get(user_email) - status = False - if user: - # All tokens are 32 characters hex lowercase - if len(password) == 32: - for token in user.tokens: - if (token.check_password(password) and - (not token.ip or token.ip == ip)): - status = True - break - if not status and user.check_password(password): - status = True - if status: - if protocol == "imap" and not user.enable_imap: - status = False - elif protocol == "pop3" and not user.enable_pop: - status = False - if status and user.enabled: + if check_credentials(user, password, ip, protocol): return { "Auth-Status": "OK", "Auth-Server": server, diff --git a/core/admin/mailu/internal/views/auth.py b/core/admin/mailu/internal/views/auth.py index 26d57b3d..edd62e37 100644 --- a/core/admin/mailu/internal/views/auth.py +++ b/core/admin/mailu/internal/views/auth.py @@ -53,22 +53,10 @@ def basic_authentication(): encoded = authorization.replace("Basic ", "") user_email, password = base64.b64decode(encoded).split(b":") user = models.User.query.get(user_email.decode("utf8")) - if user and user.enabled: - password = password.decode('utf-8') - status = False - # All tokens are 32 characters hex lowercase - if len(password) == 32: - for token in user.tokens: - if (token.check_password(password) and - (not token.ip or token.ip == ip)): - status = True - break - if not status and user.check_password(password): - status = True - if status: - response = flask.Response() - response.headers["X-User"] = user.email - return response + if nginx.check_credentials(user, password.decode('utf-8'), flask.request.remote_addr, "web"): + response = flask.Response() + response.headers["X-User"] = user.email + return response response = flask.Response(status=401) response.headers["WWW-Authenticate"] = 'Basic realm="Login Required"' return response From 20d2b621aa42793eedb55219c01da3b9b8ee32f2 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Tue, 9 Feb 2021 09:33:37 +0100 Subject: [PATCH 13/17] Improve the description of CREDENTIAL_ROUNDS --- docs/configuration.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index bc2027c6..26bdb024 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -138,7 +138,7 @@ Depending on your particular deployment you most probably will want to change th Advanced settings ----------------- -The ``CREDENTIAL_ROUNDS`` (default: 12) is the number of rounds used by the password hashing scheme. You should use the default value. +The ``CREDENTIAL_ROUNDS`` (default: 12) setting is the number of rounds used by the password hashing scheme. The number of rounds can be reduced in case faster authentication is needed or increased when additional protection is desired. Keep in mind that this is a mitigation against offline attacks on password hashes, aiming to prevent credential stuffing (due to password re-use) on other systems. The ``SESSION_COOKIE_SECURE`` (default: True) setting controls the secure flag on the cookies of the administrative interface. It should only be turned off if you intend to access it over plain HTTP. From 45e5cb9bb37b49f294c973b98ed9cf8f0607498b Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Tue, 9 Feb 2021 09:37:13 +0100 Subject: [PATCH 14/17] Improve the towncrier messages --- towncrier/newsfragments/1194.misc | 1 - towncrier/newsfragments/1662.feature | 2 +- towncrier/newsfragments/1753.feature | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) delete mode 100644 towncrier/newsfragments/1194.misc create mode 100644 towncrier/newsfragments/1753.feature diff --git a/towncrier/newsfragments/1194.misc b/towncrier/newsfragments/1194.misc deleted file mode 100644 index 7cbf2b94..00000000 --- a/towncrier/newsfragments/1194.misc +++ /dev/null @@ -1 +0,0 @@ -Switch to bcrypt_sha256, remove PASSWORD_SCHEME and replace it with CREDENTIAL_ROUNDS diff --git a/towncrier/newsfragments/1662.feature b/towncrier/newsfragments/1662.feature index 4fc8b2fd..f8219757 100644 --- a/towncrier/newsfragments/1662.feature +++ b/towncrier/newsfragments/1662.feature @@ -1 +1 @@ -Enable support of all hash types passlib supports. Convert them to the default scheme on first use. +Enable support of all hash types passlib supports. diff --git a/towncrier/newsfragments/1753.feature b/towncrier/newsfragments/1753.feature new file mode 100644 index 00000000..09eb834a --- /dev/null +++ b/towncrier/newsfragments/1753.feature @@ -0,0 +1 @@ +Switch to bcrypt_sha256, replace PASSWORD_SCHEME with CREDENTIAL_ROUNDS and dynamically update existing hashes on first login From 1c5b58cba4da0b411aa4cd6bb911196e8a0fbf25 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Tue, 9 Feb 2021 11:19:28 +0100 Subject: [PATCH 15/17] Remove scheme_dict --- core/admin/mailu/models.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/core/admin/mailu/models.py b/core/admin/mailu/models.py index b7a4d501..fab2103a 100644 --- a/core/admin/mailu/models.py +++ b/core/admin/mailu/models.py @@ -362,13 +362,6 @@ class User(Base, Email): self.reply_enddate > now ) - scheme_dict = {'PBKDF2': "pbkdf2_sha512", - 'BLF-CRYPT': "bcrypt", - 'SHA512-CRYPT': "sha512_crypt", - 'SHA256-CRYPT': "sha256_crypt", - 'MD5-CRYPT': "md5_crypt", - 'CRYPT': "des_crypt"} - def get_password_context(self): schemes = registry.list_crypt_handlers() # scrypt throws a warning if the native wheels aren't found @@ -392,7 +385,7 @@ class User(Base, Email): # on its own if self.password.startswith("{"): scheme = self.password.split('}')[0][1:] - if scheme in self.scheme_dict: + if scheme in ['PBKDF2', 'BLF-CRYPT', 'SHA512-CRYPT', 'SHA256-CRYPT', 'MD5-CRYPT', 'CRYPT']: reference = reference[len(scheme)+2:] result, new_hash = context.verify_and_update(password, reference) From 5f05fee8b32209d4888de324cc3e3a578ff83715 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Tue, 9 Feb 2021 11:23:49 +0100 Subject: [PATCH 16/17] Don't need regexps anymore --- core/admin/mailu/models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/core/admin/mailu/models.py b/core/admin/mailu/models.py index fab2103a..c8426fa2 100644 --- a/core/admin/mailu/models.py +++ b/core/admin/mailu/models.py @@ -8,7 +8,6 @@ from flask import current_app as app import flask_sqlalchemy import sqlalchemy -import re import time import os import glob From 96ae54d04db3ff26edf8413e99dc980d610562a8 Mon Sep 17 00:00:00 2001 From: Florent Daigniere Date: Thu, 11 Feb 2021 23:14:09 +0100 Subject: [PATCH 17/17] CryptContext should be a singleton --- core/admin/mailu/models.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/core/admin/mailu/models.py b/core/admin/mailu/models.py index c8426fa2..a63c33a5 100644 --- a/core/admin/mailu/models.py +++ b/core/admin/mailu/models.py @@ -304,6 +304,7 @@ class User(Base, Email): """ A user is an email address that has a password to access a mailbox. """ __tablename__ = "user" + _ctx = None domain = db.relationship(Domain, backref=db.backref('users', cascade='all, delete-orphan')) @@ -361,7 +362,10 @@ class User(Base, Email): self.reply_enddate > now ) - def get_password_context(self): + def get_password_context(): + if User._ctx: + return User._ctx + schemes = registry.list_crypt_handlers() # scrypt throws a warning if the native wheels aren't found schemes.remove('scrypt') @@ -369,15 +373,15 @@ class User(Base, Email): for scheme in schemes: if scheme.endswith('plaintext'): schemes.remove(scheme) - return context.CryptContext( + User._ctx = context.CryptContext( schemes=schemes, default='bcrypt_sha256', bcrypt_sha256__rounds=app.config['CREDENTIAL_ROUNDS'], deprecated='auto' ) + return User._ctx def check_password(self, password): - context = self.get_password_context() reference = self.password # strip {scheme} if that's something mailu has added # passlib will identify *crypt based hashes just fine @@ -387,7 +391,7 @@ class User(Base, Email): if scheme in ['PBKDF2', 'BLF-CRYPT', 'SHA512-CRYPT', 'SHA256-CRYPT', 'MD5-CRYPT', 'CRYPT']: reference = reference[len(scheme)+2:] - result, new_hash = context.verify_and_update(password, reference) + result, new_hash = User.get_password_context().verify_and_update(password, reference) if new_hash: self.password = new_hash db.session.add(self) @@ -401,7 +405,7 @@ class User(Base, Email): if raw: self.password = password else: - self.password = self.get_password_context().hash(password) + self.password = User.get_password_context().hash(password) def get_managed_domains(self): if self.global_admin: