Re: CI failures:

• No idea why tests fail on macos-latest 3.10 but pass on macos-11, looks like the former can't find a Rust toolchain, but the latter can? Weird ...
• I have never used mypy for incremental type checking in Python, I'll need help figuring out what it doesn't like here.
• Coverage: Yeah this is kind of obvious, not everything new is covered by unit tests yet.

Nice, very excited for this to land!

* there's a bit of overlap with #5556, which I'd like to get in first (@meitinger already had the pleasure of fixing merge conflicts with my stuff).  That shouldn't be a substantial obstacle though. :)

I figured as much. I still wanted to file this PR to get some initial feedback rather than waiting for more stuff to land.

* if possible I'd like to reuse the ProxyConnectionHandler instead of subclassing it. That should become more straightforward with @meitinger's PR, which changes the location of transparent mode address resolution

Ok, sounds good to me. Hope resolving those merge conflicts won't be too bad.

* The macOS failure we should tackle at its root, i.e. provide matching binary wheels. Building additional Rust code in CI takes too much time.

Yeah, I'll try to figure out how to use Github Actions to publish binary wheels to PyPI ... looks like there's something I can use here: https://github.com/messense/maturin-action

Yeah, I'll try to figure out how to use Github Actions to publish binary wheels to PyPI ... looks like there's something I can use here: https://github.com/messense/maturin-action

Cross compilation is a can of worms. Let's just spawn macos-latest and windows-latest jobs that build the additional wheels? :)

PS: I managed to build binary wheels for Windows (x64, x86), mac OS (x64, aarch64), and linux (x86_64, i686) by using a GitHub Action: https://github.com/decathorpe/mitmproxy_wireguard/actions/runs/2958218239

Once I figure out how to upload those files to PyPI, that should make things considerably easier. Are there any targets that are important for mitmproxy which are missing from this list?

Targets are good! Not sure if we even want or need Windows x86.

This job here is pretty much best practice: https://github.com/mitmproxy/pdoc/blob/021cbec1424f91caf9dbd35105a7d8a6fbda8f8c/.github/workflows/main.yml#L63

Instead of the python setup.py bits you want to download the artifacts of course. Speaking of which, I think you need different names per OS. See

Targets are good! Not sure if we even want or need Windows x86.

This job here is pretty much best practice: https://github.com/mitmproxy/pdoc/blob/021cbec1424f91caf9dbd35105a7d8a6fbda8f8c/.github/workflows/main.yml#L63

Thanks for the link, looks like exactly what I needed.

Instead of the python setup.py bits you want to download the artifacts of course. Speaking of which, I think you need different names per OS. See

mitmproxy/.github/workflows/main.yml

Line 139 in 1706a9b

- uses: actions/upload-artifact@v3

.

Looks like this works fine as-is. When downloading the artifact, I got a ZIP archive that's called "wheels" that contained all the files from the different upload-artifact actions, which I then uploaded with twine, which worked perfectly:

https://pypi.org/project/mitmproxy-wireguard/#files

Since that's now squared away, I'll try to actually rebase this PR tomorrow, so we can see whether the binary wheels work for the CI here.

Looks like this works fine as-is. When downloading the artifact, I got a ZIP archive that's called "wheels" that contained all the files from the different upload-artifact actions, which I then uploaded with twine, which worked perfectly:

Awesome! This used to not work, great to see that they apparently fixed it. 😃

I pushed a rebased version of the "add a wireguard mode spec" commit, and tried to address feedback wrt/ how to locate configuration files. Instead of specifying a "name" (which seemed to be both ambiguous to the user and not clear how to implement for us), the mode spec can optionally specify the directory from which configuration can be loaded from if it's already present, or in which configuration files will be written if they don't exist yet (or if the generate option was specified). The default directory, if it's not specified, will probably be ~/.mitmproxy, and the file name is always mitmproxy_wireguard[_peerN].conf, to simplify things.

So, for example, when using --mode wireguard:generate=./test,peers=2, configuration files would be written to

• ./test/mitmproxy_wireguard.conf
• ./test/mitmproxy_wireguard_peer0.conf
• ./test/mitmproxy_wireguard_peer1.conf

I think this should address the problem of "where to place files by default" and "how to expose this setting to the user". What do you think?

Thanks for the update! Maybe I have missed this, but have we found a definite use case for a peers argument?

If the peers argument is not specified, the default is 1. If a user later wants to connect more clients than the number of configured peers, they need to regenerate the configurations, invalidating previously generated files and encryption keys.

Keeping valid configurations for existing peers around when adding configuration for more peers would mean I need to write parsing code for WireGuard client configuration files in addition to the already existing server configuration file parser and/or make the code for configuration file handling inmitmproxy_wireguard a whole lot more complicated. Allowing the user to specify the expected number of peers in advance and overriding existing files when generating configurations for more peers looks like the simplest solution to me.

Do we need to make a distinction between load and generate, or could we just do generate-if-not-exists? I'd be happy to keep things as simple as possible for now.

Yeah, keeping it simple was my intention. Right now, there are three cases:

1. neither load nor generate are specified: Attempt to load WireGuard server configuration from default location. If no configuration file is present, generate configuration files with default settings.
2. load is specified: Attempt to load WireGuard server configuration. If no configuration file is present, fail (since "load" was specified explicitly).
3. generate is specified: Generate new WireGuard configuration files, and override files that potentially already exist (since "generate" was specified explicitly).

These three cases should cover a) a sane default (load or generate with defaults), b) an explicit "use this configuration file" case, and c) an explicit "generate new configuration files even if they already exist" case. I think this is a good balance between simplicity and UX.

Second draft of WireGuard server mode:

• mitmproxy / mitmdump / mitmweb --mode wireguard all run without errors
• binary wheels of mitmproxy_wireguard were used on all targets 🎉
• good test coverage for WireGuard mode spec
• bad test coverage for WireGuard server mode (not sure if this can even be tested in CI?)
• BUT: it doesn't seem to work yet, there doesn't seem to be any incoming data. not sure why.

PS: It looks like mitmproxy does not use the "logging" module from the python standard library? mitmproxy_wireguard is currently explicitly compatible with and reliant on Python's "logging" module (with pyo3-logging), but if that's not going to work, would it be OK to set up logging on the Rust side instead (i.e. at server initialization)?

PS: It looks like mitmproxy does not use the "logging" module from the python standard library? mitmproxy_wireguard is currently explicitly compatible with and reliant on Python's "logging" module (with pyo3-logging), but if that's not going to work, would it be OK to set up logging on the Rust side instead (i.e. at server initialization)?

Yeah, we need to fix interop here. I think the correct way is to actually deprecate our custom logging shenanigans and switch to the builtin logging module. Let's keep using pyo3-logging here.

• (not sure if this can even be tested in CI?)

This is a very good point. If you look at boringtun you find that tests are absent there as well. 🙃 I think we have two approaches:

1. YOLO
2. Write a small standalone (Rust) binary that takes client_key, server_pubkey, listen_port and then performs TCP and UDP echo roundtrips using tokio/boringtun. In the Python test suite we spawn a server and then just shell out to that.

Either is fine with me. The standalone thingy doesn't sound too bad though. :)

The custom DatagramTransport looks good! The lack of incoming data is a WireGuard handshake problem. If I hardcode the configuration:

self._wireguard_conf = wg.WireGuardServerConf.custom(51820, "EG47ZWjYjr+Y97TQ1A7sVl7Xn3mMWDnvjU/VxU769ls=", ["Test1sbpTFmJULgSlJ5hJ1RdzsXWrl3Mg7k9UTN//jE="])

I get tons of output. I haven't tracked down what the actual issue is, but I suspect we should vastly simplify configuration parsing.

Oh oh ... ok, that explains why "nothing is happening". I'll look into it.

* Do we really need to create or parse WireGuard ini files? I don't think we need to. It's perfectly fine for us to have our own custom file format which just contains two private keys as JSON:
  `~/.mitmproxy/wireguard.conf`
  ```json
  {
       "mitmproxy_key": "...",
       "client_key": "...",
  }
  ```

That would be simpler, yes ... but (cont'd below)

  We only support one peer until we find a reason later on why we would need more peers on the same port.

Supporting variable number of peers is easy, not sure it's worth ripping it out again. The default is already one peer, so unless somebody would explicitly do something different, this is already what happens.

* When `wireguard:~/.mitmproxy/wireguard.conf` is passed to mitmproxy, we do the following:
  
  1. Check if `~/.mitmproxy/wireguard.conf` exists, and create one if it doesn't.
     (mitm-wg only provides a `genkey()`  method for this)
  2. Read keys from `foo.conf`, fail if file format is invalid.

* In `WireGuardServerInstance.start`, we
  
  1. Invoke `WireGuardServerConf.custom`  with the keys we have (using mitm-wg's `pubkey(key)` to get public keys). This basically becomes our only interface - we don't even need a WireGuardServerConf object really, we can just pass keys directly to `start_server`.
  2. Log a WireGuard client configuration file to mitmproxy's event log [like we do here](https://github.com/decathorpe/mitmproxy_wireguard/blob/eb6f47fcb8b1f7489b033346ee50ca789b3b860c/echo_test_server.py#L48-L59)[1](#user-content-fn-1-151b76ec0aa8cb162ee3c47d9763abfc). This means that `mitmdump --mode wireguard` will basically print the configuration to stdout on startup.

This means we don't need to maintain the ~750 lines of conf parsing that are currently in mitm-wg. Does that make sense? Or is that too simple and I'm overlooking something?

My intention with the configuration parser / writer code was to be compatible with official / other WireGuard servers and clients, which all (?) understand the same file format. For example, it should be possible to just drop the generated client configuration file into /etc/wireguard/ and use it, or import the peer configuration file into the official WireGuard Android client.

If we don't provide a config file, the user would have to either put together a config file themselves, or copy public / private keys and settings into their WireGuard client app to manually configure the tunnel. I thought that providing a ready-to-use / importable config file would be much nicer - and since the biggest benefit of using the WireGuard mode over the transparent mode would be better UX, I thought that this would be important.

Footnotes

1. We later add `WireGuardServerInstance.client_config()` which returns the config as a string. Then we can reuse the same code for `WireGuardServerInstance.to_json()`, which adds the config so that it's available in mitmweb. [leftwards_arrow_with_hook](#user-content-fnref-1-151b76ec0aa8cb162ee3c47d9763abfc)

Yeah, extending the to_json method to show WireGuard configuration was on my TODO list.

2. Write a small standalone (Rust) binary that takes client_key, server_pubkey, listen_port and then performs TCP and UDP echo roundtrips using tokio/boringtun. In the Python test suite we spawn a server and then just shell out to that.

Either is fine with me. The standalone thingy doesn't sound too bad though. :)

Writing a standalone test client in Rust shouldn't be hard, but how would you expect to ship it / install it in CI? I don't think it can be included in the sdist or the binary wheel that are built by maturin, so it would have to be a separate "test client binary" built by Cargo, and it would then probably need to be built + installed with "cargo install" in CI (which would take ~20 minutes) which is probably not what you want.

So for now I'm leading towards option 1. YOLO ...

I found the problem that prevented handshakes in mitmproxy_wireguard (the generated config file included the wrong public key for the server).

With the latest code, I can use the official WireGuard Android VPN to connect to mitmproxy, view mitm.it successfully, download a certificate, install it, and see instagram trying to ping facebook servers with tracking data in mitmweb 😉

My intention with the configuration parser / writer code was to be compatible with official / other WireGuard servers and clients, which all (?) understand the same file format. For example, it should be possible to just drop the generated client configuration file into /etc/wireguard/ and use it, or import the peer configuration file into the official WireGuard Android client.

To be clear, I want mitmdump to print out a valid configuration file on startup which can then be pasted into /etc/wireguard/ or the client. I just don't think we want to write this to disk and maintain a full parser/serializer.

Supporting variable number of peers is easy, not sure it's worth ripping it out again. The default is already one peer, so unless somebody would explicitly do something different, this is already what happens.

Supporting more than one peer means:

1. a more complex spec format (key-value vs a single path)
2. extra UI work to nicely display a variable number of client configurations.
3. extra handling of mismatches between the specified number of peers and an existing configuration file, etc.

I don'd mind keeping support for multiple peers in the Rust bits, but outside of that it's still quite a bit of unnecessary complexity.

If we don't provide a config file, the user would have to either put together a config file themselves, or copy public / private keys and settings into their WireGuard client app to manually configure the tunnel. I thought that providing a ready-to-use / importable config file would be much nicer - and since the biggest benefit of using the WireGuard mode over the transparent mode would be better UX, I thought that this would be important.

Here's the kind of UX I'm envisioning:

$ mitmdump --mode wireguard
WireGuard server listening at *:51820. 
Client configuration:
───────────────────────────────────────────────────────────────────────────
[Interface]
PrivateKey = qG8b7LI/s+ezngWpXqj5A7Nj988hbGL+eQ8ePki0iHk=
Address = 10.0.0.1/32

[Peer]
PublicKey = mitmV5Wo7pRJrHNAKhZEI0nzqqeO8u4fXG+zUbZEXA0=
AllowedIPs = 0.0.0.0/0
Endpoint = 192.168.0.4:51820
───────────────────────────────────────────────────────────────────────────

The benefit of printing this to stdout is that the configuration is right in your face and you don't need to go search for a file/consult the docs. For other devices, the configuration file needs to adjust the local IP address of the endpoint in the configuration, so we want that to be dynamic anyways (and not just written once to a file).

Taking things one step further, it would be awesome to display a QR code in mitmweb which can then just be scanned on Android. Does that make more sense now?

Writing a standalone test client in Rust shouldn't be hard, but how would you expect to ship it / install it in CI?

Compile it once for Linux and include the compiled binary in the repo. Skip the tests on other OSes.

With the latest code, I can use the official WireGuard Android VPN to connect to mitmproxy, view mitm.it successfully, download a certificate, install it, and see instagram trying to ping facebook servers with tracking data in mitmweb 😉

🎉 🎉 🎉

I really just skimmed over the discussion, I'm very excited for WireGuard support!

Taking things one step further, it would be awesome to display a QR code in mitmweb which can then just be scanned on Android. Does that make more sense now?

Or in mitmproxy as well? 👀 https://pythonhosted.org/PyQRCode/rendering.html#terminal-rendering (was the first thing I found)

I just don't think we want to write this to disk and maintain a full parser/serializer.

Aaaaaah ... I get it now.

When writing the original implementation, my first thought was to re-use the existing / standard WireGuard configuration file format instead of reinventing the wheel. But we can use a simpler serialization format (i.e. JSON), since we don't actually need to store files on disk that are compatible with other WireGuard tools, and only need to display / print client configuration as necessary.

I'll need to think about what to rip out of the current implementation and how to implement the new approach ...

I think this is ready for review now. Configuration handling is much simpler now.

The Configuration.pretty_print() method can generate configuration file(s) for WireGuard client(s) if supplied with the appropriate "endpoint" (the public network address of the machine that's running mitmproxy), the "address" (list of IP addresses to bind to the WireGuard interface), and the "allowed_ips" (the list of IP ranges for which traffic will be sent over WireGuard; will likely be "0.0.0.0/0" in most cases).

I also tested generating QR codes from this pretty-printed configuration file (with something like qrencode -r test.conf -o out.png -s 4) and was able to imported it successfully into the official WireGuard Android client and connect to mitmproxy this way.

Looks like your two most recent commits introduced two new test failures,

They've been there before :)

Protocol and Transport are indeed different things, but we really need neither here except for sending. No particular opinions on how we solve it as long as it works. :)

Looks like your two most recent commits introduced two new test failures,

They've been there before :)

No, I'm very sure they weren't. The CI was all-green before you pushed the "simplify configuration" commits. I'm very careful not to push changes that fail tox -e py, tox -e mypy, or tox -e flake8.

Protocol and Transport are indeed different things, but we really need neither here except for sending. No particular opinions on how we solve it as long as it works. :)

🤷🏼‍♂️ Well, if you don't care, then I probably shouldn't either.

I bumped the dependency to the just-released mitmproxy_wireguard v0.1.2 which reverts the ChecksumCapability::ignored change. It was the reason why TCP connections were broken in v0.1.1.

No, I'm very sure they weren't.

Seems like I can't GitHub. Sorry. 🫣

🤷🏼‍♂️ Well, if you don't care, then I probably shouldn't either.

From mitmproxy's perspective we only really care about DatagramReader/DatagramWriter, which happen to have some internal dependencies on transport/protocol. We shouldn't try to copy all that asyncio complexity, if Reader/Writer are happy we're good. :)

I bumped the dependency to the just-released mitmproxy_wireguard v0.1.2 which reverts the ChecksumCapability::ignored change. It was the reason why TCP connections were broken in v0.1.1.

Can you elaborate on what exactly was broken without checksums? I'm surprised to see packets with in valid checksums outside of the test client.

If we need checksums we should revert 80b63bf991e26e2aedde53f3d2ea0cd99a72f801 entirely, otherwise we'll have a sneaky memory leak. Finally we need to adjust the test client to compute proper checksums, but that should be doable.

Can you elaborate on what exactly was broken without checksums?

I am not sure why, but the TCP stack just didn't establish connections at all when adding ChecksumCapabilities::ignored to the interface. I tested it with mitmweb, and it showed no TCP flows with this change. Removing it again, I see the expected flows again.

Note that the virtual network device operates on IP packets, so ignoring checksums there should only affect verification of the IP header checksum (it appears that verifying IP header checksum is the default behaviour when sending and receiving packets in smoltcp). Checking TCP packet payload checksum is independent of this, IIUC. I added the "early fail if TCP checksum mismatch" check back to mitmproxy_wireguard.

Ah. Random hypothesis: With ChecksumCapabilities::ignored, smoltcp also stops calculating correct checksums, which my client ignores but your client refuses to accept.

Checking TCP packet payload checksum is independent of this, IIUC.

I think smoltcp uses the device capabilities for all checksums, including TCP:
https://github.com/smoltcp-rs/smoltcp/blob/6b8b667be5878fbf288365df9260996116b81fa6/src/iface/interface.rs#L2620-L2625
https://github.com/smoltcp-rs/smoltcp/blob/6b8b667be5878fbf288365df9260996116b81fa6/src/wire/tcp.rs#L808-L810

I'll adjust the test client to emit correct checksums.

One unrelated question: I noticed the MTU in test_echo_server from decathorpe/mitmproxy_wireguard@09325da.
I think the 64k MTU in wg-quick comes from here: https://git.zx2c4.com/wireguard-tools/tree/src/wg-quick/linux.bash#n125
Would it work to just increase our recv/send buffer sizes from 1500 to 64k so what we support all cases? (except for the smoltcp device, which can stay at 1500). I don't have a test setup where I run into such problems.

I'll adjust the test client to emit correct checksums.

Great, thanks!

One unrelated question: I noticed the MTU in test_echo_server from decathorpe/mitmproxy_wireguard@09325da. I think the 64k MTU in wg-quick comes from here: https://git.zx2c4.com/wireguard-tools/tree/src/wg-quick/linux.bash#n125 Would it work to just increase our recv/send buffer sizes from 1500 to 64k so what we support all cases? (except for the smoltcp device, which can stay at 1500). I don't have a test setup where I run into such problems.

If I remember correctly, this happened when I ran echo_test_server.py on 0.0.0.0 and had the wireguard configuration for it connect to it via 0.0.0.0 as well. I don't speak bash, but it seems plausible that wg-quick might set the MTU to 64 KiB if it thinks it's not sending packets over a network interface at all.

The TCP buffer sizes are already 64 KiB:
https://github.com/decathorpe/mitmproxy_wireguard/blob/0.1.2/src/network.rs#L290-L293

The WireGuard and UDP packet buffer sizes are 1500 bytes:
https://github.com/decathorpe/mitmproxy_wireguard/blob/0.1.2/src/wireguard.rs#L120
https://github.com/decathorpe/mitmproxy_wireguard/blob/0.1.2/src/wireguard.rs#L130

I can increase those to 64 * 1024 as well. That should work around the problem with truncated packets when using MTU > 1420?

EDIT: I don't think that will work. The TCP handling code passes incoming packets to the virtual network device, which cannot handle receiving / sending packets larger than its MTU, and the maximum value accepted by smoltcp is only 9216 bytes (9 KiB ?) anyway:

https://docs.rs/smoltcp/latest/smoltcp/phy/struct.DeviceCapabilities.html#structfield.max_transmission_unit

So I think even for "local" connections the MTU needs to be <= 1420 (which is also recommended by the WireGuard spec, except when using PPPoE, then the maximum MTU is 1412).

Yes, could you try that and see if it fixes things? :-)

Yes, could you try that and see if it fixes things? :-)

Sorry, the edit to my previous commit was probably not the most visible place to put this:

I don't think that will work. The TCP handling code passes incoming packets to the virtual network device, which cannot handle receiving / sending packets larger than its MTU, and the maximum value accepted by smoltcp is only 9216 bytes (9 KiB ?).

So, if we start to allow packets that are larger than the MTU set in the virtual network device, they will be truncated by smoltcp. It might be possible to dynamically set MTU when initializing the virtual network device, but I don't see an obvious way to determine the "safe" MTU at runtime. So setting it to 1420 is probably the safest and easiest solution.

I also pushed a new release of mitmproxy_wireguard to GitHub (with binaries attached as an asset) for the test client built for linux-x86_64, macos-{x86_64,aarch64}, and windows-x64. These should be usable for testing the WireGuard server mode here?

https://github.com/decathorpe/mitmproxy_wireguard/releases/tag/0.1.4

I also pushed a new release of mitmproxy_wireguard to GitHub (with binaries attached as an asset) for the test client built for linux-x86_64, macos-{x86_64,aarch64}, and windows-x64. These should be usable for testing the WireGuard server mode here?

Absolutely, Honestly I don't see much change for the test binary at all, just take a random x64 Linux binary, copy it into the repo here and build a simple test with it. :)

Looks like the test almost works.

But somehow the WireGuardServerInstance._server attribute is None in the echo_udp function (link below), even though the server is running and assert inst.is_running succeeds ...

6c22471#diff-1ebfcdb52f4d132251ed110e13c2ce7bf39f0e2d9d39b38ed9f4c742d8ef070dR123

CI output:
https://github.com/mitmproxy/mitmproxy/actions/runs/3066908783/jobs/4952615131#step:7:1511

I finally managed to fix the test client (tracked down a packet that didn't have correct checksums). Not sure what your commits changed about the binaries though? Either way, pushed the latest version of the test client binaries as of 0.1.6.

Looks like the CI is finally happy, with the exception of some missed lines in test coverage?

🎉