SSL protocol, SSL cipher, etc.
<rdar://problem/45979891>
Created attachment 354871 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
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 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
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
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
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
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
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
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
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
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.
Created attachment 356614 [details] Patch
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.
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
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
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 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.
Created attachment 357511 [details] Patch
Created attachment 357512 [details] [Image] After Patch is applied
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.
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 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
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
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
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
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
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
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.
Created attachment 357530 [details] Patch
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.
Comment on attachment 357530 [details] Patch Clearing flags on attachment: 357530 Committed r239698: <https://trac.webkit.org/changeset/239698>
All reviewed patches have been landed. Closing bug.
> 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.
> > 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.
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.