From 63e31de14e8d0dbaf9a172198de112aadc96bd94 Mon Sep 17 00:00:00 2001 From: Maximilian Hils Date: Mon, 29 Mar 2021 13:25:21 +0200 Subject: [PATCH] reuse certificate errors to avoid unnecessary connections --- mitmproxy/connection.py | 8 ++++- mitmproxy/proxy/events.py | 2 +- mitmproxy/proxy/layers/http/__init__.py | 30 ++++++++++++------- mitmproxy/proxy/layers/tls.py | 3 +- test/mitmproxy/proxy/layers/http/test_http.py | 15 ++++++++++ test/mitmproxy/proxy/test_layer.py | 2 +- 6 files changed, 45 insertions(+), 15 deletions(-) diff --git a/mitmproxy/connection.py b/mitmproxy/connection.py index ad862cc43..c0ca423f2 100644 --- a/mitmproxy/connection.py +++ b/mitmproxy/connection.py @@ -43,7 +43,13 @@ class Connection(serializable.Serializable, metaclass=ABCMeta): sockname: Optional[Address] """Our local `(ip, port)` tuple for this connection.""" error: Optional[str] = None - """A string describing the connection error.""" + """ + A string describing a general error with connections to this address. + + The purpose of this property is to signal that new connections to the particular endpoint should not be attempted, + for example because it uses an untrusted TLS certificate. Regular (unexpected) disconnects do not set the error + property. This property is only reused per client connection. + """ tls: bool = False """ diff --git a/mitmproxy/proxy/events.py b/mitmproxy/proxy/events.py index 1b6b6ea02..a7e7bc5d4 100644 --- a/mitmproxy/proxy/events.py +++ b/mitmproxy/proxy/events.py @@ -84,7 +84,7 @@ class CommandCompleted(Event): command_reply_subclasses[command_cls] = cls def __repr__(self): - return f"Reply({repr(self.command)})" + return f"Reply({repr(self.command)},{repr(self.reply)})" command_reply_subclasses: typing.Dict[commands.Command, typing.Type[CommandCompleted]] = {} diff --git a/mitmproxy/proxy/layers/http/__init__.py b/mitmproxy/proxy/layers/http/__init__.py index 5d71e70bc..1283657d6 100644 --- a/mitmproxy/proxy/layers/http/__init__.py +++ b/mitmproxy/proxy/layers/http/__init__.py @@ -683,32 +683,40 @@ class HttpLayer(layer.Layer): # Do we already have a connection we can re-use? if reuse: for connection in self.connections: - # see "tricky multiplexing edge case" in make_http_connection for an explanation - conn_is_pending_or_h2 = ( - connection.alpn == b"h2" - or connection in self.waiting_for_establishment - ) - h2_to_h1 = self.context.client.alpn == b"h2" and not conn_is_pending_or_h2 connection_suitable = ( event.connection_spec_matches(connection) - and not h2_to_h1 ) if connection_suitable: if connection in self.waiting_for_establishment: self.waiting_for_establishment[connection].append(event) return - elif connection.connected: + elif connection.error: stream = self.command_sources.pop(event) - yield from self.event_to_child(stream, GetHttpConnectionCompleted(event, (connection, None))) + yield from self.event_to_child(stream, GetHttpConnectionCompleted(event, (None, connection.error))) return + elif connection.connected: + # see "tricky multiplexing edge case" in make_http_connection for an explanation + h2_to_h1 = self.context.client.alpn == b"h2" and connection.alpn != b"h2" + if not h2_to_h1: + stream = self.command_sources.pop(event) + yield from self.event_to_child(stream, GetHttpConnectionCompleted(event, (connection, None))) + return else: pass # the connection is at least half-closed already, we want a new one. - can_use_context_connection = ( + context_connection_matches = ( self.context.server not in self.connections and - self.context.server.connected and event.connection_spec_matches(self.context.server) ) + can_use_context_connection = ( + context_connection_matches + and self.context.server.connected + ) + if context_connection_matches and self.context.server.error: + stream = self.command_sources.pop(event) + yield from self.event_to_child(stream, GetHttpConnectionCompleted(event, (None, self.context.server.error))) + return + context = self.context.fork() stack = tunnel.LayerStack() diff --git a/mitmproxy/proxy/layers/tls.py b/mitmproxy/proxy/layers/tls.py index 0dea00820..5aa314403 100644 --- a/mitmproxy/proxy/layers/tls.py +++ b/mitmproxy/proxy/layers/tls.py @@ -193,8 +193,9 @@ class _TLSLayer(tunnel.TunnelLayer): elif last_err == ('SSL routines', 'ssl3_get_record', 'wrong version number') and data[:4].isascii(): err = f"The remote server does not speak TLS." else: # pragma: no cover - # TODO: Add test case one we find one. + # TODO: Add test case once we find one. err = f"OpenSSL {e!r}" + self.conn.error = err return False, err else: # Here we set all attributes that are only known *after* the handshake. diff --git a/test/mitmproxy/proxy/layers/http/test_http.py b/test/mitmproxy/proxy/layers/http/test_http.py index ced153632..ed9451cba 100644 --- a/test/mitmproxy/proxy/layers/http/test_http.py +++ b/test/mitmproxy/proxy/layers/http/test_http.py @@ -1002,3 +1002,18 @@ def test_dont_reuse_closed(tctx): >> DataReceived(server2, b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n") << SendData(tctx.client, b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n") ) + + +def test_reuse_error(tctx): + """Test that an errored connection is reused.""" + tctx.server.address = ("example.com", 443) + tctx.server.error = "tls verify failed" + error_html = Placeholder(bytes) + assert ( + Playbook(http.HttpLayer(tctx, HTTPMode.transparent), hooks=False) + >> DataReceived(tctx.client, b"GET / HTTP/1.1\r\n\r\n") + << SendData(tctx.client, error_html) + << CloseConnection(tctx.client) + ) + assert b"502 Bad Gateway" in error_html() + assert b"tls verify failed" in error_html() diff --git a/test/mitmproxy/proxy/test_layer.py b/test/mitmproxy/proxy/test_layer.py index c9dbbfe90..206daff77 100644 --- a/test/mitmproxy/proxy/test_layer.py +++ b/test/mitmproxy/proxy/test_layer.py @@ -52,7 +52,7 @@ class TestLayer: << commands.Log(" >! DataReceived(client, b'foo')", "debug") >> tutils.reply(None, to=-3) << commands.Log(" >> Reply(OpenConnection({'connection': Server(" - "{'id': '…rverid', 'address': None, 'state': })}))", "debug") + "{'id': '…rverid', 'address': None, 'state': })}),None)", "debug") << commands.Log(" !> DataReceived(client, b'foo')", "debug") << commands.Log("baz", "info")