Support manually accepting invalid SSL certificates with NetworkSession
Created attachment 273986 [details] Patch
Can an API test be written for this?
(In reply to comment #2) > Can an API test be written for this? Not easily. The easiest way I can think of would be to use https://bugs.webkit.org/show_bug.cgi?id=148719 and do all the steps of an ssl handshake manually. That would be fun, but probably not a good use of time.
The test I have been doing manually is going to badssl.com and clicking on something.
Comment on attachment 273986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273986&action=review > Source/WebKit2/ChangeLog:5 > + rdar://problem/24847398 This should be wrapped in <> > Source/WebKit2/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). Can you add your manual test step here, so others can try it? > Source/WebKit2/NetworkProcess/NetworkLoad.cpp:350 > + completionHandler(AuthenticationChallengeDisposition::RejectProtectionSpace, { }); Why change from the explicit constructor syntax to an implicit credential constructor?
Comment on attachment 273986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273986&action=review >> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:350 >> + completionHandler(AuthenticationChallengeDisposition::RejectProtectionSpace, { }); > > Why change from the explicit constructor syntax to an implicit credential constructor? To be consistent. The only AuthenticationChallengeDisposition that actually uses the credential is UseCredential. The others are ignored. See https://developer.apple.com/library/ios/documentation/Foundation/Reference/NSURLSessionDelegate_protocol/index.html#//apple_ref/doc/c_ref/NSURLSessionAuthChallengeDisposition
Created attachment 274005 [details] Patch
I manually verified this fixes the behavior on iOS with NetworkSession, too.
Comment on attachment 274005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274005&action=review > Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:354 > + CFIndex length1 = CFDataGetLength(data1.get()); > + CFIndex length2 = CFDataGetLength(data2.get()); > + if (length1 != length2) > + return false; > + if (memcmp(CFDataGetBytePtr(data1.get()), CFDataGetBytePtr(data2.get()), length1)) > + return false; Probably better to use CFEqual here: if (!CFEqual(data1.get(), data2.get())) return false; Wish there was a CF equivalent to isEqualToData: but there does not seem to be. > Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:362 > + if (NSArray *certArray = [NSURLRequest allowsSpecificHTTPSCertificateForHost:host]) { Why not call this certificateArray or certificates? Also, I suggest early return instead of nesting the entire function inside an if statement. > Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:363 > + RetainPtr<CFStringRef> cfHost = host.createCFString(); I wouldn’t bother putting this into a local variable. Instead I would use a local variable, or helper function, for the super-long boolean expression: bool requireServerCertificates = challenge.protectionSpace().authenticationScheme() == WebCore::ProtectionSpaceAuthenticationScheme::ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested; auto policy = adoptCF(SecPolicyCreateSSL(requireServerCertificates, host.createCFString().get())); or: auto policy = adoptCF(SecPolicyCreateSSL(requireServerCertificates(challenge), host.createCFString().get())); > Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:366 > + SecTrustRef trustRef = nil; Should be nullptr. It’s right to use nil for Objective-C types, but not for CF types.
Thanks for the r+, Darin. I'm almost done with a new patch with a unit test.
(In reply to comment #9) > > Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:354 > > + CFIndex length1 = CFDataGetLength(data1.get()); > > + CFIndex length2 = CFDataGetLength(data2.get()); > > + if (length1 != length2) > > + return false; > > + if (memcmp(CFDataGetBytePtr(data1.get()), CFDataGetBytePtr(data2.get()), length1)) > > + return false; > > Probably better to use CFEqual here: > > if (!CFEqual(data1.get(), data2.get())) > return false; > > Wish there was a CF equivalent to isEqualToData: but there does not seem to > be. I call CFEqual on the SecCertificateRefs in the next patch. Much cleaner.
Created attachment 274313 [details] Patch
Landed in http://trac.webkit.org/changeset/198347 I have a unit test on my computer I plan to land soon separately.