Expose SSLInfo::pkp_bypassed to devtools

Expose SSLInfo::pkp_bypassed to devtools security panel.

This adds the necessary plumbing to expose in devtools when public-key pinning is bypassed (due to a local trust anchor), by way of SecurityStyleExplanations. The goal is to allow the security panel to display when pins are bypassed, in order to help developers debug pinning implementations.

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

[dadrian, 2016-06-15 20:16:02]

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

[dadrian, 2016-06-15 20:16:03]

estark: I'm throwing up this CL because I'm not sure these browser tests are
going through a full TLS handshake, which I think is the cause of
ChromeSecurityStateModelClientTest.PKPBypassed test case failing. Alternatively,
I have an actual bug, but it didn't look like a lot of the relevant TLS
functions had symbols in the browser_test binary.

[estark, 2016-06-15 20:36:48]

https://codereview.chromium.org/2066483009/diff/1/chrome/browser/ssl/chrome_s...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2066483009/diff/1/chrome/browser/ssl/chrome_s...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:589:
browser(), https_server_.GetURL("/ssl/fuck-youhpkp-headers.html"));
I'm building with this patch to try to see what's going on, but in the meantime,
it seems like you would have to do two navigations to get this to work. Once
where the connection conforms to the pins, so that TransportSecurityState
records the pins, and then another to violate the pins with a private root. If
the public key hashes don't match the Public-Key-Pins header, then the header
just gets ignored.

[dadrian, 2016-06-24 18:28:01]

On 2016/06/15 20:36:48, estark wrote:
>
https://codereview.chromium.org/2066483009/diff/1/chrome/browser/ssl/chrome_s...
> File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
> (right):
> 
>
https://codereview.chromium.org/2066483009/diff/1/chrome/browser/ssl/chrome_s...
> chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:589:
> browser(), https_server_.GetURL("/ssl/fuck-youhpkp-headers.html"));
> I'm building with this patch to try to see what's going on, but in the
meantime,
> it seems like you would have to do two navigations to get this to work. Once
> where the connection conforms to the pins, so that TransportSecurityState
> records the pins, and then another to violate the pins with a private root. If
> the public key hashes don't match the Public-Key-Pins header, then the header
> just gets ignored.

I converted it to inject the pins directly into TSS by way of URLRequestContext,
but still no luck. Any ideas?

[estark, 2016-06-24 20:31:11]

https://codereview.chromium.org/2066483009/diff/20001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2066483009/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:616:
security_state->AddHPKP(https_server_.host_port_pair().host(), expiration,
Aha, looks like host() is empty at this point because |https_server_| isn't
started yet. It works if you call https_server_.Start() in SetUpOnMainThread()
before SetUpOnIOThread() runs.

(I hate to admit this but I usually printf-debug stuff like this. The browser
test binary seems to take forever to load in gdb and I get impatient.)

[dadrian, 2016-06-24 21:19:56]

Alright, this should now be good to go!

https://codereview.chromium.org/2066483009/diff/20001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2066483009/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:616:
security_state->AddHPKP(https_server_.host_port_pair().host(), expiration,
On 2016/06/24 20:31:11, estark wrote:
> Aha, looks like host() is empty at this point because |https_server_| isn't
> started yet. It works if you call https_server_.Start() in SetUpOnMainThread()
> before SetUpOnIOThread() runs.
> 
> (I hate to admit this but I usually printf-debug stuff like this. The browser
> test binary seems to take forever to load in gdb and I get impatient.)

Nice catch! I usually only resort to gdb when things are super gnarly.
Unfortunately, in this case I had printed out the URL after the server started.

Sigh. Computers. http://i.imgur.com/c4jt321.png

[estark, 2016-06-27 17:38:27]

estark@chromium.org changed reviewers:
+ lgarron@chromium.org

[estark, 2016-06-27 17:38:28]

Looking good!

Adding lgarron to take a look at the devtools plumbing.

As an aside, it's kinda ridiculous that you had to add this bool to like 5
different structs/classes. :(

https://codereview.chromium.org/2066483009/diff/40001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2066483009/diff/40001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:600:
io_loop_finished_event_.Wait();
Huh, I've never seen this |io_loop_finished_event_| pattern before. Was there
somewhere you saw this as an example?

I'm not sure if it's necessary to do this. Did the test fail without it? I would
have thought that the SetUpOnIOThread() event is guaranteed to run before
NavigateToURL() completes, because NavigateToURL() waits for IO-thread events
that are posted after this.

https://codereview.chromium.org/2066483009/diff/40001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:670:
CheckBrokenSecurityStyle(observer, -150, browser());
What's the magic number? Should be a net error?

https://codereview.chromium.org/2066483009/diff/40001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:670:
CheckBrokenSecurityStyle(observer, -150, browser());
Would be good to also test that |pkp_bypassed| is not set on the
SecurityStyleExplanations.

https://codereview.chromium.org/2066483009/diff/40001/chrome/test/data/ssl/hp...
File chrome/test/data/ssl/hpkp-headers.html (right):

https://codereview.chromium.org/2066483009/diff/40001/chrome/test/data/ssl/hp...
chrome/test/data/ssl/hpkp-headers.html:1: This file is boring; all the action's
in the .mock-http-headers.
Doesn't look like you need this file (and the .mock-http-headers).

[dadrian, 2016-06-27 23:07:41]

https://codereview.chromium.org/2066483009/diff/40001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2066483009/diff/40001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:600:
io_loop_finished_event_.Wait();
On 2016/06/27 17:38:28, estark wrote:
> Huh, I've never seen this |io_loop_finished_event_| pattern before. Was there
> somewhere you saw this as an example?
> 
> I'm not sure if it's necessary to do this. Did the test fail without it? I
would
> have thought that the SetUpOnIOThread() event is guaranteed to run before
> NavigateToURL() completes, because NavigateToURL() waits for IO-thread events
> that are posted after this.

I don't remember what test it was, but I directly copied it from the test I
based this off of. I think you linked it to me in chat a while back? Regardless,
it definitely works without it, so I've removed it.

https://codereview.chromium.org/2066483009/diff/40001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:670:
CheckBrokenSecurityStyle(observer, -150, browser());
On 2016/06/27 17:38:28, estark wrote:
> What's the magic number? Should be a net error?

Yup, it's net::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN, fixed.

https://codereview.chromium.org/2066483009/diff/40001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:670:
CheckBrokenSecurityStyle(observer, -150, browser());
On 2016/06/27 17:38:28, estark wrote:
> Would be good to also test that |pkp_bypassed| is not set on the
> SecurityStyleExplanations.

Done.

https://codereview.chromium.org/2066483009/diff/40001/chrome/test/data/ssl/hp...
File chrome/test/data/ssl/hpkp-headers.html (right):

https://codereview.chromium.org/2066483009/diff/40001/chrome/test/data/ssl/hp...
chrome/test/data/ssl/hpkp-headers.html:1: This file is boring; all the action's
in the .mock-http-headers.
On 2016/06/27 17:38:28, estark wrote:
> Doesn't look like you need this file (and the .mock-http-headers).

Sigh. One of these days I'll remember to delete all my local debugging changes.
:/

[lgarron, 2016-06-28 00:27:42]

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

https://codereview.chromium.org/2066483009/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:899: { "name":
"pkpBypassed", "type": "boolean", "description": "True if pinning was bypassed
due to a local trust anchor", "optional": "true" }
Please make sure to match indentation. Looks good otherwise.

[estark, 2016-06-28 02:43:53]

lgtm with a few nits/cleanups

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:596: //
io_loop_finished_event_.Wait();
delete instead of commenting (same on line 616)

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:623:
IN_PROC_BROWSER_TEST_F(PKPModelClientTest, PKPBypass) {
There should probably also be a test (either in this test or another test) that
if you set up a SecurityStyleTestObserver, it observes a
SecurityStyleExplanations with |pkp_bypassed| set to true.

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:624: //
Need a local trust anchor
Belongs above line 627, I think (and make it a full sentence)

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:638: //
Page should be secure
unnecessary comment IMO

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:650: //
Need a local trust anchor
inaccurate, right? (this one's a known root)

https://codereview.chromium.org/2066483009/diff/60001/components/security_sta...
File components/security_state/security_state_model.cc (right):

https://codereview.chromium.org/2066483009/diff/60001/components/security_sta...
components/security_state/security_state_model.cc:268: ran_mixed_content(false)
{}
initialize |pkp_bypassed| here, and also check it in operator== below

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

https://codereview.chromium.org/2066483009/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:899: { "name":
"pkpBypassed", "type": "boolean", "description": "True if pinning was bypassed
due to a local trust anchor", "optional": "true" }
nit: I have a slight preference for "public key pinning" instead of "pinning"
(more googleable if something doesn't know what it is)

[estark, 2016-06-28 02:48:08]

Oh, sorry, one more request: can you add a bit more detail to the CL
description? When you add devtools or content owners, they'll probably
appreciate some context.

I'd ordinarily suggest pfeldman@ but he's OOO, so good reviewers might be
dgozman@ for devtools and avi@ for the rest of //content.

[dadrian, 2016-06-28 18:16:44]

Description was changed from

==========
Expose SSLInfo::pkp_bypassed to devtools

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

to

==========
Expose SSLInfo::pkp_bypassed to devtools security panel.

This adds the necessary plumbing to expose in devtools when public-key pinning
is bypassed (due to a local trust anchor), by way of SecurityStyleExplanations.
The goal is to allow the security panel to display when pins are bypassed, in
order to help developers debug pinning implementations.

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

[dadrian, 2016-06-28 18:45:57]

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

[dadrian, 2016-06-28 18:45:58]

Addressed nits and added reviewers for the remaining files.

avi@chromium.org: Please review changes in //content.

dgozman@chromium.org: Please review changes in browser_protocol.json.

Thanks!

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:596: //
io_loop_finished_event_.Wait();
On 2016/06/28 02:43:52, estark wrote:
> delete instead of commenting (same on line 616)

Done.

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:623:
IN_PROC_BROWSER_TEST_F(PKPModelClientTest, PKPBypass) {
On 2016/06/28 02:43:52, estark wrote:
> There should probably also be a test (either in this test or another test)
that
> if you set up a SecurityStyleTestObserver, it observes a
> SecurityStyleExplanations with |pkp_bypassed| set to true.

Done.

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:624: //
Need a local trust anchor
On 2016/06/28 02:43:52, estark wrote:
> Belongs above line 627, I think (and make it a full sentence)

Done.

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:638: //
Page should be secure
On 2016/06/28 02:43:52, estark wrote:
> unnecessary comment IMO

Done.

https://codereview.chromium.org/2066483009/diff/60001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:650: //
Need a local trust anchor
On 2016/06/28 02:43:52, estark wrote:
> inaccurate, right? (this one's a known root)

Done.

https://codereview.chromium.org/2066483009/diff/60001/components/security_sta...
File components/security_state/security_state_model.cc (right):

https://codereview.chromium.org/2066483009/diff/60001/components/security_sta...
components/security_state/security_state_model.cc:268: ran_mixed_content(false)
{}
On 2016/06/28 02:43:52, estark wrote:
> initialize |pkp_bypassed| here, and also check it in operator== below

Done.

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

https://codereview.chromium.org/2066483009/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:899: { "name":
"pkpBypassed", "type": "boolean", "description": "True if pinning was bypassed
due to a local trust anchor", "optional": "true" }
On 2016/06/28 00:27:41, lgarron wrote:
> Please make sure to match indentation. Looks good otherwise.

Done.

[dgozman, 2016-06-28 19:02:40]

https://codereview.chromium.org/2066483009/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2066483009/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:899: { "name":
"pkpBypassed", "type": "boolean", "description": "True if public key pinning was
bypassed due to a local trust anchor", "optional": "true" }
Please add a period at the end of description.

https://codereview.chromium.org/2066483009/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:899: { "name":
"pkpBypassed", "type": "boolean", "description": "True if public key pinning was
bypassed due to a local trust anchor", "optional": "true" }
We prefer to add things to protocol together with usage on front-end side and
inspector layout test. I suggest you to not expose this in protocol yet (in this
patch), and instead expose it in the next patch where you will actually use it.

[Avi (use Gerrit), 2016-06-28 19:12:06]

content lgtm

[dadrian, 2016-06-28 21:17:28]

The CQ bit was checked by dadrian@google.com

[dadrian, 2016-06-28 21:17:29]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2066483009/#ps120001
(title: "Remove change to browser_protocol.json")

[commit-bot: I haz the power, 2016-06-28 21:18:59]

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-06-28 23:07:47]

Description was changed from

==========
Expose SSLInfo::pkp_bypassed to devtools security panel.

This adds the necessary plumbing to expose in devtools when public-key pinning
is bypassed (due to a local trust anchor), by way of SecurityStyleExplanations.
The goal is to allow the security panel to display when pins are bypassed, in
order to help developers debug pinning implementations.

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

to

==========
Expose SSLInfo::pkp_bypassed to devtools security panel.

This adds the necessary plumbing to expose in devtools when public-key pinning
is bypassed (due to a local trust anchor), by way of SecurityStyleExplanations.
The goal is to allow the security panel to display when pins are bypassed, in
order to help developers debug pinning implementations.

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

[commit-bot: I haz the power, 2016-06-28 23:07:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-28 23:09:14]

Description was changed from

==========
Expose SSLInfo::pkp_bypassed to devtools security panel.

This adds the necessary plumbing to expose in devtools when public-key pinning
is bypassed (due to a local trust anchor), by way of SecurityStyleExplanations.
The goal is to allow the security panel to display when pins are bypassed, in
order to help developers debug pinning implementations.

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

to

==========
Expose SSLInfo::pkp_bypassed to devtools security panel.

This adds the necessary plumbing to expose in devtools when public-key pinning
is bypassed (due to a local trust anchor), by way of SecurityStyleExplanations.
The goal is to allow the security panel to display when pins are bypassed, in
order to help developers debug pinning implementations.

BUG=566144

Committed: https://crrev.com/bfa0a24b4ff377d337a704d01db5d5957b6eafa4
Cr-Commit-Position: refs/heads/master@{#402586}
==========

[commit-bot: I haz the power, 2016-06-28 23:09:16]

Patchset 7 (id:??) landed as
https://crrev.com/bfa0a24b4ff377d337a704d01db5d5957b6eafa4
Cr-Commit-Position: refs/heads/master@{#402586}