Bug 191447 - Web Inspector: Network: show secure certificate details per-request
Summary: Web Inspector: Network: show secure certificate details per-request
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 191497 191458 191498 191539 191597
  Show dependency treegraph
 
Reported: 2018-11-08 17:11 PST by Devin Rousso
Modified: 2018-11-13 13:20 PST (History)
8 users (show)

See Also:


Attachments
[Patch] WIP (115.88 KB, patch)
2018-11-08 17:31 PST, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
[Image] After Patch is applied (830.21 KB, image/png)
2018-11-08 17:32 PST, Devin Rousso
no flags Details
[Patch] WIP (129.53 KB, patch)
2018-11-08 22:45 PST, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (134.04 KB, patch)
2018-11-08 23:57 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (84.80 KB, patch)
2018-11-09 11:34 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (88.21 KB, patch)
2018-11-09 16:41 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (88.21 KB, patch)
2018-11-09 17:04 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.92 MB, application/zip)
2018-11-09 18:09 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.59 MB, application/zip)
2018-11-09 19:10 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.25 MB, application/zip)
2018-11-09 19:44 PST, Build Bot
no flags Details
Patch (102.71 KB, patch)
2018-11-09 20:52 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (102.78 KB, patch)
2018-11-09 21:16 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (102.77 KB, patch)
2018-11-09 21:35 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (101.86 KB, patch)
2018-11-09 22:53 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (102.34 KB, patch)
2018-11-09 23:03 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (Insecure) (654.35 KB, image/png)
2018-11-09 23:03 PST, Devin Rousso
no flags Details
Patch (117.22 KB, patch)
2018-11-12 18:47 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (117.27 KB, patch)
2018-11-12 20:07 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.48 MB, application/zip)
2018-11-12 21:38 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-11-08 17:11:34 PST
.
Comment 1 Devin Rousso 2018-11-08 17:12:11 PST
<rdar://problem/30019476>
Comment 2 Devin Rousso 2018-11-08 17:31:49 PST
Created attachment 354296 [details]
[Patch] WIP

Testing to see how this builds on various platforms.  Will add tests after EWS.
Comment 3 Devin Rousso 2018-11-08 17:32:40 PST
Created attachment 354297 [details]
[Image] After Patch is applied
Comment 4 Build Bot 2018-11-08 17:33:56 PST Comment hidden (obsolete)
Comment 5 Devin Rousso 2018-11-08 22:45:06 PST
Created attachment 354317 [details]
[Patch] WIP
Comment 6 Build Bot 2018-11-08 22:48:16 PST Comment hidden (obsolete)
Comment 7 Devin Rousso 2018-11-08 23:57:08 PST
Created attachment 354318 [details]
[Patch] WIP
Comment 8 Devin Rousso 2018-11-09 11:34:49 PST
Created attachment 354360 [details]
Patch

Split out code for <https://webkit.org/b/191458>
Comment 9 Joseph Pecoraro 2018-11-09 13:22:32 PST
Comment on attachment 354360 [details]
Patch

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

Overall this looks good to me. I'll just want to see an updated patch.

> Source/JavaScriptCore/inspector/protocol/Security.json:8
> +            "description": "Security information for the request.",

This description is poor.

> Source/JavaScriptCore/inspector/protocol/Security.json:11
> +                { "name": "created", "type": "number", "optional": true },

Is this really a `created` date or the valid start date?
Given these are seconds since epoch, shouldn't these be $ref of "Network.Walltime"?

> Source/JavaScriptCore/inspector/protocol/Security.json:14
> +                { "name": "ipAddresses", "type": "array", "items": { "type": "string" }, "optional": true }

Could this get a description. What are these ip addresses? Specific hosts the certificate is limited to?

> Source/WebCore/loader/ResourceLoader.h:130
> +    WEBCORE_EXPORT bool shouldIncludeCertificateInfo() const;

Was the WEBCORE_EXPORT needed? I only see uses in WebCore.

> Source/WebCore/platform/network/CertificateInfoBase.h:37
> +    bool containsNonRootSHA1SignedCertificate() const { return false; }

Should all of these methods be virtual? Otherwise, how do these call down into the sub-class implementations?

> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:108
> +    auto firstCertificate = checked_cf_cast<SecCertificateRef>(CFArrayGetValueAtIndex(chain, 0));

Perhaps we should name this `leafCertificate`.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.css:31
> +    color: var(--network-dns-color);

How does this look in dark mode? Is it readable?

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:66
> +        let hasInsecureScheme = this._resource.urlComponents.scheme !== "https" && this._resource.urlComponents.scheme !== "wss" && this._resource.urlComponents.scheme !== "sftp";

This seems like it could be a helper somewhere. If generalized I wonder if we should also `data:` as secure even if it shouldn't be reached here.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:72
> +        if (hasInsecureConnectionTiming || hasInsecureScheme) {
> +            if (!this._insecureMessageElement)
> +                this._insecureMessageElement = WI.createMessageTextView(WI.UIString("The resource was requested insecurely."), true);
> +            this.element.appendChild(this._insecureMessageElement);
> +            return;
> +        }

Can we get a screenshot of this appearance?

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:178
> +            this._certificateSection.markIncompleteSectionWithMessage(WI.UIString("No response security information"));

Normally the incomplete section messages end in a period.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:184
> +            this._certificateSection.markIncompleteSectionWithMessage(WI.UIString("No response security certificate"));

Normally the incomplete section messages end in a period.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:190
> +        let formatDate = (key, timestamp) => {

Maybe name this `appendFormattedDate`.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:203
> +        formatDate(WI.UIString("Created"), certificate.created);
> +        formatDate(WI.UIString("Expires"), certificate.expires);

I still question if this is "Created".

  - Apple's Native UI: "Not Valid Before", "Not Valid After"
  - Chrome: "Valid From", "Valid Until"
  - Firefox: "Begins On", "Expires On"

We could match the Apple Native UI, but I tend to prefer wording like these:

  - "Valid From", "Valid Until"
  - "Valid Starting", "Valid Ending"
  - "Valid Beginning", "Expires"

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:205
> +        let limitValues = (key, values, className) => {

Maybe name this `appendList`

> LayoutTests/ChangeLog:10
> +        * http/tests/inspector/network/resource-response-security-expected.txt: Added.
> +        * http/tests/inspector/network/resource-response-security.html: Added.

You may need to skip this on other ports, I don't think they will have dates for example.
Comment 10 Devin Rousso 2018-11-09 13:31:38 PST
Comment on attachment 354360 [details]
Patch

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

>> Source/WebCore/loader/ResourceLoader.h:130
>> +    WEBCORE_EXPORT bool shouldIncludeCertificateInfo() const;
> 
> Was the WEBCORE_EXPORT needed? I only see uses in WebCore.

It's used once in WebKit/WebProcess/Network/WebLoaderStrategy.cpp:274

>> Source/WebCore/platform/network/CertificateInfoBase.h:37
>> +    bool containsNonRootSHA1SignedCertificate() const { return false; }
> 
> Should all of these methods be virtual? Otherwise, how do these call down into the sub-class implementations?

The sub-classes fully override these methods where needed.  Then again, there's no real reason for me to not make them virtual.

>> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:108
>> +    auto firstCertificate = checked_cf_cast<SecCertificateRef>(CFArrayGetValueAtIndex(chain, 0));
> 
> Perhaps we should name this `leafCertificate`.

I think that's the actual name for it 😛
Comment 11 Devin Rousso 2018-11-09 16:41:10 PST
Created attachment 354412 [details]
Patch
Comment 12 Devin Rousso 2018-11-09 17:04:04 PST
Created attachment 354418 [details]
Patch
Comment 13 Build Bot 2018-11-09 18:09:18 PST Comment hidden (obsolete)
Comment 14 Build Bot 2018-11-09 18:09:19 PST Comment hidden (obsolete)
Comment 15 Build Bot 2018-11-09 19:10:22 PST Comment hidden (obsolete)
Comment 16 Build Bot 2018-11-09 19:10:24 PST Comment hidden (obsolete)
Comment 17 Build Bot 2018-11-09 19:43:58 PST Comment hidden (obsolete)
Comment 18 Build Bot 2018-11-09 19:44:00 PST Comment hidden (obsolete)
Comment 19 Devin Rousso 2018-11-09 20:52:31 PST
Created attachment 354435 [details]
Patch

Fix win build
Comment 20 Devin Rousso 2018-11-09 21:16:33 PST
Created attachment 354437 [details]
Patch
Comment 21 Devin Rousso 2018-11-09 21:35:48 PST
Created attachment 354439 [details]
Patch
Comment 22 Devin Rousso 2018-11-09 22:53:32 PST
Created attachment 354445 [details]
Patch
Comment 23 Devin Rousso 2018-11-09 23:03:25 PST
Created attachment 354447 [details]
Patch

Add dark mode styles
Comment 24 Devin Rousso 2018-11-09 23:03:55 PST
Created attachment 354448 [details]
[Image] After Patch is applied (Insecure)
Comment 25 Joseph Pecoraro 2018-11-12 15:34:25 PST
(In reply to Devin Rousso from comment #24)
> Created attachment 354448 [details]
> [Image] After Patch is applied (Insecure)

Hmm, this doesn't look particularly good. I'd expect normal sectioning with it saying insecure not an entire content view. For now this is good but we should improve on this.
Comment 26 Joseph Pecoraro 2018-11-12 15:47:20 PST
Comment on attachment 354447 [details]
Patch

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

r=me with comments

> Source/WebCore/platform/network/cf/CertificateInfo.cpp:1
> +/*

Any reason this file isn't titled CertificateInfoCF.cpp? Pretty much all the other files in this directory have that suffix.

> Source/WebCore/platform/network/cf/CertificateInfo.cpp:123
> +        const Seconds absoluteReferenceDate(978307200);

Probably deserves a comment. This is the 1970 epoch?

> Source/WebCore/platform/network/cf/CertificateInfo.h:-76
> -#ifndef NDEBUG
> -    void dump() const;
> -#endif

You could still provide a `dump()` implementation in a platform/network/cocoa/CertificateInfoCocoa.mm. Here is would be, and the implementation would be identical to what it was before.

    #ifndef NDEBUG
    #if PLATFORM(COCOA)
        void dump() const;
    #endif
    #endif

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:65
> +        if (!this._resource.loadedSecurely) {

What if we don't have any network details at this point.

Scenario:
1. Load webkit.org
2. Open inspector
3. Go to Network Tab
4. Select a Resource
5. Select Security
  => We don't have secure timing details, does this mean it says loaded insecurely!?

We might want to present the "incomplete details" text.
Comment 27 Devin Rousso 2018-11-12 17:12:24 PST
Comment on attachment 354447 [details]
Patch

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

>> Source/WebCore/platform/network/cf/CertificateInfo.cpp:1
>> +/*
> 
> Any reason this file isn't titled CertificateInfoCF.cpp? Pretty much all the other files in this directory have that suffix.

None of the other platform specific `CertificateInfo` files had their respective platform as a suffix, so I was matching how they did it.  I'll change all of them to match their respective platforms.

>> Source/WebCore/platform/network/cf/CertificateInfo.cpp:123
>> +        const Seconds absoluteReferenceDate(978307200);
> 
> Probably deserves a comment. This is the 1970 epoch?

No, this is 1 Jan 2001 00:00:00 GMT
<https://developer.apple.com/documentation/corefoundation/cfabsolutetime>

>> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:65
>> +        if (!this._resource.loadedSecurely) {
> 
> What if we don't have any network details at this point.
> 
> Scenario:
> 1. Load webkit.org
> 2. Open inspector
> 3. Go to Network Tab
> 4. Select a Resource
> 5. Select Security
>   => We don't have secure timing details, does this mean it says loaded insecurely!?
> 
> We might want to present the "incomplete details" text.

`loadedSecurely` only returns false if:
  1. the URL scheme is not "https", "wss", or "sftp"
  2. there IS a `connectionStart` but there ISN'T a `secureConnectionStart` (e.g. the timing started a connection but never started a secure connection)

In the case that WebInspector is opened after the resource has finished loading, `loadedSecurely` will only use (1), as there won't be a `connectionStart` so (2) won't apply
Comment 28 Devin Rousso 2018-11-12 17:58:58 PST
Comment on attachment 354447 [details]
Patch

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

>>> Source/WebCore/platform/network/cf/CertificateInfo.cpp:1
>>> +/*
>> 
>> Any reason this file isn't titled CertificateInfoCF.cpp? Pretty much all the other files in this directory have that suffix.
> 
> None of the other platform specific `CertificateInfo` files had their respective platform as a suffix, so I was matching how they did it.  I'll change all of them to match their respective platforms.

Actually, I think this is so that including "CertificateInfo.h" will work regardless of platform (see ResourceResponse for another example).  I'll only change the *.cpp files.
Comment 29 Devin Rousso 2018-11-12 18:47:08 PST
Created attachment 354622 [details]
Patch
Comment 30 Build Bot 2018-11-12 18:49:55 PST Comment hidden (obsolete)
Comment 31 Devin Rousso 2018-11-12 20:07:31 PST
Created attachment 354635 [details]
Patch

Fix debug build
Comment 32 Build Bot 2018-11-12 20:10:35 PST Comment hidden (obsolete)
Comment 33 Build Bot 2018-11-12 21:38:02 PST Comment hidden (obsolete)
Comment 34 Build Bot 2018-11-12 21:38:04 PST Comment hidden (obsolete)
Comment 35 WebKit Commit Bot 2018-11-12 23:07:29 PST
Comment on attachment 354635 [details]
Patch

Clearing flags on attachment: 354635

Committed r238122: <https://trac.webkit.org/changeset/238122>
Comment 36 WebKit Commit Bot 2018-11-12 23:07:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 37 Truitt Savell 2018-11-13 08:57:21 PST
It looks like revision https://trac.webkit.org/changeset/238122/webkit

has caused multiple crashing tests on Mac Debug WK2

Crashing tests:
inspector/worker/console-basic.html
inspector/worker/debugger-pause.html
inspector/worker/debugger-scripts.html
inspector/worker/runtime-basic.html
inspector/worker/worker-create-and-terminate.html

Log for inspector/worker/console-basic.html:
https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r238129%20(5514)/inspector/worker/console-basic-crash-log.txt

Combined History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fworker%2Fconsole-basic.html%20inspector%2Fworker%2Fdebugger-pause.html%20inspector%2Fworker%2Fdebugger-scripts.html%20inspector%2Fworker%2Fruntime-basic.html%20inspector%2Fworker%2Fworker-create-and-terminate.html