Bug 193355 – Adopt new SPI to evaluate server certificate trust

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[youenn fablet]

Adopt new SPI to evaluate server certificate trust

[youenn fablet]

Created attachment 358919 [details]
Patch

[youenn fablet]

<rdar://problem/30641705>

[Alex Christensen]

Comment on attachment 358919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358919&action=review

> Source/WebKit/ChangeLog:9
> +        If ssuccessful, let loading proceed as usual.

successful

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:557
> +            if (!canNSURLSessionTrustEvaluate())
> +                return completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil);

On platforms that don't have the SPI we will need to keep existing behavior.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:585
> +    auto completionHandlerCopy = Block_copy(completionHandler);

Let's use BlockPtr instead of Block_copy/Block_release

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1028
> +void NetworkSessionCocoa::continueDidReceiveChallenge(const WebCore::AuthenticationChallenge& challenge, NetworkDataTaskCocoa::TaskIdentifier taskIdentifier, NetworkDataTaskCocoa* networkDataTask, WTF::CompletionHandler<void(WebKit::AuthenticationChallengeDisposition, const WebCore::Credential&)>&& completionHandler)

WTF:: is unnecessary.

[youenn fablet]

Created attachment 360386 [details]
Patch

[EWS Watchlist]

Attachment 360386 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:484:  The parameter name "canHandle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 360386 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360386&action=review

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:56
> +@interface NSURLSession ()

This should be in CFNetworkSPI.h.  We should add a HAVE_STRICT_TRUST_EVALUATE in Platform.h with proper OS version checks.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:554
> +        if (canNSURLSessionTrustEvaluate()) {

This needs an else which does the exact same thing as the current existing behavior.  That should also be used on OSes where _strictTrustEvaluate is not available.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:564
> +                _session->continueDidReceiveChallenge(challenge, taskIdentifier, networkDataTask.get(), [completionHandler = WTFMove(completionHandler), secTrust = WTFMove(secTrust)] (WebKit::AuthenticationChallengeDisposition disposition, const WebCore::Credential& credential) mutable {

This is a lot of code.  It should be put in a subroutine which takes a completion handler with a BOOL-like result (whether it was determined to be a good server or not).

[youenn fablet]

Comment on attachment 360386 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360386&action=review

>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:56
>> +@interface NSURLSession ()
> 
> This should be in CFNetworkSPI.h.  We should add a HAVE_STRICT_TRUST_EVALUATE in Platform.h with proper OS version checks.

Ok

>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:554
>> +        if (canNSURLSessionTrustEvaluate()) {
> 
> This needs an else which does the exact same thing as the current existing behavior.  That should also be used on OSes where _strictTrustEvaluate is not available.

There is no need for else since we return at the end of the if.
Behavior if strictEvaluate is not there should be preserved.

[youenn fablet]

Created attachment 360478 [details]
Patch

[EWS Watchlist]

Attachment 360478 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:484:  The parameter name "canHandle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 360478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360478&action=review

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:571
> +        if (canNSURLSessionTrustEvaluate()) {

This needs to be inside the !canHandleHTTPSServerTrustEvaluation case.  Otherwise we break API compatibility.

> LayoutTests/http/tests/ssl/certificate-validation.html:33
> +    const iframe = await with_iframe("https://localhost:8443");

I'm not sure this is a good test.  Often we will have a cached connection to localhost:8443

[youenn fablet]

Created attachment 360497 [details]
Patch

[EWS Watchlist]

Attachment 360497 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:484:  The parameter name "canHandle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 360497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360497&action=review

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:532
> +    if (trustResult == noErr || networkDataTask->state() == NetworkDataTask::State::Canceling || networkDataTask->state() == NetworkDataTask::State::Completed) {

We need to null check networkDataTask here before using it.  I'm also not sure we should check these two states.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:537
> +    RetainPtr<SecTrustRef> secTrust = challenge.protectionSpace.serverTrust;

This could be declared inside the lambda capture:
secTrust = retainPtr(challenge.protectionSpace.serverTrust)

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1044
> +

extra space

[youenn fablet]

Created attachment 360503 [details]
Patch for landing

[EWS Watchlist]

Attachment 360503 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:484:  The parameter name "canHandle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 360503 [details]
Patch for landing

Clearing flags on attachment: 360503

Committed r240689: <https://trac.webkit.org/changeset/240689>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.