mitmproxy_rs icon indicating copy to clipboard operation
mitmproxy_rs copied to clipboard

WireGuard keylog file

Open ntqbit opened this issue 11 months ago • 4 comments

Implements https://github.com/mitmproxy/mitmproxy/issues/7418.

I had to fork the upstream WireGuard library boringtun since the upstream version does not allow to extract handshake keys. The fork is here: https://github.com/ntqbit/boringtun/tree/feature/key_logger

The changes made are non-breaking, neither in fork of boringtun nor in mitmproxy_rs.

ntqbit avatar Jan 08 '25 11:01 ntqbit

Thanks! Forking boringtun over this is not worth the added maintenance cost unfortunately. Could you please try to get upstream boringtun to include your changes? I'm happy to track a git rev between releases, but not a fork.

mhils avatar Jan 08 '25 11:01 mhils

Understood. I'll create a pull request to the upstream boringtun.

ntqbit avatar Jan 08 '25 11:01 ntqbit

It appears that maintainers of boringtun aren't actively reviewing pull requests, so it's likely that my changes won't be merged in the near future.

I'd like to propose an alternative approach: to use a .patch file with the changes on top of the upstream boringtun, with rust's build.rs script taking care of seamlessly cloning the repo, applying the changes and building boringtun. The interference of the new changes with the existing code of boringtun can be minimized to reduce the risk of merge conflicts with new versions of boringtun.

What do you think about this approach?

ntqbit avatar Jan 15 '25 11:01 ntqbit

What do you think about this approach?

That actually sounds worse than tracking a fork. As much as I understand that you would like to have this shipped, I think for now the approach should be:

  1. For now, whoever wants to log WireGuard secrets, needs to build mitmproxy_rs from this PR.
  2. If https://github.com/cloudflare/boringtun/pull/427 gets merged upstream, we continue here (understanding that this may take time).
  3. If, at some point in the future, we require more changes for (still unmaintained) boringtun, we fork and incorporate this patch as well.

Sorry if this is disappointing. I agree it's a cool feature, but for now users wanting to do this need to jump through one extra hoop (install from this PR) to reduce our maintenance burden going forward.

mhils avatar Jan 15 '25 12:01 mhils