Bug 192374 – Enable HTTP and HTTPS proxies on iOS and make it a property of the NSURLSession

• Component: WebKit2
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 356565 [details]
patch

[Alex Christensen]

Comment on attachment 356565 [details]
patch

This is expanding the use of process-global state.  I'll make a better alternative.

[Alex Christensen]

Created attachment 356583 [details]
Patch

[Alex Christensen]

Try something like this.

[EWS Watchlist]

Comment on attachment 356583 [details]
Patch

Attachment 356583 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10274980

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-video-only-dataavailable.html

[EWS Watchlist]

Created attachment 356586 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Alex Christensen]

Created attachment 356623 [details]
Patch

[Alex Christensen]

Rebased after r238900

[Saam Barati]

Comment on attachment 356623 [details]
Patch

How am I supposed to use this?

Does this break PLT?

[Alexey Proskuryakov]

Comment on attachment 356623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356623&action=review

> Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.mm:79
> +- (void)setSocksProxy:(NSURL *)proxy

Please capitalize SOCKS. Also, is it SOCKS5 specifically?

What is the difference between this SPI and existing ways to use SOCKS proxy, as suggested by a web search for "iOS SOCKS proxy"?

[Alex Christensen]

This patch probably breaks PLT.  We should probably still check in NSUserDefaults if the value hasn't been set to ease the transition to using this SPI.

This SPI is different because it affects the NetworkProcess of WKWebViews using the affected WKWebsiteDataStore.  I'm not sure which SOCKS version CFNetwork uses.  Someone will need to verify that this works and fix the NSUserDefaults issue, but this is closer to the direction we should be heading in than the initial patch which just expands the use of process-global state, which we are trying to reduce and eliminate.

[Saam Barati]

(In reply to Alex Christensen from comment #11)
> This patch probably breaks PLT.  We should probably still check in
> NSUserDefaults if the value hasn't been set to ease the transition to using
> this SPI.
> 
> This SPI is different because it affects the NetworkProcess of WKWebViews
> using the affected WKWebsiteDataStore.  I'm not sure which SOCKS version
> CFNetwork uses.  Someone will need to verify that this works and fix the
> NSUserDefaults issue, but this is closer to the direction we should be
> heading in than the initial patch which just expands the use of
> process-global state, which we are trying to reduce and eliminate.

"Someone will need to verify that this works": what's the "this"? PLT? Your SPI?

Does your SPI allow multiple proxies in the same network process?

[Alex Christensen]

(In reply to Saam Barati from comment #12)
> "Someone will need to verify that this works": what's the "this"? PLT? Your
> SPI?
Both.

> Does your SPI allow multiple proxies in the same network process?
No, but neither did the old code to my knowledge.

[Alex Christensen]

(In reply to Alex Christensen from comment #13)
> (In reply to Saam Barati from comment #12)
> > Does your SPI allow multiple proxies in the same network process?
> No, but neither did the old code to my knowledge.
Actually, yes, but in different NSURLSessions.

[Alexey Proskuryakov]

Still not sure what the purpose of this patch is. If we just want to use a proxy in some automation scenario, it sounds like that doesn't require WebKit changes, or does it?

[Alex Christensen]

It requires WebKit changes to get it to work the same way on iOS as on Mac.  It also requires WebKit changes to progress towards a separate goal of having no process-global state in the NetworkProcess.

[Alexey Proskuryakov]

> It requires WebKit changes to get it to work the same way on iOS as on Mac.

I'm not sure what "it" is in this context, so can't tell if it's appropriate to try making "it" work the same way.

[Stephanie Lewis]

(In reply to Alexey Proskuryakov from comment #15)
> Still not sure what the purpose of this patch is. If we just want to use a
> proxy in some automation scenario, it sounds like that doesn't require
> WebKit changes, or does it?

This enables the safari specific proxy we use on Mac for iOS.  This is good for PLT because:
a) it allows to us to enable/disable a proxy via defaults instead having to store/restore network state using system tools
b) it means that all other traffic (i.e. any calls to iCloud by other services) do not get proxied and therefore fail.

[Alexey Proskuryakov]

Why do we want to use a proxy with PLT? That would result in testing a different code path in underlying network libraries.

[Stephanie Lewis]

redacted

[Radar WebKit Bug Importer]

<rdar://problem/46506286>

[Stephanie Lewis]

(In reply to Alexey Proskuryakov from comment #19)
> Why do we want to use a proxy with PLT? That would result in testing a
> different code path in underlying network libraries.

some background: there are 2 ways to redirect to our proxy (actually there are more but these are the interesting ones)
a) /etc/hosts 
pros: doesn't block all traffic (but blocks more than safari proxy), isn't a completely different codepath
cons: complicated setup, source of failures for user, can leave your device in a bad state
b) safari proxy
pros: doesn't block all traffic, easy to set up/tear down, invisible to users
cons: proxy code path is different in CFNetwork

For Mac /etc/hosts created many many user issues and user convenience was more important than taking a slightly different code path.  
No one used their carry phone for plt testing on iOS so the trade-off was different

[Saam Barati]

I don't think we want a SOCKS proxy. We need a Proxy that interacts at the HTTP/HTTPS layer.

[Alex Christensen]

Created attachment 356712 [details]
Patch

[Alex Christensen]

Created attachment 356713 [details]
Patch

[Alex Christensen]

Comment on attachment 356713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356713&action=review

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:607
> +static NSDictionary *proxyDictionary(const URL& httpProxy, const URL& httpsProxy)

I'm not completely sure I got the layout of this dictionary right.  Saam or Stephanie, could you test this patch and make sure it works as desired?

[Alex Christensen]

Created attachment 356714 [details]
Patch

[Saam Barati]

Comment on attachment 356714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356714&action=review

We need this to work over HTTPS on iOS

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:621
> +        [dictionary setObject:@1 forKey:(NSString *)kCFNetworkProxiesHTTPSEnable];

Is this actually needed?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:622
> +        [dictionary setObject:httpsProxy.host().toString() forKey:(NSString *)kCFNetworkProxiesHTTPSProxy];

You should use the dictionary keys that I used in my original patch so this can work both on iOS and macOS.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:79
> +        httpsProxy = URL(URL(), [defaults stringForKey:WebKit2HTTPSProxyDefaultsKey]);

Does this URL constructor work with nullptr as the NSString?

[Saam Barati]

(In reply to Alex Christensen from comment #26)
> Comment on attachment 356713 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356713&action=review
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:607
> > +static NSDictionary *proxyDictionary(const URL& httpProxy, const URL& httpsProxy)
> 
> I'm not completely sure I got the layout of this dictionary right.  Saam or
> Stephanie, could you test this patch and make sure it works as desired?

Just saw this. I will test this (but will probably not happen until tomorrow)

[Saam Barati]

Comment on attachment 356714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356714&action=review

>> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:79
>> +        httpsProxy = URL(URL(), [defaults stringForKey:WebKit2HTTPSProxyDefaultsKey]);
> 
> Does this URL constructor work with nullptr as the NSString?

Just read through and this is fine since this is really calling into String's ctor

[Alex Christensen]

(In reply to Saam Barati from comment #28)
> Comment on attachment 356714 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356714&action=review
> 
> We need this to work over HTTPS on iOS
I've read that if you set the http proxy the https proxy will automatically be set on iOS. I haven't tested it, though.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:621
> > +        [dictionary setObject:@1 forKey:(NSString *)kCFNetworkProxiesHTTPSEnable];
> 
> Is this actually needed?
I don't know.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:622
> > +        [dictionary setObject:httpsProxy.host().toString() forKey:(NSString *)kCFNetworkProxiesHTTPSProxy];
> 
> You should use the dictionary keys that I used in my original patch so this
> can work both on iOS and macOS.
The dictionary keys are unavailable on iOS.

[Saam Barati]

Comment on attachment 356714 [details]
Patch

I'm going to take over making this work. There needs to be some changes to get it to do what we want.

[Saam Barati]

Created attachment 356739 [details]
patch

I've confirmed that this works on iOS

[Alex Christensen]

(In reply to Saam Barati from comment #32)
> Comment on attachment 356714 [details]
> Patch
> 
> I'm going to take over making this work. There needs to be some changes to
> get it to do what we want.

Thanks!  I'm not surprised that some tweaks are necessary.

(In reply to Saam Barati from comment #33)
> Created attachment 356739 [details]
> patch
> 
> I've confirmed that this works on iOS

\o/

[Alex Christensen]

Comment on attachment 356739 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356739&action=review

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:259
> +    parameters.httpProxy = [defaults stringForKey:(NSString *)WebKit2HTTPProxyDefaultsKey];

Let's add a bundle check for this to make sure it's only used in Safari.  Everywhere this default is read.

[Saam Barati]

(In reply to Alex Christensen from comment #35)
> Comment on attachment 356739 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356739&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:259
> > +    parameters.httpProxy = [defaults stringForKey:(NSString *)WebKit2HTTPProxyDefaultsKey];
> 
> Let's add a bundle check for this to make sure it's only used in Safari. 
> Everywhere this default is read.

Will do.

[Saam Barati]

Created attachment 356931 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 356931 [details]
patch for landing

Clearing flags on attachment: 356931

Committed r239023: <https://trac.webkit.org/changeset/239023>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 192571

[Saam Barati]

Going to investigate what went wrong with this change

[Saam Barati]

Comment on attachment 356931 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=356931&action=review

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:621
> +            [dictionary setObject:@(*port) forKey:(NSString *)kCFStreamPropertyHTTPProxyHost];

Surprisingly, specifying the port as a host broke things :-)

[Saam Barati]

Created attachment 357506 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 357506 [details]
patch for landing

Clearing flags on attachment: 357506

Committed r239322: <https://trac.webkit.org/changeset/239322>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 357506 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=357506&action=review

> Source/WebKit/ChangeLog:13
> +        $ defaults write -g WebKit2HTTPProxy -string "http://localhost:8080"
> +        $ defaults write -g WebKit2HTTPSProxy -string "http://localhost:8080"
> +        

What are the limits on who / how these defaults can be set? Can any process set these defaults? For the whole OS?

[Saam Barati]

(In reply to Geoffrey Garen from comment #46)
> Comment on attachment 357506 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357506&action=review
> 
> > Source/WebKit/ChangeLog:13
> > +        $ defaults write -g WebKit2HTTPProxy -string "http://localhost:8080"
> > +        $ defaults write -g WebKit2HTTPSProxy -string "http://localhost:8080"
> > +        
> 
> What are the limits on who / how these defaults can be set? Can any process
> set these defaults? For the whole OS?

On Mac, it's the same as it's always been, with the new additional restriction that we only read these values if we're Safari. If you can write to these defaults in such a way that we can read them, you can launch Safari with a Proxy.

On iOS, no app will be able to write to these defaults *and* impersonate Safari. However, some system processes almost certainly can write to these defaults.