Bug 191458 - Web Inspector: Network: add button to show system certificate dialog
Summary: Web Inspector: Network: add button to show system certificate dialog
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: 191447
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-08 22:41 PST by Devin Rousso
Modified: 2018-11-17 10:27 PST (History)
6 users (show)

See Also:


Attachments
Patch (77.95 KB, patch)
2018-11-11 17:14 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (86.34 KB, patch)
2018-11-13 00:47 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (86.29 KB, patch)
2018-11-13 09:27 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (87.46 KB, patch)
2018-11-14 11:15 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (89.05 KB, patch)
2018-11-14 16:21 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (89.06 KB, patch)
2018-11-14 16:44 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (92.92 KB, patch)
2018-11-16 15:55 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (97.00 KB, patch)
2018-11-16 23:53 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (93.45 KB, patch)
2018-11-17 00:33 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (92.63 KB, patch)
2018-11-17 00:38 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (92.84 KB, patch)
2018-11-17 00:56 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.67 MB, application/zip)
2018-11-17 02:54 PST, EWS Watchlist
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 22:41:26 PST
For now, this will only work on macOS using SecurityInterface.
Comment 1 Radar WebKit Bug Importer 2018-11-11 16:51:07 PST
<rdar://problem/45977019>
Comment 2 Devin Rousso 2018-11-11 17:14:38 PST
Created attachment 354518 [details]
Patch
Comment 3 Joseph Pecoraro 2018-11-12 16:11:26 PST
Comment on attachment 354518 [details]
Patch

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

This looks great to me! I think there is a concern around versioning when sending these across systems if they ever change. But as is should work great.

> LayoutTests/http/tests/inspector/network/getSerializedCertificate.html:33
> +        test(resolve, reject) {
> +            WI.Resource.awaitEvent(WI.Resource.Event.ResponseReceived)
> +            .then((event) => InspectorTest.expectException(() => NetworkAgent.getSerializedCertificate(event.target.requestIdentifier)))
> +            .then(resolve, reject);
> +
> +            InspectorTest.evaluateInPage(`createInsecureRequest()`)
> +            .catch(reject);
> +        }

Write these as async tests? Seems it would read very linearly:

    async test() {
        InspectorTest.evaluateInPage(`createInsecureRequest()`);
        await event = WI.Resource.awaitEvent(WI.Resource.Event.ResponseReceived);
        await InspectorTest.expectException(() => NetworkAgent.getSerializedCertificate(event.target.requestIdentifier)));
    }

> Source/JavaScriptCore/inspector/protocol/Network.json:207
> +                { "name": "serializedCertificate", "type": "string" }

Might be worth adding a description that this is a base64 encoded WebCore::CertificateInfo.

If the CertificateInfo data changes this won't work with remote inspection across systems. For example if inspecting a WebKit version that had a different serialization of CertificateInfo. We might need to version this, or at least make it so `InspectorFrontendHost.showCertificate` will fail and gracefully display a message when it cannot decode the certificate info.

> Source/WebCore/platform/network/cf/CertificateInfo.h:162
> +    Vector<CFTypeRef, 32> values(size);

Hmm, 32 seems like a weird inline capacity to have here, but it's not changing.
Comment 4 Devin Rousso 2018-11-12 17:05:35 PST
Comment on attachment 354518 [details]
Patch

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

>> LayoutTests/http/tests/inspector/network/getSerializedCertificate.html:33
>> +        }
> 
> Write these as async tests? Seems it would read very linearly:
> 
>     async test() {
>         InspectorTest.evaluateInPage(`createInsecureRequest()`);
>         await event = WI.Resource.awaitEvent(WI.Resource.Event.ResponseReceived);
>         await InspectorTest.expectException(() => NetworkAgent.getSerializedCertificate(event.target.requestIdentifier)));
>     }

I agree that it looks nicer, but I have an aversion to calling code before it's associated listener has been added.  I don't have a good reason for this, but it's just something I look out for.  If it's any consolation, this is almost linear, in that everything except for the last line is in the order you'd expect (the last line is what kicks off the code, whereas everything before it is setup).

>> Source/JavaScriptCore/inspector/protocol/Network.json:207
>> +                { "name": "serializedCertificate", "type": "string" }
> 
> Might be worth adding a description that this is a base64 encoded WebCore::CertificateInfo.
> 
> If the CertificateInfo data changes this won't work with remote inspection across systems. For example if inspecting a WebKit version that had a different serialization of CertificateInfo. We might need to version this, or at least make it so `InspectorFrontendHost.showCertificate` will fail and gracefully display a message when it cannot decode the certificate info.

As discussed offline, I'll have `InspectorFrontendHost::showCertificate` return true/false depending on whether the deserialization was successful or not.
Comment 5 Devin Rousso 2018-11-13 00:47:36 PST
Created attachment 354657 [details]
Patch
Comment 6 Devin Rousso 2018-11-13 09:27:08 PST
Created attachment 354676 [details]
Patch
Comment 7 Joseph Pecoraro 2018-11-13 11:20:11 PST
Comment on attachment 354676 [details]
Patch

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

I think this is good, but I have a few questions.

> Source/JavaScriptCore/inspector/protocol/Network.json:202
> +            "name": "getSerializedCertificate",

Nit: This could use a description.

Reminder to email our ITMLKit friends.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:54
> +    static supportsShowingCertificates()
> +    {
> +        return window.NetworkAgent && NetworkAgent.getSerializedCertificate && InspectorFrontendHost.showCertificate;
> +    }

I wonder if we should have a `InspectorFrontendHost.supportsShowCertificate()` so that other ports that don't yet have an implementation won't show the "View Certificate" button that doesn't work.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1073
> +            if (InspectorFrontendHost.showCertificate(serializedCertificates))
> +                return;

We should display some kind of error dialog, and probably disable the button if this fails. Maybe showCertificate needs a callback.

> Source/WebKit/UIProcess/RemoteWebInspectorProxy.h:47
> +namespace WebCore {
> +
> +class CertificateInfo;
> +
> +}

Style: Normally we drop the empty lines.

> Source/WebKit/UIProcess/WebInspectorProxy.h:55
>  namespace WebCore {
> +
> +class CertificateInfo;

Ditto.

> Source/WebKit/UIProcess/mac/WebInspectorProxyMac.mm:428
>  
> +

Style: Extra newline.

> Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.h:38
> +namespace WebCore {
> +
> +class CertificateInfo;
> +
> +}

Ditto.

> Source/WebKit/WebProcess/WebPage/WebInspectorUI.h:38
>  namespace WebCore {
> +
>  class InspectorController;
> +class CertificateInfo;
> +
>  }

Ditto.

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.h:46
>  namespace WebCore {
> +
> +class CertificateInfo;

Ditto.

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:294
> +#if HAVE(SEC_TRUST_SERIALIZATION)
> +    [certificatePanel beginSheetForWindow:window modalDelegate:nil didEndSelector:NULL contextInfo:nullptr trust:certificateInfo.trust() showGroup:YES];
> +#else
> +    [certificatePanel beginSheetForWindow:window modalDelegate:nil didEndSelector:NULL contextInfo:nullptr certificates:(NSArray *)certificateInfo.certificateChain() showGroup:YES];
> +#endif

What happens if the certificateChain is empty, or if the trust is NULL (not sure if that can actually happen though).

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:298
> +    [[certificatePanel certificateView] setDisplayDetails:YES];
> +    [[certificatePanel certificateView] setDisplayTrust:YES];
> +    [[certificatePanel certificateView] setEditableTrust:NO];

You may want to pull the view out into a local to avoid calling certificateView 3 times (it doesn't get optimized out like C++).

    SFCertificateView *certificateView = [certificatePanel certificateView];
    [certificateView setDisplayDetails:YES];
    ...

It may be worth a comment that we do this after the beingSheetForWindow because the view doesn't exist before hand.

Should we also just auto-disclose the details? That seems like it would be useful if the developer is trying to view additional details.
https://developer.apple.com/documentation/securityinterface/sfcertificateview/1588428-setdetailsdisclosed?language=objc

    [certificateView setDetailsDisclosed:YES]

> Source/WebKitLegacy/win/WebCoreSupport/WebInspectorClient.h:44
>  namespace WebCore {
> +

Ditto.
Comment 8 Devin Rousso 2018-11-14 10:36:00 PST
Comment on attachment 354676 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:1073
>> +                return;
> 
> We should display some kind of error dialog, and probably disable the button if this fails. Maybe showCertificate needs a callback.

`InspectorFrontendHost.showCertificate` returns `false` when we were unable to show the certificate, and in that case a message gets added to the console.  I'd be fine with adding a `window.alert`, but I wasn't sure about that considering there's no precedent.
Comment 9 Devin Rousso 2018-11-14 11:15:38 PST
Created attachment 354838 [details]
Patch
Comment 10 Joseph Pecoraro 2018-11-14 11:24:14 PST
(In reply to Devin Rousso from comment #8)
> Comment on attachment 354676 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354676&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Models/Resource.js:1073
> >> +                return;
> > 
> > We should display some kind of error dialog, and probably disable the button if this fails. Maybe showCertificate needs a callback.
> 
> `InspectorFrontendHost.showCertificate` returns `false` when we were unable
> to show the certificate, and in that case a message gets added to the
> console.  I'd be fine with adding a `window.alert`, but I wasn't sure about
> that considering there's no precedent.

We don't want a window.alert, and that probably won't work as we haven't set up a UIDelegate for that. The console message is probably good enough.
Comment 11 Joseph Pecoraro 2018-11-14 14:28:16 PST
Comment on attachment 354838 [details]
Patch

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

Everything looks good but you came up with a few ideas (the warning icon if button fails, and IFH.supports).

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:54
> +    static supportsShowingCertificates()
> +    {
> +        return window.NetworkAgent && NetworkAgent.getSerializedCertificate && InspectorFrontendHost.showCertificate;
> +    }

I still think we should have an IFH.supportsShowCertificate so that we don't show the button on ports that it won't work on.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1072
> +            await Promise.reject(); // Don't expose the error.

Lets change these `await Promise.rejects()` to just `throw undefined` or even just including a error `throw new Error("...");`.

> Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:121
> +    platformShowCertificate(certificateInfo);

The bottom of this file needs an empty implementation of this for wincairo, along with the other RemoteWebInspectorProxy::platform* functions.
Comment 12 Devin Rousso 2018-11-14 16:21:13 PST
Created attachment 354867 [details]
Patch
Comment 13 Devin Rousso 2018-11-14 16:44:57 PST
Created attachment 354872 [details]
Patch
Comment 14 Joseph Pecoraro 2018-11-16 11:42:24 PST
Comment on attachment 354872 [details]
Patch

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

r=me!

> Source/WebCore/platform/network/cf/CertificateInfo.h:103
> +namespace WTF {
> +
> +namespace Persistence {

Style: Normally we don't have the extra newline between these (and the closing braces). I see you added them, you may want to remove them.

> Source/WebCore/platform/network/cf/CertificateInfo.h:128
> +
> +

Style: Extra newline, carried from the original but we can clean it up.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:54
> +        return InspectorFrontendHost.supportsShowCertificate && InspectorFrontendHost.showCertificate && window.NetworkAgent && NetworkAgent.getSerializedCertificate;

This isn't actually calling supportsShowCertificate, just checking if it exists which will always be true.

Further it looks like we don't have the IFH stub anymore, so we don't need to feature check IFH functions and can assume it always exists.

I think this should be:

    static supportsShowCertificate()
    {
        return InspectorFrontendHost.supportsShowCertificate() && window.NetworkAgent && NetworkAgent.getSerializedCertificate;
    }

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1074
> +            throw errorString; // Don't expose the error.

The comment doesn't seem useful.

> Source/WebKit/UIProcess/RemoteWebInspectorProxy.h:117
> +    void platformShowCertificate(const WebCore::CertificateInfo&);

To get the Windows ports to build you need to include a stub in WebInspectorProxyWPE.cpp and WebInspectorProxyWin.cpp for this.
Comment 15 Devin Rousso 2018-11-16 13:18:05 PST
Comment on attachment 354872 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:54
>> +        return InspectorFrontendHost.supportsShowCertificate && InspectorFrontendHost.showCertificate && window.NetworkAgent && NetworkAgent.getSerializedCertificate;
> 
> This isn't actually calling supportsShowCertificate, just checking if it exists which will always be true.
> 
> Further it looks like we don't have the IFH stub anymore, so we don't need to feature check IFH functions and can assume it always exists.
> 
> I think this should be:
> 
>     static supportsShowCertificate()
>     {
>         return InspectorFrontendHost.supportsShowCertificate() && window.NetworkAgent && NetworkAgent.getSerializedCertificate;
>     }

I made `supportsShowCertificate` an attribute on IFH, so it should be doing logic.
Comment 16 Joseph Pecoraro 2018-11-16 15:48:40 PST
Comment on attachment 354872 [details]
Patch

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

> Source/WebCore/platform/network/soup/CertificateInfo.h:74
> +template<> struct Coder<GRefPtr<GByteArray>> {

Does this need #if USE(GLIB) around it and friends (to line 127).
Comment 17 Joseph Pecoraro 2018-11-16 15:49:32 PST
> I made `supportsShowCertificate` an attribute on IFH, so it should be doing logic.

Oh nice! I read the .h and though it was the .idl!
Comment 18 Devin Rousso 2018-11-16 15:55:18 PST
Created attachment 355145 [details]
Patch
Comment 19 Devin Rousso 2018-11-16 23:53:48 PST
Created attachment 355185 [details]
Patch
Comment 20 Devin Rousso 2018-11-17 00:33:27 PST
Created attachment 355187 [details]
Patch
Comment 21 Devin Rousso 2018-11-17 00:38:03 PST
Created attachment 355188 [details]
Patch
Comment 22 Devin Rousso 2018-11-17 00:56:31 PST
Created attachment 355189 [details]
Patch

Try adding include for WebCoreArgumentCoders.h
Comment 23 EWS Watchlist 2018-11-17 02:54:40 PST Comment hidden (obsolete)
Comment 24 EWS Watchlist 2018-11-17 02:54:42 PST Comment hidden (obsolete)
Comment 25 WebKit Commit Bot 2018-11-17 10:27:50 PST
Comment on attachment 355189 [details]
Patch

Clearing flags on attachment: 355189

Committed r238350: <https://trac.webkit.org/changeset/238350>
Comment 26 WebKit Commit Bot 2018-11-17 10:27:52 PST
All reviewed patches have been landed.  Closing bug.