Add default to disable legacy TLS versions
Created attachment 376811 [details] Patch
Comment on attachment 376811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376811&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:284 > + parameters.defaultDataStoreParameters.networkSessionParameters.minimumTLSVersion = [defaults boolForKey:@"WebKitDisableLegacyTLS"] ? Can we use an experimental feature flag for this instead? That way, it's easier for folks to toggle it when testing.
Created attachment 376817 [details] Patch
This goes with rdar://54530661 Experimental features don't go in the NetworkProcess :( Also, there's no easy way to set experimental features for all apps, but there is a way for NSUserDefaults
Comment on attachment 376817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376817&action=review > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:72 > + auto minimumTLSVersion = [defaults boolForKey:@"WebKitDisableLegacyTLS"] ? Lets not add another [defaults <fooForKey>] in here. The others need to be reworked to be API/SPI calls instead of relying on NSUserDefaults. Please do the same for yours. Otherwise this all seems fine,
(In reply to Brady Eidson from comment #5) > Comment on attachment 376817 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376817&action=review > > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:72 > > + auto minimumTLSVersion = [defaults boolForKey:@"WebKitDisableLegacyTLS"] ? > > Lets not add another [defaults <fooForKey>] in here. The others need to be > reworked to be API/SPI calls instead of relying on NSUserDefaults. Please do > the same for yours. > > Otherwise this all seems fine. So, I wrote this comment WITHOUT having look at the previous conversation. I stand by it. No NSUserDefaults please. (Doesn't have to be experimental)
Comment on attachment 376817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376817&action=review >>> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:72 >>> + auto minimumTLSVersion = [defaults boolForKey:@"WebKitDisableLegacyTLS"] ? >> >> Lets not add another [defaults <fooForKey>] in here. The others need to be reworked to be API/SPI calls instead of relying on NSUserDefaults. Please do the same for yours. >> >> Otherwise this all seems fine, > > So, I wrote this comment WITHOUT having look at the previous conversation. > > I stand by it. No NSUserDefaults please. (Doesn't have to be experimental) +1 to no NSUserDefaults. Doesn't seem like something we want apps to use, and NSUserDefaults is a free hole for people to fiddle with bits. We do have the few debug-style globally-settable preferences (like layer borders) but this seems like it doesn't fit into that category either.
Created attachment 376944 [details] Patch
Comment on attachment 376944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376944&action=review > Source/WTF/ChangeLog:3 > + Add default to disable legacy TLS versions I'll change the title to "Disable legacy TLS versions and add a temporary default to re-enable it"
Created attachment 376952 [details] Patch
We need an NSUserDefault for the corresponding internal change, and we can't use a debug WebPreference because it is needed when the NetworkSessionParameters are made, and we don't know which WKWebView to use if it were a preference on a WKWebView. The default now turns it off instead of on.
Comment on attachment 376952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376952&action=review > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:284 > + parameters.defaultDataStoreParameters.networkSessionParameters.enableLegacyTLS = [defaults boolForKey:@"WebKitEnableLegacyTLS"]; Oof > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:72 > + bool enableLegacyTLS = [defaults boolForKey:@"WebKitDisableLegacyTLS"]; Oof.
Comment on attachment 376952 [details] Patch We're doing enable, right? Make it enable everywhere.
Created attachment 377021 [details] patch for landing
Comment on attachment 377021 [details] patch for landing Clearing flags on attachment: 377021 Committed r249019: <https://trac.webkit.org/changeset/249019>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54606353>
Comment on attachment 377021 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=377021&action=review > Source/WTF/wtf/Platform.h:1614 > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000 || PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) > +#define HAVE_TLS_PROTOCOL_VERSION_T 1 > +#endif What about non-IOS IOS_FAMILY platforms?
(In reply to Darin Adler from comment #18) > Comment on attachment 377021 [details] > patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377021&action=review > > > Source/WTF/wtf/Platform.h:1614 > > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000 || PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) > > +#define HAVE_TLS_PROTOCOL_VERSION_T 1 > > +#endif > > What about non-IOS IOS_FAMILY platforms? I second this (and in fact the leftover deprecation warnings are breaking the build).
http://trac.webkit.org/r249340