Display when PKP is bypassed in devtools

Display when PKP is bypassed in devtools

This adds a security explanation to devtools for when public-key pinning is bypassed due to a local trust anchor. It does not affect the security state of the page, and enforces that "Info" explanations are shown last.

BUG=566144
Committed: https://crrev.com/727e23f969ad3c54184efd67c6fae4bc7935b72a
Cr-Commit-Position: refs/heads/master@{#404253}

[dadrian, 2016-07-01 00:12:59]

dadrian@google.com changed reviewers:
+ estark@chromium.org, lgarron@chromium.org

[dadrian, 2016-07-01 00:13:00]

From what I can tell, this works? My testing was limited, anyone got a Superfish
box lying around? :)

Adding estark and lgarron to tell me all the ways I've done Javascript wrong.

[estark, 2016-07-01 01:02:43]

devtools js style is not my forte but lgtm from what I can tell. Oh, could you
attach a screenshot of what it looks like to the bug?

https://codereview.chromium.org/2118583003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-pkp-bypassed-expected.txt
(right):

https://codereview.chromium.org/2118583003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-pkp-bypassed-expected.txt:1:
Tests addition of explanation when all page resources are transferred securely
(no-mixed-content).
Looks like this description needs to be updated. (Should say something about the
pkp explanation showing up.)

[lgarron, 2016-07-01 01:33:40]

LGTM with nit.

https://codereview.chromium.org/2118583003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2118583003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:619: var
pkpBypassedExplanation = WebInspector.UIString("Public-key pinning was bypassed
by a local root certificate.");
Unused? (It seems to be hardcoded separately below. We should make sure stuff is
DRY.)

[dadrian, 2016-07-01 01:58:44]

dadrian@google.com changed reviewers:
+ dgozman@chromium.org

[dadrian, 2016-07-01 01:58:45]

dgozman@chromium.org: We're back with the rest of the devtools changes from
Tuesday! Looking for review on the remaining files.

https://codereview.chromium.org/2118583003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-pkp-bypassed-expected.txt
(right):

https://codereview.chromium.org/2118583003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-pkp-bypassed-expected.txt:1:
Tests addition of explanation when all page resources are transferred securely
(no-mixed-content).
On 2016/07/01 01:02:43, estark wrote:
> Looks like this description needs to be updated. (Should say something about
the
> pkp explanation showing up.)

Done.

https://codereview.chromium.org/2118583003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2118583003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:619: var
pkpBypassedExplanation = WebInspector.UIString("Public-key pinning was bypassed
by a local root certificate.");
On 2016/07/01 01:33:39, lgarron wrote:
> Unused? (It seems to be hardcoded separately below. We should make sure stuff
is
> DRY.)

Whoops, Done.

[dgozman, 2016-07-05 17:37:21]

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:825: { "name":
"mixedContentStatus", "$ref": "MixedContentStatus", "description": "Information
about mixed content on the page.", "optional": true },
Why all of these are optional, if we set them in security_handler
unconditionally? Let's remove optional from mixedContentStatus,
schemeIsCrypthographic and pkpBypassed.

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:827: { "name":
"pkpBypassed", "type": "boolean", "description": "True if public-key pinning was
bypassed due to a local trust anochor.", "optional": true }
typo: anochor

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js
(right):

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js:79:
WebInspector.PageSecurityState = function(securityState, explanations,
mixedContentStatus, schemeIsCryptographic, pkpBypassed) {
style: while you are here, please move { to the next line

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:647:
this._addExplanation(/** @type {!SecurityAgent.SecurityStateExplanation} */ ({
I wonder why we send a separate flag to frontend instead of generating an
explanation on the backend?

[dadrian, 2016-07-06 18:35:46]

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:825: { "name":
"mixedContentStatus", "$ref": "MixedContentStatus", "description": "Information
about mixed content on the page.", "optional": true },
On 2016/07/05 17:37:20, dgozman wrote:
> Why all of these are optional, if we set them in security_handler
> unconditionally? Let's remove optional from mixedContentStatus,
> schemeIsCrypthographic and pkpBypassed.

Done.

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:827: { "name":
"pkpBypassed", "type": "boolean", "description": "True if public-key pinning was
bypassed due to a local trust anochor.", "optional": true }
On 2016/07/05 17:37:20, dgozman wrote:
> typo: anochor

Done.

I had to reorder the parameters to put the optionals at the end.

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js
(right):

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js:79:
WebInspector.PageSecurityState = function(securityState, explanations,
mixedContentStatus, schemeIsCryptographic, pkpBypassed) {
On 2016/07/05 17:37:20, dgozman wrote:
> style: while you are here, please move { to the next line

Done.

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:647:
this._addExplanation(/** @type {!SecurityAgent.SecurityStateExplanation} */ ({
On 2016/07/05 17:37:20, dgozman wrote:
> I wonder why we send a separate flag to frontend instead of generating an
> explanation on the backend?

I was matching mixed content status. PKP bypass is a touchy issue in terms of
whether or not it should affect security state, so here we're basically just
leveraging the UI framework, rather than adding a new security state.

[dgozman, 2016-07-06 19:01:35]

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:647:
this._addExplanation(/** @type {!SecurityAgent.SecurityStateExplanation} */ ({
On 2016/07/06 18:35:46, dadrian wrote:
> On 2016/07/05 17:37:20, dgozman wrote:
> > I wonder why we send a separate flag to frontend instead of generating an
> > explanation on the backend?
> 
> I was matching mixed content status. PKP bypass is a touchy issue in terms of
> whether or not it should affect security state, so here we're basically just
> leveraging the UI framework, rather than adding a new security state. 

Do you mean we are going to do something more advanced with pkp bypass flag
later? Because I don't see anything specific in this patch yet. Am I missing
something?

My reasoning is the smaller the protocol the easier, unless we need extra
details for specific logic.

https://codereview.chromium.org/2118583003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js
(right):

https://codereview.chromium.org/2118583003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js:101: *
@param {?SecurityAgent.MixedContentStatus=} mixedContentStatus
Remove optional modifier here as well.

https://codereview.chromium.org/2118583003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js:108: var
pageSecurityState = new WebInspector.PageSecurityState(securityState,
mixedContentStatus || null, schemeIsCryptographic || false, pkpBypassed ||
false, explanations || []);
Remove:
|| null
|| false

[lgarron, 2016-07-06 22:19:43]

https://codereview.chromium.org/2118583003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2118583003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:649:
"summary": WebInspector.UIString("Public-Key Pinning"),
Could you change this to "Public-Key Pinning Bypassed"?

The screenshot made me realize the summary implies that PKP is active, but we
only show it when it is *not* active. I'd rather have us be as clear as
possible.

[dadrian, 2016-07-07 00:59:06]

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2118583003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:647:
this._addExplanation(/** @type {!SecurityAgent.SecurityStateExplanation} */ ({
On 2016/07/06 19:01:34, dgozman wrote:
> On 2016/07/06 18:35:46, dadrian wrote:
> > On 2016/07/05 17:37:20, dgozman wrote:
> > > I wonder why we send a separate flag to frontend instead of generating an
> > > explanation on the backend?
> > 
> > I was matching mixed content status. PKP bypass is a touchy issue in terms
of
> > whether or not it should affect security state, so here we're basically just
> > leveraging the UI framework, rather than adding a new security state. 
> 
> Do you mean we are going to do something more advanced with pkp bypass flag
> later? Because I don't see anything specific in this patch yet. Am I missing
> something?
> 
> My reasoning is the smaller the protocol the easier, unless we need extra
> details for specific logic.

Moved to SecurityStyleExplanation.

https://codereview.chromium.org/2118583003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2118583003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:649:
"summary": WebInspector.UIString("Public-Key Pinning"),
On 2016/07/06 22:19:43, lgarron wrote:
> Could you change this to "Public-Key Pinning Bypassed"?
> 
> The screenshot made me realize the summary implies that PKP is active, but we
> only show it when it is *not* active. I'd rather have us be as clear as
> possible.

Done.

[dgozman, 2016-07-07 01:55:48]

lgtm from devtools side.
@lgarron: will you take another look from security side?

https://codereview.chromium.org/2118583003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure.html
(right):

https://codereview.chromium.org/2118583003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure.html:10:
InspectorTest.mainTarget.model(WebInspector.SecurityModel).dispatchEventToListeners(WebInspector.SecurityModel.EventTypes.SecurityStateChanged,
new WebInspector.PageSecurityState(SecurityAgent.SecurityState.Secure, [],
mixedContentStatus, true, false));
Left-over from previous patchset?

https://codereview.chromium.org/2118583003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2118583003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:637: if
(explanation.securityState == SecurityAgent.SecurityState.Info) {
style: ===

[dadrian, 2016-07-07 01:59:48]

https://codereview.chromium.org/2118583003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure.html
(right):

https://codereview.chromium.org/2118583003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/security/security-all-resources-secure.html:10:
InspectorTest.mainTarget.model(WebInspector.SecurityModel).dispatchEventToListeners(WebInspector.SecurityModel.EventTypes.SecurityStateChanged,
new WebInspector.PageSecurityState(SecurityAgent.SecurityState.Secure, [],
mixedContentStatus, true, false));
On 2016/07/07 01:55:47, dgozman wrote:
> Left-over from previous patchset?

Yup. Done.

https://codereview.chromium.org/2118583003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2118583003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:637: if
(explanation.securityState == SecurityAgent.SecurityState.Info) {
On 2016/07/07 01:55:47, dgozman wrote:
> style: ===

Done.

[dadrian, 2016-07-07 16:47:59]

dadrian@google.com changed reviewers:
+ creis@chromium.org

[dadrian, 2016-07-07 16:48:00]

creis@chromium.org: Can you approve the one line change in
security_style_explanations.h?

[Charlie Reis, 2016-07-07 17:06:32]

content/public LGTM with nits.

https://codereview.chromium.org/2118583003/diff/80001/content/public/browser/...
File content/public/browser/security_style_explanations.h (right):

https://codereview.chromium.org/2118583003/diff/80001/content/public/browser/...
content/public/browser/security_style_explanations.h:26: // different
explanations of "secure", "warning", and "broken" severity
Please list "info" here as well.

https://codereview.chromium.org/2118583003/diff/80001/content/public/browser/...
content/public/browser/security_style_explanations.h:52:
std::vector<SecurityStyleExplanation> secure_explanations;
Can you put a comment on this block saying something about how the embedder
should handle these vectors?

[dadrian, 2016-07-07 17:33:03]

https://codereview.chromium.org/2118583003/diff/80001/content/public/browser/...
File content/public/browser/security_style_explanations.h (right):

https://codereview.chromium.org/2118583003/diff/80001/content/public/browser/...
content/public/browser/security_style_explanations.h:26: // different
explanations of "secure", "warning", and "broken" severity
On 2016/07/07 17:06:32, Charlie Reis wrote:
> Please list "info" here as well.

Done.

https://codereview.chromium.org/2118583003/diff/80001/content/public/browser/...
content/public/browser/security_style_explanations.h:52:
std::vector<SecurityStyleExplanation> secure_explanations;
On 2016/07/07 17:06:32, Charlie Reis wrote:
> Can you put a comment on this block saying something about how the embedder
> should handle these vectors?

Done.

[dadrian, 2016-07-07 17:34:01]

Description was changed from

==========
Display when PKP is bypassed in devtools

This adds a security explanation to devtools for when public-key pinning is
bypassed due to a local trust anchor. It does not affect the security state of
the page.

BUG=566144
==========

to

==========
Display when PKP is bypassed in devtools

This adds a security explanation to devtools for when public-key pinning is
bypassed due to a local trust anchor. It does not affect the security state of
the page, and enforces that "Info" explanations are shown last.

BUG=566144
==========

[lgarron, 2016-07-07 19:46:50]

LGTM, since the new DevTools changes are ones I asked dadrian@ to patch in. :-)

[dadrian, 2016-07-07 19:47:19]

The CQ bit was checked by dadrian@google.com

[dadrian, 2016-07-07 19:47:19]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
dgozman@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2118583003/#ps100001
(title: "Add comments")

[commit-bot: I haz the power, 2016-07-07 19:47:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 22:18:06]

Description was changed from

==========
Display when PKP is bypassed in devtools

This adds a security explanation to devtools for when public-key pinning is
bypassed due to a local trust anchor. It does not affect the security state of
the page, and enforces that "Info" explanations are shown last.

BUG=566144
==========

to

==========
Display when PKP is bypassed in devtools

This adds a security explanation to devtools for when public-key pinning is
bypassed due to a local trust anchor. It does not affect the security state of
the page, and enforces that "Info" explanations are shown last.

BUG=566144
==========

[commit-bot: I haz the power, 2016-07-07 22:18:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-07 22:18:38]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 22:19:35]

Description was changed from

==========
Display when PKP is bypassed in devtools

This adds a security explanation to devtools for when public-key pinning is
bypassed due to a local trust anchor. It does not affect the security state of
the page, and enforces that "Info" explanations are shown last.

BUG=566144
==========

to

==========
Display when PKP is bypassed in devtools

This adds a security explanation to devtools for when public-key pinning is
bypassed due to a local trust anchor. It does not affect the security state of
the page, and enforces that "Info" explanations are shown last.

BUG=566144

Committed: https://crrev.com/727e23f969ad3c54184efd67c6fae4bc7935b72a
Cr-Commit-Position: refs/heads/master@{#404253}
==========

[commit-bot: I haz the power, 2016-07-07 22:19:37]

Patchset 6 (id:??) landed as
https://crrev.com/727e23f969ad3c54184efd67c6fae4bc7935b72a
Cr-Commit-Position: refs/heads/master@{#404253}