MITM and Chained proxy support

[MediumOne]

PR for MITM + Chained proxy support. Basic scenarios work. Raising an early PR to get feedback and review comments.

PS : This is my first PR to any project. :) Please do let me know if there's anything to be modified/changed.

[MediumOne]

Currently, this PR does not support MITM + chained proxy where the chained proxy is SSL enabled. Working on it.

[ganskef]

Hi @MediumOne, thank you for the PR. I had a little vacation last week, but I've fetched it now.

[ganskef]

Hello MediumOne,

great work. I haven't thought, this is such a little modification within
the current LittleProxy implementation. Please have a look at this Netty
4.1 feature too: netty/netty#2817 . The examples
demonstrate a chain of chained proxies and it supports https. This could
be simplify the client part of the proxy in the future.

I'm happy to see, you've implemented a test. All tests succeeds in my
environment. In my experience the inherited test framework was hard to
understand and to extend with special requirements - a lot of work.

First thoughts:

https://github.com/adamfisk/LittleProxy/pull/251/files#diff-7f535123edb811193a7050ce04e30ea0R275

I see, you've saved the original code, but please consider to simplify
this condition. It's a single boolean return with tree conditions, I think.

Your comment suggests to extract this part into a separate method with
an explaining name, eventually.

https://github.com/adamfisk/LittleProxy/pull/251/files#diff-7f535123edb811193a7050ce04e30ea0R572

A littleness, but please avoid the trailing whitespace.

https://github.com/adamfisk/LittleProxy/pull/251/files#diff-7f535123edb811193a7050ce04e30ea0R655

The formatting looks inconsistent here :-) . And another comment, please
consider a method, too.

Next, I will cherry pick it into my application (since it depends on a
modified MitmManager) and give it some more manual testing.

Great feature. Thank you again!

Regards Frank

Am 03.11.2015 um 19:59 schrieb Sivasubramaniam S:

Currently, this PR does not support MITM + chained proxy where the
chained proxy is SSL enabled. Working on it.

—
Reply to this email directly or view it on GitHub
#251 (comment).

[MediumOne]

Thanks @ganskef. Even I was surprised that it took little modification to add this feature. :-)

Please have a look at this Netty
4.1 feature too: netty/netty#2817 . The examples
demonstrate a chain of chained proxies and it supports https.

I will look into it. Can you please share a link to the netty example using this proxy handler? I am unable to find any examples using this handler.

In my experience the inherited test framework was hard to
understand and to extend with special requirements - a lot of work.

IMO, I found it easy to extend the test cases for this feature. :-) I need to modify the class level documentation of my test case. I just copy-pasted it and forgot the change it. I will make this change.

I see, you've saved the original code, but please consider to simplify
this condition. It's a single boolean return with tree conditions, I think.

Sure, I will refactor this condition. I think I also need to add a comment explaining why we return true when the currentRequest is null. I will do this.

About the whitespace and formatting, I will correct them. Didn't pay attention to them. :-)

Currently, I am working on making this MITM proxy work with a chained proxy that has SSL enabled (Client --> P1 (MITM) --> P2 (HTTPS) --> Server). I have got this scenario working. I am working on cleaning up method signatures and adding test cases.

Can I push commits to this branch or create a new PR with all the changes together?

[ganskef]

This test is a good example: https://github.com/netty/netty/blob/master/handler-proxy/src/test/java/io/netty/handler/proxy/ProxyHandlerTest.java

This PR is all yours. It's okay in only once or two branches. If you're ready to merge, tell it. We'll discuss it and it will be merged into master. But no hurry, I will be waiting for @jekh , I think.

[MediumOne]

Added support for MITM + chaining with SSL enabled proxy. Two newly added tests are failing - MitmWithBadClientAuthenticationTCPChainedProxyTest and MitmWithBadServerAuthenticationTCPChainedProxyTest. Currently ignored them. Working on it.

[bwzhang2011]

@MediumOne, any update with such issue ?

[MediumOne]

Hi @bwzhang2011,

I could not work on it in the past few days. But the implementation already completed should solve almost all of the use cases.

Can you please detail the use case you are trying to solve?

[bwzhang2011]

@MediumOne, thanks for view and following up. my use cases is similar to description of #249. from such issue, I noticed that many chose different way to implement reverse proxy and I also noticed that the MITM way as my solution is based on the chainProxy so I will also care for such with the integration and I also confused that MITM and chainedProxy could not be set together so I'd like to integrate your contribution to have a test and hope it will be merged not long in the future.

[bwzhang2011]

@MediumOne, I run the test by it failed with one of the test but not in your test code. it appeared in the
TimeoutTest.testIdleConnectionTimeout.

from the testing result, the result return with 504 timeout tag but the resource code expect 502 bad gateway result.

[dparis]

@jekh @ganskef I'm building a custom in-house proxy for my company which needs to support chaining as well as header inspection for HTTPS requests. Am I right to think that this would require MITM + chaining to be active? Got an idea when this functionality might be reviewed and/or included for distribution with LP? Any kind of time-frame would help me communicate with my team to set expectations.

As well, if I can be of any help with coding, reviewing, etc, I can get authorization to work on the problem full-time if it would help deliver this feature.

[ganskef]

@MediumOne I've cherry picked the first commit in https://github.com/ganskef/LittleProxy master. It's working with my offline proxy requirements with no problem. The upstream proxy tunnels HTTPS and LittleProxy does MITMing.

Chained proxy with MITM works for me via WWWOFFLE and GlimmerBlocker in my Linux desktop environment.

@dparis Yes, MITM is needed to inspect headers with a filter in LittleProxy. Using and testing chained proxy is not a natural requirement for me. Please consider to use https://github.com/ganskef/LittleProxy-parent to build LittleProxy with MITM and proxy chaining enabled. Report your results and issues here.

[MediumOne]

@ganskef Thanks for the testing and feedback. Any reasons why you have not picked the second commit? It adds support for MITM + Chaining with a "HTTPS" proxy.

[ganskef]

Hi @MediumOne, I wasn't sure, if it is in ready state, since you've ignored some tests. Thank you for creating a PR there, too. I'll test it, but it's not my intention, to create a separated fork of LittleProxy. Your feature should be integrated into adamfisk/LittleProxy. I'm waiting for #230, to use LittleProxy with my requirements.

[ganskef]

@jekh, @MediumOne oh sorry, I'm out of sync, 9 commits behind adamfisk:master, and I'm in trouble after merging upstream.
...
Update: I've reverted my master to upstream and committed again. I've tested on every iteration. (Fixed an Android issue.) Chained proxy and MITM is working for me. Tunneling ssl via a chained works, but I haven't tested a ssl enabled chained proxy yet.

[bwzhang2011]

@ganskef, @MediumOne, any update with such issue ? will it be merged as expected with mitm and chained proxy working together ? by the way, how about #249, we're expecting to make full use of littleproxy for its reverse use cases. hope those could be provide for next release.

[ganskef]

@jekh any concerns with this PR. It's tested and it works with my use cases without side effects. I think it's a valuably addition and I would like to merge it.

@bwzhang2011 #249 is closed, so I think it's obsolete. Please comment your issues there, if needed.

[jekh]

No concerns from me. This looks good and I'm very pleased to see this make it into LP. I've been working on a flexible MITM implementation for BMP, and this was the next thing on my list. Glad somebody beat me to it!

[ganskef]

So, I'll merge this with no more delay.