From c2b6e945e1c455c10e09684a0e43e19937db604c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sun, 9 Jun 2019 14:01:32 +0100 Subject: Share an SSL context object between SSL connections This involves changing how the info callbacks work. --- synapse/crypto/context_factory.py | 149 +++++++++++++++++++++++--------------- 1 file changed, 89 insertions(+), 60 deletions(-) (limited to 'synapse') diff --git a/synapse/crypto/context_factory.py b/synapse/crypto/context_factory.py index 59ea087e66..aa8b20fe7d 100644 --- a/synapse/crypto/context_factory.py +++ b/synapse/crypto/context_factory.py @@ -15,10 +15,12 @@ import logging +from service_identity import VerificationError +from service_identity.pyopenssl import verify_hostname from zope.interface import implementer from OpenSSL import SSL, crypto -from twisted.internet._sslverify import ClientTLSOptions, _defaultCurveName +from twisted.internet._sslverify import _defaultCurveName from twisted.internet.abstract import isIPAddress, isIPv6Address from twisted.internet.interfaces import IOpenSSLClientConnectionCreator from twisted.internet.ssl import CertificateOptions, ContextFactory, platformTrust @@ -70,65 +72,19 @@ def _idnaBytes(text): return idna.encode(text) -def _tolerateErrors(wrapped): - """ - Wrap up an info_callback for pyOpenSSL so that if something goes wrong - the error is immediately logged and the connection is dropped if possible. - This is a copy of twisted.internet._sslverify._tolerateErrors. For - documentation, see the twisted documentation. - """ - - def infoCallback(connection, where, ret): - try: - return wrapped(connection, where, ret) - except: # noqa: E722, taken from the twisted implementation - f = Failure() - logger.exception("Error during info_callback") - connection.get_app_data().failVerification(f) - - return infoCallback +class ClientTLSOptionsFactory(object): + """Factory for Twisted SSLClientConnectionCreators that are used to make connections + to remote servers for federation. + Uses one of two OpenSSL context objects for all connections, depending on whether + we should do SSL certificate verification. -@implementer(IOpenSSLClientConnectionCreator) -class ClientTLSOptionsNoVerify(object): - """ - Client creator for TLS without certificate identity verification. This is a - copy of twisted.internet._sslverify.ClientTLSOptions with the identity - verification left out. For documentation, see the twisted documentation. + get_options decides whether we should do SSL certificate verification and + constructs an SSLClientConnectionCreator factory accordingly. """ - def __init__(self, hostname, ctx): - self._ctx = ctx - - if isIPAddress(hostname) or isIPv6Address(hostname): - self._hostnameBytes = hostname.encode('ascii') - self._sendSNI = False - else: - self._hostnameBytes = _idnaBytes(hostname) - self._sendSNI = True - - ctx.set_info_callback(_tolerateErrors(self._identityVerifyingInfoCallback)) - - def clientConnectionForTLS(self, tlsProtocol): - context = self._ctx - connection = SSL.Connection(context, None) - connection.set_app_data(tlsProtocol) - return connection - - def _identityVerifyingInfoCallback(self, connection, where, ret): - # Literal IPv4 and IPv6 addresses are not permitted - # as host names according to the RFCs - if where & SSL.SSL_CB_HANDSHAKE_START and self._sendSNI: - connection.set_tlsext_host_name(self._hostnameBytes) - - -class ClientTLSOptionsFactory(object): - """Factory for Twisted ClientTLSOptions that are used to make connections - to remote servers for federation.""" - def __init__(self, config): self._config = config - self._options_noverify = CertificateOptions() # Check if we're using a custom list of a CA certificates trust_root = config.federation_ca_trust_root @@ -136,11 +92,13 @@ class ClientTLSOptionsFactory(object): # Use CA root certs provided by OpenSSL trust_root = platformTrust() - self._options_verify = CertificateOptions(trustRoot=trust_root) + self._verify_ssl_context = CertificateOptions(trustRoot=trust_root).getContext() + self._verify_ssl_context.set_info_callback(self._context_info_cb) - def get_options(self, host): - # Use _makeContext so that we get a fresh OpenSSL CTX each time. + self._no_verify_ssl_context = CertificateOptions().getContext() + self._no_verify_ssl_context.set_info_callback(self._context_info_cb) + def get_options(self, host): # Check if certificate verification has been enabled should_verify = self._config.federation_verify_certificates @@ -151,6 +109,77 @@ class ClientTLSOptionsFactory(object): should_verify = False break - if should_verify: - return ClientTLSOptions(host, self._options_verify._makeContext()) - return ClientTLSOptionsNoVerify(host, self._options_noverify._makeContext()) + ssl_context = ( + self._verify_ssl_context if should_verify else self._no_verify_ssl_context + ) + + return SSLClientConnectionCreator(host, ssl_context, should_verify) + + @staticmethod + def _context_info_cb(ssl_connection, where, ret): + """The 'information callback' for our openssl context object.""" + # we assume that the app_data on the connection object has been set to + # a TLSMemoryBIOProtocol object. (This is done by SSLClientConnectionCreator) + tls_protocol = ssl_connection.get_app_data() + try: + # ... we further assume that SSLClientConnectionCreator has set the + # 'tls_verifier' attribute to a ConnectionVerifier object. + tls_protocol.tls_verifier.verify_context_info_cb(ssl_connection, where) + except: # noqa: E722, taken from the twisted implementation + logger.exception("Error during info_callback") + f = Failure() + tls_protocol.failVerification(f) + + +@implementer(IOpenSSLClientConnectionCreator) +class SSLClientConnectionCreator(object): + """Creates openssl connection objects for client connections. + + Replaces twisted.internet.ssl.ClientTLSOptions + """ + def __init__(self, hostname, ctx, verify_certs): + self._ctx = ctx + self._verifier = ConnectionVerifier(hostname, verify_certs) + + def clientConnectionForTLS(self, tls_protocol): + context = self._ctx + connection = SSL.Connection(context, None) + + # as per twisted.internet.ssl.ClientTLSOptions, we set the application + # data to our TLSMemoryBIOProtocol... + connection.set_app_data(tls_protocol) + + # ... and we also gut-wrench a 'tls_verifier' attribute into the + # tls_protocol so that the SSL context's info callback has something to + # call to do the cert verification. + setattr(tls_protocol, "tls_verifier", self._verifier) + return connection + + +class ConnectionVerifier(object): + """Set the SNI, and do cert verification + + This is a thing which is attached to the TLSMemoryBIOProtocol, and is called by + the ssl context's info callback. + """ + def __init__(self, hostname, verify_certs): + self._verify_certs = verify_certs + if isIPAddress(hostname) or isIPv6Address(hostname): + self._hostnameBytes = hostname.encode('ascii') + self._sendSNI = False + else: + self._hostnameBytes = _idnaBytes(hostname) + self._sendSNI = True + + self._hostnameASCII = self._hostnameBytes.decode("ascii") + + def verify_context_info_cb(self, ssl_connection, where): + if where & SSL.SSL_CB_HANDSHAKE_START and self._sendSNI: + ssl_connection.set_tlsext_host_name(self._hostnameBytes) + if where & SSL.SSL_CB_HANDSHAKE_DONE and self._verify_certs: + try: + verify_hostname(ssl_connection, self._hostnameASCII) + except VerificationError: + f = Failure() + tls_protocol = ssl_connection.get_app_data() + tls_protocol.failVerification(f) -- cgit 1.4.1 From 88d7182adaef8711bf3cc80ff604e566e517b6e6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 10 Jun 2019 10:33:00 +0100 Subject: Improve startup checks for insecure notary configs (#5392) It's not really a problem to trust notary responses signed by the old key so long as we are also doing TLS validation. This commit adds a check to the config parsing code at startup to check that we do not have the insecure matrix.org key without tls validation, and refuses to start without it. This allows us to remove the rather alarming-looking warning which happens at runtime. --- changelog.d/5392.bugfix | 1 + synapse/config/key.py | 27 +++++++++++++++++++++++---- synapse/crypto/keyring.py | 7 ------- 3 files changed, 24 insertions(+), 11 deletions(-) create mode 100644 changelog.d/5392.bugfix (limited to 'synapse') diff --git a/changelog.d/5392.bugfix b/changelog.d/5392.bugfix new file mode 100644 index 0000000000..295a7cfce1 --- /dev/null +++ b/changelog.d/5392.bugfix @@ -0,0 +1 @@ +Remove redundant warning about key server response validation. diff --git a/synapse/config/key.py b/synapse/config/key.py index aba7092ccd..424875feae 100644 --- a/synapse/config/key.py +++ b/synapse/config/key.py @@ -41,6 +41,15 @@ validation or TLS certificate validation. This is likely to be very insecure. If you are *sure* you want to do this, set 'accept_keys_insecurely' on the keyserver configuration.""" +RELYING_ON_MATRIX_KEY_ERROR = """\ +Your server is configured to accept key server responses without TLS certificate +validation, and which are only signed by the old (possibly compromised) +matrix.org signing key 'ed25519:auto'. This likely isn't what you want to do, +and you should enable 'federation_verify_certificates' in your configuration. + +If you are *sure* you want to do this, set 'accept_keys_insecurely' on the +trusted_key_server configuration.""" + logger = logging.getLogger(__name__) @@ -340,10 +349,20 @@ def _parse_key_servers(key_servers, federation_verify_certificates): result.verify_keys[key_id] = verify_key if ( - not verify_keys - and not server.get("accept_keys_insecurely") - and not federation_verify_certificates + not federation_verify_certificates and + not server.get("accept_keys_insecurely") ): - raise ConfigError(INSECURE_NOTARY_ERROR) + _assert_keyserver_has_verify_keys(result) yield result + + +def _assert_keyserver_has_verify_keys(trusted_key_server): + if not trusted_key_server.verify_keys: + raise ConfigError(INSECURE_NOTARY_ERROR) + + # also check that they are not blindly checking the old matrix.org key + if trusted_key_server.server_name == "matrix.org" and any( + key_id == "ed25519:auto" for key_id in trusted_key_server.verify_keys + ): + raise ConfigError(RELYING_ON_MATRIX_KEY_ERROR) diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 96964b0d50..6f603f1961 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -750,13 +750,6 @@ class PerspectivesKeyFetcher(BaseV2KeyFetcher): verify_signed_json(response, perspective_name, perspective_keys[key_id]) verified = True - if perspective_name == "matrix.org" and key_id == "ed25519:auto": - logger.warning( - "Trusting trusted_key_server responses signed by the " - "compromised matrix.org signing key 'ed25519:auto'. " - "This is a placebo." - ) - if not verified: raise KeyLookupError( "Response not signed with a known key: signed with: %r, known keys: %r" -- cgit 1.4.1 From d11c634ced532ed5ecdbefb45f0b5ae5cb2f9826 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 10 Jun 2019 15:55:12 +0100 Subject: clean up impl, and import idna directly --- synapse/crypto/context_factory.py | 26 +++++++++++--------------- synapse/python_dependencies.py | 1 + 2 files changed, 12 insertions(+), 15 deletions(-) (limited to 'synapse') diff --git a/synapse/crypto/context_factory.py b/synapse/crypto/context_factory.py index aa8b20fe7d..2f23782630 100644 --- a/synapse/crypto/context_factory.py +++ b/synapse/crypto/context_factory.py @@ -15,6 +15,7 @@ import logging +import idna from service_identity import VerificationError from service_identity.pyopenssl import verify_hostname from zope.interface import implementer @@ -58,20 +59,6 @@ class ServerContextFactory(ContextFactory): return self._context -def _idnaBytes(text): - """ - Convert some text typed by a human into some ASCII bytes. This is a - copy of twisted.internet._idna._idnaBytes. For documentation, see the - twisted documentation. - """ - try: - import idna - except ImportError: - return text.encode("idna") - else: - return idna.encode(text) - - class ClientTLSOptionsFactory(object): """Factory for Twisted SSLClientConnectionCreators that are used to make connections to remote servers for federation. @@ -162,13 +149,21 @@ class ConnectionVerifier(object): This is a thing which is attached to the TLSMemoryBIOProtocol, and is called by the ssl context's info callback. """ + # This code is based on twisted.internet.ssl.ClientTLSOptions. + def __init__(self, hostname, verify_certs): self._verify_certs = verify_certs + if isIPAddress(hostname) or isIPv6Address(hostname): self._hostnameBytes = hostname.encode('ascii') self._sendSNI = False else: - self._hostnameBytes = _idnaBytes(hostname) + # twisted's ClientTLSOptions falls back to the stdlib impl here if + # idna is not installed, but points out that lacks support for + # IDNA2008 (http://bugs.python.org/issue17305). + # + # We can rely on having idna. + self._hostnameBytes = idna.encode(hostname) self._sendSNI = True self._hostnameASCII = self._hostnameBytes.decode("ascii") @@ -176,6 +171,7 @@ class ConnectionVerifier(object): def verify_context_info_cb(self, ssl_connection, where): if where & SSL.SSL_CB_HANDSHAKE_START and self._sendSNI: ssl_connection.set_tlsext_host_name(self._hostnameBytes) + if where & SSL.SSL_CB_HANDSHAKE_DONE and self._verify_certs: try: verify_hostname(ssl_connection, self._hostnameASCII) diff --git a/synapse/python_dependencies.py b/synapse/python_dependencies.py index c78f2cb15e..db09ff285f 100644 --- a/synapse/python_dependencies.py +++ b/synapse/python_dependencies.py @@ -44,6 +44,7 @@ REQUIREMENTS = [ "canonicaljson>=1.1.3", "signedjson>=1.0.0", "pynacl>=1.2.1", + "idna>=2", "service_identity>=16.0.0", # our logcontext handling relies on the ability to cancel inlineCallbacks -- cgit 1.4.1 From efe7b3176ecfe81cb7eb94a6882228ba5682278d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 10 Jun 2019 15:58:35 +0100 Subject: Fix federation connections to literal IP addresses turns out we need a shiny version of service_identity to enforce this correctly. --- synapse/crypto/context_factory.py | 13 ++++++++----- synapse/python_dependencies.py | 4 +++- 2 files changed, 11 insertions(+), 6 deletions(-) (limited to 'synapse') diff --git a/synapse/crypto/context_factory.py b/synapse/crypto/context_factory.py index 2f23782630..0639c228cb 100644 --- a/synapse/crypto/context_factory.py +++ b/synapse/crypto/context_factory.py @@ -17,7 +17,7 @@ import logging import idna from service_identity import VerificationError -from service_identity.pyopenssl import verify_hostname +from service_identity.pyopenssl import verify_hostname, verify_ip_address from zope.interface import implementer from OpenSSL import SSL, crypto @@ -156,7 +156,7 @@ class ConnectionVerifier(object): if isIPAddress(hostname) or isIPv6Address(hostname): self._hostnameBytes = hostname.encode('ascii') - self._sendSNI = False + self._is_ip_address = True else: # twisted's ClientTLSOptions falls back to the stdlib impl here if # idna is not installed, but points out that lacks support for @@ -164,17 +164,20 @@ class ConnectionVerifier(object): # # We can rely on having idna. self._hostnameBytes = idna.encode(hostname) - self._sendSNI = True + self._is_ip_address = False self._hostnameASCII = self._hostnameBytes.decode("ascii") def verify_context_info_cb(self, ssl_connection, where): - if where & SSL.SSL_CB_HANDSHAKE_START and self._sendSNI: + if where & SSL.SSL_CB_HANDSHAKE_START and not self._is_ip_address: ssl_connection.set_tlsext_host_name(self._hostnameBytes) if where & SSL.SSL_CB_HANDSHAKE_DONE and self._verify_certs: try: - verify_hostname(ssl_connection, self._hostnameASCII) + if self._is_ip_address: + verify_ip_address(ssl_connection, self._hostnameASCII) + else: + verify_hostname(ssl_connection, self._hostnameASCII) except VerificationError: f = Failure() tls_protocol = ssl_connection.get_app_data() diff --git a/synapse/python_dependencies.py b/synapse/python_dependencies.py index db09ff285f..6efd81f204 100644 --- a/synapse/python_dependencies.py +++ b/synapse/python_dependencies.py @@ -45,7 +45,9 @@ REQUIREMENTS = [ "signedjson>=1.0.0", "pynacl>=1.2.1", "idna>=2", - "service_identity>=16.0.0", + + # validating SSL certs for IP addresses requires service_identity 18.1. + "service_identity>=18.1.0", # our logcontext handling relies on the ability to cancel inlineCallbacks # (https://twistedmatrix.com/trac/ticket/4632) which landed in Twisted 18.7. -- cgit 1.4.1 From 81b8fdedf2e2504555b76ebbedfac88521d3c93f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 10 Jun 2019 17:51:11 +0100 Subject: rename gutwrenched attr --- synapse/crypto/context_factory.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'synapse') diff --git a/synapse/crypto/context_factory.py b/synapse/crypto/context_factory.py index 0639c228cb..2bc5cc3807 100644 --- a/synapse/crypto/context_factory.py +++ b/synapse/crypto/context_factory.py @@ -110,8 +110,10 @@ class ClientTLSOptionsFactory(object): tls_protocol = ssl_connection.get_app_data() try: # ... we further assume that SSLClientConnectionCreator has set the - # 'tls_verifier' attribute to a ConnectionVerifier object. - tls_protocol.tls_verifier.verify_context_info_cb(ssl_connection, where) + # '_synapse_tls_verifier' attribute to a ConnectionVerifier object. + tls_protocol._synapse_tls_verifier.verify_context_info_cb( + ssl_connection, where + ) except: # noqa: E722, taken from the twisted implementation logger.exception("Error during info_callback") f = Failure() @@ -124,6 +126,7 @@ class SSLClientConnectionCreator(object): Replaces twisted.internet.ssl.ClientTLSOptions """ + def __init__(self, hostname, ctx, verify_certs): self._ctx = ctx self._verifier = ConnectionVerifier(hostname, verify_certs) @@ -136,10 +139,10 @@ class SSLClientConnectionCreator(object): # data to our TLSMemoryBIOProtocol... connection.set_app_data(tls_protocol) - # ... and we also gut-wrench a 'tls_verifier' attribute into the + # ... and we also gut-wrench a '_synapse_tls_verifier' attribute into the # tls_protocol so that the SSL context's info callback has something to # call to do the cert verification. - setattr(tls_protocol, "tls_verifier", self._verifier) + setattr(tls_protocol, "_synapse_tls_verifier", self._verifier) return connection @@ -149,13 +152,14 @@ class ConnectionVerifier(object): This is a thing which is attached to the TLSMemoryBIOProtocol, and is called by the ssl context's info callback. """ + # This code is based on twisted.internet.ssl.ClientTLSOptions. def __init__(self, hostname, verify_certs): self._verify_certs = verify_certs if isIPAddress(hostname) or isIPv6Address(hostname): - self._hostnameBytes = hostname.encode('ascii') + self._hostnameBytes = hostname.encode("ascii") self._is_ip_address = True else: # twisted's ClientTLSOptions falls back to the stdlib impl here if -- cgit 1.4.1 From 01674479651cd12c995581911cfb58e5d5493495 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 10 Jun 2019 18:17:43 +0100 Subject: 1.0.0rc2 --- CHANGES.md | 11 +++++++++++ changelog.d/5392.bugfix | 1 - changelog.d/5415.bugfix | 1 - changelog.d/5417.bugfix | 1 - synapse/__init__.py | 2 +- 5 files changed, 12 insertions(+), 4 deletions(-) delete mode 100644 changelog.d/5392.bugfix delete mode 100644 changelog.d/5415.bugfix delete mode 100644 changelog.d/5417.bugfix (limited to 'synapse') diff --git a/CHANGES.md b/CHANGES.md index 4dea0f6319..523cdb1153 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,14 @@ +Synapse 1.0.0rc2 (2019-06-10) +============================= + +Bugfixes +-------- + +- Remove redundant warning about key server response validation. ([\#5392](https://github.com/matrix-org/synapse/issues/5392)) +- Fix bug where old keys stored in the database with a null valid until timestamp caused all verification requests for that key to fail. ([\#5415](https://github.com/matrix-org/synapse/issues/5415)) +- Fix excessive memory using with default `federation_verify_certificates: true` configuration. ([\#5417](https://github.com/matrix-org/synapse/issues/5417)) + + Synapse 1.0.0rc1 (2019-06-07) ============================= diff --git a/changelog.d/5392.bugfix b/changelog.d/5392.bugfix deleted file mode 100644 index 295a7cfce1..0000000000 --- a/changelog.d/5392.bugfix +++ /dev/null @@ -1 +0,0 @@ -Remove redundant warning about key server response validation. diff --git a/changelog.d/5415.bugfix b/changelog.d/5415.bugfix deleted file mode 100644 index 83629e193d..0000000000 --- a/changelog.d/5415.bugfix +++ /dev/null @@ -1 +0,0 @@ -Fix bug where old keys stored in the database with a null valid until timestamp caused all verification requests for that key to fail. diff --git a/changelog.d/5417.bugfix b/changelog.d/5417.bugfix deleted file mode 100644 index 54be963a4e..0000000000 --- a/changelog.d/5417.bugfix +++ /dev/null @@ -1 +0,0 @@ -Fix excessive memory using with default `federation_verify_certificates: true` configuration. \ No newline at end of file diff --git a/synapse/__init__.py b/synapse/__init__.py index 77a4cfc3a5..8dc07fe73c 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -27,4 +27,4 @@ try: except ImportError: pass -__version__ = "1.0.0rc1" +__version__ = "1.0.0rc2" -- cgit 1.4.1