Bug 191539 - Web Inspector: Network: show secure connection details per-request
Summary: Web Inspector: Network: show secure connection 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: 191447
Blocks: 192406 192407
  Show dependency treegraph
 
Reported: 2018-11-11 21:42 PST by Devin Rousso
Modified: 2019-01-08 13:08 PST (History)
12 users (show)

See Also:


Attachments
Patch (38.12 KB, patch)
2018-11-14 16:42 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.85 MB, application/zip)
2018-11-14 17:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.30 MB, application/zip)
2018-11-14 18:45 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.27 MB, application/zip)
2018-11-14 20:48 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.05 MB, application/zip)
2018-11-14 23:42 PST, Build Bot
no flags Details
Patch (51.51 KB, patch)
2018-12-05 10:04 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.41 MB, application/zip)
2018-12-05 11:13 PST, Build Bot
no flags Details
Patch (48.05 KB, patch)
2018-12-17 18:10 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (749.20 KB, image/png)
2018-12-17 18:10 PST, Devin Rousso
no flags Details
Archive of layout-test-results from ews102 for mac-sierra (2.70 MB, application/zip)
2018-12-17 19:21 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (2.58 MB, application/zip)
2018-12-17 20:08 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.68 MB, application/zip)
2018-12-17 20:15 PST, Build Bot
no flags Details
Patch (48.65 KB, patch)
2018-12-17 21:25 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-11-11 21:42:59 PST
SSL protocol, SSL cipher, etc.
Comment 1 Radar WebKit Bug Importer 2018-11-11 21:43:22 PST
<rdar://problem/45979891>
Comment 2 Devin Rousso 2018-11-14 16:42:47 PST
Created attachment 354871 [details]
Patch
Comment 3 Build Bot 2018-11-14 16:46:19 PST
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 4 Joseph Pecoraro 2018-11-14 16:58:42 PST
Comment on attachment 354871 [details]
Patch

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

> LayoutTests/http/tests/inspector/network/resource-response-security.html:32
> +                InspectorTest.expectGreaterThan(connection.protocol.length, 0, "Connection should have protocol.");
> +                InspectorTest.expectGreaterThan(connection.cipher.length, 0, "Connection should have cipher.");

Can we output these in the test? That would be cool.

> Source/JavaScriptCore/inspector/protocol/Security.json:31
> +                { "name": "connection", "$ref": "Connection", "optional": true },

I think you'll have to move this to Network.Metrics, which happens at a later time than response received. I think NSURLSession's didFinishCollectingMetrics happens after didReceiveResponse, so the values would likely be empty where this is doing it now in InspectorNetworkAgent. But do test first.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:211
> +        if (!certificate || Object.values(certificate).every((value) => !value)) {

We may want to handle an isEmptyObject.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:124
> +    case kTLSProtocolMaxSupported:
> +        return emptyString();

Should we assert this is not reached? It sounds like this is not a valid value, its just for enumeration.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:134
> +    case SSL_NULL_WITH_NULL_NULL:
> +        return "SSL_NULL_WITH_NULL_NULL"_s;

Your idea, turn this into a macro, so we only have the string once!
Comment 5 Build Bot 2018-11-14 17:44:08 PST Comment hidden (obsolete)
Comment 6 Build Bot 2018-11-14 17:44:10 PST Comment hidden (obsolete)
Comment 7 Build Bot 2018-11-14 18:45:28 PST Comment hidden (obsolete)
Comment 8 Build Bot 2018-11-14 18:45:30 PST Comment hidden (obsolete)
Comment 9 Build Bot 2018-11-14 20:48:18 PST Comment hidden (obsolete)
Comment 10 Build Bot 2018-11-14 20:48:19 PST Comment hidden (obsolete)
Comment 11 Build Bot 2018-11-14 23:42:18 PST Comment hidden (obsolete)
Comment 12 Build Bot 2018-11-14 23:42:20 PST Comment hidden (obsolete)
Comment 13 Devin Rousso 2018-12-05 09:54:33 PST
Comment on attachment 354871 [details]
Patch

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

>> LayoutTests/http/tests/inspector/network/resource-response-security.html:32
>> +                InspectorTest.expectGreaterThan(connection.cipher.length, 0, "Connection should have cipher.");
> 
> Can we output these in the test? That would be cool.

I did some testing, and it appears that we use TLS1.2 with the TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 cipher.  I'd rather keep it as is, however, in the case that this changes in the future to be a different protocol/cipher.
Comment 14 Devin Rousso 2018-12-05 10:04:13 PST
Created attachment 356614 [details]
Patch
Comment 15 Build Bot 2018-12-05 10:05:47 PST Comment hidden (obsolete)
Comment 16 Build Bot 2018-12-05 11:13:33 PST Comment hidden (obsolete)
Comment 17 Build Bot 2018-12-05 11:13:35 PST Comment hidden (obsolete)
Comment 18 Joseph Pecoraro 2018-12-07 11:49:18 PST
Comment on attachment 356614 [details]
Patch

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

Please add a screenshot for the UI (even though it should be pretty straightforward).

I do think this shouldn't hang on the same Security object, that starts getting weird because it gets updated in different places for different reasons.

The rest of this patch looks great though.

> Source/JavaScriptCore/inspector/protocol/Network.json:271
> +                { "name": "security", "$ref": "Security.Security", "optional": true, "description": "Resolved security information for the completed request." }

I'd just call this "connection" and $ref "Security.Connection" directly. I didn't think you'd go this route of using the same object. This would also avoid the `updateSecurity` weird merging that happens later.

Nit: Does the word "Resolved" in the description add anything?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:137
> +    // STRINGIFY_CIPHER(SSL_NULL_WITH_NULL_NULL);

Would be nice to include a pointer to the value that this is shared with. (Or just putting it along side that value).
Comment 19 Joseph Pecoraro 2018-12-07 11:50:32 PST
Comment on attachment 356614 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:122
> +        return "Unknown"_s;

Hmm, should this be localizable? I guess we don't expect it to be reached.
Comment 20 Devin Rousso 2018-12-17 18:10:10 PST
Created attachment 357511 [details]
Patch
Comment 21 Devin Rousso 2018-12-17 18:10:19 PST
Created attachment 357512 [details]
[Image] After Patch is applied
Comment 22 Build Bot 2018-12-17 18:12:38 PST Comment hidden (obsolete)
Comment 23 Joseph Pecoraro 2018-12-17 18:25:53 PST
Comment on attachment 357511 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/protocol/Network.json:97
> +                { "name": "securityConnection", "$ref": "Security.Connection", "optional": true, "description": "Connection information for the completed request." }

Why not just name this `connection`?

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:214
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)

Alexey was just talking about moving these to USE/HAVE macros. For now I think its okay as is, I actually prefer to see this info at these sites.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:197
> +        this._connectionSection.appendKeyValuePair(WI.UIString("Protocol"), connection.protocol);
> +        this._connectionSection.appendKeyValuePair(WI.UIString("Cipher"), connection.cipher);

The backend / protocol allows for only one of these properties. There should probably be a default value such as an emdash.

    connection.protocol || emDash
Comment 24 Build Bot 2018-12-17 19:21:31 PST Comment hidden (obsolete)
Comment 25 Build Bot 2018-12-17 19:21:33 PST Comment hidden (obsolete)
Comment 26 Build Bot 2018-12-17 20:08:33 PST Comment hidden (obsolete)
Comment 27 Build Bot 2018-12-17 20:08:35 PST Comment hidden (obsolete)
Comment 28 Build Bot 2018-12-17 20:15:44 PST Comment hidden (obsolete)
Comment 29 Build Bot 2018-12-17 20:15:46 PST Comment hidden (obsolete)
Comment 30 Devin Rousso 2018-12-17 21:18:39 PST
Comment on attachment 357511 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/Network.json:97
>> +                { "name": "securityConnection", "$ref": "Security.Connection", "optional": true, "description": "Connection information for the completed request." }
> 
> Why not just name this `connection`?

Mainly for future-proofing's sake, just in case we wanted to add some different `connection` information in the future.  This way the data is a bit more specific (especially for the frontend, which doesn't have any of the type info).

>> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:197
>> +        this._connectionSection.appendKeyValuePair(WI.UIString("Cipher"), connection.cipher);
> 
> The backend / protocol allows for only one of these properties. There should probably be a default value such as an emdash.
> 
>     connection.protocol || emDash

The backend/protocol allows for any combination of the two.  There's no strict "limit" that there only be one valid at a time.  A default value is a good idea, however.
Comment 31 Devin Rousso 2018-12-17 21:25:13 PST
Created attachment 357530 [details]
Patch
Comment 32 Build Bot 2018-12-17 21:27:13 PST Comment hidden (obsolete)
Comment 33 WebKit Commit Bot 2019-01-07 14:19:08 PST
Comment on attachment 357530 [details]
Patch

Clearing flags on attachment: 357530

Committed r239698: <https://trac.webkit.org/changeset/239698>
Comment 34 WebKit Commit Bot 2019-01-07 14:19:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 35 Alexey Proskuryakov 2019-01-08 12:55:38 PST
> Alexey was just talking about moving these to USE/HAVE macros. For now I think its okay as is, I actually prefer to see this info at these sites.

This is not a matter or preference. Please move the version check to its proper place.
Comment 36 Joseph Pecoraro 2019-01-08 13:08:14 PST
> > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:214
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)
> 
> Alexey was just talking about moving these to USE/HAVE macros.

This is now expected style as evident by the style checker warnings:

    ERROR: Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:214:  Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h.  [build/version_check] [5]
    ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:94:  Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h.  [build/version_check] [5]
    ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:134:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
    ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:661:  Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h.  [build/version_check] [5]

We should clean this up.