bugfix: make msc3967 idempotent

changelog.d/16943.bugfix

@@ -0,0 +1 @@
+Make the CSAPI endpoint `/keys/device_signing/upload` idempotent.

docker/complement/conf/workers-shared-extra.yaml.j2

@@ -102,6 +102,8 @@ experimental_features:
   msc3391_enabled: true
   # Filtering /messages by relation type.
   msc3874_enabled: true
+  # no UIA for x-signing upload for the first time
+  msc3967_enabled: true
 
 server_notices:
   system_mxid_localpart: _server

scripts-dev/complement.sh

@@ -214,7 +214,7 @@ fi
 
 extra_test_args=()
 
-test_packages="./tests/csapi ./tests ./tests/msc3874 ./tests/msc3890 ./tests/msc3391 ./tests/msc3930 ./tests/msc3902"
+test_packages="./tests/csapi ./tests ./tests/msc3874 ./tests/msc3890 ./tests/msc3391 ./tests/msc3930 ./tests/msc3902 ./tests/msc3967"
 
 # Enable dirty runs, so tests will reuse the same container where possible.
 # This significantly speeds up tests, but increases the possibility of test pollution.

synapse/handlers/e2e_keys.py

@@ -1476,6 +1476,42 @@ async def check_cross_signing_setup(self, user_id: str) -> Tuple[bool, bool]:
         else:
             return exists, self.clock.time_msec() < ts_replacable_without_uia_before
 
+    async def has_different_keys(self, user_id: str, body: JsonDict) -> bool:
+        """
+        Check if a key provided in `body` differs from the same key stored in the DB. Returns
+        true on the first difference. If a key exists in `body` but does not exist in the DB,
+        returns True. If `body` has no keys, this always returns False.
+        Note by 'key' we mean Matrix key rather than JSON key.
+
+        The purpose of this function is to detect whether or not we need to apply UIA checks.
+        We must apply UIA checks if any key in the database is being overwritten. If a key is
+        being inserted for the first time, or if the key exactly matches what is in the database,
+        then no UIA check needs to be performed.
+
+        Args:
+            user_id: The user who sent the `body`.
+            body: The JSON request body from POST /keys/device_signing/upload
+        Returns:
+            True if any key in `body` has a different value in the database.
+        """
+        # Ensure that each key provided in the request body exactly matches the one we have stored.
+        # The first time we see the DB having a different key to the matching request key, bail.
+        # Note: we do not care if the DB has a key which the request does not specify, as we only
+        # care about *replacements* or *insertions* (i.e UPSERT)
+        req_body_key_to_db_key = {
+            "master_key": "master",
+            "self_signing_key": "self_signing",
+            "user_signing_key": "user_signing",
+        }
+        for req_body_key, db_key in req_body_key_to_db_key.items():
+            if req_body_key in body:
+                existing_key = await self.store.get_e2e_cross_signing_key(
+                    user_id, db_key
+                )
+                if existing_key != body[req_body_key]:
+                    return True
+        return False
+
 
 def _check_cross_signing_key(
     key: JsonDict, user_id: str, key_type: str, signing_key: Optional[VerifyKey] = None

synapse/rest/client/keys.py

@@ -409,7 +409,18 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
             # But first-time setup is fine
 
         elif self.hs.config.experimental.msc3967_enabled:
-            # If we already have a master key then cross signing is set up and we require UIA to reset
+            # MSC3967 allows this endpoint to 200 OK for idempotency. Resending exactly the same
+            # keys should just 200 OK without doing a UIA prompt.
+            keys_are_different = await self.e2e_keys_handler.has_different_keys(
+                user_id, body
+            )
+            if not keys_are_different:
+                # FIXME: we do not fallthrough to upload_signing_keys_for_user because confusingly
+                # if we do, we 500 as it looks like it tries to INSERT the same key twice, causing a
+                # unique key constraint violation. This sounds like a bug?
+                return 200, {}
+            # the keys are different, is x-signing set up? If no, then the keys don't exist which is
+            # why they are different. If yes, then we need to UIA to change them.
             if is_cross_signing_setup:
                 await self.auth_handler.validate_user_via_ui_auth(
                     requester,
@@ -420,7 +431,6 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
                     can_skip_ui_auth=False,
                 )
             # Otherwise we don't require UIA since we are setting up cross signing for first time
-
         else:
             # Previous behaviour is to always require UIA but allow it to be skipped
             await self.auth_handler.validate_user_via_ui_auth(

tests/handlers/test_e2e_keys.py

@@ -1101,6 +1101,56 @@ def test_query_devices_remote_no_sync(self) -> None:
             },
         )
 
+    def test_has_different_keys(self) -> None:
+        """check that has_different_keys returns True when the keys provided are different to what
+        is in the database."""
+        local_user = "@boris:" + self.hs.hostname
+        keys1 = {
+            "master_key": {
+                # private key: 2lonYOM6xYKdEsO+6KrC766xBcHnYnim1x/4LFGF8B0
+                "user_id": local_user,
+                "usage": ["master"],
+                "keys": {
+                    "ed25519:nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unI9kDYcHwk": "nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unI9kDYcHwk"
+                },
+            }
+        }
+        self.get_success(self.handler.upload_signing_keys_for_user(local_user, keys1))
+        is_different = self.get_success(
+            self.handler.has_different_keys(
+                local_user,
+                {
+                    "master_key": keys1["master_key"],
+                },
+            )
+        )
+        self.assertEqual(is_different, False)
+        # change the usage => different keys
+        keys1["master_key"]["usage"] = ["develop"]
+        is_different = self.get_success(
+            self.handler.has_different_keys(
+                local_user,
+                {
+                    "master_key": keys1["master_key"],
+                },
+            )
+        )
+        self.assertEqual(is_different, True)
+        keys1["master_key"]["usage"] = ["master"]  # reset
+        # change the key => different keys
+        keys1["master_key"]["keys"] = {
+            "ed25519:nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unIc0rncs": "nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unIc0rncs"
+        }
+        is_different = self.get_success(
+            self.handler.has_different_keys(
+                local_user,
+                {
+                    "master_key": keys1["master_key"],
+                },
+            )
+        )
+        self.assertEqual(is_different, True)
+
     def test_query_devices_remote_sync(self) -> None:
         """Tests that querying keys for a remote user that we share a room with,
         but haven't yet fetched the keys for, returns the cross signing keys