Unify proxy modes, introduce UDP protocol detection. #5556

meitinger · 2022-08-26T13:22:19Z

Description

Contains changes for #5535.

Removed transparent mode in UDP. The way transparent mode for UDP is implemented is far from ideal: It's only supported on Linux, requires sudo for the whole process, prevents full coverage of udp.py and is not even used/enabled.
Together with add Windows OS proxy mode #5543, I'll plan to do re-add transparent UDP mode later in a similar fashion, i.e. a separate process, asking for sudo if necessary, setting everything up for the user. In the meantime, udp.py has now full coverage.
Updated AsyncioServerInstance and ProxyMode to support both protocols for one ProxyMode. In the process, the need for additional ServerInstances disappeared, so ProxyModes now fully describe proto, port and layer.
Removed make_connection_id. Since we don't know on what layer we'll detect what protocol, we can't use the DNS-ID in ServerManager connection-ID trick anymore - same as goes for QUIC, if we don't want to alter SendData and DataReceived (see Add QUIC support. #5435 (review)).
Changed the proxy mode syntax to what was discussed in New Proxy Modes #5535. Also DNS has now --mode reverse:dns://8.8.8.8 if used with an upstream, --mode dns will launch a local-resolve server.
Adjusted DNS layer to match packets by their ID. As mentioned above, this was previously done through ServerManager connection-IDs, but if we detect DNS higher up, or in the future use some transparent mode, this can't be done this way anymore.
Copied and adjusted TCP layer decisions for UDP.

meitinger · 2022-08-27T15:37:43Z

@mhils: Before I'm adjusting and (re-)adding tests, could you maybe have a quick look as to whether the way the "new" modes are implemented is something you'd sign off on? Thanks :)

mhils

Overall this looks great, mostly dumb clarification questions below! Please don't read any judgement/sentiment into these comments, reviewing on mobile is tricky and they might be stupid questions. 😅

mhils · 2022-08-27T04:10:47Z

mitmproxy/proxy/mode.py

@@ -0,0 +1,173 @@
+"""


What's the reason for moving this out of mode_specs?

Python and it's cyclic dependencies 😅
But I'll restore the 1:1 ServerInstances, which will move the layers-dependency to mode_servers.py again, so I can move mode.py back into mode_specs.py.

setup.cfg

mhils · 2022-08-27T16:23:54Z

mitmproxy/proxy/layers/dns.py

@@ -40,40 +39,37 @@ class DNSLayer(layer.Layer):
    Layer that handles resolving DNS queries.
    """

-    flow: dns.DNSFlow
+    flows: dict[int, dns.DNSFlow]


How/why do we end up having multiple DNS flows here now?

So the history here is: We had that before, in the DNS draft. Then we moved the ID into the connection_id of proxyserver (i.e. essentially the dict). However, if we want to do protocol detection and fancy transparent stuff, DNS might not be at the top layer, so the layer itself will have to do it.

It's the same with QUIC. We've talked about connection migration (which currently also uses connection_id in proxyserver) and the change of SendData and DataReceived to include the remote address, that ideally should not happen. So there we will also need a layer that does the routing instead, keeping the old SendData and not using custom connection_id.

And that's actually a good thing: Having uniform connection id's is the only way to reliably do inject_udp in the future.

mhils · 2022-08-27T16:48:23Z

mitmproxy/proxy/mode.py

+            raise RuntimeError("Proxy modes are frozen.")
+
+
+class AsyncioProxyMode(ProxyMode, metaclass=ABCMeta):


I feel this complicates things more than it helps. Maybe we can move description into ProxyMode itself and then keep the layer information as part of individual server instance subclasses? That avoids the special-casing in ServerInstance.make, keeps all modes as immediate subclasses of ProxyMode, retains a dumb 1-1 mapping between modes and instances, and avoids any circular spec -> layer -> spec dependencies. This may entirely be "code I wrote myself is easier to understand" bias, so feel free to go ahead with this approach if you feel it's more straightforward. Just a subjective impression. :)

I guess it's just my code-compressing attitude, but you're right. Having 1:1 ServerInstances removes all the if TYPE_CHECKING stuff, so I'll undo the changes.

mhils · 2022-08-27T23:50:53Z

Looks fantastic now! 🍰 😃

meitinger · 2022-08-28T21:55:10Z

@mhils This should be ready now :) I've updated the docs as well, that's why I didn't want to merge before you've had the chance to look over the wording. Once we support QUIC and HTTP3, I think it's time to add a table as in #5535.

mhils

Very nice! 🍰

mhils · 2022-08-29T15:23:11Z

docs/src/content/concepts-modes.md

+    In case you want to inspect raw traffic only for some hosts and HTTP for
+    others, have a look at the [tcp_hosts]({{< relref "concepts-options" >}}#tcp_hosts)
+    and [udp_hosts]({{< relref "concepts-options" >}}#udp_hosts) options.
+- Say you want to capture DNS traffic to Google's Public DNS server? Then you


Looking at this it probably would make sense to make the default port scheme-specific for reverse proxy mode. DNS should be on 53 by default. :)

You're right, here's the PR: #5563 :)

meitinger added 6 commits August 26, 2022 02:42

[modes] new unified syntax

33e4e21

[modes] full coverage udp.py

8aa9aa7

[modes] mypy and coverage

98b2329

[modes] split mode_spec into two files

45e9129

[modes] adjust DNS layer

0f81f02

[modes] add udp layer decision

a4e3ea1

mhils reviewed Aug 27, 2022

View reviewed changes

meitinger added 2 commits August 27, 2022 22:29

[modes] use 1:1 ServerInstances

8f3fe43

[modes] fix paste issue

b7c4afd

mhils mentioned this pull request Aug 27, 2022

Mitmweb: Show Proxy State #5555

Merged

meitinger added 3 commits August 28, 2022 19:12

[modes] update tests

785a40c

Merge remote-tracking branch 'upstream/main' into new-modes

4791fbd

[modes] fix typo

ead157b

meitinger marked this pull request as ready for review August 28, 2022 17:31

meitinger added 2 commits August 28, 2022 23:45

[modes] updated docs

44d37f6

[modes] fix typo

2f229f2

meitinger mentioned this pull request Aug 29, 2022

implement UDP equivalent of TcpStream decathorpe/mitmproxy_wireguard#7

Closed

meitinger requested a review from mhils August 29, 2022 23:40

mhils approved these changes Aug 30, 2022

View reviewed changes

mhils merged commit 1706a9b into mitmproxy:main Aug 30, 2022

meitinger mentioned this pull request Aug 30, 2022

Make default_port protocol specific. #5563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify proxy modes, introduce UDP protocol detection. #5556

Unify proxy modes, introduce UDP protocol detection. #5556

meitinger commented Aug 26, 2022 •

edited

meitinger commented Aug 27, 2022

mhils left a comment

mhils Aug 27, 2022

meitinger Aug 27, 2022

mhils Aug 27, 2022

meitinger Aug 27, 2022

mhils Aug 27, 2022

meitinger Aug 27, 2022

mhils commented Aug 27, 2022

meitinger commented Aug 28, 2022

mhils left a comment

mhils Aug 29, 2022

meitinger Aug 30, 2022

		raise RuntimeError("Proxy modes are frozen.")


		class AsyncioProxyMode(ProxyMode, metaclass=ABCMeta):

Unify proxy modes, introduce UDP protocol detection. #5556

Unify proxy modes, introduce UDP protocol detection. #5556

Conversation

meitinger commented Aug 26, 2022 • edited

Description

meitinger commented Aug 27, 2022

mhils left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhils commented Aug 27, 2022

meitinger commented Aug 28, 2022

mhils left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meitinger commented Aug 26, 2022 •

edited