Make `ConsoleAddon.edit_focus()` extensible by addons

bburky commented 5 hours ago

Problem Description

I implemented a custom pretty printer and edit mode for SAMLRequest and SAMLResponse parameters. This generally went well, I implemented a contentviews.View (extending ViewXmlHtml). I also implemented an editor for editing SAML XML that will re-encode the data after editing: https://github.com/bburky/mitmproxy-addons/blob/main/saml.py

Ideally I could have integrated my editor into the existing flow editor list in mitmproxy, but ConsoleAddon.edit_focus() isn't extensible: https://github.com/mitmproxy/mitmproxy/blob/v10.4.2/mitmproxy/tools/console/consoleaddons.py#L414

Instead I registered a command, which works well enough, but I had to do hacky things:

I wanted to auto register a keyboard shortcut for the addon, I did this with ConsoleMaster.keymap.add(), but that feels like a hack.
- Do you want to support addons providing built in shortcuts, or should they tell the user to edit keys.yaml?
To launch the user's editor I had to call ConsoleMaster.spawn_editor(). This isn't really part of the documented API of mitmproxy. The master object is accessible via loader.master in load(), but this still seems very hacky.
- Is there a better way to do this?
If I know in render_priority() that I can't handle a request, is there a way to indicate that?

Proposal

Make ConsoleAddon.edit_focus() extensible by addons

Provide a documented API for ConsoleMaster.spawn_editor() (and possibly support it in mitmweb too).

Maybe provide a way for addons to register keyboard shortcuts? (Relatedly, would also be nice to register commands in mitmweb that become GUI buttons. Possibly could share an API.)

Maybe suppress the View entirely if render_priority() returns a negative number?

Additional context

This is just some feedback and questions while I wrote this addon. It generally actually worked quite well, and as long as I was ok with reaching into the ConsoleMaster object, I was able to even work around the issues.

Let me know if you want any of the above questions broken out into separate issues.

I'm pretty happy with how I was able to re-implement most of the features I cared about from https://github.com/simplesamlphp/SAML-tracer in <200 lines of Python. This code is lightly tested, but I could contribute it in a PR if you'd like. I used this to exploit a misconfigured IdP that wasn't validating SAML signatures.

mhils commented 4 hours ago

Hi @bburky! Thanks for sharing, this looks really cool. This kind of feedback is super cool and very much appreciated. 😃 🍰

Make ConsoleAddon.edit_focus() extensible by addons Provide a documented API for ConsoleMaster.spawn_editor() (and possibly support it in mitmweb too).

Rephrasing this a bit: I think it would be super awesome if we could improve our contentview API so that it roundtrips. I.e. you would see a pretty-printed SAMLRequest, edit it with your favorite text editor, and then the contentview would re-encode it. Clearly won't work for all contentviews, but there are many examples where it would be super nice (Protobufs, ...).

FWIW we did have this on our Google Summer of Code Ideas List this year, but haven't had anyone work on it yet. :)

Maybe provide a way for addons to register keyboard shortcuts? (Relatedly, would also be nice to register commands in mitmweb that become GUI buttons. Possibly could share an API.)

Sounds generally useful. One practical issue here is that we'd generally want feature parity for web and console, but they don't share keyboard handling at the moment. This would be below improved contentviews on my priority list though. 😉

Maybe suppress the View entirely if render_priority() returns a negative number?

In lack of a better API, returning 0 is the best approach at the moment. I absolutely would not mind a PR to filter out negative numbers! :)

Let me know if you are interested in working on any of this - even just as design docs -, happy to mentor! 😃

bburky commented 4 hours ago

improve our contentview API so that it roundtrips

This would be awesome. However, note that "destructive" things are fine for pretty printing, but not for roundtrips. I deliberately pretty printed XML for preview, but don't do so for the editor roundtrip: it may invalidate XML signatures. (This is possibly just a boolean prety_print flag in the API that enables slightly destructive things)

Other than maybe the render_priority() thing, I'll have to dive deeper into the code to see how hard these are to solve. I'll see if anything makes sense to work on

Do you want me to PR my SAML addon? Do you want me to delete or comment out some of the hacky parts if I submit it?

mhils commented 3 hours ago

Do you want me to PR my SAML addon? Do you want me to delete or comment out some of the hacky parts if I submit it?

I think it fits perfectly into https://github.com/mitmproxy/mitmproxy/tree/main/examples/contrib (and doesn't need extra cleanup for that). 😃

mitmproxy / mitmproxy