For developers who are working on improving performance at the transport layer, being able to see which requests are on which connections, and the properties of the connections, is very useful. It would be great if there were a way to show the state of network connections. In particular, it would be great to expose: * Which protocol is being used (HTTP, HTTP/1.x over TLS, SPDY, HTTP/2, Websockets). * The TLS version negotiated (SSL 3.0, TLS 1.0, TLS 1.1, TLS 1.2), cipher suite negotiated, certificates, stapled OCSP response, etc., whether the certificate validated successfully, whether there were any SSL-related warnings (e.g. weak keys, lack of support for the renegotiation extension, deprecated cipher suite in use). Some of this information is available for the document request using the Page Info UI by clicking the site identity panel -> More information -> Security (Technical details). However, the page info UI doesn't make sense on a modern web page because there are almost always multiple connections to multiple hosts involved. Often, when somebody is actually trying to work on configuring their SSL stuff, they'd like a view of the SSL info across all the connections on the page. Imagine that the normal view had a narrow "SSL" or "Protocol" column that indicated whether SSL is being used or which protocol is being used. Also, imagine we had an option to expand this column into a set of columns that provided more details: TLS version, cipher suite, whether OCSP stapling was used, etc. (This would be similar to how the timeline column can be toggled between compact and wide views.) This would allow somebody to quickly determine which servers/connections need to be changed to better configurations. Presumably some of the normal columns would have to be collapsed when the SSL column is expanded to the multi-column view. A lot (all?) of this information is already exposed by looking at the channel.securityInfo.QueryInterface(nsITransportSecurityInfo) and channel.securityInfo.QueryInterface(nsISSLStatusProvider).SSLStatus (nsISSLStatus). I, or others on the Security Engineering team, would be happy to expose more information as needed to support this work. I think that this would be a major improvement to our developer tools that would make Firefox the best browser for investigating SSL-related configuration issues.

This would be immensely useful. As it is, the UI for extracting this info from the padlock icon in the URL bar is not overly useful. Besides, the point about wanting to see these properties on a per-asset basis is particularly relevant to anyone configuring SSL across different domains, with possibly different servers and configurations. Cipher suite and protocol (like SPDY) are probably the two I look for most, but those additional items, such as TLS version, SSL warnings, and especially OSCP (relevant now more than ever), would be incredible details to expose.

I've been working on adding security information to the netmonitor for a while and it's finally starting to come along. It's not exactly what this bug describes but it's a start. Feedback and suggestions are welcome. This first patch adds the server side support for capturing security information. The information is extracted from nsIChannel when nsIRequestObserver.onStartRequest is called. A new method, parseSecurityInfo, is added to network-helper.js which extracts information from securityInfo and nsIRequest objects. It categorizes the request to one of three states: * "secure": the request security state contained STATE_IS_SECURE flag. * "insecure": the request did not contain any security information which means it was performed over insecure connection (i.e. http). * "broken": the request failed due to security issue. The request is marked "broken" if it can't be classified as "secure" or "insecure". For requests with "broken" state the error message from nsITransportSecurityInfo is added to the returned object. For "insecure" requests the returned object only contains the security state. For "secure" requests the returned object contains information about protocol version, cipher suite, certificate HSTS and HPKP. This is pretty much everything I was able to dig from currently available services/interfaces. The two xpcshell tests check that parseSecurityInfo maps the securityInfo fields as expected. HSTS/HPKP detection is tested by two mochitests and they utilize .sjs files from browser-chrome tests.

This patch adds the client side components required to display the security information received from server. An icon is added to the left of domain that denotes the security status of the request. The icons are borrowed from the urlbar identity icons and depending on the connection security state they are * "insecure": gray globe as shown in http pages. * "secure": gray lock as shown in non-EV https pages. * "broken": an orange triangle as shown for pages with mixed active content. For insecure and secure requests the icons are quite obvious. The orange triangle breaks consistency with the urlbar a bit but I think it works fine as an issue indicator in the netmonitor. The icon is shown once the state is available - prior to that there's a blank space reserved for it. Thus older servers should work fine with these changes. We could also use trait to hide the slot conditionally but the current version doesn't look that odd and requires no extra code to handle different clients. A new "Security" tab is added to the details sidebar that shows the details collected by the debugger server. It's only visible for "secure" and "broken" requests and can be opened directly by clicking the icon. The details pane borrows some strings from Certificate Viewer. There's of course few tests that should cover the basic functionality this patch introduces. Here's a try run with both the patches applied and nothing seems to be (permanently) broken: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=24ca0d2d8d50

Here's a screenshot of the UI showing security state details.

Here's a screenshot of the UI showing security error. The layout is a bit bad but the message comes as whitespace formatted so it's kind of hard to fit it in a narrow pane and make it look good the same time.

Wow, this is very cool. Reviewing.

Here's a revised version with following fixes to the server side code: - if a https request was canceled at some (specific) stage, it was marked as broken. Now it is marked as insecure. - updated unit tests to match behavior above - added a comment to parseSecurityInfo about all the possible situations it (tries) to handle. If security of a connection is weak[1], the current version marks the requests as insecure as there's no way to know, why the security is weak. Bug 1092835 has recently been relanded into mozilla-inbound which makes it possible to determine the cause for weakness and a warning will be printed to the console. Those warnings could be integrated with these changes but right now they're not implemented. [1] http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCallbacks.cpp#1267

Here's a revised version to the client side code which stops the cursor from turning into a pointer before the state icon is visible.

Comment on attachment 8536190 [details] [diff] [review] netmonitor-per-request-tls-status-part-2.patch Review of attachment 8536190 [details] [diff] [review]: ----------------------------------------------------------------- Very nice, great start! ::: browser/devtools/netmonitor/netmonitor-controller.js @@ +665,5 @@ > + * @param object aResponse > + * The message received from the server. > + */ > + _onSecurityInfo: function(aResponse) { > + NetMonitorView.RequestsMenu.updateRequest(aResponse.from, { Nit: bad indentation here. ::: browser/devtools/netmonitor/netmonitor-view.js @@ +1633,5 @@ > + */ > + _onSecurityIconClick: function(e) { > + let state = this.selectedItem.attachment.securityState; > + if (state === "broken" || state === "secure") { > + NetMonitorView.NetworkDetails.widget.selectedIndex = 6; Add a comment describing that 6 represents the security panel. @@ +2087,5 @@ > // Switch to the "Headers" tabpanel if the "Preview" previously selected > + // and this is not an HTML response or "Security" was selected but this > + // request has no security information. > + if (!isHtml && this.widget.selectedIndex == 5 || > + !hasSecurityInfo && this.widget.selectedIndex == 6) { Same here, 5 and 6 look too magical. A better approach might be selectedPanel == $("selector"); ::: browser/devtools/netmonitor/test/browser_net_html-preview.js @@ -22,5 @@ > "The first tab in the details pane should be selected."); > is($("#preview-tab").hidden, true, > "The preview tab should be hidden for non html responses."); > - is($("#preview-tabpanel").hidden, true, > - "The preview tabpanel should be hidden for non html responses."); So what happens to non-html responses now? We should still hide that tabpanel. @@ -32,5 @@ > "The fifth tab in the details pane should be selected."); > is($("#preview-tab").hidden, false, > "The preview tab should be visible now."); > - is($("#preview-tabpanel").hidden, false, > - "The preview tabpanel should be visible now."); Ditto. Let's keep the previous behavior.

Comment on attachment 8536188 [details] [diff] [review] netmonitor-per-request-tls-status-part-1.patch Review of attachment 8536188 [details] [diff] [review]: ----------------------------------------------------------------- I'm not the right person to review all of this. Maybe panos?

Thank you for the feedback. (In reply to Victor Porof [:vporof][:vp] from comment #9) > Comment on attachment 8536190 [details] [diff] [review] > netmonitor-per-request-tls-status-part-2.patch > > Review of attachment 8536190 [details] [diff] [review]: > ----------------------------------------------------------------- > > Very nice, great start! > > ::: browser/devtools/netmonitor/netmonitor-controller.js > @@ +665,5 @@ > > + * @param object aResponse > > + * The message received from the server. > > + */ > > + _onSecurityInfo: function(aResponse) { > > + NetMonitorView.RequestsMenu.updateRequest(aResponse.from, { > > Nit: bad indentation here. Fixed. > ::: browser/devtools/netmonitor/netmonitor-view.js > @@ +1633,5 @@ > > + */ > > + _onSecurityIconClick: function(e) { > > + let state = this.selectedItem.attachment.securityState; > > + if (state === "broken" || state === "secure") { > > + NetMonitorView.NetworkDetails.widget.selectedIndex = 6; > > Add a comment describing that 6 represents the security panel. Done. > @@ +2087,5 @@ > > // Switch to the "Headers" tabpanel if the "Preview" previously selected > > + // and this is not an HTML response or "Security" was selected but this > > + // request has no security information. > > + if (!isHtml && this.widget.selectedIndex == 5 || > > + !hasSecurityInfo && this.widget.selectedIndex == 6) { > > Same here, 5 and 6 look too magical. > A better approach might be selectedPanel == $("selector"); Changed to use selectedPanel. > ::: browser/devtools/netmonitor/test/browser_net_html-preview.js > @@ -22,5 @@ > > "The first tab in the details pane should be selected."); > > is($("#preview-tab").hidden, true, > > "The preview tab should be hidden for non html responses."); > > - is($("#preview-tabpanel").hidden, true, > > - "The preview tabpanel should be hidden for non html responses."); > > So what happens to non-html responses now? We should still hide that > tabpanel. If the #preview-{tabpanel,tab} is hidden and I choose security tab, the security panel is blank. There's a note in MDN[1] which says tabpanels should never be hidden and apparently there's a good reason for that. Maybe the rendered panel is the nth visible tabpanel instead of nth tabpanel... [1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/tabpanels

Comment on attachment 8536188 [details] [diff] [review] netmonitor-per-request-tls-status-part-1.patch Panos, could you could take a look at the server side changes as per Victor's suggestion? Perhaps someone from security team should check this too...

Comment on attachment 8536188 [details] [diff] [review] netmonitor-per-request-tls-status-part-1.patch Review of attachment 8536188 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me, apart from a couple of minor issues, thanks! Flagging dkeeler for additional review on the security bits in network-helper.js and the tests. ::: toolkit/devtools/webconsole/network-helper.js @@ +584,5 @@ > + break; > + default: > + info.protocolVersion = "Unknown"; > + Cu.reportError("parseSecurityInfo: protocolVersion " + > + SSLStatus.protocolVersion + " is unknown"); Since this is in toolkit, better avoid Cu.reportError() and use DevToolsUtils.reportException() instead. Some embeddings may not provide Cu.reportError(). @@ +616,5 @@ > + sha1: cert.sha1Fingerprint, > + sha256: cert.sha256Fingerprint, > + }; > + } else { > + Cu.reportError("Secure connection established without certificate."); Ditto.

Comment on attachment 8536188 [details] [diff] [review] netmonitor-per-request-tls-status-part-1.patch Review of attachment 8536188 [details] [diff] [review]: ----------------------------------------------------------------- This is great! I just had a few comments to add. ::: toolkit/devtools/webconsole/network-helper.js @@ +503,5 @@ > + * - errorMessage: full error message from nsITransportSecurityInfo. > + * If state == secure: > + * - protocolVersion: one of SSLv3, TLSv1, TLSv1.1, TLSv1.2. > + * - cipherSuite: the cipher suite used in this connection. > + * - cert: information about certificate used in this connection It would be great to also have the base64-encoded DER bytes of the certificate (use getRawDER) (and, actually, the entire chain the server sent, but at the moment that's only available if the connection failed - see nsITransportSecurityInfo.failedCertChain). @@ +624,5 @@ > + const sss = Cc["@mozilla.org/ssservice;1"] > + .getService(Ci.nsISiteSecurityService); > + > + // Check Strict-Transport-Security status for this domain. > + info.hsts = sss.isSecureURI(sss.HEADER_HSTS, request.URI, 0); If we're in a private browsing context, we should pass nsISocketProvider::NO_PERMANENT_STORAGE as the flags argument to isSecureURI. @@ +628,5 @@ > + info.hsts = sss.isSecureURI(sss.HEADER_HSTS, request.URI, 0); > + > + // Public Key Pinning; isSecureURI only supports HEADER_HSTS, use > + // isSecureHost instead for HPKP. > + info.hpkp = sss.isSecureHost(sss.HEADER_HPKP, request.URI.host, 0); Same here. @@ +631,5 @@ > + // isSecureHost instead for HPKP. > + info.hpkp = sss.isSecureHost(sss.HEADER_HPKP, request.URI.host, 0); > + } else { > + info.hsts = false; > + info.hpkh = false; typo: info.hpkp

Comment on attachment 8536188 [details] [diff] [review] netmonitor-per-request-tls-status-part-1.patch Review of attachment 8536188 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/webconsole/network-helper.js @@ +548,5 @@ > + * => .errorMessage is not available. > + * => state === "insecure" > + * > + * - request is HTTPS but it uses a weak cipher or old protocol, see > + * security/manager/ssl/src/nsNSSCallbacks.cpp:1267 For future source code archeologists, you should use a link to a specific revision here, as lines move around frequently. Something like: http://hg.mozilla.org/mozilla-central/annotate/0e441ff66c5e/security/manager/ssl/src/nsNSSCallbacks.cpp#l1233 (I think it already moved once since you wrote your patch!)

Thanks to everyone for the feedback. Here's a new version of the toolkit/ changes to address the feedback. Here's a try run with the latest patches: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fe0a8a380479 (In reply to Panos Astithas [:past] from comment #13) > Comment on attachment 8536188 [details] [diff] [review] > netmonitor-per-request-tls-status-part-1.patch > > Review of attachment 8536188 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks fine to me, apart from a couple of minor issues, thanks! Flagging > dkeeler for additional review on the security bits in network-helper.js and > the tests. > > ::: toolkit/devtools/webconsole/network-helper.js > @@ +584,5 @@ > > + break; > > + default: > > + info.protocolVersion = "Unknown"; > > + Cu.reportError("parseSecurityInfo: protocolVersion " + > > + SSLStatus.protocolVersion + " is unknown"); > > Since this is in toolkit, better avoid Cu.reportError() and use > DevToolsUtils.reportException() instead. Some embeddings may not provide > Cu.reportError(). Done. (In reply to David Keeler (:keeler) [use needinfo?] from comment #14) > Comment on attachment 8536188 [details] [diff] [review] > netmonitor-per-request-tls-status-part-1.patch > > Review of attachment 8536188 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is great! I just had a few comments to add. > > ::: toolkit/devtools/webconsole/network-helper.js > @@ +503,5 @@ > > + * - errorMessage: full error message from nsITransportSecurityInfo. > > + * If state == secure: > > + * - protocolVersion: one of SSLv3, TLSv1, TLSv1.1, TLSv1.2. > > + * - cipherSuite: the cipher suite used in this connection. > > + * - cert: information about certificate used in this connection > > It would be great to also have the base64-encoded DER bytes of the > certificate (use getRawDER) (and, actually, the entire chain the server > sent, but at the moment that's only available if the connection failed - see > nsITransportSecurityInfo.failedCertChain). It'd be great if it were possible transfer the entire certificate chain from server to client and show it in the standard "View Cert" window. However, these patches are already quite large as is so maybe this should be left as a follow up enchantment. > @@ +624,5 @@ > > + const sss = Cc["@mozilla.org/ssservice;1"] > > + .getService(Ci.nsISiteSecurityService); > > + > > + // Check Strict-Transport-Security status for this domain. > > + info.hsts = sss.isSecureURI(sss.HEADER_HSTS, request.URI, 0); > > If we're in a private browsing context, we should pass > nsISocketProvider::NO_PERMANENT_STORAGE as the flags argument to isSecureURI. Done. > @@ +631,5 @@ > > + // isSecureHost instead for HPKP. > > + info.hpkp = sss.isSecureHost(sss.HEADER_HPKP, request.URI.host, 0); > > + } else { > > + info.hsts = false; > > + info.hpkh = false; > > typo: info.hpkp Nice catch. Fixed and added an unit test to ensure the behavior. (In reply to J. Ryan Stinnett [:jryans] from comment #15) > Comment on attachment 8536188 [details] [diff] [review] > netmonitor-per-request-tls-status-part-1.patch > > Review of attachment 8536188 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/devtools/webconsole/network-helper.js > @@ +548,5 @@ > > + * => .errorMessage is not available. > > + * => state === "insecure" > > + * > > + * - request is HTTPS but it uses a weak cipher or old protocol, see > > + * security/manager/ssl/src/nsNSSCallbacks.cpp:1267 > > For future source code archeologists, you should use a link to a specific > revision here, as lines move around frequently. > > Something like: > > http://hg.mozilla.org/mozilla-central/annotate/0e441ff66c5e/security/manager/ > ssl/src/nsNSSCallbacks.cpp#l1233 > > (I think it already moved once since you wrote your patch!) Good point. Changed.

Here's an improved version of the server side code with following changes: - Split certificate and protocol version parsing into separate methods for better maintainability. - Created separate unit tests for the new methods and removed overlap with the other tests. - Minor cleanup of comments, newlines etc.

Here's a improved version for the client side code with following fixes: - Corrected the width of the domain field. - Made the security icon clickable after requests were sorted and added test for this.

Rebase.

I've been testing these patches during holidays and ironed out bugs that I have noticed so these should be ready for review. There's still few things that could be handled better: - Warnings. I have few ideas how to integrate them but currently they're handled poorly. - Full certificate chains can't be shown. I think it would be better to land the base first (maybe after next uplift) and follow up on those issues afterwards. This is the patch containing changes to the WebConsoleActor. Please add other reviewers for these patches if you can think of someone who should take a look at these too.

These are the UI changes needed to present the security information. One issue I have noticed with this is that the sidebar is too narrow to fit all the tabs causing the security tab to be hidden. I believe this is worked in bug 1101569 so it should help with the problem. Try run with both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13b977f27812

Thanks for the patch! I'm on holiday vacation now, so I'll probably look at this early next week.

Comment on attachment 8541900 [details] [diff] [review] netmonitor-security-info-part-2-ui.patch Review of attachment 8541900 [details] [diff] [review]: ----------------------------------------------------------------- This is pretty awesome. r+, but with a concern below. ::: browser/devtools/netmonitor/test/browser_net_html-preview.js @@ -22,5 @@ > "The first tab in the details pane should be selected."); > is($("#preview-tab").hidden, true, > "The preview tab should be hidden for non html responses."); > - is($("#preview-tabpanel").hidden, true, > - "The preview tabpanel should be hidden for non html responses."); Why are these checks removed? The 'preview' panel should still be hidden for non html responses.

(In reply to Sami Jaktholm from comment #11) > > If the #preview-{tabpanel,tab} is hidden and I choose security tab, the > security panel is blank. There's a note in MDN[1] which says tabpanels > should never be hidden and apparently there's a good reason for that. Maybe > the rendered panel is the nth visible tabpanel instead of nth tabpanel... > > [1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/tabpanels That may be the case, but the Preview panel should still be hidden on non HTML responses, at least for now. Maybe make the Security panel come before the Preview one, so you can still safely hide it?

(In reply to Victor Porof [:vporof][:vp] from comment #24) > That may be the case, but the Preview panel should still be hidden on non > HTML responses, at least for now. Maybe make the Security panel come before > the Preview one, so you can still safely hide it? That should work. Updated the patch and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b80397eeb298 Carrying r+ over but please check that I didn't miss anything if you got time.

Here's a version with r=past appended to the commit message ready to be checked in.

Here's a version of the UI patch with r=vporof appended to the commit message ready to be checked in.

Comment on attachment 8544516 [details] [diff] [review] netmonitor-security-info-part-1-actor-support.patch For situations like this when everything is landing at the same time, you can just use checkin-needed. The checkin? flag is a PITA for us since our automated bug marking tools can't clear it.

https://hg.mozilla.org/integration/fx-team/rev/7194852248c0 https://hg.mozilla.org/integration/fx-team/rev/5bc97468e511

Backed out for intermittent failures in browser_net_security-state.js. https://hg.mozilla.org/integration/fx-team/rev/046d6ecc36ba https://treeherder.mozilla.org/logviewer.html#?job_id=1604514&repo=fx-team

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #28) > For situations like this when everything is landing at the same time, you > can just use checkin-needed. The checkin? flag is a PITA for us since our > automated bug marking tools can't clear it. Good to know. Sorry about that. I was able to reproduce the failure locally and it seems to be caused by cross origin requests performed by the test(s). The test hangs while waiting event timings from the server. I was able to determine that NetworkMonitor.observeActivity occasionally returned early at [1]. This caused it to ignore ACTIVITY_SUBTYPE_TRANSACTION_CLOSE that was supposed to flush the event timings at [2]. This patch adds an .sjs file that returns correct "Access-Control-Allow-Origin: *" header and makes the tests use that file. I made the patch separate to ease review and it is built on top of netmonitor-security-info-part-2-ui.patch so they both need to be applied in correct order. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d010ce55c55d [1] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/network-monitor.js#610-612 [2] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/network-monitor.js#949-951

Let's try this again.

https://hg.mozilla.org/integration/fx-team/rev/648fe3c2fd57 https://hg.mozilla.org/integration/fx-team/rev/52db8d5bfea6 https://hg.mozilla.org/integration/fx-team/rev/321dfaa82c02

sorry failed again - https://treeherder.mozilla.org/logviewer.html#?job_id=1617472&repo=fx-team and so i had to back this out

Apparently the test domain, include-subdomains.pinning.example.com, was removed in by rev 4f547cd4ce3d (bug 1096197) [1] which landed after my try push. I'm not really sure why this has ever worked as the removed test domain should have served a certificate (cert=staticPinningBad) that violates the static pin causing the connection to fail. The actual pinning is tested in [2] and it spins up a per-case test server separate from the one serving domains mentioned in build/pgo/server-locations.txt which we can easily use. In order to test the security information with a mochitest we must have a domain without any security issues. That'll require a separate test server with correct cert and a static pin (as done in [2]). Another possibility would be to test static pinning with an unit test (if SiteSecurityService is available in xpcshell). It only requires a domain with a static pin and that exists (at least for now). However it's not exactly the same as using a mochitest with full debugger stack but it's still something. I'll check if unit test works. [1] https://hg.mozilla.org/mozilla-central/rev/4f547cd4ce3d#l6.46 [2] https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_pinning.js and https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/head_psm.js#241

Refresh part two that fails to apply due to bug 731318 being backed out.

Here's a patch that removes the static pin case from the failing mochitest and adds an unit test for that use case. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ce3751d9585

https://hg.mozilla.org/integration/fx-team/rev/c4d908fa4442 https://hg.mozilla.org/integration/fx-team/rev/7f0b79f1904f https://hg.mozilla.org/integration/fx-team/rev/14d1166ae92b https://hg.mozilla.org/integration/fx-team/rev/b7cb3b5da58d

Alright, let's see if it sticks this time!

https://hg.mozilla.org/mozilla-central/rev/c4d908fa4442 https://hg.mozilla.org/mozilla-central/rev/7f0b79f1904f https://hg.mozilla.org/mozilla-central/rev/14d1166ae92b https://hg.mozilla.org/mozilla-central/rev/b7cb3b5da58d

relnoted as "New Network Panel security pane"