From 23b08135998e932d5d600941bd42389db0628a11 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Tue, 22 Jan 2019 21:58:50 +1100 Subject: Require ECDH key exchange & remove dh_params (#4429) * remove dh_params and set better cipher string --- synapse/crypto/context_factory.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'synapse/crypto') diff --git a/synapse/crypto/context_factory.py b/synapse/crypto/context_factory.py index 02b76dfcfb..6ba3eca7b2 100644 --- a/synapse/crypto/context_factory.py +++ b/synapse/crypto/context_factory.py @@ -46,8 +46,10 @@ class ServerContextFactory(ContextFactory): if not config.no_tls: context.use_privatekey(config.tls_private_key) - context.load_tmp_dh(config.tls_dh_params_path) - context.set_cipher_list("!ADH:HIGH+kEDH:!AECDH:HIGH+kEECDH") + # https://hynek.me/articles/hardening-your-web-servers-ssl-ciphers/ + context.set_cipher_list( + "ECDH+AESGCM:ECDH+CHACHA20:ECDH+AES256:ECDH+AES128:!aNULL:!SHA1" + ) def getContext(self): return self._context -- cgit 1.5.1 From 6bfa735a69717cf24f2196f1abb801d031f6993a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 22 Jan 2019 11:04:20 +0000 Subject: Make key fetches use regular federation client (#4426) All this magic is redundant. --- changelog.d/4426.misc | 1 + synapse/crypto/keyclient.py | 149 -------------------------------------------- synapse/crypto/keyring.py | 30 +++------ 3 files changed, 8 insertions(+), 172 deletions(-) create mode 100644 changelog.d/4426.misc delete mode 100644 synapse/crypto/keyclient.py (limited to 'synapse/crypto') diff --git a/changelog.d/4426.misc b/changelog.d/4426.misc new file mode 100644 index 0000000000..cda50438e0 --- /dev/null +++ b/changelog.d/4426.misc @@ -0,0 +1 @@ +Remove redundant SynapseKeyClientProtocol magic \ No newline at end of file diff --git a/synapse/crypto/keyclient.py b/synapse/crypto/keyclient.py deleted file mode 100644 index d40e4b8591..0000000000 --- a/synapse/crypto/keyclient.py +++ /dev/null @@ -1,149 +0,0 @@ -# -*- coding: utf-8 -*- -# Copyright 2014-2016 OpenMarket Ltd -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -import logging - -from six.moves import urllib - -from canonicaljson import json - -from twisted.internet import defer, reactor -from twisted.internet.error import ConnectError -from twisted.internet.protocol import Factory -from twisted.names.error import DomainError -from twisted.web.http import HTTPClient - -from synapse.http.endpoint import matrix_federation_endpoint -from synapse.util import logcontext - -logger = logging.getLogger(__name__) - -KEY_API_V2 = "/_matrix/key/v2/server/%s" - - -@defer.inlineCallbacks -def fetch_server_key(server_name, tls_client_options_factory, key_id): - """Fetch the keys for a remote server.""" - - factory = SynapseKeyClientFactory() - factory.path = KEY_API_V2 % (urllib.parse.quote(key_id), ) - factory.host = server_name - endpoint = matrix_federation_endpoint( - reactor, server_name, tls_client_options_factory, timeout=30 - ) - - for i in range(5): - try: - with logcontext.PreserveLoggingContext(): - protocol = yield endpoint.connect(factory) - server_response, server_certificate = yield protocol.remote_key - defer.returnValue((server_response, server_certificate)) - except SynapseKeyClientError as e: - logger.warn("Error getting key for %r: %s", server_name, e) - if e.status.startswith(b"4"): - # Don't retry for 4xx responses. - raise IOError("Cannot get key for %r" % server_name) - except (ConnectError, DomainError) as e: - logger.warn("Error getting key for %r: %s", server_name, e) - except Exception: - logger.exception("Error getting key for %r", server_name) - raise IOError("Cannot get key for %r" % server_name) - - -class SynapseKeyClientError(Exception): - """The key wasn't retrieved from the remote server.""" - status = None - pass - - -class SynapseKeyClientProtocol(HTTPClient): - """Low level HTTPS client which retrieves an application/json response from - the server and extracts the X.509 certificate for the remote peer from the - SSL connection.""" - - timeout = 30 - - def __init__(self): - self.remote_key = defer.Deferred() - self.host = None - self._peer = None - - def connectionMade(self): - self._peer = self.transport.getPeer() - logger.debug("Connected to %s", self._peer) - - if not isinstance(self.path, bytes): - self.path = self.path.encode('ascii') - - if not isinstance(self.host, bytes): - self.host = self.host.encode('ascii') - - self.sendCommand(b"GET", self.path) - if self.host: - self.sendHeader(b"Host", self.host) - self.endHeaders() - self.timer = reactor.callLater( - self.timeout, - self.on_timeout - ) - - def errback(self, error): - if not self.remote_key.called: - self.remote_key.errback(error) - - def callback(self, result): - if not self.remote_key.called: - self.remote_key.callback(result) - - def handleStatus(self, version, status, message): - if status != b"200": - # logger.info("Non-200 response from %s: %s %s", - # self.transport.getHost(), status, message) - error = SynapseKeyClientError( - "Non-200 response %r from %r" % (status, self.host) - ) - error.status = status - self.errback(error) - self.transport.abortConnection() - - def handleResponse(self, response_body_bytes): - try: - json_response = json.loads(response_body_bytes) - except ValueError: - # logger.info("Invalid JSON response from %s", - # self.transport.getHost()) - self.transport.abortConnection() - return - - certificate = self.transport.getPeerCertificate() - self.callback((json_response, certificate)) - self.transport.abortConnection() - self.timer.cancel() - - def on_timeout(self): - logger.debug( - "Timeout waiting for response from %s: %s", - self.host, self._peer, - ) - self.errback(IOError("Timeout waiting for response")) - self.transport.abortConnection() - - -class SynapseKeyClientFactory(Factory): - def protocol(self): - protocol = SynapseKeyClientProtocol() - protocol.path = self.path - protocol.host = self.host - return protocol diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index 515ebbc148..3a96980bed 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -14,10 +14,11 @@ # See the License for the specific language governing permissions and # limitations under the License. -import hashlib import logging from collections import namedtuple +from six.moves import urllib + from signedjson.key import ( decode_verify_key_bytes, encode_verify_key_base64, @@ -30,13 +31,11 @@ from signedjson.sign import ( signature_ids, verify_signed_json, ) -from unpaddedbase64 import decode_base64, encode_base64 +from unpaddedbase64 import decode_base64 -from OpenSSL import crypto from twisted.internet import defer from synapse.api.errors import Codes, SynapseError -from synapse.crypto.keyclient import fetch_server_key from synapse.util import logcontext, unwrapFirstError from synapse.util.logcontext import ( LoggingContext, @@ -503,31 +502,16 @@ class Keyring(object): if requested_key_id in keys: continue - (response, tls_certificate) = yield fetch_server_key( - server_name, self.hs.tls_client_options_factory, requested_key_id + response = yield self.client.get_json( + destination=server_name, + path="/_matrix/key/v2/server/" + urllib.parse.quote(requested_key_id), + ignore_backoff=True, ) if (u"signatures" not in response or server_name not in response[u"signatures"]): raise KeyLookupError("Key response not signed by remote server") - if "tls_fingerprints" not in response: - raise KeyLookupError("Key response missing TLS fingerprints") - - certificate_bytes = crypto.dump_certificate( - crypto.FILETYPE_ASN1, tls_certificate - ) - sha256_fingerprint = hashlib.sha256(certificate_bytes).digest() - sha256_fingerprint_b64 = encode_base64(sha256_fingerprint) - - response_sha256_fingerprints = set() - for fingerprint in response[u"tls_fingerprints"]: - if u"sha256" in fingerprint: - response_sha256_fingerprints.add(fingerprint[u"sha256"]) - - if sha256_fingerprint_b64 not in response_sha256_fingerprints: - raise KeyLookupError("TLS certificate not allowed by fingerprints") - response_keys = yield self.process_v2_response( from_server=server_name, requested_ids=[requested_key_id], -- cgit 1.5.1 From 97fd29c019ae92cd3dc0635de249acfc9c892340 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 24 Jan 2019 09:34:44 +0000 Subject: Don't send IP addresses as SNI (#4452) The problem here is that we have cut-and-pasted an impl from Twisted, and then failed to maintain it. It was fixed in Twisted in https://github.com/twisted/twisted/pull/1047/files; let's do the same here. --- changelog.d/4452.bugfix | 1 + synapse/crypto/context_factory.py | 15 ++++-- .../federation/test_matrix_federation_agent.py | 63 ++++++++++++++++++++-- 3 files changed, 73 insertions(+), 6 deletions(-) create mode 100644 changelog.d/4452.bugfix (limited to 'synapse/crypto') diff --git a/changelog.d/4452.bugfix b/changelog.d/4452.bugfix new file mode 100644 index 0000000000..a715ca3788 --- /dev/null +++ b/changelog.d/4452.bugfix @@ -0,0 +1 @@ +Don't send IP addresses as SNI diff --git a/synapse/crypto/context_factory.py b/synapse/crypto/context_factory.py index 6ba3eca7b2..286ad80100 100644 --- a/synapse/crypto/context_factory.py +++ b/synapse/crypto/context_factory.py @@ -17,6 +17,7 @@ from zope.interface import implementer from OpenSSL import SSL, crypto 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 from twisted.python.failure import Failure @@ -98,8 +99,14 @@ class ClientTLSOptions(object): def __init__(self, hostname, ctx): self._ctx = ctx - self._hostname = hostname - self._hostnameBytes = _idnaBytes(hostname) + + 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) ) @@ -111,7 +118,9 @@ class ClientTLSOptions(object): return connection def _identityVerifyingInfoCallback(self, connection, where, ret): - if where & SSL.SSL_CB_HANDSHAKE_START: + # 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) diff --git a/tests/http/federation/test_matrix_federation_agent.py b/tests/http/federation/test_matrix_federation_agent.py index eb963d80fb..7a3881f558 100644 --- a/tests/http/federation/test_matrix_federation_agent.py +++ b/tests/http/federation/test_matrix_federation_agent.py @@ -46,7 +46,7 @@ class MatrixFederationAgentTests(TestCase): _srv_resolver=self.mock_resolver, ) - def _make_connection(self, client_factory): + def _make_connection(self, client_factory, expected_sni): """Builds a test server, and completes the outgoing client connection Returns: @@ -69,9 +69,17 @@ class MatrixFederationAgentTests(TestCase): # tell the server tls protocol to send its stuff back to the client, too server_tls_protocol.makeConnection(FakeTransport(client_protocol, self.reactor)) - # finally, give the reactor a pump to get the TLS juices flowing. + # give the reactor a pump to get the TLS juices flowing. self.reactor.pump((0.1,)) + # check the SNI + server_name = server_tls_protocol._tlsConnection.get_servername() + self.assertEqual( + server_name, + expected_sni, + "Expected SNI %s but got %s" % (expected_sni, server_name), + ) + # fish the test server back out of the server-side TLS protocol. return server_tls_protocol.wrappedProtocol @@ -113,7 +121,10 @@ class MatrixFederationAgentTests(TestCase): self.assertEqual(port, 8448) # make a test server, and wire up the client - http_server = self._make_connection(client_factory) + http_server = self._make_connection( + client_factory, + expected_sni=b"testserv", + ) self.assertEqual(len(http_server.requests), 1) request = http_server.requests[0] @@ -150,6 +161,52 @@ class MatrixFederationAgentTests(TestCase): json = self.successResultOf(treq.json_content(response)) self.assertEqual(json, {"a": 1}) + def test_get_ip_address(self): + """ + Test the behaviour when the server name contains an explicit IP (with no port) + """ + + # the SRV lookup will return an empty list (XXX: why do we even do an SRV lookup?) + self.mock_resolver.resolve_service.side_effect = lambda _: [] + + # then there will be a getaddrinfo on the IP + self.reactor.lookups["1.2.3.4"] = "1.2.3.4" + + test_d = self._make_get_request(b"matrix://1.2.3.4/foo/bar") + + # Nothing happened yet + self.assertNoResult(test_d) + + self.mock_resolver.resolve_service.assert_called_once() + + # Make sure treq is trying to connect + clients = self.reactor.tcpClients + self.assertEqual(len(clients), 1) + (host, port, client_factory, _timeout, _bindAddress) = clients[0] + self.assertEqual(host, '1.2.3.4') + self.assertEqual(port, 8448) + + # make a test server, and wire up the client + http_server = self._make_connection( + client_factory, + expected_sni=None, + ) + + self.assertEqual(len(http_server.requests), 1) + request = http_server.requests[0] + self.assertEqual(request.method, b'GET') + self.assertEqual(request.path, b'/foo/bar') + # XXX currently broken + # self.assertEqual( + # request.requestHeaders.getRawHeaders(b'host'), + # [b'1.2.3.4:8448'] + # ) + + # finish the request + request.finish() + self.reactor.pump((0.1,)) + self.successResultOf(test_d) + def _check_logcontext(context): current = LoggingContext.current_context() -- cgit 1.5.1