Reject OTK uploads if clients exceed 500 server-side keys

changelog.d/20162.feature

@@ -0,0 +1 @@
+Limit the number of end-to-end encryption one-time keys stored per device, per algorithm, rejecting uploads which would exceed the new `max_one_time_keys_per_device` limit with a `400 Bad Request`.

docs/usage/configuration/config_documentation.md

@@ -729,6 +729,21 @@ Example configuration:
 delete_stale_devices_after: 1y
 ```
 ---
+### `max_one_time_keys_per_device`
+
+*(integer)* The maximum number of end-to-end encryption one-time keys, per key algorithm, to keep for each device. Uploads which would take a device over this are rejected with a `400 Bad Request` / `M_TOO_LARGE` error.
+
+The spec allows clients to discard their oldest private one-time keys once they hold too many, and both libolm (at 100 keys) and vodozemac (at 5000) do so, so keys beyond that bound could never be used anyway. This limit protects against clients that keep uploading keys regardless of how many the server already holds, which would otherwise leave the server handing out keys the device has forgotten. Rejecting the upload, rather than quietly discarding keys, makes the problem visible to the client.
+
+Server admins should take care not to set this below the number of keys clients aim to keep on the server - e.g. 50 for clients built on the matrix-rust-sdk - as every upload from such clients would then be rejected, leaving them unable to publish any one-time keys at all.
+
+Defaults to `500`.
+
+Example configuration:
+```yaml
+max_one_time_keys_per_device: 1000
+```
+---
 ### `email`
 
 *(object)* Configuration for sending emails from Synapse.

schema/synapse-config.schema.yaml

@@ -800,6 +800,31 @@ properties:
     default: null
     examples:
       - 1y
+  max_one_time_keys_per_device:
+    type: integer
+    minimum: 1
+    description: >-
+      The maximum number of end-to-end encryption one-time keys, per key
+      algorithm, to keep for each device. Uploads which would take a device over
+      this are rejected with a `400 Bad Request` / `M_TOO_LARGE` error.
+
+
+      The spec allows clients to discard their oldest private one-time keys once
+      they hold too many, and both libolm (at 100 keys) and vodozemac (at 5000)
+      do so, so keys beyond that bound could never be used anyway. This limit
+      protects against clients that keep uploading keys regardless of how many
+      the server already holds, which would otherwise leave the server handing
+      out keys the device has forgotten. Rejecting the upload, rather than
+      quietly discarding keys, makes the problem visible to the client.
+
+
+      Server admins should take care not to set this below the number of keys
+      clients aim to keep on the server - e.g. 50 for clients built on the
+      matrix-rust-sdk - as every upload from such clients would then be
+      rejected, leaving them unable to publish any one-time keys at all.
+    default: 500
+    examples:
+      - 1000
   email:
     type: object
     description: >-

synapse/config/server.py

@@ -67,6 +67,20 @@ def is_str_list(val: Any, allow_empty: bool) -> TypeGuard[list[str]]:
 Please see https://element-hq.github.io/synapse/latest/upgrade.html#direct-tcp-replication-is-no-longer-supported-migrate-to-redis
 """
 
+# The number of one-time keys that clients built on the matrix-rust-sdk aim to keep on
+# the server (vodozemac's `PUBLIC_MAX_ONE_TIME_KEYS`), topping up to exactly this many
+# whenever they fall below it. Allowing fewer than this per device means every such
+# top-up is rejected, leaving those clients unable to publish any one-time keys at all.
+SENSIBLE_MIN_ONE_TIME_KEYS_PER_DEVICE = 50
+
+LOW_ONE_TIME_KEYS_PER_DEVICE_WARNING = """\
+WARNING: The 'max_one_time_keys_per_device' configuration setting is lower than the
+%i one-time keys that clients built on the matrix-rust-sdk keep on the server. Those
+clients will be unable to upload any one-time keys. See the config documentation at
+    https://element-hq.github.io/synapse/latest/usage/configuration/config_documentation.html#max_one_time_keys_per_device
+--------------------------------------------------------------------------------
+"""
+
 # by default, we attempt to listen on both '::' *and* '0.0.0.0' because some OSes
 # (Windows, macOS, other BSD/Linux where net.ipv6.bindv6only is set) will only listen
 # on IPv6 when '::' is set.
@@ -685,6 +699,26 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
         # due to resource constraints
         self.admin_contact = config.get("admin_contact", None)
 
+        # The maximum number of one-time keys of each algorithm to keep for a device.
+        # Uploads which would exceed it are rejected, which protects against clients
+        # that keep uploading keys they will never be able to use.
+        self.max_one_time_keys_per_device: int = config.get(
+            "max_one_time_keys_per_device", 500
+        )
+        if (
+            not isinstance(self.max_one_time_keys_per_device, int)
+            or self.max_one_time_keys_per_device < 1
+        ):
+            raise ConfigError(
+                "'max_one_time_keys_per_device' must be a positive integer",
+                ("max_one_time_keys_per_device",),
+            )
+        if self.max_one_time_keys_per_device < SENSIBLE_MIN_ONE_TIME_KEYS_PER_DEVICE:
+            logger.warning(
+                LOW_ONE_TIME_KEYS_PER_DEVICE_WARNING,
+                SENSIBLE_MIN_ONE_TIME_KEYS_PER_DEVICE,
+            )
+
         ip_range_blocklist = config.get(
             "ip_range_blacklist", DEFAULT_IP_RANGE_BLOCKLIST
         )

synapse/handlers/e2e_keys.py

@@ -20,6 +20,7 @@
 #
 #
 import logging
+from collections import Counter
 from typing import TYPE_CHECKING, Iterable, Mapping
 
 import attr
@@ -121,6 +122,9 @@ def __init__(self, hs: "HomeServer"):
         self._query_appservices_for_keys = (
             hs.config.experimental.msc3984_appservice_key_query
         )
+        self._max_one_time_keys_per_device = (
+            hs.config.server.max_one_time_keys_per_device
+        )
 
         self._task_scheduler.register_action(
             self._delete_old_one_time_keys_task, "delete_old_otks"
@@ -989,6 +993,28 @@ async def _upload_one_time_keys_for_user(
                         (algorithm, key_id, encode_canonical_json(key).decode("ascii"))
                     )
 
+            # Reject uploads which would take the device over the limit, rather than
+            # quietly discarding keys, so that a client which keeps uploading keys
+            # regardless of how many the server holds gets told about it.
+            counts = await self.store.count_e2e_one_time_keys(user_id, device_id)
+            for algorithm, new_count in Counter(
+                algorithm for algorithm, _, _ in new_keys
+            ).items():
+                total = counts.get(algorithm, 0) + new_count
+                if total > self._max_one_time_keys_per_device:
+                    raise SynapseError(
+                        400,
+                        "Uploading %i more %s one-time keys would leave the device "
+                        "holding %i, over the limit of %i"
+                        % (
+                            new_count,
+                            algorithm,
+                            total,
+                            self._max_one_time_keys_per_device,
+                        ),
+                        Codes.TOO_LARGE,
+                    )
+
             log_kv({"message": "Inserting new one_time_keys.", "keys": new_keys})
             await self.store.add_e2e_one_time_keys(
                 user_id, device_id, time_now, new_keys

tests/config/test_server.py

@@ -229,6 +229,21 @@ def generate_config(value: Any) -> JsonDict:
             with self.assertRaises(ConfigError):
                 _read_config(generate_config(disallowed_value))
 
+    def test_max_one_time_keys_per_device_enforces_positive_int(self) -> None:
+        """
+        Test that the configured maximum number of one-time keys must be a positive
+        value, as a device can never hold fewer than one key
+        """
+
+        def generate_config(value: Any) -> JsonDict:
+            return {"max_one_time_keys_per_device": value}
+
+        _read_config(generate_config(1))
+
+        for disallowed_value in (-1, 0, 0.5):
+            with self.assertRaises(ConfigError):
+                _read_config(generate_config(disallowed_value))
+
     @parameterized.expand(
         [
             [

tests/handlers/test_e2e_keys.py

@@ -405,6 +405,55 @@ def test_claim_one_time_key_bulk_ordering(self) -> None:
         for key_id in claimed_keys.keys():
             self.assertIn(key_id, ["alg1:k20", "alg1:k21", "alg1:k22"])
 
+    @override_config({"max_one_time_keys_per_device": 5})
+    def test_upload_one_time_keys_over_limit_is_rejected(self) -> None:
+        """Uploading one-time keys which would take a device over the limit fails"""
+        local_user = "@boris:" + self.hs.hostname
+        device_id = "xyz"
+
+        # Uploading exactly up to the limit is fine.
+        res = self.get_success(
+            self.handler.upload_keys_for_user(
+                local_user,
+                device_id,
+                {"one_time_keys": {f"alg1:k{i}": f"key{i}" for i in range(1, 6)}},
+            )
+        )
+        self.assertEqual(res["one_time_key_counts"]["alg1"], 5)
+
+        # Uploading any more is rejected, with a 400.
+        error = self.get_failure(
+            self.handler.upload_keys_for_user(
+                local_user,
+                device_id,
+                {"one_time_keys": {"alg1:k6": "key6"}},
+            ),
+            SynapseError,
+        ).value
+        self.assertEqual(error.code, 400)
+        self.assertEqual(error.errcode, Codes.TOO_LARGE)
+
+        # The limit is per algorithm, so keys of another algorithm are still accepted.
+        res = self.get_success(
+            self.handler.upload_keys_for_user(
+                local_user,
+                device_id,
+                {"one_time_keys": {"alg2:k1": "key1"}},
+            )
+        )
+        self.assertEqual(res["one_time_key_counts"]["alg1"], 5)
+        self.assertEqual(res["one_time_key_counts"]["alg2"], 1)
+
+        # Nothing from the rejected upload was stored.
+        keys = self.get_success(
+            self.store.get_e2e_one_time_keys(
+                local_user, device_id, [f"k{i}" for i in range(1, 7)]
+            )
+        )
+        self.assertEqual(
+            set(keys), {("alg1", f"k{i}") for i in range(1, 6)} | {("alg2", "k1")}
+        )
+
     def test_fallback_key(self) -> None:
         local_user = "@boris:" + self.hs.hostname
         device_id = "xyz"