[Webkit-unassigned] [Bug 184821] New: Deal with DeprecatedGlobalSettings::allowsAnySSLCertificate and WebProcessPool::allowsAnySSLCertificateForServiceWorker

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=184821

            Bug ID: 184821
           Summary: Deal with
                    DeprecatedGlobalSettings::allowsAnySSLCertificate and
                    WebProcessPool::allowsAnySSLCertificateForServiceWorke
                    r
           Product: WebKit
           Version: WebKit Nightly Build
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: WebCore Misc.
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: mcatanzaro at igalia.com
                CC: achristensen at apple.com, cdumez at apple.com,
                    youennf at gmail.com

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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180420/4ab51eaa/attachment.html>