Merge #1753

1753: Better password storage r=nextgens a=nextgens

## What type of PR?

Enhancement: optimization of the logic to speedup authentication requests, support the import of most hashes passlib supports.

## What does this PR do?

- it changes the default password cold-storage format to sha256+bcrypt
- it enhances the logic to ensure that no CPU cycles are wasted when valid credentials are found
- it fixes token authentication on /webdav/
- it lowers the number of rounds used for token storage (on the basis that they are high-entropy: not bruteforceable and speed matters)
- it introduces a new setting to set the number of rounds used by the password hashing function (CREDENTIAL_ROUNDS). The setting can be adjusted as required and existing hashes will be migrated to the new cost-factor.
- it updates the version of passlib in use and enables all supported hash types (that will be converted to the current settings on first use)
- it removes the PASSWORD_SCHEME setting

### Related issue(s)
- close #1194
- close #1662
- close #1706

## Prerequistes
Before we can consider review and merge, please make sure the following list is done and checked.
If an entry in not applicable, you can check it or remove it from the list.

- [x] In case of feature or enhancement: documentation updated accordingly
- [x] Unless it's docs or a minor change: add [changelog](https://mailu.io/master/contributors/guide.html#changelog) entry file.


Co-authored-by: Florent Daigniere <nextgens@freenetproject.org>

core/admin/mailu/configuration.py

@@ -52,9 +52,9 @@ DEFAULT_CONFIG = {
     'RECAPTCHA_PUBLIC_KEY': '',
     'RECAPTCHA_PRIVATE_KEY': '',
     # Advanced settings
-    'PASSWORD_SCHEME': 'PBKDF2',
     'LOG_LEVEL': 'WARNING',
     'SESSION_COOKIE_SECURE': True,
+    'CREDENTIAL_ROUNDS': 12,
     # Host settings
     'HOST_IMAP': 'imap',
     'HOST_LMTP': 'imap:2525',

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,20 +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 check_credentials(user, password, ip, protocol):
-        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):
-                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:
             return {
                 "Auth-Status": "OK",
                 "Auth-Server": server,

core/admin/mailu/internal/views/auth.py

@@ -53,7 +53,7 @@ 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")):
+        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

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', [])

core/admin/mailu/models.py

@@ -1,14 +1,13 @@
 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
 
 import flask_sqlalchemy
 import sqlalchemy
-import re
 import time
 import os
 import glob
@@ -305,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'))
@@ -362,25 +362,38 @@ class User(Base, Email):
             self.reply_enddate > now
         )
 
-    scheme_dict = {'PBKDF2': "pbkdf2_sha512",
+    def get_password_context():
-                   'BLF-CRYPT': "bcrypt",
+        if User._ctx:
-                   'SHA512-CRYPT': "sha512_crypt",
+            return User._ctx
-                   'SHA256-CRYPT': "sha256_crypt",
-                   'MD5-CRYPT': "md5_crypt",
-                   'CRYPT': "des_crypt"}
 
-    def get_password_context(self):
+        schemes = registry.list_crypt_handlers()
-        return context.CryptContext(
+        # scrypt throws a warning if the native wheels aren't found
-            schemes=self.scheme_dict.values(),
+        schemes.remove('scrypt')
-            default=self.scheme_dict[app.config['PASSWORD_SCHEME']],
+        # we can't leave plaintext schemes as they will be misidentified
+        for scheme in schemes:
+            if scheme.endswith('plaintext'):
+                schemes.remove(scheme)
+        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
-        reference = re.match('({[^}]+})?(.*)', self.password).group(2)
+        # strip {scheme} if that's something mailu has added
-        result = context.verify(password, reference)
+        # passlib will identify *crypt based hashes just fine
-        if result and context.identify(reference) != context.default_scheme():
+        # on its own
-            self.set_password(password)
+        if self.password.startswith("{"):
+            scheme = self.password.split('}')[0][1:]
+            if scheme in ['PBKDF2', 'BLF-CRYPT', 'SHA512-CRYPT', 'SHA256-CRYPT', 'MD5-CRYPT', 'CRYPT']:
+                reference = reference[len(scheme)+2:]
+
+        result, new_hash = User.get_password_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,10 @@ 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 = User.get_password_context().hash(password)
 
     def get_managed_domains(self):
         if self.global_admin:
@@ -493,10 +503,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

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)

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

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

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
+# Number of rounds used by the password hashing scheme
-# (value: PBKDF2, BLF-CRYPT, SHA512-CRYPT, SHA256-CRYPT)
+CREDENTIAL_ROUNDS=12
-PASSWORD_SCHEME=PBKDF2
 
 # Header to take the real ip from
 REAL_IP_HEADER=

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
+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.
-default value, unless you are importing password from a separate system and
-want to keep using the old password encryption scheme.
 
 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.
 

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 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' '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' || exit 1
 echo "User testing succesfull!"

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: []

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

towncrier/newsfragments/1662.feature

@@ -0,0 +1 @@
+Enable support of all hash types passlib supports.

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