Bug 192374 - Enable HTTP and HTTPS proxies on iOS and make it a property of the NSURLSession
Summary: Enable HTTP and HTTPS proxies on iOS and make it a property of the NSURLSession
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on: 192571
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-04 12:59 PST by Saam Barati
Modified: 2018-12-18 10:24 PST (History)
14 users (show)

See Also:


Attachments
patch (7.67 KB, patch)
2018-12-04 18:09 PST, Saam Barati
no flags Details | Formatted Diff | Diff
Patch (14.88 KB, patch)
2018-12-04 22:32 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.45 MB, application/zip)
2018-12-05 00:02 PST, Build Bot
no flags Details
Patch (17.45 KB, patch)
2018-12-05 11:11 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (21.20 KB, patch)
2018-12-05 21:47 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (21.18 KB, patch)
2018-12-05 21:48 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (21.11 KB, patch)
2018-12-05 22:01 PST, Alex Christensen
sbarati: review-
Details | Formatted Diff | Diff
patch (21.43 KB, patch)
2018-12-06 12:10 PST, Saam Barati
achristensen: review+
Details | Formatted Diff | Diff
patch for landing (22.19 KB, patch)
2018-12-09 16:21 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (22.19 KB, patch)
2018-12-17 17:36 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-12-04 12:59:00 PST
...
Comment 1 Saam Barati 2018-12-04 18:09:19 PST
Created attachment 356565 [details]
patch
Comment 2 Alex Christensen 2018-12-04 22:19:33 PST
Comment on attachment 356565 [details]
patch

This is expanding the use of process-global state.  I'll make a better alternative.
Comment 3 Alex Christensen 2018-12-04 22:32:21 PST
Created attachment 356583 [details]
Patch
Comment 4 Alex Christensen 2018-12-04 22:32:44 PST
Try something like this.
Comment 5 Build Bot 2018-12-05 00:02:01 PST
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
Comment 6 Build Bot 2018-12-05 00:02:03 PST
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
Comment 7 Alex Christensen 2018-12-05 11:11:28 PST
Created attachment 356623 [details]
Patch
Comment 8 Alex Christensen 2018-12-05 11:12:03 PST
Rebased after r238900
Comment 9 Saam Barati 2018-12-05 11:19:57 PST
Comment on attachment 356623 [details]
Patch

How am I supposed to use this?

Does this break PLT?
Comment 10 Alexey Proskuryakov 2018-12-05 11:20:21 PST
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"?
Comment 11 Alex Christensen 2018-12-05 11:36:23 PST
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.
Comment 12 Saam Barati 2018-12-05 12:04:29 PST
(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?
Comment 13 Alex Christensen 2018-12-05 12:10:00 PST
(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.
Comment 14 Alex Christensen 2018-12-05 12:10:21 PST
(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.
Comment 15 Alexey Proskuryakov 2018-12-05 14:01:50 PST
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?
Comment 16 Alex Christensen 2018-12-05 14:28:25 PST
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.
Comment 17 Alexey Proskuryakov 2018-12-05 14:45:51 PST
> 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.
Comment 18 Stephanie Lewis 2018-12-05 14:59:16 PST
(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.
Comment 19 Alexey Proskuryakov 2018-12-05 15:06:43 PST
Why do we want to use a proxy with PLT? That would result in testing a different code path in underlying network libraries.
Comment 20 Stephanie Lewis 2018-12-05 16:19:44 PST
redacted
Comment 21 Radar WebKit Bug Importer 2018-12-05 17:03:38 PST
<rdar://problem/46506286>
Comment 22 Stephanie Lewis 2018-12-05 17:09:15 PST
(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
Comment 23 Saam Barati 2018-12-05 18:56:39 PST
I don't think we want a SOCKS proxy. We need a Proxy that interacts at the HTTP/HTTPS layer.
Comment 24 Alex Christensen 2018-12-05 21:47:13 PST
Created attachment 356712 [details]
Patch
Comment 25 Alex Christensen 2018-12-05 21:48:09 PST
Created attachment 356713 [details]
Patch
Comment 26 Alex Christensen 2018-12-05 21:55:51 PST
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?
Comment 27 Alex Christensen 2018-12-05 22:01:34 PST
Created attachment 356714 [details]
Patch
Comment 28 Saam Barati 2018-12-05 22:22:10 PST
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?
Comment 29 Saam Barati 2018-12-05 22:23:05 PST
(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)
Comment 30 Saam Barati 2018-12-05 22:37:18 PST
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
Comment 31 Alex Christensen 2018-12-06 08:53:54 PST
(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.
Comment 32 Saam Barati 2018-12-06 11:47:46 PST
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.
Comment 33 Saam Barati 2018-12-06 12:10:24 PST
Created attachment 356739 [details]
patch

I've confirmed that this works on iOS
Comment 34 Alex Christensen 2018-12-06 14:11:04 PST
(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/
Comment 35 Alex Christensen 2018-12-07 17:25:23 PST
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.
Comment 36 Saam Barati 2018-12-09 16:07:05 PST
(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.
Comment 37 Saam Barati 2018-12-09 16:21:55 PST
Created attachment 356931 [details]
patch for landing
Comment 38 WebKit Commit Bot 2018-12-09 17:56:24 PST
Comment on attachment 356931 [details]
patch for landing

Clearing flags on attachment: 356931

Committed r239023: <https://trac.webkit.org/changeset/239023>
Comment 39 WebKit Commit Bot 2018-12-09 17:56:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 40 WebKit Commit Bot 2018-12-10 14:56:20 PST
Re-opened since this is blocked by bug 192571
Comment 41 Saam Barati 2018-12-17 15:21:16 PST
Going to investigate what went wrong with this change
Comment 42 Saam Barati 2018-12-17 17:35:45 PST
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 :-)
Comment 43 Saam Barati 2018-12-17 17:36:45 PST
Created attachment 357506 [details]
patch for landing
Comment 44 WebKit Commit Bot 2018-12-17 22:34:05 PST
Comment on attachment 357506 [details]
patch for landing

Clearing flags on attachment: 357506

Committed r239322: <https://trac.webkit.org/changeset/239322>
Comment 45 WebKit Commit Bot 2018-12-17 22:34:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 46 Geoffrey Garen 2018-12-18 09:48:48 PST
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?
Comment 47 Saam Barati 2018-12-18 10:24:14 PST
(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.