Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proxy handling #2

Open
wants to merge 2 commits into
base: orfox-tb_GECKO380esr_2015050513_RELBRANCH
Choose a base branch
from

Conversation

str4d
Copy link

@str4d str4d commented Jul 18, 2015

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.

@amoghbl1
Copy link
Owner

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

@str4d
Copy link
Author

str4d commented Jul 19, 2015

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.)

@amoghbl1
Copy link
Owner

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
Copy link
Owner

Also, if you could squash all the context commits together, something like what I've done at 68551e6, that would be easier to manage.

@str4d str4d force-pushed the orfox-tb_GECKO380esr_2015050513_RELBRANCH branch from 9f36647 to 5f9c832 Compare July 19, 2015 07:05
@str4d
Copy link
Author

str4d commented Jul 19, 2015

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.

@amoghbl1
Copy link
Owner

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!

@str4d
Copy link
Author

str4d commented Jul 20, 2015

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 :)

@amoghbl1
Copy link
Owner

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

@str4d str4d force-pushed the orfox-tb_GECKO380esr_2015050513_RELBRANCH branch from 5f9c832 to 50e5a6c Compare July 20, 2015 04:25
@str4d
Copy link
Author

str4d commented Jul 20, 2015

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
Copy link
Author

str4d commented Jul 20, 2015

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).

@amoghbl1
Copy link
Owner

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 :)

@n8fr8
Copy link
Contributor

n8fr8 commented Jul 20, 2015

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

@n8fr8
Copy link
Contributor

n8fr8 commented Jul 20, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants