Merge #2094

2094: Sessions tweaks r=mergify[bot] a=nextgens

## What type of PR?

bug-fix

## What does this PR do?

- Make all sessions permanent, introduce SESSION_TIMEOUT and PERMANENT_SESSION_LIFETIME.
- Prevent the creation of a session before there is a login attempt
- Ensure that webmail tokens are in sync with sessions

### Related issue(s)
- close #2080 

## Prerequisites
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/workflow.html#changelog) entry file.


Co-authored-by: Florent Daigniere <nextgens@freenetproject.org>
Co-authored-by: Dimitri Huisman <diman@huisman.xyz>

core/admin/mailu/configuration.py

@@ -70,7 +70,8 @@ DEFAULT_CONFIG = {
     # Advanced settings
     'LOG_LEVEL': 'WARNING',
     'SESSION_KEY_BITS': 128,
-    'SESSION_LIFETIME': 24,
+    'SESSION_TIMEOUT': 3600,
+    'PERMANENT_SESSION_LIFETIME': 30*24*3600,
     'SESSION_COOKIE_SECURE': True,
     'CREDENTIAL_ROUNDS': 12,
     'TZ': 'Etc/UTC',
@@ -152,7 +153,11 @@ class ConfigManager:
         self.config['SESSION_STORAGE_URL'] = f'redis://{self.config["REDIS_ADDRESS"]}/3'
         self.config['SESSION_COOKIE_SAMESITE'] = 'Strict'
         self.config['SESSION_COOKIE_HTTPONLY'] = True
-        self.config['PERMANENT_SESSION_LIFETIME'] = timedelta(hours=int(self.config['SESSION_LIFETIME']))
+        self.config['SESSION_PERMANENT'] = True
+        self.config['SESSION_TIMEOUT'] = int(self.config['SESSION_TIMEOUT'])
+        self.config['PERMANENT_SESSION_LIFETIME'] = int(self.config['PERMANENT_SESSION_LIFETIME'])
+        self.config['AUTH_RATELIMIT_IP_V4_MASK'] = int(self.config['AUTH_RATELIMIT_IP_V4_MASK'])
+        self.config['AUTH_RATELIMIT_IP_V6_MASK'] = int(self.config['AUTH_RATELIMIT_IP_V6_MASK'])
         hostnames = [host.strip() for host in self.config['HOSTNAMES'].split(',')]
         self.config['AUTH_RATELIMIT_EXEMPTION'] = set(ipaddress.ip_network(cidr, False) for cidr in (cidr.strip() for cidr in self.config['AUTH_RATELIMIT_EXEMPTION'].split(',')) if cidr)
         self.config['MESSAGE_RATELIMIT_EXEMPTION'] = set([s for s in self.config['MESSAGE_RATELIMIT_EXEMPTION'].lower().replace(' ', '').split(',') if s])

core/admin/mailu/internal/nginx.py

@@ -1,4 +1,4 @@
-from mailu import models
+from mailu import models, utils
 from flask import current_app as app
 
 import re
@@ -32,8 +32,8 @@ def check_credentials(user, password, ip, protocol=None, auth_port=None):
         return False
     is_ok = False
     # webmails
-    if len(password) == 64 and auth_port in ['10143', '10025']:
-        if user.verify_temp_token(password):
+    if auth_port in ['10143', '10025'] and password.startswith('token-'):
+        if utils.verify_temp_token(user.get_id(), password):
             is_ok = True
     # All tokens are 32 characters hex lowercase
     if not is_ok and len(password) == 32:

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

@@ -68,8 +68,9 @@ def user_authentication():
     if (not flask_login.current_user.is_anonymous
         and flask_login.current_user.enabled):
         response = flask.Response()
-        response.headers["X-User"] = models.IdnaEmail.process_bind_param(flask_login, flask_login.current_user.get_id(), "")
-        response.headers["X-User-Token"] = models.User.get_temp_token(flask_login.current_user.get_id())
+        email = flask_login.current_user.get_id()
+        response.headers["X-User"] = models.IdnaEmail.process_bind_param(flask_login, email, "")
+        response.headers["X-User-Token"] = utils.gen_temp_token(email, flask.session)
         return response
     return flask.abort(403)
 

core/admin/mailu/models.py

@@ -16,7 +16,6 @@ import passlib.hash
 import passlib.registry
 import time
 import os
-import hmac
 import smtplib
 import idna
 import dns.resolver
@@ -645,15 +644,6 @@ in clear-text regardless of the presence of the cache.
         user = cls.query.get(email)
         return user if (user and user.enabled and user.check_password(password)) else None
 
-    @classmethod
-    def get_temp_token(cls, email):
-        user = cls.query.get(email)
-        return hmac.new(app.temp_token_key, bytearray("{}|{}".format(time.strftime('%Y%m%d'), email), 'utf-8'), 'sha256').hexdigest() if (user and user.enabled) else None
-
-    def verify_temp_token(self, token):
-        return hmac.compare_digest(self.get_temp_token(self.email), token)
-
-
 
 class Alias(Base, Email):
     """ An alias is an email address that redirects to some destination.

core/admin/mailu/sso/views/languages.py

@@ -3,5 +3,6 @@ import flask
 
 @sso.route('/language/<language>', methods=['POST'])
 def set_language(language=None):
-    flask.session['language'] = language
+    if language:
+        flask.session['language'] = language
     return flask.Response(status=200)

core/admin/mailu/utils.py

@@ -28,6 +28,7 @@ import flask_babel
 import ipaddress
 import redis
 
+from datetime import datetime, timedelta
 from flask.sessions import SessionMixin, SessionInterface
 from itsdangerous.encoding import want_bytes
 from werkzeug.datastructures import CallbackDict
@@ -78,9 +79,9 @@ limiter = limiter.LimitWraperFactory()
 def extract_network_from_ip(ip):
     n = ipaddress.ip_network(ip)
     if n.version == 4:
-        return str(n.supernet(prefixlen_diff=(32-int(app.config["AUTH_RATELIMIT_IP_V4_MASK"]))).network_address)
+        return str(n.supernet(prefixlen_diff=(32-app.config["AUTH_RATELIMIT_IP_V4_MASK"])).network_address)
     else:
-        return str(n.supernet(prefixlen_diff=(128-int(app.config["AUTH_RATELIMIT_IP_V6_MASK"]))).network_address)
+        return str(n.supernet(prefixlen_diff=(128-app.config["AUTH_RATELIMIT_IP_V6_MASK"])).network_address)
 
 def is_exempt_from_ratelimits(ip):
     ip = ipaddress.ip_address(ip)
@@ -92,6 +93,8 @@ babel = flask_babel.Babel()
 @babel.localeselector
 def get_locale():
     """ selects locale for translation """
+    if not app.config['SESSION_COOKIE_NAME'] in flask.request.cookies:
+        return flask.request.accept_languages.best_match(app.config.translations.keys())
     language = flask.session.get('language')
     if not language in app.config.translations:
         language = flask.request.accept_languages.best_match(app.config.translations.keys())
@@ -118,13 +121,10 @@ proxy = PrefixMiddleware()
 # Data migrate
 migrate = flask_migrate.Migrate()
 
-
 # session store (inspired by https://github.com/mbr/flask-kvsession)
 class RedisStore:
     """ Stores session data in a redis db. """
 
-    has_ttl = True
-
     def __init__(self, redisstore):
         self.redis = redisstore
 
@@ -155,8 +155,6 @@ class RedisStore:
 class DictStore:
     """ Stores session data in a python dict. """
 
-    has_ttl = False
-
     def __init__(self):
         self.dict = {}
 
@@ -164,7 +162,7 @@ class DictStore:
         """ load item from store. """
         return self.dict[key]
 
-    def put(self, key, value, ttl_secs=None):
+    def put(self, key, value, ttl=None):
         """ save item to store. """
         self.dict[key] = value
 
@@ -233,7 +231,8 @@ class MailuSession(CallbackDict, SessionMixin):
 
     def destroy(self):
         """ destroy session for security reasons. """
-
+        if 'webmail_token' in self:
+            self.app.session_store.delete(self['webmail_token'])
         self.delete()
 
         self._uid = None
@@ -247,12 +246,8 @@ class MailuSession(CallbackDict, SessionMixin):
 
     def regenerate(self):
         """ generate new id for session to avoid `session fixation`. """
-
         self.delete()
-
         self._sid = None
-        self._created = self.app.session_config.gen_created()
-
         self.modified = True
 
     def delete(self):
@@ -263,9 +258,7 @@ class MailuSession(CallbackDict, SessionMixin):
 
     def save(self):
         """ Save session to store. """
-
         set_cookie = False
-
         # set uid from dict data
         if self._uid is None:
             self._uid = self.app.session_config.gen_uid(self.get('_user_id', ''))
@@ -274,6 +267,11 @@ class MailuSession(CallbackDict, SessionMixin):
         if self._sid is None:
             self._sid = self.app.session_config.gen_sid()
             set_cookie = True
+            if 'webmail_token' in self:
+                app.session_store.put(self['webmail_token'],
+                        self.sid,
+                        app.config['PERMANENT_SESSION_LIFETIME'],
+                )
 
         # get new session key
         key = self.sid
@@ -282,14 +280,11 @@ class MailuSession(CallbackDict, SessionMixin):
         if key != self._key:
             self.delete()
 
-        # remember time to refresh
-        self['_refresh'] = int(time.time()) + self.app.permanent_session_lifetime.total_seconds()/2
-
         # save session
         self.app.session_store.put(
             key,
             pickle.dumps(dict(self)),
-            self.app.permanent_session_lifetime.total_seconds()
+            app.config['SESSION_TIMEOUT'],
         )
 
         self._key = key
@@ -299,11 +294,6 @@ class MailuSession(CallbackDict, SessionMixin):
 
         return set_cookie
 
-    def needs_refresh(self):
-        """ Checks if server side session needs to be refreshed. """
-
-        return int(time.time()) > self.get('_refresh', 0)
-
 class MailuSessionConfig:
     """ Stores sessions crypto config """
 
@@ -348,7 +338,7 @@ class MailuSessionConfig:
         """ Generate base64 representation of creation time. """
         return self._encode(int(now or time.time()).to_bytes(8, byteorder='big').lstrip(b'\0'))
 
-    def parse_key(self, key, app=None, validate=False, now=None):
+    def parse_key(self, key, app=None, now=None):
         """ Split key into sid, uid and creation time. """
 
         if not (isinstance(key, bytes) and self._key_min <= len(key) <= self._key_max):
@@ -363,13 +353,12 @@ class MailuSessionConfig:
         if created is None or self._decode(uid) is None or self._decode(sid) is None:
             return None
 
-        # validate creation time when requested or store does not support ttl
-        if validate or not app.session_store.has_ttl:
-            if now is None:
-                now = int(time.time())
-            created = int.from_bytes(created, byteorder='big')
-            if not created < now < created + app.permanent_session_lifetime.total_seconds():
-                return None
+        # validate creation time
+        if now is None:
+            now = int(time.time())
+        created = int.from_bytes(created, byteorder='big')
+        if not created <= now <= created + app.config['PERMANENT_SESSION_LIFETIME']:
+            return None
 
         return (uid, sid, crt)
 
@@ -408,23 +397,12 @@ class MailuSessionInterface(SessionInterface):
         if session.accessed:
             response.vary.add('Cookie')
 
-        set_cookie = session.permanent and app.config['SESSION_REFRESH_EACH_REQUEST']
-        need_refresh = session.needs_refresh()
-
-        # save modified session or refresh unmodified session
-        if session.modified or need_refresh:
-            set_cookie |= session.save()
-
-        # set cookie on refreshed permanent sessions
-        if need_refresh and session.permanent:
-            set_cookie = True
-
-        # set or update cookie if necessary
-        if set_cookie:
+        # save session and update cookie if necessary
+        if session.save():
             response.set_cookie(
                 app.session_cookie_name,
                 session.sid,
-                expires=self.get_expiration_time(app, session),
+                expires=datetime.now()+timedelta(seconds=app.config['PERMANENT_SESSION_LIFETIME']),
                 httponly=self.get_cookie_httponly(app),
                 domain=self.get_cookie_domain(app),
                 path=self.get_cookie_path(app),
@@ -444,7 +422,7 @@ class MailuSessionExtension:
 
         count = 0
         for key in app.session_store.list():
-            if not app.session_config.parse_key(key, app, validate=True, now=now):
+            if not app.session_config.parse_key(key, app, now=now):
                 app.session_store.delete(key)
                 count += 1
 
@@ -498,3 +476,24 @@ class MailuSessionExtension:
 
 cleaned = Value('i', False)
 session = MailuSessionExtension()
+
+# this is used by the webmail to authenticate IMAP/SMTP
+def verify_temp_token(email, token):
+    try:
+        if token.startswith('token-'):
+            sessid = app.session_store.get(token)
+            if sessid:
+                session = MailuSession(sessid, app)
+                if session.get('_user_id', '') == email:
+                    return True
+    except:
+        pass
+
+def gen_temp_token(email, session):
+    token = session.get('webmail_token', 'token-'+secrets.token_urlsafe())
+    session['webmail_token'] = token
+    app.session_store.put(token,
+            session.sid,
+            app.config['PERMANENT_SESSION_LIFETIME'],
+    )
+    return token

docs/configuration.rst

@@ -181,7 +181,7 @@ The ``CREDENTIAL_ROUNDS`` (default: 12) setting is the number of rounds used by
 
 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.
 
-``SESSION_LIFETIME`` (default: 24) is the length in hours a session is valid for on the administrative interface.
+``SESSION_TIMEOUT`` (default: 3600) is the maximum amount of time in seconds between requests before a session is invalidated. ``PERMANENT_SESSION_LIFETIME`` (default: 108000) is the maximum amount of time in seconds a session can be kept alive for if it hasn't timed-out.
 
 The ``LOG_LEVEL`` setting is used by the python start-up scripts as a logging threshold.
 Log messages equal or higher than this priority will be printed.

towncrier/newsfragments/2080.bugfix

@@ -0,0 +1 @@
+Ensure that webmail tokens expire in sync with sessions

towncrier/newsfragments/2094.bugfix

@@ -0,0 +1 @@
+Introduce SESSION_TIMEOUT (1h) and PERMANENT_SESSION_LIFETIME (30d)

webmails/roundcube/config.inc.php

@@ -13,6 +13,7 @@ $config['log_driver'] = 'stdout';
 $config['zipdownload_selection'] = true;
 $config['enable_spellcheck'] = true;
 $config['spellcheck_engine'] = 'pspell';
+$config['session_lifetime'] = {{ SESSION_TIMEOUT_MINUTES | int }};
 
 // Mail servers
 $config['default_host'] = '{{ FRONT_ADDRESS or "front" }}';

webmails/roundcube/start.py

@@ -62,6 +62,10 @@ context["PLUGINS"] = ",".join(f"'{p}'" for p in plugins)
 # add overrides
 context["INCLUDES"] = sorted(inc for inc in os.listdir("/overrides") if inc.endswith(".inc")) if os.path.isdir("/overrides") else []
 
+# calculate variables for config file
+env["SESSION_TIMEOUT_MINUTES"] = str(int(env.get("SESSION_TIMEOUT", "3600")) // 60 ) if int(env.get("SESSION_TIMEOUT", "3600")) >= 60 else "1"
+context.update(env)
+
 # create config files
 conf.jinja("/php.ini", context, "/usr/local/etc/php/conf.d/roundcube.ini")
 conf.jinja("/config.inc.php", context, "/var/www/html/config/config.inc.php")