diff --git a/changelog.d/19915.feature b/changelog.d/19915.feature new file mode 100644 index 0000000000..39446ae6a1 --- /dev/null +++ b/changelog.d/19915.feature @@ -0,0 +1 @@ +Don't validate signatures with unknown algorithms for master keys, and allow updates to signatures. \ No newline at end of file diff --git a/synapse/handlers/e2e_keys.py b/synapse/handlers/e2e_keys.py index 64f705a3da..6284f75319 100644 --- a/synapse/handlers/e2e_keys.py +++ b/synapse/handlers/e2e_keys.py @@ -1272,21 +1272,27 @@ class E2eKeysHandler: master_key_signature_list = [] sigs = signed_master_key["signatures"] for signing_key_id, signature in sigs[user_id].items(): - _, signing_device_id = signing_key_id.split(":", 1) - if ( - signing_device_id not in devices - or signing_key_id not in devices[signing_device_id]["keys"] - ): - # signed by an unknown device, or the - # device does not have the key - raise SynapseError(400, "Invalid signature", Codes.INVALID_SIGNATURE) + algorithm, signing_device_id = signing_key_id.split(":", 1) + # we only check the signature for known algorithms + if algorithm == "ed25519": + if ( + signing_device_id not in devices + or signing_key_id not in devices[signing_device_id]["keys"] + ): + # signed by an unknown device, or the + # device does not have the key + raise SynapseError( + 400, "Invalid signature", Codes.INVALID_SIGNATURE + ) - # get the key and check the signature - pubkey = devices[signing_device_id]["keys"][signing_key_id] - verify_key = decode_verify_key_bytes(signing_key_id, decode_base64(pubkey)) - _check_device_signature( - user_id, verify_key, signed_master_key, stored_master_key - ) + # get the key and check the signature + pubkey = devices[signing_device_id]["keys"][signing_key_id] + verify_key = decode_verify_key_bytes( + signing_key_id, decode_base64(pubkey) + ) + _check_device_signature( + user_id, verify_key, signed_master_key, stored_master_key + ) master_key_signature_list.append( SignatureListItem(signing_key_id, user_id, master_key_id, signature) diff --git a/synapse/storage/database.py b/synapse/storage/database.py index 023014276b..1de0cc5fe0 100644 --- a/synapse/storage/database.py +++ b/synapse/storage/database.py @@ -120,6 +120,7 @@ UNIQUE_INDEX_BACKGROUND_UPDATES = { "event_push_summary": "event_push_summary_unique_index2", "receipts_linearized": "receipts_linearized_unique_index", "receipts_graph": "receipts_graph_unique_index", + "e2e_cross_signing_signatures": "e2e_cross_signing_signatures_add_key_id_to_index", } diff --git a/synapse/storage/databases/main/end_to_end_keys.py b/synapse/storage/databases/main/end_to_end_keys.py index c93ebd3dda..3075d6a05e 100644 --- a/synapse/storage/databases/main/end_to_end_keys.py +++ b/synapse/storage/databases/main/end_to_end_keys.py @@ -78,6 +78,10 @@ class DeviceKeyLookupResult: class EndToEndKeyBackgroundStore(SQLBaseStore): + CROSS_SIGNING_KEYS_REMOVE_DUPLICATES_NAME = ( + "e2e_cross_signing_signatures_remove_duplicates" + ) + def __init__( self, database: DatabasePool, @@ -101,6 +105,80 @@ class EndToEndKeyBackgroundStore(SQLBaseStore): columns=("user_id", "device_id", "algorithm", "ts_added_ms"), ) + self.db_pool.updates.register_background_update_handler( + self.CROSS_SIGNING_KEYS_REMOVE_DUPLICATES_NAME, + self._background_cross_signing_signatures_remove_duplicates, + ) + + self.db_pool.updates.register_background_index_update( + update_name="e2e_cross_signing_signatures_add_key_id_to_index", + index_name="e2e_cross_signing_signatures_idx3", + table="e2e_cross_signing_signatures", + columns=("user_id", "target_user_id", "target_device_id", "key_id"), + unique=True, + replaces_index="e2e_cross_signing_signatures2_idx", + ) + + async def _background_cross_signing_signatures_remove_duplicates( + self, progress: dict, batch_size: int + ) -> int: + """Removes duplicate cross-signing signatures so that we can add a + unique index on `(user_id, target_user_id, target_device_id, key_id)` to + `e2e_cross_signing_signatures`. + """ + + def _remove_duplicate_signatures_txn(txn: LoggingTransaction) -> int: + sql = """ + SELECT user_id, key_id, target_user_id, target_device_id, MAX(signature) + FROM e2e_cross_signing_signatures + GROUP BY user_id, key_id, target_user_id, target_device_id + HAVING COUNT(*) > 1 + LIMIT ? + """ + txn.execute(sql, (batch_size,)) + duplicate_keys = cast(list[tuple[str, str, str, str, str]], list(txn)) + + for ( + user_id, + key_id, + target_user_id, + target_device_id, + signature, + ) in duplicate_keys: + sql = """ + DELETE FROM e2e_cross_signing_signatures + WHERE + user_id = ? AND + key_id = ? AND + target_user_id = ? AND + target_device_id = ? + """ + txn.execute(sql, (user_id, key_id, target_user_id, target_device_id)) + + sql = """ + INSERT INTO e2e_cross_signing_signatures + (user_id, key_id, target_user_id, target_device_id, signature) + VALUES + (?, ?, ?, ?, ?) + """ + txn.execute( + sql, (user_id, key_id, target_user_id, target_device_id, signature) + ) + + return len(duplicate_keys) + + number_deleted = await self.db_pool.runInteraction( + self.CROSS_SIGNING_KEYS_REMOVE_DUPLICATES_NAME, + _remove_duplicate_signatures_txn, + ) + + if number_deleted < batch_size: + await self.db_pool.updates._end_background_update( + self.CROSS_SIGNING_KEYS_REMOVE_DUPLICATES_NAME + ) + + return number_deleted + class EndToEndKeyWorkerStore(EndToEndKeyBackgroundStore, CacheInvalidationWorkerStore): def __init__( @@ -1808,7 +1886,7 @@ class EndToEndKeyWorkerStore(EndToEndKeyBackgroundStore, CacheInvalidationWorker ) async def store_e2e_cross_signing_signatures( - self, user_id: str, signatures: "Iterable[SignatureListItem]" + self, user_id: str, signatures: "list[SignatureListItem]" ) -> None: """Stores cross-signing signatures. @@ -1819,28 +1897,23 @@ class EndToEndKeyWorkerStore(EndToEndKeyBackgroundStore, CacheInvalidationWorker def _store_e2e_cross_signing_signatures( txn: LoggingTransaction, - signatures: "Iterable[SignatureListItem]", + signatures: "list[SignatureListItem]", ) -> None: - self.db_pool.simple_insert_many_txn( + self.db_pool.simple_upsert_many_txn( txn, "e2e_cross_signing_signatures", - keys=( - "user_id", - "key_id", - "target_user_id", - "target_device_id", - "signature", - ), - values=[ + key_names=("user_id", "key_id", "target_user_id", "target_device_id"), + key_values=[ ( user_id, item.signing_key_id, item.target_user_id, item.target_device_id, - item.signature, ) for item in signatures ], + value_names=("signature",), + value_values=[(item.signature,) for item in signatures], ) to_invalidate = [ diff --git a/synapse/storage/schema/main/delta/94/09_e2e_cross_signing_signatures_index.sql b/synapse/storage/schema/main/delta/94/09_e2e_cross_signing_signatures_index.sql new file mode 100644 index 0000000000..cde6ca1500 --- /dev/null +++ b/synapse/storage/schema/main/delta/94/09_e2e_cross_signing_signatures_index.sql @@ -0,0 +1,21 @@ +-- +-- This file is licensed under the Affero General Public License (AGPL) version 3. +-- +-- Copyright (C) 2026 New Vector, Ltd +-- +-- This program is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as +-- published by the Free Software Foundation, either version 3 of the +-- License, or (at your option) any later version. +-- +-- See the GNU Affero General Public License for more details: +-- . + +-- Remove any rows `e2e_cross_signing_signatures` that have duplicate cross-signing signatures. +-- Ensures that rows are unique across `(user_id, target_user_id, target_device_id, key_id)`, so that +-- we can create an index on those columns. See +-- `./10_e2e_cross_signing_signatures_add_key_id_to_index.sql`, which adds said +-- index. + +INSERT INTO background_updates (ordering, update_name, progress_json) VALUES + (9409, 'e2e_cross_signing_signatures_remove_duplicates', '{}'); diff --git a/synapse/storage/schema/main/delta/94/10_e2e_cross_signing_signatures_add_key_id_to_index.sql b/synapse/storage/schema/main/delta/94/10_e2e_cross_signing_signatures_add_key_id_to_index.sql new file mode 100644 index 0000000000..65b5df6c86 --- /dev/null +++ b/synapse/storage/schema/main/delta/94/10_e2e_cross_signing_signatures_add_key_id_to_index.sql @@ -0,0 +1,18 @@ +-- +-- This file is licensed under the Affero General Public License (AGPL) version 3. +-- +-- Copyright (C) 2026 New Vector, Ltd +-- +-- This program is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as +-- published by the Free Software Foundation, either version 3 of the +-- License, or (at your option) any later version. +-- +-- See the GNU Affero General Public License for more details: +-- . + +-- Adds the `key_id` to the e2e_cross_signing_signatures index, since the +-- ("user_id", "key_id", "target_user_id", "target_device_id") should be +-- unique. +INSERT INTO background_updates (ordering, update_name, progress_json) VALUES + (9410, 'e2e_cross_signing_signatures_add_key_id_to_index', '{}'); \ No newline at end of file diff --git a/tests/handlers/test_e2e_keys.py b/tests/handlers/test_e2e_keys.py index a4f9d55a13..4790c339d9 100644 --- a/tests/handlers/test_e2e_keys.py +++ b/tests/handlers/test_e2e_keys.py @@ -817,6 +817,103 @@ class E2eKeysHandlerTestCase(unittest.HomeserverTestCase): self.assertDictEqual(devices["device_keys"][local_user]["abc"], device_key_1) self.assertDictEqual(devices["device_keys"][local_user]["def"], device_key_2) + def test_update_signature_master_key(self) -> None: + """should be able to update a signature on the Master signing key with an unknown algorithm""" + local_user = "@boris:" + self.hs.hostname + master_key: JsonDict = { + # private key: HvQBbU+hc2Zr+JP1sE0XwBe1pfZZEYtJNPJLZJtS+F8 + "user_id": local_user, + "usage": ["master"], + "keys": { + "ed25519:EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ": "EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ" + }, + "signatures": {local_user: {"unknown:abcdefg": "abcdefg"}}, + } + self_signing_key = { + # private key: 2lonYOM6xYKdEsO+6KrC766xBcHnYnim1x/4LFGF8B0 + "user_id": local_user, + "usage": ["self_signing"], + "keys": { + "ed25519:nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unI9kDYcHwk": "nqOvzeuGWT/sRx3h7+MHoInYj3Uk2LD/unI9kDYcHwk" + }, + } + master_signing_key = key.decode_signing_key_base64( + "ed25519", + "EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ", + "HvQBbU+hc2Zr+JP1sE0XwBe1pfZZEYtJNPJLZJtS+F8", + ) + sign.sign_json(self_signing_key, local_user, master_signing_key) + self.get_success( + self.handler.upload_signing_keys_for_user( + local_user, + {"master_key": master_key, "self_signing_key": self_signing_key}, + ) + ) + + device_key: JsonDict = { + "user_id": local_user, + "device_id": "abc", + "algorithms": [ + "m.olm.curve25519-aes-sha2", + RoomEncryptionAlgorithms.MEGOLM_V1_AES_SHA2, + ], + "keys": { + "ed25519:abc": "base64+ed25519+key", + "curve25519:abc": "base64+curve25519+key", + }, + "signatures": {local_user: {"ed25519:abc": "base64+signature"}}, + } + self.get_success( + self.handler.upload_keys_for_user( + local_user, "abc", {"device_keys": device_key} + ) + ) + + # Update the signature and upload it. + master_key["signatures"][local_user]["unknown:abcdefg"] = "ABCDEFG" + self.get_success( + self.handler.upload_signatures_for_device_keys( + local_user, + { + local_user: { + "EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ": master_key + } + }, + ) + ) + + devices = self.get_success( + self.handler.query_devices( + {"device_keys": {local_user: []}}, 0, local_user, "device123" + ) + ) + if "unsigned" in devices["master_keys"][local_user]: + del devices["master_keys"][local_user]["unsigned"] + self.assertDictEqual(devices["master_keys"][local_user], master_key) + + # Update the signature again and upload it. + master_key["signatures"][local_user]["unknown:abcdefg"] = "AbCdEfG" + self.get_success( + self.handler.upload_signatures_for_device_keys( + local_user, + { + local_user: { + "EmkqvokUn8p+vQAGZitOk4PWjp7Ukp3txV2TbMPEiBQ": master_key + } + }, + ) + ) + + # Assert that we were able to update the signature. + devices = self.get_success( + self.handler.query_devices( + {"device_keys": {local_user: []}}, 0, local_user, "device123" + ) + ) + if "unsigned" in devices["master_keys"][local_user]: + del devices["master_keys"][local_user]["unsigned"] + self.assertDictEqual(devices["master_keys"][local_user], master_key) + def test_self_signing_key_doesnt_show_up_as_device(self) -> None: """signing keys should be hidden when fetching a user's devices""" local_user = "@boris:" + self.hs.hostname diff --git a/tests/storage/test_end_to_end_keys.py b/tests/storage/test_end_to_end_keys.py index 24fdb0bf6d..537393f3fc 100644 --- a/tests/storage/test_end_to_end_keys.py +++ b/tests/storage/test_end_to_end_keys.py @@ -22,6 +22,7 @@ from twisted.internet.testing import MemoryReactor from synapse.server import HomeServer +from synapse.storage.database import LoggingTransaction from synapse.util.clock import Clock from tests.unittest import HomeserverTestCase @@ -118,3 +119,98 @@ class EndToEndKeyStoreTestCase(HomeserverTestCase): self.assertIn("user2", res) self.assertNotIn("device1", res["user2"]) self.assertIn("device2", res["user2"]) + + def test_bg_signatures_migration(self) -> None: + updater = self.hs.get_datastores().main.db_pool.updates + + # drop the constraint so we can insert duplicate signatures + def f(txn: LoggingTransaction) -> None: + txn.execute("DROP INDEX e2e_cross_signing_signatures_idx3") + + self.get_success(self.store.db_pool.runInteraction("", f)) + + # save multiple copies of the same key in the database + for _i in range(2): + self.get_success( + self.store.db_pool.simple_insert( + "e2e_cross_signing_signatures", + { + "user_id": "@alice:example.org", + "key_id": "ed25519:abcdefg", + "target_user_id": "@alice:example.org", + "target_device_id": "hijklmnop", + "signature": "some+signature", + }, + ) + ) + + for _i in range(2): + self.get_success( + self.store.db_pool.simple_insert( + "e2e_cross_signing_signatures", + { + "user_id": "@alice:example.org", + "key_id": "ed25519:hijklmnop", + "target_user_id": "@alice:example.org", + "target_device_id": "abcdefg", + "signature": "some+signature", + }, + ) + ) + + # run the background task to remove duplicates + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + values={ + "update_name": "e2e_cross_signing_signatures_remove_duplicates", + "progress_json": "{}", + }, + ) + ) + + self.get_success( + updater.run_background_updates(False), + ) + + # re-add the unique index + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + values={ + "update_name": "e2e_cross_signing_signatures_add_key_id_to_index", + "progress_json": "{}", + }, + ) + ) + + self.get_success( + updater.run_background_updates(False), + ) + + # check that we only have one copy of each key + expected_values = [ + ( + "@alice:example.org", + "ed25519:abcdefg", + "@alice:example.org", + "hijklmnop", + "some+signature", + ), + ( + "@alice:example.org", + "ed25519:hijklmnop", + "@alice:example.org", + "abcdefg", + "some+signature", + ), + ] + + res = self.get_success( + self.store.db_pool.execute( + "", + "SELECT user_id, key_id, target_user_id, target_device_id, signature from e2e_cross_signing_signatures ORDER BY key_id", + ) + ) + self.assertEqual(len(res), len(expected_values)) + self.assertEqual(res, expected_values)