184821 2018-04-20 09:09:26 -0700 Deal with DeprecatedGlobalSettings::allowsAnySSLCertificate and WebProcessPool::allowsAnySSLCertificateForServiceWorker 2018-04-24 10:08:19 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build PC Linux NEW https://bugs.webkit.org/show_bug.cgi?id=184804 P2 Normal --- 1 mcatanzaro webkit-unassigned achristensen cdumez youennf oldest_to_newest 1416277 0 mcatanzaro 2018-04-20 09:09:26 -0700 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? 1417179 1 achristensen 2018-04-24 10:08:19 -0700 I'd be ok with in-place renames.