Summary: | Enable HTTP and HTTPS proxies on iOS and make it a property of the NSURLSession | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||||||||||||
Component: | WebKit2 | Assignee: | Saam Barati <saam> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, ap, beidson, bfulgham, cdumez, commit-queue, ews-watchlist, ggaren, lforschler, product-security, rniwa, simon.fraser, slewis, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 192571 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Saam Barati
2018-12-04 12:59:00 PST
Created attachment 356565 [details]
patch
Comment on attachment 356565 [details]
patch
This is expanding the use of process-global state. I'll make a better alternative.
Created attachment 356583 [details]
Patch
Try something like this. 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 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
Created attachment 356623 [details]
Patch
Comment on attachment 356623 [details]
Patch
How am I supposed to use this?
Does this break PLT?
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"? 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. (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? (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. (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. 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? 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. > 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.
(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. Why do we want to use a proxy with PLT? That would result in testing a different code path in underlying network libraries. redacted (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 I don't think we want a SOCKS proxy. We need a Proxy that interacts at the HTTP/HTTPS layer. Created attachment 356712 [details]
Patch
Created attachment 356713 [details]
Patch
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? Created attachment 356714 [details]
Patch
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? (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 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 (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 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.
Created attachment 356739 [details]
patch
I've confirmed that this works on iOS
(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 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. (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. Created attachment 356931 [details]
patch for landing
Comment on attachment 356931 [details] patch for landing Clearing flags on attachment: 356931 Committed r239023: <https://trac.webkit.org/changeset/239023> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 192571 Going to investigate what went wrong with this change 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 :-) Created attachment 357506 [details]
patch for landing
Comment on attachment 357506 [details] patch for landing Clearing flags on attachment: 357506 Committed r239322: <https://trac.webkit.org/changeset/239322> All reviewed patches have been landed. Closing bug. 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? (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. |