Add Proxy support #395

oldshuren · 2016-05-30T19:17:53Z

This pull request added support to use SocketRocket behind http proxy.

The proxy setting is read from the system configuration. PAC is also supported.

If there is proxy in the system configuration. Instead open network stream to the web socket server, the network stream to the proxy server is opened. Then we send HTTP CONNECT with the web socket server address to the proxy server. After receiving 200 OK status, it operates like normal network stream.

ghost · 2016-05-30T19:24:54Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

oldshuren · 2016-05-31T15:29:14Z

Signed GLA

ghost · 2016-05-31T15:30:04Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

nlutsenko · 2016-05-31T18:25:32Z

SocketRocket/SRWebSocket.m

+}
+
+// get proxy setting from device setting
+-(void) configureProxy


Nit: Please match the overall style - aka - (void)_configureProxy

nlutsenko · 2016-05-31T18:31:33Z

@oldshuren, than you so much for figuring out the CFNetwork approach to auto-proxy configuration.
The PR looks like a really great start. I put some nitpicky notes on it, please note that all of these are applicable to the entire code change.

Also, if we could make the proxy configuration a self contained set of functions, or even a self contained class - that would be much better for code maintainability and reuse.

madlymad · 2016-06-01T08:42:28Z

I can verify that the patch works for the HTTPS connection through proxy that is set with the pac file URL in the device's WiFi settings. The corporate proxy does not support socks connections.

Tested on iPhone 4S (iOS 9.3.1 build 13E283).

oldshuren · 2016-06-01T18:17:45Z

I think we can make a class to handle the proxy configuration and
connecting to the http server. The completion block will be invoke with
either two input/output stream for success, or NSError for error.

BTW, if I make the change, do I need to submit a new pull request?

Thanks,

Dong

On 5/31/16 2:31 PM, Nikita Lutsenko wrote:

@oldshuren https://github.com/oldshuren, than you so much for
figuring out the CFNetwork approach to auto-proxy configuration.
The PR looks like a really great start. I put some nitpicky notes on
it, please note that all of these are applicable to the entire code
change.

Also, if we could make the proxy configuration a self contained set of
functions, or even a self contained class - that would be much better
for code maintainability and reuse.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#395 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHRzfHpapIOusIjjdNEe1CiDE8y1_yMeks5qHH6VgaJpZM4Ip_XE.

nlutsenko · 2016-06-01T18:39:24Z

I think we can make a class to handle the proxy configuration and
connecting to the http server. The completion block will be invoke with
either two input/output stream for success, or NSError for error.

Sounds great. Looking forward to this!

BTW, if I make the change, do I need to submit a new pull request?

You can just force push the existing remote branch with git push -f which will update the pull request here automatically.

oldshuren · 2016-06-01T20:54:58Z

Or create a class called NetSocket, which will handle proxy, open
network connection, send and receiving raw data?

Dong

On 5/31/16 2:31 PM, Nikita Lutsenko wrote:

@oldshuren https://github.com/oldshuren, than you so much for
figuring out the CFNetwork approach to auto-proxy configuration.
The PR looks like a really great start. I put some nitpicky notes on
it, please note that all of these are applicable to the entire code
change.

Also, if we could make the proxy configuration a self contained set of
functions, or even a self contained class - that would be much better
for code maintainability and reuse.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#395 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHRzfHpapIOusIjjdNEe1CiDE8y1_yMeks5qHH6VgaJpZM4Ip_XE.

nlutsenko · 2016-06-01T21:04:26Z

I would say - we are good just by using a single class for proxy setup here, I wouldn't advise a big change, like NetSocket implementation just yet, since it's slightly out of scope of the intention here (http proxy handling).

gaborvass · 2016-06-02T19:52:07Z

Hi,

Would be possible to add SOCKS proxy support as well?

Thanks,
Gabor

oldshuren · 2016-06-02T21:57:20Z

I guess it is possible, but you have to use PAC, because you can not
input SOCK directly in iOS.

I'll take a look after I finish the HTTP proxy.

Cheers,

Dong

On 6/2/16 3:52 PM, Gabor Vass wrote:

Hi,

Would be possible to add SOCKS proxy support as well?

Thanks,
Gabor

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#395 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHRzfLrwdIplbXjgSfWxR2R2nY5OV9mXks5qHzR3gaJpZM4Ip_XE.

gaborvass · 2016-06-02T22:04:15Z

you can add it on ios like this:

CFMutableDictionaryRef socksConfig = CFDictionaryCreateMutableCopy(NULL, 0, proxyDict);
CFStringRef socksProxyHost = CFDictionaryGetValue(proxyDict, @"SOCKSProxy");
CFNumberRef socksProxyPort = CFDictionaryGetValue(proxyDict, @"SOCKSPort");
if (socksProxyHost != NULL && socksProxyPort != NULL)
{
CFDictionarySetValue(socksConfig, kCFStreamPropertySOCKSProxyHost, socksProxyHost);
CFDictionarySetValue(socksConfig, kCFStreamPropertySOCKSProxyPort, socksProxyPort);
CFDictionarySetValue(socksConfig, kCFStreamPropertySOCKSVersion, kCFStreamSocketSOCKSVersion4);
CFReadStreamSetProperty(readStream, kCFStreamPropertySOCKSProxy, proxyDict);
CFWriteStreamSetProperty(writeStream, kCFStreamPropertySOCKSProxy, proxyDict);
CFRelease(socksProxyHost);
CFRelease(socksProxyPort);
}
CFRelease(socksConfig);

cheers,
Gabor

oldshuren · 2016-06-03T14:12:08Z

I pushed new update. Proxy handling was moved to ProxyConnect.{h|m}

Socks support is added. It turns out Socks support is easier that HTTP, Apple's CFNetwork already have Socks support, you just need to configuration from system settings.

oldshuren · 2016-06-03T14:19:31Z

@gaborvass I did it just like your suggestion, but on the NSStream. And the system configure only provide host, port, username and password. So I just ignore the version attribute.

I tested it with the ssh Socks tunnel, it works perfectly.

gaborvass · 2016-06-03T14:21:10Z

@oldshuren thanks a lot!

nlutsenko · 2016-06-03T18:21:42Z

SocketRocket/ProxyConnect.h

@@ -0,0 +1,6 @@
+#import <Foundation/Foundation.h>


This is missing the copyright declaration, which is required for all our source files.
Please copy it from SRError.h

nlutsenko · 2016-06-03T18:35:26Z

@oldshuren, this looks good overall, few nits here and there (there is more but I skipped them for now).

Overall notes:

Runloop schedule/unschedule is hacky, but will work just as fine.
Love the fact that we decoupled this code from the SRWebSocket itself, it's way more manageable this way!
We have a thread safety problem here, because the completion block is not guaranteed to be called on the same thread as it was scheduled, you might want to wrap the completion either in the work queue scheduling or maybe add a locking around it (the latter is outside of the scope of the current change)
Since the root folder of SocketRocket is intended for public classes only - merging this right now will make it public, which is something we would want to avoid. Can you please move the ProxyConnect into SocketRocket/Internal?

As per more nits, styling, structuring:
Would you want to go through few iterations before I merge this? Or would it be more helpful if we merge this after fixing few things and I'll take a pass on structuring/linting/adding thread safety and unifying the style here?

The minimum required changes to merge regardless of the path we take:

Adding copyrights
Renaming ProxyConnect to SRProxyConnect
Moving SRProxyConnect into SocketRocket/Internal

Let me know how I can support you here.

oldshuren · 2016-06-03T19:27:17Z

Initially I had the stream running in the default Runloop, the normal
connection worked fine, but the connection to the proxy server didn't
work. So I'm just using SRWebSocket own Runloop.

You don't need to unschedule in the proxy handling code, but it requires
more change in the main SRWebSocket.m :(

I think I'll do the minimum necessary changes and let you change the
other part. It is much easier this way.

Thanks,

Dong

On 6/3/16 2:35 PM, Nikita Lutsenko wrote:

@oldshuren https://github.com/oldshuren, this looks good overall,
few nits here and there (there is more but I skipped them for now).

Overall notes:

Runloop schedule/unschedule is hacky, but will work just as fine.

Love the fact that we decoupled this code from the |SRWebSocket|
itself, it's way more manageable this way!

We have a thread safety problem here, because the completion block
is not guaranteed to be called on the same thread as it was
scheduled, you might want to wrap the completion either in the
work queue scheduling.

Since the root folder of |SocketRocket| is intended for public
classes only - merging this right now will make it public, which
is something we would want to avoid. Can you please move the
|ProxyConnect| into |SocketRocket/Internal|?

As per more nits, styling, structuring:
Would you want to go through few iterations before I merge this? Or
would it be more helpful if we merge this after fixing few things and
I'll take a pass on structuring/linting/adding thread safety and
unifying the style here?

The minimum required changes to merge this:

Adding copyrights

Renaming |ProxyConnect| to |SRProxyConnect|

Moving |SRProxyConnect| into |SocketRocket/Internal|

Let me know how I can support you here.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#395 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHRzfJxbllxStzhJwJcw8Ls2lT3Id4Oxks5qIHP_gaJpZM4Ip_XE.

oldshuren · 2016-06-03T20:23:52Z

On 6/3/16 2:35 PM, Nikita Lutsenko wrote:

We have a thread safety problem here, because the completion block
is not guaranteed to be called on the same thread as it was
scheduled, you might want to wrap the completion either in the
work queue scheduling.

I tried to call the completion handler in the invoking thread, that is
main thread. There seems some race conditions around, it sometimes
worked, sometime didn't.

The proxy code doesn't have access to the SRWebSocket's worker queue. So
I cannot wrap it in proxy code. But in the main SRWebSocket, didConnect
is wrapped in the work queue, as

             dispatch_async(_workQueue, ^{
                     [weakSelf didConnect];
                 });

So I think it should be fine.

Dong

oldshuren · 2016-06-03T20:36:26Z

One question, does the copyrights have to say Facebook?

Thanks,

Dong

On 6/3/16 2:35 PM, Nikita Lutsenko wrote:

@oldshuren https://github.com/oldshuren, this looks good overall,
few nits here and there (there is more but I skipped them for now).

Overall notes:

Runloop schedule/unschedule is hacky, but will work just as fine.

Love the fact that we decoupled this code from the |SRWebSocket|
itself, it's way more manageable this way!

We have a thread safety problem here, because the completion block
is not guaranteed to be called on the same thread as it was
scheduled, you might want to wrap the completion either in the
work queue scheduling.

Since the root folder of |SocketRocket| is intended for public
classes only - merging this right now will make it public, which
is something we would want to avoid. Can you please move the
|ProxyConnect| into |SocketRocket/Internal|?

As per more nits, styling, structuring:
Would you want to go through few iterations before I merge this? Or
would it be more helpful if we merge this after fixing few things and
I'll take a pass on structuring/linting/adding thread safety and
unifying the style here?

The minimum required changes to merge this:

Adding copyrights

Renaming |ProxyConnect| to |SRProxyConnect|

Moving |SRProxyConnect| into |SocketRocket/Internal|

Let me know how I can support you here.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#395 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHRzfJxbllxStzhJwJcw8Ls2lT3Id4Oxks5qIHP_gaJpZM4Ip_XE.

nlutsenko · 2016-06-03T21:06:48Z

@oldshuren, yes, that's an explicit requirement.

oldshuren · 2016-06-04T15:02:22Z

Ok

Dong

On 6/3/16 5:07 PM, Nikita Lutsenko wrote:

@oldshuren https://github.com/oldshuren, yes, that's an explicit
requirement.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#395 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AHRzfBl0b_riLVn2swFaTNriq273IJE-ks5qIJd5gaJpZM4Ip_XE.

nlutsenko · 2016-06-05T22:44:25Z

Perfect, than you for implementing this.
Merging now, and am going to send another pull request that lints the change as well as cleans up some leftovers.

cgarbin · 2016-06-20T20:32:03Z

SocketRocket/SRWebSocket.m

@@ -1570,7 +1567,7 @@ - (NSOperationQueue *_Nullable)delegateOperationQueue

 @end

-//#define SR_ENABLE_LOG
+#define SR_ENABLE_LOG


Hi guys. Great work and apologies in advance for coming late to the party.

Did you mean to enable logging by default here for all builds?

Should be disabled on the current master branch.
Let me know if it's not. Shouldn't be enabled by default.

master branch is ok

(should have checked there first - sorry for the wasted time)

happy-tanuki · 2017-02-09T11:21:52Z

This is great job!
Why not the tickets #114 and #346 is closed?

* Add Proxy support * move proxy to a class, add socks support * should use weakSelf * Move ProxyConnect to Internal/Proxy/SRProxyConnect

Add Proxy support

d1fc9a7

ghost added the GH Review: review-needed label May 30, 2016

ghost added the CLA Signed label May 31, 2016

nlutsenko reviewed May 31, 2016
View reviewed changes

nlutsenko added GH Review: needs-revision and removed GH Review: review-needed labels May 31, 2016

move proxy to a class, add socks support

5fb2f87

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Jun 3, 2016

should use weakSelf

4d1baed

nlutsenko reviewed Jun 3, 2016
View reviewed changes

nlutsenko added GH Review: needs-revision and removed GH Review: review-needed labels Jun 3, 2016

nlutsenko removed their assignment Jun 3, 2016

Move ProxyConnect to Internal/Proxy/SRProxyConnect

a901963

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jun 4, 2016

nlutsenko self-assigned this Jun 5, 2016

nlutsenko added GH Review: accepted and removed GH Review: review-needed labels Jun 5, 2016

nlutsenko merged commit d8a3a39 into facebookincubator:master Jun 5, 2016

nlutsenko mentioned this pull request Jun 8, 2016

It cannot connect with SSL certification since last update. #402

Closed

cgarbin reviewed Jun 20, 2016
View reviewed changes

nlutsenko added this to the 0.6.0 milestone Oct 31, 2016

madlymad pushed a commit to madlymad/SocketRocket that referenced this pull request Jul 6, 2017

Add Proxy support (facebookincubator#395)

2109c1f

* Add Proxy support * move proxy to a class, add socks support * should use weakSelf * Move ProxyConnect to Internal/Proxy/SRProxyConnect

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Proxy support #395

Add Proxy support #395

oldshuren commented May 30, 2016

ghost commented May 30, 2016

oldshuren commented May 31, 2016

ghost commented May 31, 2016

nlutsenko May 31, 2016

nlutsenko commented May 31, 2016

madlymad commented Jun 1, 2016

oldshuren commented Jun 1, 2016

nlutsenko commented Jun 1, 2016

oldshuren commented Jun 1, 2016

nlutsenko commented Jun 1, 2016

gaborvass commented Jun 2, 2016

oldshuren commented Jun 2, 2016

gaborvass commented Jun 2, 2016

oldshuren commented Jun 3, 2016

oldshuren commented Jun 3, 2016

gaborvass commented Jun 3, 2016

nlutsenko Jun 3, 2016

nlutsenko commented Jun 3, 2016 •

edited

oldshuren commented Jun 3, 2016

oldshuren commented Jun 3, 2016

oldshuren commented Jun 3, 2016

nlutsenko commented Jun 3, 2016

oldshuren commented Jun 4, 2016

nlutsenko commented Jun 5, 2016

cgarbin Jun 20, 2016

nlutsenko Jun 20, 2016

cgarbin Jun 20, 2016

happy-tanuki commented Feb 9, 2017 •

edited

Add Proxy support #395

Add Proxy support #395

Conversation

oldshuren commented May 30, 2016

ghost commented May 30, 2016

oldshuren commented May 31, 2016

ghost commented May 31, 2016

nlutsenko May 31, 2016

Choose a reason for hiding this comment

nlutsenko commented May 31, 2016

madlymad commented Jun 1, 2016

oldshuren commented Jun 1, 2016

nlutsenko commented Jun 1, 2016

oldshuren commented Jun 1, 2016

nlutsenko commented Jun 1, 2016

gaborvass commented Jun 2, 2016

oldshuren commented Jun 2, 2016

gaborvass commented Jun 2, 2016

oldshuren commented Jun 3, 2016

oldshuren commented Jun 3, 2016

gaborvass commented Jun 3, 2016

nlutsenko Jun 3, 2016

Choose a reason for hiding this comment

nlutsenko commented Jun 3, 2016 • edited

oldshuren commented Jun 3, 2016

oldshuren commented Jun 3, 2016

oldshuren commented Jun 3, 2016

nlutsenko commented Jun 3, 2016

oldshuren commented Jun 4, 2016

nlutsenko commented Jun 5, 2016

cgarbin Jun 20, 2016

Choose a reason for hiding this comment

nlutsenko Jun 20, 2016

Choose a reason for hiding this comment

cgarbin Jun 20, 2016

Choose a reason for hiding this comment

happy-tanuki commented Feb 9, 2017 • edited

nlutsenko commented Jun 3, 2016 •

edited

happy-tanuki commented Feb 9, 2017 •

edited