From 5906be858900e134d99dd94f0ca9e8bd1db14c05 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 20 Aug 2019 15:27:08 +0100 Subject: [PATCH 1/5] Add config option for keys to use to sign keys This allows servers to separate keys that are used to sign remote keys when acting as a notary server. --- docs/sample_config.yaml | 8 ++++++++ synapse/config/key.py | 35 +++++++++++++++++++++++++++++++---- synapse/crypto/keyring.py | 12 +++++++----- 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 0c6be30e513d..c96eb0cf2dee 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1027,6 +1027,14 @@ signing_key_path: "CONFDIR/SERVERNAME.signing.key" # #trusted_key_servers: # - server_name: "matrix.org" +# + +# The additional signing keys to use when acting as a trusted key server, on +# top of the normal signing keys. +# +# Can contain multiple keys, one per line. +# +#key_server_signing_keys_path: "key_server_signing_keys.key" # Enable SAML2 for registration and login. Uses pysaml2. diff --git a/synapse/config/key.py b/synapse/config/key.py index fe8386985cbc..f1a1efcb7f70 100644 --- a/synapse/config/key.py +++ b/synapse/config/key.py @@ -76,7 +76,7 @@ def read_config(self, config, config_dir_path, **kwargs): config_dir_path, config["server_name"] + ".signing.key" ) - self.signing_key = self.read_signing_key(signing_key_path) + self.signing_key = self.read_signing_keys(signing_key_path, "signing_key") self.old_signing_keys = self.read_old_signing_keys( config.get("old_signing_keys", {}) @@ -85,6 +85,15 @@ def read_config(self, config, config_dir_path, **kwargs): config.get("key_refresh_interval", "1d") ) + self.key_server_signing_keys = list(self.signing_key) + key_server_signing_keys_path = config.get("key_server_signing_keys_path") + if key_server_signing_keys_path: + self.key_server_signing_keys.extend( + self.read_signing_keys( + key_server_signing_keys_path, "key_server_signing_keys_path" + ) + ) + # if neither trusted_key_servers nor perspectives are given, use the default. if "perspectives" not in config and "trusted_key_servers" not in config: key_servers = [{"server_name": "matrix.org"}] @@ -210,16 +219,34 @@ def generate_config_section( # #trusted_key_servers: # - server_name: "matrix.org" + # + + # The additional signing keys to use when acting as a trusted key server, on + # top of the normal signing keys. + # + # Can contain multiple keys, one per line. + # + #key_server_signing_keys_path: "key_server_signing_keys.key" """ % locals() ) - def read_signing_key(self, signing_key_path): - signing_keys = self.read_file(signing_key_path, "signing_key") + def read_signing_keys(self, signing_key_path, name): + """Read the signing keys in the given path. + + Args: + signing_key_path (str) + name (str): Associated config key name + + Returns: + list[SigningKey] + """ + + signing_keys = self.read_file(signing_key_path, name) try: return read_signing_keys(signing_keys.splitlines(True)) except Exception as e: - raise ConfigError("Error reading signing_key: %s" % (str(e))) + raise ConfigError("Error reading %s: %s" % (name, str(e))) def read_old_signing_keys(self, old_signing_keys): keys = {} diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 6c3e885e72e6..a3b55e349edf 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -540,11 +540,13 @@ def process_v2_response(self, from_server, response_json, time_added_ms): verify_key=verify_key, valid_until_ts=key_data["expired_ts"] ) - # re-sign the json with our own key, so that it is ready if we are asked to - # give it out as a notary server - signed_key_json = sign_json( - response_json, self.config.server_name, self.config.signing_key[0] - ) + # re-sign the json with our own keys, so that it is ready if we are + # asked to give it out as a notary server + signed_key_json = response_json + for signing_key in self.config.key_server_signing_keys: + signed_key_json = sign_json( + signed_key_json, self.config.server_name, signing_key + ) signed_key_json_bytes = encode_canonical_json(signed_key_json) From 97cbc96093dcd878bc823f34d71437a08786a3e4 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 21 Aug 2019 10:39:45 +0100 Subject: [PATCH 2/5] Only sign when we respond to remote key requests --- synapse/crypto/keyring.py | 11 +-------- synapse/rest/key/v2/remote_key_resource.py | 28 ++++++++++++---------- 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index a3b55e349edf..abeb0ac26e77 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -30,7 +30,6 @@ from signedjson.sign import ( SignatureVerifyException, encode_canonical_json, - sign_json, signature_ids, verify_signed_json, ) @@ -540,15 +539,7 @@ def process_v2_response(self, from_server, response_json, time_added_ms): verify_key=verify_key, valid_until_ts=key_data["expired_ts"] ) - # re-sign the json with our own keys, so that it is ready if we are - # asked to give it out as a notary server - signed_key_json = response_json - for signing_key in self.config.key_server_signing_keys: - signed_key_json = sign_json( - signed_key_json, self.config.server_name, signing_key - ) - - signed_key_json_bytes = encode_canonical_json(signed_key_json) + signed_key_json_bytes = encode_canonical_json(response_json) yield make_deferred_yieldable( defer.gatherResults( diff --git a/synapse/rest/key/v2/remote_key_resource.py b/synapse/rest/key/v2/remote_key_resource.py index 031a3166936c..f3398c9523e3 100644 --- a/synapse/rest/key/v2/remote_key_resource.py +++ b/synapse/rest/key/v2/remote_key_resource.py @@ -13,7 +13,9 @@ # limitations under the License. import logging -from io import BytesIO + +from canonicaljson import json +from signedjson.sign import sign_json from twisted.internet import defer @@ -95,6 +97,7 @@ def __init__(self, hs): self.store = hs.get_datastore() self.clock = hs.get_clock() self.federation_domain_whitelist = hs.config.federation_domain_whitelist + self.config = hs.config @wrap_json_request_handler async def _async_render_GET(self, request): @@ -214,15 +217,14 @@ def query_keys(self, request, query, query_remote_on_cache_miss=False): yield self.fetcher.get_keys(cache_misses) yield self.query_keys(request, query, query_remote_on_cache_miss=False) else: - result_io = BytesIO() - result_io.write(b'{"server_keys":') - sep = b"[" - for json_bytes in json_results: - result_io.write(sep) - result_io.write(json_bytes) - sep = b"," - if sep == b"[": - result_io.write(sep) - result_io.write(b"]}") - - respond_with_json_bytes(request, 200, result_io.getvalue()) + signed_keys = [] + for key_json in json_results: + key_json = json.loads(key_json) + for signing_key in self.config.key_server_signing_keys: + key_json = sign_json(key_json, self.config.server_name, signing_key) + + signed_keys.append(key_json) + + results = {"server_keys": signed_keys} + + respond_with_json_bytes(request, 200, json.dumps(results).encode("utf-8")) From 62fb643cdca80568a404c46a255384cd73b6e16b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 21 Aug 2019 10:41:29 +0100 Subject: [PATCH 3/5] Newsfile --- changelog.d/5895.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5895.feature diff --git a/changelog.d/5895.feature b/changelog.d/5895.feature new file mode 100644 index 000000000000..c394a3772cb6 --- /dev/null +++ b/changelog.d/5895.feature @@ -0,0 +1 @@ +Add config option to sign remote key query responses with a separate key. From 7af5a63063aa69888ab59ee997cc3d1459d25af4 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 23 Aug 2019 14:52:11 +0100 Subject: [PATCH 4/5] Fixup review comments --- docs/sample_config.yaml | 4 ++-- synapse/crypto/keyring.py | 4 ++-- synapse/rest/key/v2/remote_key_resource.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index c96eb0cf2dee..ae1cafc5f310 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1029,8 +1029,8 @@ signing_key_path: "CONFDIR/SERVERNAME.signing.key" # - server_name: "matrix.org" # -# The additional signing keys to use when acting as a trusted key server, on -# top of the normal signing keys. +# The signing keys to use when acting as a trusted key server. If not specified +# defaults to the server signing key. # # Can contain multiple keys, one per line. # diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index abeb0ac26e77..2d7434fb2f52 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -539,7 +539,7 @@ def process_v2_response(self, from_server, response_json, time_added_ms): verify_key=verify_key, valid_until_ts=key_data["expired_ts"] ) - signed_key_json_bytes = encode_canonical_json(response_json) + key_json_bytes = encode_canonical_json(response_json) yield make_deferred_yieldable( defer.gatherResults( @@ -551,7 +551,7 @@ def process_v2_response(self, from_server, response_json, time_added_ms): from_server=from_server, ts_now_ms=time_added_ms, ts_expires_ms=ts_valid_until_ms, - key_json_bytes=signed_key_json_bytes, + key_json_bytes=key_json_bytes, ) for key_id in verify_keys ], diff --git a/synapse/rest/key/v2/remote_key_resource.py b/synapse/rest/key/v2/remote_key_resource.py index f3398c9523e3..55580bc59ecf 100644 --- a/synapse/rest/key/v2/remote_key_resource.py +++ b/synapse/rest/key/v2/remote_key_resource.py @@ -14,7 +14,7 @@ import logging -from canonicaljson import json +from canonicaljson import encode_canonical_json, json from signedjson.sign import sign_json from twisted.internet import defer @@ -227,4 +227,4 @@ def query_keys(self, request, query, query_remote_on_cache_miss=False): results = {"server_keys": signed_keys} - respond_with_json_bytes(request, 200, json.dumps(results).encode("utf-8")) + respond_with_json_bytes(request, 200, encode_canonical_json(results)) From fe0ac98e6653903cce43b1c5a3be77ef4f626867 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 23 Aug 2019 14:54:20 +0100 Subject: [PATCH 5/5] Don't implicitly include server signing key --- synapse/config/key.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/synapse/config/key.py b/synapse/config/key.py index f1a1efcb7f70..ba2199bcebea 100644 --- a/synapse/config/key.py +++ b/synapse/config/key.py @@ -85,14 +85,13 @@ def read_config(self, config, config_dir_path, **kwargs): config.get("key_refresh_interval", "1d") ) - self.key_server_signing_keys = list(self.signing_key) key_server_signing_keys_path = config.get("key_server_signing_keys_path") if key_server_signing_keys_path: - self.key_server_signing_keys.extend( - self.read_signing_keys( - key_server_signing_keys_path, "key_server_signing_keys_path" - ) + self.key_server_signing_keys = self.read_signing_keys( + key_server_signing_keys_path, "key_server_signing_keys_path" ) + else: + self.key_server_signing_keys = list(self.signing_key) # if neither trusted_key_servers nor perspectives are given, use the default. if "perspectives" not in config and "trusted_key_servers" not in config: @@ -221,8 +220,8 @@ def generate_config_section( # - server_name: "matrix.org" # - # The additional signing keys to use when acting as a trusted key server, on - # top of the normal signing keys. + # The signing keys to use when acting as a trusted key server. If not specified + # defaults to the server signing key. # # Can contain multiple keys, one per line. #