spesmilo
Use SOCKSRandomAuth for stream isolation
This PR adds an optional feature (enabled by default if a SOCKS proxy is in use) that makes each outgoing connection to an Electrum server go over an isolated Tor circuit. This improves Sybil-resistance by preventing a single Tor exit relay from having full control over Electrum's view of the network. It may also improve anonymity (by making certain types of traffic analysis more difficult), and it may also improve performance (by avoiding bottlenecked Tor relays).
Stream isolation can be toggled in the Qt GUI's proxy dialog. I didn't attempt to add a Kivy GUI toggle for this, as I don't have an easy way to test Kivy changes.
This PR is conceptually similar to https://github.com/bitcoin/bitcoin/pull/5911 .
This PR is dependent on https://github.com/kyuupichan/aiorpcX/pull/23 ; don't merge this PR until Electrum upgrades to a version of aiorpcX that includes that PR.
Good stuff, idea looks simple but useful.
serialize_proxy and deserialize_proxy are a mess
I would rather not make them even more complex.
It's just a bad idea to try to fit all this data in a single string. Do people invoke electrum from terminal with a proxy string? That might be like the only usecase for it. The str <-> dict conversion should be killed by fire. And the delimiters are colons so e.g. note FIXME "raw IPv6 address fails here"
We should use a dict everywhere... @ecdsa ?
Should this setting really be exposed to the GUI? If we knew for a proxy whether it is a Tor proxy, could we just turn it on for Tor proxies and off otherwise?
The TorDetector class in the Qt network_dialog has logic to tell if a proxy is a Tor proxy, specifically this method:
https://github.com/spesmilo/electrum/blob/e7304ce23ec90340dc8d12905267fdbd92c2492d/electrum/gui/qt/network_dialog.py#L534-L545
So this method could be moved out of Qt to e.g. util.
proxy could be a dict everywhere, with a field signalling whether it is a Tor proxy. If the field was missing, we would use above method is_tor_port to populate it.
Or maybe proxy could be some custom class so that this populate logic could be a method on it that automatically gets called on demand. (and then we would convert between proxy dict and proxy class; but strings should go)
Do people invoke electrum from terminal with a proxy string? That might be like the only usecase for it.
I suspect that this is a somewhat common use case (especially among Tor users who are trying to be certain to avoid proxy leaks), but I'm pretty sure there are better ways to let users specify proxy settings on the command line than the current strategy of combining all the proxy data into one string. Using a separate command-line flag for each component of the proxy data would probably be okay. (I don't know if that's compatible with the idea of using a dict for everything inside the config file... thoughts?)
I definitely agree that the current encoding method is ugly and error-prone, and I'd be in favor of replacing it.
Should this setting really be exposed to the GUI? If we knew for a proxy whether it is a Tor proxy, could we just turn it on for Tor proxies and off otherwise?
I suspect that the detection method you suggest will break on some edge cases. For example, I believe SubgraphOS ships with an intermediate SOCKS proxy that sits between the application and Tor's SOCKS proxy (the intermediate proxy does stuff like TLS policy enforcement); such an intermediate proxy will probably pass through the SOCKS authentication data to Tor but it might not mimic Tor's HTTP proxy detection. This kind of breakage is especially dangerous because it would cause stream isolation to silently be disabled.
There's probably a safer approach though. If the SOCKS proxy returns an error indicating that the username/password was incorrect, then that's an indication that we're not talking to Tor (either directly or via an intermediate proxy), and we can retry the connection without using a SOCKS username/password. Unfortunately, aiorpcX doesn't have a dedicated exception type for this; it uses SOCKSFailure, which is ambiguous. Should I add a commit to my aiorpcX PR that adds a more specific exception type so that we can detect this case reliably?