Bug 184821 - Deal with DeprecatedGlobalSettings::allowsAnySSLCertificate and WebProcessPool::allowsAnySSLCertificateForServiceWorker
Summary: Deal with DeprecatedGlobalSettings::allowsAnySSLCertificate and WebProcessPoo...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-20 09:09 PDT by Michael Catanzaro
Modified: 2018-04-24 10:08 PDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2018-04-20 09:09:26 PDT
DeprecatedGlobalSettings::allowsAnySSLCertificate is kind of nuts, because it only affects web sockets (it's only checked it SocketStreamHandleImplSoup.cpp and SocketStreamHandleImplCFNet.cpp) but sounds like a setting that affects general SSL certificate verification. Should be renamed to allowsAnySSLCertificateForWebSocketTesting. This requires a bunch of renames elsewhere, since there is a chain of confusing calls. It starts out as allowsAnySSLCertificate in WKTR, then that calls WKContextSetAllowsAnySSLCertificateForWebSocketTesting which is the ideal accurate name, then that drops the "Testing" and calls WebProcessPool::setAllowsAnySSLCertificateForWebSocket (which is still OKish), then in NetworkProcess.cpp we lose the "ForWebSocket" when setting DeprecatedGlobalSettings::setAllowsAnySSLCertificate. Would be better to use a consistent name throughout, either setAllowsAnySSLCertificateForWebSocketTesting (my preference) or setAllowsAnySSLCertificateForWebSocket. Anyway, that's simple enough, since it just requires deciding on a name.

Then there are other problems: (a) DeprecatedGlobalSettings is obviously deprecated. I guess the setting should move to the page object somehow? Does anybody know where the preferred place for this is? (b) It's only used under WebCore/platform, which is a layering violation, so the SocketStreamHandleImpl classes need some sort of policy delegate that WebCore would call to check the real setting.

Next, there's also WebProcessPool::allowsAnySSLCertificateForServiceWorker, which should probably be renamed to WebProcessPool::allowsAnySSLCertificateForServiceWorkerTesting. Additionally, I'm a bit confused that it's being checked in ServiceWorkerProcessProxy.cpp, because that is UI process. But surely certificate verification should only ever happen in the network process. Also, there's no use of SoupNetworkSession there at all, which is bad because that's where the real shouldIgnoreTLSErrors and allowSpecificHTTPSCertificateForHost settings are implemented for soup ports. (I have no idea where these checks are implemented on other ports; shame there's no generic place to do them.)

That's kind of a lot. Thoughts?
Comment 1 Alex Christensen 2018-04-24 10:08:19 PDT
I'd be ok with in-place renames.