From 7efd1d87c2c424365c99ba6103135edb1845fd88 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Wed, 3 Apr 2019 20:07:29 +1100 Subject: Run black on the rest of the storage module (#4996) --- synapse/storage/keys.py | 45 ++++++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 27 deletions(-) (limited to 'synapse/storage/keys.py') diff --git a/synapse/storage/keys.py b/synapse/storage/keys.py index 8af17921e3..030cd1e5a3 100644 --- a/synapse/storage/keys.py +++ b/synapse/storage/keys.py @@ -56,12 +56,13 @@ class KeyStore(SQLBaseStore): desc="get_server_certificate", ) tls_certificate = OpenSSL.crypto.load_certificate( - OpenSSL.crypto.FILETYPE_ASN1, tls_certificate_bytes, + OpenSSL.crypto.FILETYPE_ASN1, tls_certificate_bytes ) defer.returnValue(tls_certificate) - def store_server_certificate(self, server_name, from_server, time_now_ms, - tls_certificate): + def store_server_certificate( + self, server_name, from_server, time_now_ms, tls_certificate + ): """Stores the TLS X.509 certificate for the given server Args: server_name (str): The name of the server. @@ -75,10 +76,7 @@ class KeyStore(SQLBaseStore): fingerprint = hashlib.sha256(tls_certificate_bytes).hexdigest() return self._simple_upsert( table="server_tls_certificates", - keyvalues={ - "server_name": server_name, - "fingerprint": fingerprint, - }, + keyvalues={"server_name": server_name, "fingerprint": fingerprint}, values={ "from_server": from_server, "ts_added_ms": time_now_ms, @@ -91,19 +89,14 @@ class KeyStore(SQLBaseStore): def _get_server_verify_key(self, server_name, key_id): verify_key_bytes = yield self._simple_select_one_onecol( table="server_signature_keys", - keyvalues={ - "server_name": server_name, - "key_id": key_id, - }, + keyvalues={"server_name": server_name, "key_id": key_id}, retcol="verify_key", desc="_get_server_verify_key", allow_none=True, ) if verify_key_bytes: - defer.returnValue(decode_verify_key_bytes( - key_id, bytes(verify_key_bytes) - )) + defer.returnValue(decode_verify_key_bytes(key_id, bytes(verify_key_bytes))) @defer.inlineCallbacks def get_server_verify_keys(self, server_name, key_ids): @@ -123,8 +116,9 @@ class KeyStore(SQLBaseStore): keys[key_id] = key defer.returnValue(keys) - def store_server_verify_key(self, server_name, from_server, time_now_ms, - verify_key): + def store_server_verify_key( + self, server_name, from_server, time_now_ms, verify_key + ): """Stores a NACL verification key for the given server. Args: server_name (str): The name of the server. @@ -139,10 +133,7 @@ class KeyStore(SQLBaseStore): self._simple_upsert_txn( txn, table="server_signature_keys", - keyvalues={ - "server_name": server_name, - "key_id": key_id, - }, + keyvalues={"server_name": server_name, "key_id": key_id}, values={ "from_server": from_server, "ts_added_ms": time_now_ms, @@ -150,14 +141,14 @@ class KeyStore(SQLBaseStore): }, ) txn.call_after( - self._get_server_verify_key.invalidate, - (server_name, key_id) + self._get_server_verify_key.invalidate, (server_name, key_id) ) return self.runInteraction("store_server_verify_key", _txn) - def store_server_keys_json(self, server_name, key_id, from_server, - ts_now_ms, ts_expires_ms, key_json_bytes): + def store_server_keys_json( + self, server_name, key_id, from_server, ts_now_ms, ts_expires_ms, key_json_bytes + ): """Stores the JSON bytes for a set of keys from a server The JSON should be signed by the originating server, the intermediate server, and by this server. Updates the value for the @@ -200,6 +191,7 @@ class KeyStore(SQLBaseStore): Dict mapping (server_name, key_id, source) triplets to dicts with "ts_valid_until_ms" and "key_json" keys. """ + def _get_server_keys_json_txn(txn): results = {} for server_name, key_id, from_server in server_keys: @@ -222,6 +214,5 @@ class KeyStore(SQLBaseStore): ) results[(server_name, key_id, from_server)] = rows return results - return self.runInteraction( - "get_server_keys_json", _get_server_keys_json_txn - ) + + return self.runInteraction("get_server_keys_json", _get_server_keys_json_txn) -- cgit 1.4.1 From b43d9a920b434180b0ae12516b19d09011f37c59 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 4 Apr 2019 18:54:03 +0100 Subject: Fix docstring on get_server_keys_json --- synapse/storage/keys.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'synapse/storage/keys.py') diff --git a/synapse/storage/keys.py b/synapse/storage/keys.py index 030cd1e5a3..f24ab3eedd 100644 --- a/synapse/storage/keys.py +++ b/synapse/storage/keys.py @@ -188,8 +188,8 @@ class KeyStore(SQLBaseStore): Args: server_keys (list): List of (server_name, key_id, source) triplets. Returns: - Dict mapping (server_name, key_id, source) triplets to dicts with - "ts_valid_until_ms" and "key_json" keys. + Deferred[dict[Tuple[str, str, str|None], list[dict]]]: + Dict mapping (server_name, key_id, source) triplets to lists of dicts """ def _get_server_keys_json_txn(txn): -- cgit 1.4.1 From 3352baac4b03f3414e0a006b9413b65454d1fe91 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 8 Apr 2019 21:50:18 +0100 Subject: Remove unused server_tls_certificates functions (#5028) These have been unused since #4120, and with the demise of perspectives, it is unlikely that they will ever be used again. --- changelog.d/4992.misc | 2 +- changelog.d/5028.misc | 1 + synapse/replication/slave/storage/keys.py | 3 -- synapse/storage/keys.py | 49 +--------------------- .../storage/schema/delta/54/drop_legacy_tables.sql | 4 +- synapse/storage/schema/full_schemas/16/keys.sql | 11 ++--- 6 files changed, 7 insertions(+), 63 deletions(-) create mode 100644 changelog.d/5028.misc (limited to 'synapse/storage/keys.py') diff --git a/changelog.d/4992.misc b/changelog.d/4992.misc index 8a9eaea4cf..3ee4228c09 100644 --- a/changelog.d/4992.misc +++ b/changelog.d/4992.misc @@ -1 +1 @@ -Remove legacy tables detailed in #1830. +Remove a number of unused tables from the database schema. diff --git a/changelog.d/5028.misc b/changelog.d/5028.misc new file mode 100644 index 0000000000..3ee4228c09 --- /dev/null +++ b/changelog.d/5028.misc @@ -0,0 +1 @@ +Remove a number of unused tables from the database schema. diff --git a/synapse/replication/slave/storage/keys.py b/synapse/replication/slave/storage/keys.py index 8032f53fec..de00660c0e 100644 --- a/synapse/replication/slave/storage/keys.py +++ b/synapse/replication/slave/storage/keys.py @@ -27,8 +27,5 @@ class SlavedKeyStore(BaseSlavedStore): get_server_verify_keys = __func__(DataStore.get_server_verify_keys) store_server_verify_key = __func__(DataStore.store_server_verify_key) - get_server_certificate = __func__(DataStore.get_server_certificate) - store_server_certificate = __func__(DataStore.store_server_certificate) - get_server_keys_json = __func__(DataStore.get_server_keys_json) store_server_keys_json = __func__(DataStore.store_server_keys_json) diff --git a/synapse/storage/keys.py b/synapse/storage/keys.py index f24ab3eedd..47a9aa784b 100644 --- a/synapse/storage/keys.py +++ b/synapse/storage/keys.py @@ -13,14 +13,12 @@ # See the License for the specific language governing permissions and # limitations under the License. -import hashlib import logging import six from signedjson.key import decode_verify_key_bytes -import OpenSSL from twisted.internet import defer from synapse.util.caches.descriptors import cachedInlineCallbacks @@ -38,53 +36,8 @@ else: class KeyStore(SQLBaseStore): - """Persistence for signature verification keys and tls X.509 certificates + """Persistence for signature verification keys """ - - @defer.inlineCallbacks - def get_server_certificate(self, server_name): - """Retrieve the TLS X.509 certificate for the given server - Args: - server_name (bytes): The name of the server. - Returns: - (OpenSSL.crypto.X509): The tls certificate. - """ - tls_certificate_bytes, = yield self._simple_select_one( - table="server_tls_certificates", - keyvalues={"server_name": server_name}, - retcols=("tls_certificate",), - desc="get_server_certificate", - ) - tls_certificate = OpenSSL.crypto.load_certificate( - OpenSSL.crypto.FILETYPE_ASN1, tls_certificate_bytes - ) - defer.returnValue(tls_certificate) - - def store_server_certificate( - self, server_name, from_server, time_now_ms, tls_certificate - ): - """Stores the TLS X.509 certificate for the given server - Args: - server_name (str): The name of the server. - from_server (str): Where the certificate was looked up - time_now_ms (int): The time now in milliseconds - tls_certificate (OpenSSL.crypto.X509): The X.509 certificate. - """ - tls_certificate_bytes = OpenSSL.crypto.dump_certificate( - OpenSSL.crypto.FILETYPE_ASN1, tls_certificate - ) - fingerprint = hashlib.sha256(tls_certificate_bytes).hexdigest() - return self._simple_upsert( - table="server_tls_certificates", - keyvalues={"server_name": server_name, "fingerprint": fingerprint}, - values={ - "from_server": from_server, - "ts_added_ms": time_now_ms, - "tls_certificate": db_binary_type(tls_certificate_bytes), - }, - desc="store_server_certificate", - ) - @cachedInlineCallbacks() def _get_server_verify_key(self, server_name, key_id): verify_key_bytes = yield self._simple_select_one_onecol( diff --git a/synapse/storage/schema/delta/54/drop_legacy_tables.sql b/synapse/storage/schema/delta/54/drop_legacy_tables.sql index 77b39dc2d2..ecca005d9b 100644 --- a/synapse/storage/schema/delta/54/drop_legacy_tables.sql +++ b/synapse/storage/schema/delta/54/drop_legacy_tables.sql @@ -24,7 +24,5 @@ DROP TABLE IF EXISTS event_edge_hashes; DROP TABLE IF EXISTS event_signatures; DROP TABLE IF EXISTS feedback; DROP TABLE IF EXISTS room_hosts; +DROP TABLE IF EXISTS server_tls_certificates; DROP TABLE IF EXISTS state_forward_extremities; - - - diff --git a/synapse/storage/schema/full_schemas/16/keys.sql b/synapse/storage/schema/full_schemas/16/keys.sql index ca0ca1b694..11cdffdbb3 100644 --- a/synapse/storage/schema/full_schemas/16/keys.sql +++ b/synapse/storage/schema/full_schemas/16/keys.sql @@ -12,14 +12,9 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -CREATE TABLE IF NOT EXISTS server_tls_certificates( - server_name TEXT, -- Server name. - fingerprint TEXT, -- Certificate fingerprint. - from_server TEXT, -- Which key server the certificate was fetched from. - ts_added_ms BIGINT, -- When the certifcate was added. - tls_certificate bytea, -- DER encoded x509 certificate. - UNIQUE (server_name, fingerprint) -); + +-- we used to create a table called server_tls_certificates, but this is no +-- longer used, and is removed in delta 54. CREATE TABLE IF NOT EXISTS server_signature_keys( server_name TEXT, -- Server name. -- cgit 1.4.1 From 18b69be00f9fa79cf2b237992ef1f0094d1dc453 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 8 Apr 2019 14:51:07 +0100 Subject: Rewrite Datastore.get_server_verify_keys Rewrite this so that it doesn't hammer the database. --- synapse/crypto/keyring.py | 38 +++++++++++------------- synapse/storage/keys.py | 74 ++++++++++++++++++++++++++++------------------ tests/storage/test_keys.py | 53 +++++++++++++++++++++++++++++++-- 3 files changed, 113 insertions(+), 52 deletions(-) (limited to 'synapse/storage/keys.py') diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index ede120b2a6..834b107705 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -301,13 +301,12 @@ class Keyring(object): # complete this VerifyKeyRequest. result_keys = results.get(server_name, {}) for key_id in verify_request.key_ids: - if key_id in result_keys: + key = result_keys.get(key_id) + if key: with PreserveLoggingContext(): - verify_request.deferred.callback(( - server_name, - key_id, - result_keys[key_id], - )) + verify_request.deferred.callback( + (server_name, key_id, key) + ) break else: # The else block is only reached if the loop above @@ -341,27 +340,24 @@ class Keyring(object): @defer.inlineCallbacks def get_keys_from_store(self, server_name_and_key_ids): """ - Args: - server_name_and_key_ids (list[(str, iterable[str])]): + server_name_and_key_ids (iterable(Tuple[str, iterable[str]]): list of (server_name, iterable[key_id]) tuples to fetch keys for Returns: - Deferred: resolves to dict[str, dict[str, VerifyKey]]: map from + Deferred: resolves to dict[str, dict[str, VerifyKey|None]]: map from server_name -> key_id -> VerifyKey """ - res = yield logcontext.make_deferred_yieldable(defer.gatherResults( - [ - run_in_background( - self.store.get_server_verify_keys, - server_name, key_ids, - ).addCallback(lambda ks, server: (server, ks), server_name) - for server_name, key_ids in server_name_and_key_ids - ], - consumeErrors=True, - ).addErrback(unwrapFirstError)) - - defer.returnValue(dict(res)) + keys_to_fetch = ( + (server_name, key_id) + for server_name, key_ids in server_name_and_key_ids + for key_id in key_ids + ) + res = yield self.store.get_server_verify_keys(keys_to_fetch) + keys = {} + for (server_name, key_id), key in res.items(): + keys.setdefault(server_name, {})[key_id] = key + defer.returnValue(keys) @defer.inlineCallbacks def get_keys_from_perspectives(self, server_name_and_key_ids): diff --git a/synapse/storage/keys.py b/synapse/storage/keys.py index 47a9aa784b..7036541792 100644 --- a/synapse/storage/keys.py +++ b/synapse/storage/keys.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- # Copyright 2014-2016 OpenMarket Ltd +# Copyright 2019 New Vector Ltd. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -13,15 +14,15 @@ # See the License for the specific language governing permissions and # limitations under the License. +import itertools import logging import six from signedjson.key import decode_verify_key_bytes -from twisted.internet import defer - -from synapse.util.caches.descriptors import cachedInlineCallbacks +from synapse.util import batch_iter +from synapse.util.caches.descriptors import cached, cachedList from ._base import SQLBaseStore @@ -38,36 +39,50 @@ else: class KeyStore(SQLBaseStore): """Persistence for signature verification keys """ - @cachedInlineCallbacks() - def _get_server_verify_key(self, server_name, key_id): - verify_key_bytes = yield self._simple_select_one_onecol( - table="server_signature_keys", - keyvalues={"server_name": server_name, "key_id": key_id}, - retcol="verify_key", - desc="_get_server_verify_key", - allow_none=True, - ) - if verify_key_bytes: - defer.returnValue(decode_verify_key_bytes(key_id, bytes(verify_key_bytes))) + @cached() + def _get_server_verify_key(self, server_name_and_key_id): + raise NotImplementedError() - @defer.inlineCallbacks - def get_server_verify_keys(self, server_name, key_ids): - """Retrieve the NACL verification key for a given server for the given - key_ids + @cachedList( + cached_method_name="_get_server_verify_key", list_name="server_name_and_key_ids" + ) + def get_server_verify_keys(self, server_name_and_key_ids): + """ Args: - server_name (str): The name of the server. - key_ids (iterable[str]): key_ids to try and look up. + server_name_and_key_ids (iterable[Tuple[str, str]]): + iterable of (server_name, key-id) tuples to fetch keys for + Returns: - Deferred: resolves to dict[str, VerifyKey]: map from - key_id to verification key. + Deferred: resolves to dict[Tuple[str, str], VerifyKey|None]: + map from (server_name, key_id) -> VerifyKey, or None if the key is + unknown """ keys = {} - for key_id in key_ids: - key = yield self._get_server_verify_key(server_name, key_id) - if key: - keys[key_id] = key - defer.returnValue(keys) + + def _get_keys(txn, batch): + """Processes a batch of keys to fetch, and adds the result to `keys`.""" + + # batch_iter always returns tuples so it's safe to do len(batch) + sql = ( + "SELECT server_name, key_id, verify_key FROM server_signature_keys " + "WHERE 1=0" + ) + " OR (server_name=? AND key_id=?)" * len(batch) + + txn.execute(sql, tuple(itertools.chain.from_iterable(batch))) + + for row in txn: + server_name, key_id, key_bytes = row + keys[(server_name, key_id)] = decode_verify_key_bytes( + key_id, bytes(key_bytes) + ) + + def _txn(txn): + for batch in batch_iter(server_name_and_key_ids, 50): + _get_keys(txn, batch) + return keys + + return self.runInteraction("get_server_verify_keys", _txn) def store_server_verify_key( self, server_name, from_server, time_now_ms, verify_key @@ -93,8 +108,11 @@ class KeyStore(SQLBaseStore): "verify_key": db_binary_type(verify_key.encode()), }, ) + # invalidate takes a tuple corresponding to the params of + # _get_server_verify_key. _get_server_verify_key only takes one + # param, which is itself the 2-tuple (server_name, key_id). txn.call_after( - self._get_server_verify_key.invalidate, (server_name, key_id) + self._get_server_verify_key.invalidate, ((server_name, key_id),) ) return self.runInteraction("store_server_verify_key", _txn) diff --git a/tests/storage/test_keys.py b/tests/storage/test_keys.py index 7170ae76c7..6bfaa00fe9 100644 --- a/tests/storage/test_keys.py +++ b/tests/storage/test_keys.py @@ -15,6 +15,8 @@ import signedjson.key +from twisted.internet.defer import Deferred + import tests.unittest KEY_1 = signedjson.key.decode_verify_key_base64( @@ -35,10 +37,55 @@ class KeyStoreTestCase(tests.unittest.HomeserverTestCase): self.get_success(d) d = store.get_server_verify_keys( - "server1", ["ed25519:key1", "ed25519:key2", "ed25519:key3"] + [ + ("server1", "ed25519:key1"), + ("server1", "ed25519:key2"), + ("server1", "ed25519:key3"), + ] ) res = self.get_success(d) + self.assertEqual(len(res.keys()), 3) + self.assertEqual(res[("server1", "ed25519:key1")].version, "key1") + self.assertEqual(res[("server1", "ed25519:key2")].version, "key2") + + # non-existent result gives None + self.assertIsNone(res[("server1", "ed25519:key3")]) + + def test_cache(self): + """Check that updates correctly invalidate the cache.""" + + store = self.hs.get_datastore() + + key_id_1 = "ed25519:key1" + key_id_2 = "ed25519:key2" + + d = store.store_server_verify_key("srv1", "from_server", 0, KEY_1) + self.get_success(d) + d = store.store_server_verify_key("srv1", "from_server", 0, KEY_2) + self.get_success(d) + + d = store.get_server_verify_keys([("srv1", key_id_1), ("srv1", key_id_2)]) + res = self.get_success(d) + self.assertEqual(len(res.keys()), 2) + self.assertEqual(res[("srv1", key_id_1)], KEY_1) + self.assertEqual(res[("srv1", key_id_2)], KEY_2) + + # we should be able to look up the same thing again without a db hit + res = store.get_server_verify_keys([("srv1", key_id_1)]) + if isinstance(res, Deferred): + res = self.successResultOf(res) + self.assertEqual(len(res.keys()), 1) + self.assertEqual(res[("srv1", key_id_1)], KEY_1) + + new_key_2 = signedjson.key.get_verify_key( + signedjson.key.generate_signing_key("key2") + ) + d = store.store_server_verify_key("srv1", "from_server", 10, new_key_2) + self.get_success(d) + + d = store.get_server_verify_keys([("srv1", key_id_1), ("srv1", key_id_2)]) + res = self.get_success(d) self.assertEqual(len(res.keys()), 2) - self.assertEqual(res["ed25519:key1"].version, "key1") - self.assertEqual(res["ed25519:key2"].version, "key2") + self.assertEqual(res[("srv1", key_id_1)], KEY_1) + self.assertEqual(res[("srv1", key_id_2)], new_key_2) -- cgit 1.4.1