Bug 193355 - Adopt new SPI to evaluate server certificate trust
Summary: Adopt new SPI to evaluate server certificate trust
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-11 11:36 PST by youenn fablet
Modified: 2019-01-29 16:16 PST (History)
10 users (show)

See Also:


Attachments
Patch (13.03 KB, patch)
2019-01-11 11:39 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (25.54 KB, patch)
2019-01-28 15:23 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (26.39 KB, patch)
2019-01-29 11:14 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (26.62 KB, patch)
2019-01-29 13:56 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (26.45 KB, patch)
2019-01-29 14:54 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-01-11 11:36:56 PST
Adopt new SPI to evaluate server certificate trust
Comment 1 youenn fablet 2019-01-11 11:39:38 PST
Created attachment 358919 [details]
Patch
Comment 2 youenn fablet 2019-01-11 11:48:05 PST
<rdar://problem/30641705>
Comment 3 Alex Christensen 2019-01-14 10:50:25 PST
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.
Comment 4 youenn fablet 2019-01-28 15:23:07 PST
Created attachment 360386 [details]
Patch
Comment 5 EWS Watchlist 2019-01-28 15:25:41 PST
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.
Comment 6 Alex Christensen 2019-01-28 17:22:33 PST
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).
Comment 7 youenn fablet 2019-01-28 18:17:40 PST
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.
Comment 8 youenn fablet 2019-01-29 11:14:20 PST
Created attachment 360478 [details]
Patch
Comment 9 EWS Watchlist 2019-01-29 11:15:54 PST
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.
Comment 10 Alex Christensen 2019-01-29 13:17:49 PST
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
Comment 11 youenn fablet 2019-01-29 13:56:30 PST
Created attachment 360497 [details]
Patch
Comment 12 EWS Watchlist 2019-01-29 13:59:32 PST
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.
Comment 13 Alex Christensen 2019-01-29 14:21:45 PST
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
Comment 14 youenn fablet 2019-01-29 14:54:26 PST
Created attachment 360503 [details]
Patch for landing
Comment 15 EWS Watchlist 2019-01-29 14:55:53 PST
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.
Comment 16 WebKit Commit Bot 2019-01-29 16:16:44 PST
Comment on attachment 360503 [details]
Patch for landing

Clearing flags on attachment: 360503

Committed r240689: <https://trac.webkit.org/changeset/240689>
Comment 17 WebKit Commit Bot 2019-01-29 16:16:46 PST
All reviewed patches have been landed.  Closing bug.