From 9ff759e3e013dbe0da7042a2268b73a13f729d57 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 27 Jul 2021 11:05:03 +0100 Subject: [PATCH 1/6] Specify known device key algorithms as constants --- synapse/api/constants.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 8363c2bb0f5f..8c7ad2a40762 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -127,6 +127,14 @@ class ToDeviceEventTypes: RoomKeyRequest = "m.room_key_request" +class DeviceKeyAlgorithms: + """Spec'd algorithms for the generation of per-device keys""" + + ED25519 = "ed25519" + CURVE25519 = "curve25519" + SIGNED_CURVE25519 = "signed_curve25519" + + class EduTypes: Presence = "m.presence" From 97144bf5ccfc870ab7a3a0379231cbf8f53031a9 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 27 Jul 2021 11:19:07 +0100 Subject: [PATCH 2/6] Return 0 by default for a device key algorithm count --- synapse/handlers/sync.py | 3 +++ synapse/storage/databases/main/end_to_end_keys.py | 9 ++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 150a4f291e53..353f64ac80fa 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -1093,6 +1093,9 @@ async def generate_sync_result( one_time_key_counts: JsonDict = {} unused_fallback_key_types: List[str] = [] if device_id: + # TODO: If this is an incremental sync and there's been no change in a count + # since the provided since token, then simply omit that algorithm's entry + # altogether. one_time_key_counts = await self.store.count_e2e_one_time_keys( user_id, device_id ) diff --git a/synapse/storage/databases/main/end_to_end_keys.py b/synapse/storage/databases/main/end_to_end_keys.py index 78ae68ec685c..1edc96042bbe 100644 --- a/synapse/storage/databases/main/end_to_end_keys.py +++ b/synapse/storage/databases/main/end_to_end_keys.py @@ -21,6 +21,7 @@ from twisted.enterprise.adbapi import Connection +from synapse.api.constants import DeviceKeyAlgorithms from synapse.logging.opentracing import log_kv, set_tag, trace from synapse.storage._base import SQLBaseStore, db_to_json from synapse.storage.database import DatabasePool, make_in_list_sql_clause @@ -381,9 +382,15 @@ def _count_e2e_one_time_keys(txn): " GROUP BY algorithm" ) txn.execute(sql, (user_id, device_id)) - result = {} + + # Initially set the key count to 0. This ensures that the client will always + # receive *some count*, even if it's 0. + result = {DeviceKeyAlgorithms.SIGNED_CURVE25519: 0} + + # Override entries with the count of any keys we pulled from the database for algorithm, key_count in txn: result[algorithm] = key_count + return result return await self.db_pool.runInteraction( From 25becda7a36fea6db21d84feb9509a13ff2c8c33 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 27 Jul 2021 11:44:17 +0100 Subject: [PATCH 3/6] Changelog --- changelog.d/10485.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10485.bugfix diff --git a/changelog.d/10485.bugfix b/changelog.d/10485.bugfix new file mode 100644 index 000000000000..7fcbe40f3242 --- /dev/null +++ b/changelog.d/10485.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug where Synapse would signal "no change" in device one-time key counts, whereas in reality the number of keys had been exhausted. \ No newline at end of file From ac561a329bc66be8f85e5bb5b7998653a26e867a Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Tue, 27 Jul 2021 11:58:15 +0100 Subject: [PATCH 4/6] Update changelog.d/10485.bugfix Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/10485.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10485.bugfix b/changelog.d/10485.bugfix index 7fcbe40f3242..9b44006dc0b2 100644 --- a/changelog.d/10485.bugfix +++ b/changelog.d/10485.bugfix @@ -1 +1 @@ -Fix a long-standing bug where Synapse would signal "no change" in device one-time key counts, whereas in reality the number of keys had been exhausted. \ No newline at end of file +Fix a long-standing bug where Synapse would not inform clients that a device had exhausted its one-time-key pool, potentially causing problems decrypting events. From dce54654c9f4895ca79411824e4ee6b1ecf9b618 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 27 Jul 2021 12:31:04 +0100 Subject: [PATCH 5/6] Update TODO with ongoing spec issue --- synapse/handlers/sync.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 353f64ac80fa..f30bfcc93cf2 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -1093,9 +1093,10 @@ async def generate_sync_result( one_time_key_counts: JsonDict = {} unused_fallback_key_types: List[str] = [] if device_id: - # TODO: If this is an incremental sync and there's been no change in a count - # since the provided since token, then simply omit that algorithm's entry - # altogether. + # TODO: We should have a way to let clients differentiate between the states of: + # * no change in OTK count since the provided since token + # * the server has zero OTKs left for this device + # Spec issue: https://github.com/matrix-org/matrix-doc/issues/3298 one_time_key_counts = await self.store.count_e2e_one_time_keys( user_id, device_id ) From 3f4a71221646780770739fbe111ed41587321a1c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 27 Jul 2021 12:44:11 +0100 Subject: [PATCH 6/6] Include signed_curve25519 in expected test responses --- tests/handlers/test_e2e_keys.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/handlers/test_e2e_keys.py b/tests/handlers/test_e2e_keys.py index e0a24824cc8f..39e7b1ab254e 100644 --- a/tests/handlers/test_e2e_keys.py +++ b/tests/handlers/test_e2e_keys.py @@ -47,12 +47,16 @@ def test_reupload_one_time_keys(self): "alg2:k3": {"key": "key3"}, } + # Note that "signed_curve25519" is always returned in key count responses. This is necessary until + # https://github.com/matrix-org/matrix-doc/issues/3298 is fixed. res = self.get_success( self.handler.upload_keys_for_user( local_user, device_id, {"one_time_keys": keys} ) ) - self.assertDictEqual(res, {"one_time_key_counts": {"alg1": 1, "alg2": 2}}) + self.assertDictEqual( + res, {"one_time_key_counts": {"alg1": 1, "alg2": 2, "signed_curve25519": 0}} + ) # we should be able to change the signature without a problem keys["alg2:k2"]["signatures"]["k1"] = "sig2" @@ -61,7 +65,9 @@ def test_reupload_one_time_keys(self): local_user, device_id, {"one_time_keys": keys} ) ) - self.assertDictEqual(res, {"one_time_key_counts": {"alg1": 1, "alg2": 2}}) + self.assertDictEqual( + res, {"one_time_key_counts": {"alg1": 1, "alg2": 2, "signed_curve25519": 0}} + ) def test_change_one_time_keys(self): """attempts to change one-time-keys should be rejected""" @@ -79,7 +85,9 @@ def test_change_one_time_keys(self): local_user, device_id, {"one_time_keys": keys} ) ) - self.assertDictEqual(res, {"one_time_key_counts": {"alg1": 1, "alg2": 2}}) + self.assertDictEqual( + res, {"one_time_key_counts": {"alg1": 1, "alg2": 2, "signed_curve25519": 0}} + ) # Error when changing string key self.get_failure( @@ -89,7 +97,7 @@ def test_change_one_time_keys(self): SynapseError, ) - # Error when replacing dict key with strin + # Error when replacing dict key with string self.get_failure( self.handler.upload_keys_for_user( local_user, device_id, {"one_time_keys": {"alg2:k3": "key2"}} @@ -131,7 +139,9 @@ def test_claim_one_time_key(self): local_user, device_id, {"one_time_keys": keys} ) ) - self.assertDictEqual(res, {"one_time_key_counts": {"alg1": 1}}) + self.assertDictEqual( + res, {"one_time_key_counts": {"alg1": 1, "signed_curve25519": 0}} + ) res2 = self.get_success( self.handler.claim_one_time_keys(