tor-browser icon indicating copy to clipboard operation
tor-browser copied to clipboard

Proxy handling

Open str4d opened this issue 9 years ago • 13 comments

Currently includes these changes:

  • Bundle NetCipher (with minor changes).
  • Use StrongHttpsClient instead of DefaultHttpClient.
    • I had wanted to move to using an HttpRoutePlanner but the issues with HttpClient 4.3 (lacking SOCKS4a/5 and lacking any way to overload the newer APIs until 4.4) mean that we have to stick with the deprecated API.
  • Centralize proxy settings in org.mozilla.gecko.util.ProxySettings.

Moving to SOCKS can now be done for everything that uses StrongHttpsClient, by altering ProxySettings.setProxy(). However, I am concerned that the code using URL.openConnection() (and calling ProxySettings.getProxy()) might need to be migrated to StrongHttpsClient too? I haven't yet figured out how to overload that to add SOCKS4a/5 support, but rewriting the code that uses it to use HttpClient will be a mammoth undertaking, and make it much more difficult to pull in upstream changes. So overloading URL.openConnection() would be preferable.

str4d avatar Jul 18 '15 04:07 str4d

Hey @str4d, can you just give me the changes you made to unify the proxy?? I'm working on NetCipher myself and will look into strongHttpsClient, but just the unification of proxy prefs would be nice with this PR

amoghbl1 avatar Jul 18 '15 08:07 amoghbl1

If you were working on NetCipher yourself, you shouldn't have told me to go ahead and integrate it myself. I'd much rather not waste my time doing work that you aren't going to use, or are going to duplicate.

When you say you want the changes I made to unify proxy, what exactly do you want?

  • The original changes I made, before I integrated NetCipher? These were based on HttpRoutePlanner, which I now sadly believe is not workable.
  • The current changes, after I integrated NetCipher? I can try and extract them, but are you saying you don't want the BaseResource changes which I spent 4 hours working on? (That's the impression I get from will look into strongHttpsClient.)

str4d avatar Jul 19 '15 01:07 str4d

I meant I'd review the StrongHttpsClient stuff, sorry about the NetCipher stuff though, I'm planning of doing it in a different way, that's why I'm not taking it from this PR. The other stuff looks good though, everything but adding the NetCipher lib should be good. I will be kinda basing it off your work though, so none of this work is going to get wasted! Don't worry about it.

amoghbl1 avatar Jul 19 '15 01:07 amoghbl1

Also, if you could squash all the context commits together, something like what I've done at https://github.com/amoghbl1/tor-browser/commit/68551e6c651a240a651346f374c964a74ee14169, that would be easier to manage.

amoghbl1 avatar Jul 19 '15 01:07 amoghbl1

Good to know, thanks :) I've re-work the patch series to combine the early work into two commits. The centralized proxy settings commit still uses the route planner method, which as above doesn't work for SOCKS4a/5, but this PR on its own doesn't add SOCKS support, so it's fine.

I have removed all commits after the inclusion of NetCipher because they have dependencies on StrongHttpsClient, and if I drop my NetCipher commits then Orfox won't build. If you want them here, I can push them. Or you can merge this PR (if you are happy with it) and then I can make another PR with them.

str4d avatar Jul 19 '15 07:07 str4d

Hey @str4d, there seem to be a ton of changes with the imports which aren't exactly changes, just moving them around. Could you please remove those? That way when I have to go through all this stuff for a future iteration, it'll be easier!

amoghbl1 avatar Jul 20 '15 03:07 amoghbl1

Argh, sorry! Android Studio handles import formatting automatically, which is great - unless you want to import upstream later. I carefully removed those changes from the BaseResource commit, but I forgot to remove them from the centralization commit. Doing it now :)

str4d avatar Jul 20 '15 04:07 str4d

Awesome, thanks! Also, why exactly was the passing context thing required??

amoghbl1 avatar Jul 20 '15 04:07 amoghbl1

Because StrongHttpsClient requires a Context for loading its bundled certificates. Therefore, if you want to replace the use of DefaultHttpClient in BaseResource with StrongHttpsClient, either BaseResource needs a Context (the second commit above), or you have to rip out the strong HTTPS part of StrongHttpsClient (leaving only the SOCKS part).

On a related note, I've submitted the PR guardianproject/NetCipher#28 which adds support for multiple proxies to StrongHttpsCipher. If that is accepted, then it will make the Java part of per-TLD proxy support trivial.

str4d avatar Jul 20 '15 11:07 str4d

StrongHttpsClient also needs an extra constructor if it is to be used in BaseResource. I have that patch locally, and can make a PR with it on NetCipher if desired. I also have patches for the actual migrations of BaseResource and LoadFaviconTask to StrongHttpsClient, but those aren't useful until NetCipher has been integrated (however you plan to do so).

str4d avatar Jul 20 '15 12:07 str4d

Ok, could you split these up into two PRs?? One with the context fix and one with the proxying one. I've looked at the proxying stuff and its good to go, so I'll merge that and review the strongHttpsClient stuff after that. Thanks for the work :)

amoghbl1 avatar Jul 20 '15 12:07 amoghbl1

Just jumping in here to say I'll review this PR and full thread tonight.

n8fr8 avatar Jul 20 '15 17:07 n8fr8

Quick note though is that we decided not to rewrite all the Mozilla code to be HTTPClient-based instead of URLConnection at this point just based on time and resources. Splitting the proxying between HTTP and SOCKS doesn't inherently cause more problems, as long as we've done our job, but generally the SOCKS proxy directly to Tor is more high performance, which matters mostly for the GeckoView heavy lifting.

n8fr8 avatar Jul 20 '15 17:07 n8fr8