Merge #1359

1359: Refactor the rate limiting code r=mergify[bot] a=kaiyou

## What type of PR?

Enhancement

## What does this PR do?

Rate limiting was already redesigned to use Python limits. This
introduced some unexpected behavior, including the fact that only
one criteria is supported per limiter. Docs and setup utility are
updated with this in mind.

Also, the code was made more generic, so limiters can be delivered
for something else than authentication. Authentication-specific
code was moved directly to the authentication routine.

### Related issue(s)

No specific issue.

## 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: kaiyou <pierre@jaury.eu>

core/admin/mailu/internal/__init__.py

@@ -1,23 +1,7 @@
-from mailu.limiter import RateLimitExceeded
-
-from mailu import utils
-from flask import current_app as app
-
-import socket
 import flask
 
 
 internal = flask.Blueprint('internal', __name__, template_folder='templates')
 
 
-@internal.app_errorhandler(RateLimitExceeded)
-def rate_limit_handler(e):
-    response = flask.Response()
-    response.headers['Auth-Status'] = 'Authentication rate limit from one source exceeded'
-    response.headers['Auth-Error-Code'] = '451 4.3.2'
-    if int(flask.request.headers['Auth-Login-Attempt']) < 10:
-        response.headers['Auth-Wait'] = '3'
-    return response
-
-
 from mailu.internal.views import *

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

@@ -5,21 +5,31 @@ from flask import current_app as app
 import flask
 import flask_login
 import base64
-
+import ipaddress
 
 
 @internal.route("/auth/email")
 def nginx_authentication():
     """ Main authentication endpoint for Nginx email server
     """
-    utils.limiter.check(flask.request.headers["Client-Ip"])
+    limiter = utils.limiter.get_limiter(app.config["AUTH_RATELIMIT"], "auth-ip")
+    client_ip = flask.request.headers["Client-Ip"]
+    if not limiter.test(client_ip):
+        response = flask.Response()
+        response.headers['Auth-Status'] = 'Authentication rate limit from one source exceeded'
+        response.headers['Auth-Error-Code'] = '451 4.3.2'
+        if int(flask.request.headers['Auth-Login-Attempt']) < 10:
+            response.headers['Auth-Wait'] = '3'
+        return response
     headers = nginx.handle_authentication(flask.request.headers)
     response = flask.Response()
     for key, value in headers.items():
         response.headers[key] = str(value)
-    if ("Auth-Status" not in headers) or (headers["Auth-Status"]!="OK"):
-        utils.limiter.hit(flask.request.headers["Client-Ip"])
-
+    if ("Auth-Status" not in headers) or (headers["Auth-Status"] != "OK"):
+        limit_subnet = str(app.config["AUTH_RATELIMIT_SUBNET"]) != 'False'
+        subnet = ipaddress.ip_network(app.config["SUBNET"])
+        if limit_subnet or ipaddress.ip_address(client_ip) not in subnet:
+            limiter.hit(flask.request.headers["Client-Ip"])
     return response
 
 

core/admin/mailu/limiter.py

@@ -1,36 +1,34 @@
 import limits
 import limits.storage
 import limits.strategies
-import ipaddress
 
-class RateLimitExceeded(Exception):
-    pass
 
-class Limiter:
+class LimitWrapper(object):
+    """ Wraps a limit by providing the storage, item and identifiers
+    """
 
-    def __init__(self):
-        self.storage = None
-        self.limiter = None
-        self.rate = None
-        self.subnet = None
-        self.rate_limit_subnet = True
+    def __init__(self, limiter, limit, *identifiers):
+        self.limiter = limiter
+        self.limit = limit
+        self.base_identifiers = identifiers
+
+    def test(self, *args):
+        return self.limiter.test(self.limit, *(self.base_identifiers + args))
+
+    def hit(self, *args):
+        return self.limiter.hit(self.limit, *(self.base_identifiers + args))
+
+    def get_window_stats(self, *args):
+        return self.limiter.get_window_stats(self.limit, *(self.base_identifiers + args))
+
+
+class LimitWraperFactory(object):
+    """ Global limiter, to be used as a factory
+    """
 
     def init_app(self, app):
         self.storage = limits.storage.storage_from_string(app.config["RATELIMIT_STORAGE_URL"])
         self.limiter = limits.strategies.MovingWindowRateLimiter(self.storage)
-        self.rate = limits.parse(app.config["AUTH_RATELIMIT"])
-        self.rate_limit_subnet = str(app.config["AUTH_RATELIMIT_SUBNET"])!='False'
-        self.subnet = ipaddress.ip_network(app.config["SUBNET"])
 
-    def check(self,clientip):
-        # disable limits for internal requests (e.g. from webmail)?
-        if self.rate_limit_subnet==False and ipaddress.ip_address(clientip) in self.subnet:
-            return
-        if not self.limiter.test(self.rate,"client-ip",clientip):
-            raise RateLimitExceeded()
-
-    def hit(self,clientip):
-        # disable limits for internal requests (e.g. from webmail)?
-        if self.rate_limit_subnet==False and ipaddress.ip_address(clientip) in self.subnet:
-            return
-        self.limiter.hit(self.rate,"client-ip",clientip)
+    def get_limiter(self, limit, *args):
+        return LimitWrapper(self.limiter, limits.parse(limit), *args)

core/admin/mailu/utils.py

@@ -20,7 +20,7 @@ def handle_needs_login():
     )
 
 # Rate limiter
-limiter = limiter.Limiter()
+limiter = limiter.LimitWraperFactory()
 
 # Application translation
 babel = flask_babel.Babel()

docs/compose/.env

@@ -38,7 +38,7 @@ POSTMASTER=admin
 TLS_FLAVOR=cert
 
 # Authentication rate limit (per source IP address)
-AUTH_RATELIMIT=10/minute;1000/hour
+AUTH_RATELIMIT=10/minute
 
 # Opt-out of statistics, replace with "True" to opt out
 DISABLE_STATISTICS=False
@@ -68,6 +68,10 @@ ANTIVIRUS=none
 # Max attachment size will be 33% smaller
 MESSAGE_SIZE_LIMIT=50000000
 
+# Message rate limit for outgoing messages
+# This limit is per user
+MESSAGE_RATELIMIT=100/day
+
 # Networks granted relay permissions
 # Use this with care, all hosts in this networks will be able to send mail without authentication!
 RELAYNETS=

docs/configuration.rst

@@ -46,7 +46,6 @@ rules does also apply to auth requests coming from ``SUBNET``, especially for th
 If you disable this, ensure that the rate limit on the webmail is enforced in a different
 way (e.g. roundcube plug-in), otherwise an attacker can simply bypass the limit using webmail.
 
-
 The ``TLS_FLAVOR`` sets how Mailu handles TLS connections. Setting this value to
 ``notls`` will cause Mailu not to server any web content! More on :ref:`tls_flavor`.
 
@@ -57,6 +56,10 @@ The ``MESSAGE_SIZE_LIMIT`` is the maximum size of a single email. It should not
 be too low to avoid dropping legitimate emails and should not be too high to
 avoid filling the disks with large junk emails.
 
+The ``MESSAGE_RATELIMIT`` is the limit of messages a single user can send. This is
+meant to fight outbound spam in case of compromised or malicious account on the
+server.
+
 The ``RELAYNETS`` are network addresses for which mail is relayed for free with
 no authentication required. This should be used with great care. If you want other
 Docker services' outbound mail to be relayed, you can set this to ``172.16.0.0/12``

setup/flavors/compose/mailu.env

@@ -30,8 +30,8 @@ POSTMASTER={{ postmaster }}
 TLS_FLAVOR={{ tls_flavor }}
 
 # Authentication rate limit (per source IP address)
-{% if auth_ratelimit_pm > '0' and auth_ratelimit_ph > '0' %}
-AUTH_RATELIMIT={{ auth_ratelimit_pm }}/minute;{{ auth_ratelimit_ph }}/hour 
+{% if auth_ratelimit_pm > '0' %}
+AUTH_RATELIMIT={{ auth_ratelimit_pm }}/minute
 {% endif %}
 
 # Opt-out of statistics, replace with "True" to opt out

setup/templates/steps/config.html

@@ -47,11 +47,10 @@ Or in plain english: if receivers start to classify your mail as spam, this post
 
 <div class="form-group">
   <label>Authentication rate limit (per source IP address)</label>
-<!--   Validates number input only -->
+  <!--   Validates number input only -->
   <p><input class="form-control" style="width: 7%; display: inline;" type="number" name="auth_ratelimit_pm" 
-  		value="10" required >/minute;
-  <input class="form-control" style="width: 7%; display: inline;;" type="number" name="auth_ratelimit_ph"
-  		value="1000" required >/hour</p>
+  		value="10" required > / minute
+  </p>
 </div>
 
 <div class="form-check form-check-inline">