Bug 184821 – Deal with DeprecatedGlobalSettings::allowsAnySSLCertificate and WebProcessPool::allowsAnySSLCertificateForServiceWorker

• Component: WebCore Misc.
• Status: NEW

[Michael Catanzaro]

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?

[Alex Christensen]

I'd be ok with in-place renames.