Bug 191539 – Web Inspector: Network: show secure connection details per-request

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

SSL protocol, SSL cipher, etc.

[Radar WebKit Bug Importer]

<rdar://problem/45979891>

[Devin Rousso]

Created attachment 354871 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Joseph Pecoraro]

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!

[EWS Watchlist]

Comment on attachment 354871 [details]
Patch

Attachment 354871 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9996364

New failing tests:
http/wpt/css/css-animations/start-animation-001.html
http/tests/inspector/network/resource-response-security.html

[EWS Watchlist]

Created attachment 354879 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354871 [details]
Patch

Attachment 354871 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9996712

New failing tests:
http/tests/inspector/network/resource-response-security.html

[EWS Watchlist]

Created attachment 354884 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354871 [details]
Patch

Attachment 354871 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9997917

New failing tests:
http/tests/inspector/network/resource-response-security.html

[EWS Watchlist]

Created attachment 354886 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354871 [details]
Patch

Attachment 354871 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9999256

New failing tests:
http/tests/inspector/network/resource-response-security.html

[EWS Watchlist]

Created attachment 354892 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 356614 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:140:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1 in 26 files


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

[EWS Watchlist]

Comment on attachment 356614 [details]
Patch

Attachment 356614 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10279974

New failing tests:
http/tests/misc/resource-timing-resolution.html

[EWS Watchlist]

Created attachment 356624 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Joseph Pecoraro]

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).

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 357511 [details]
Patch

[Devin Rousso]

Created attachment 357512 [details]
[Image] After Patch is applied

[EWS Watchlist]

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


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]
Total errors found: 4 in 24 files


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

[Joseph Pecoraro]

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

[EWS Watchlist]

Comment on attachment 357511 [details]
Patch

Attachment 357511 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10449896

New failing tests:
http/tests/inspector/network/resource-response-source-memory-cache.html
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
http/tests/inspector/network/ping-type.html
http/tests/inspector/network/xhr-response-body.html

[EWS Watchlist]

Created attachment 357517 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357511 [details]
Patch

Attachment 357511 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10450108

New failing tests:
http/tests/inspector/network/resource-response-source-memory-cache.html
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
http/tests/inspector/network/ping-type.html
http/tests/inspector/network/xhr-response-body.html

[EWS Watchlist]

Created attachment 357522 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357511 [details]
Patch

Attachment 357511 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10450681

New failing tests:
http/tests/inspector/network/beacon-type.html
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
http/tests/inspector/network/ping-type.html
http/tests/inspector/network/xhr-response-body.html
http/tests/inspector/network/resource-response-source-memory-cache.html

[EWS Watchlist]

Created attachment 357524 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 357530 [details]
Patch

[EWS Watchlist]

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


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]
Total errors found: 4 in 24 files


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

[WebKit Commit Bot]

Comment on attachment 357530 [details]
Patch

Clearing flags on attachment: 357530

Committed r239698: <https://trac.webkit.org/changeset/239698>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> 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.

[Joseph Pecoraro]

> > 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.

[Alexey Proskuryakov]

Comment on attachment 357530 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:661
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)

This only enables it on iOS, so PLATFORM(IOS) would be more appropriate - unless we want this on all aligned OSes.