Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(149)

Issue 2016143002: Expose when PKP is bypassed in SSLInfo. (Closed)

Created:
4 years, 7 months ago by dadrian
Modified:
4 years, 6 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add |pkp_bypassed| to SSLInfo. This is the first step in exposing in devtools when pinning is bypassed due to a local trust anchor. To correctly expose this for all socket types, |pkp_bypassed| is also exposed in a CertificateVerifyResult, and set by each socket after calling TransportSecurityState::CheckPublicKeyPins. This also updates HttpResponseInfo to serialize the new field correctly. BUG=566144 Committed: https://crrev.com/df302c4d9de0bcd65378ecd44a4e3ec1199dea4c Cr-Commit-Position: refs/heads/master@{#399242}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Add test #

Total comments: 2

Patch Set 3 : Implement for quic #

Patch Set 4 : Add pkp_bypassed to SSLInfo #

Total comments: 18

Patch Set 5 : Address comments. Add periods. #

Total comments: 2

Patch Set 6 : Maintain old SPDY semantics #

Total comments: 2

Patch Set 7 : optional nits #

Total comments: 12

Patch Set 8 : Address comments from sleevi@ #

Total comments: 3

Patch Set 9 : Make CertVerifyResult Great Again. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+321 lines, -22 lines) Patch
M net/http/http_response_info.cc View 1 2 3 4 3 chunks +8 lines, -0 lines 0 comments Download
M net/http/http_response_info_unittest.cc View 1 2 3 4 1 chunk +14 lines, -0 lines 0 comments Download
M net/http/transport_security_state.h View 1 2 3 4 5 6 7 3 chunks +4 lines, -1 line 0 comments Download
M net/http/transport_security_state.cc View 1 2 3 4 5 6 7 6 chunks +19 lines, -18 lines 0 comments Download
M net/http/transport_security_state_unittest.cc View 1 2 3 4 5 1 chunk +18 lines, -0 lines 0 comments Download
M net/quic/crypto/proof_verifier_chromium.h View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -0 lines 2 comments Download
M net/quic/crypto/proof_verifier_chromium.cc View 1 2 3 4 5 6 7 8 2 chunks +12 lines, -1 line 0 comments Download
M net/quic/crypto/proof_verifier_chromium_test.cc View 1 2 3 4 5 6 7 8 1 chunk +85 lines, -0 lines 0 comments Download
M net/quic/quic_chromium_client_session.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M net/quic/quic_chromium_client_session.cc View 1 2 3 4 5 6 7 8 3 chunks +3 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_impl.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_impl.cc View 1 2 3 4 5 6 7 8 3 chunks +6 lines, -1 line 0 comments Download
M net/socket/ssl_client_socket_unittest.cc View 1 2 3 4 5 6 7 1 chunk +76 lines, -0 lines 0 comments Download
M net/spdy/spdy_session.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M net/ssl/ssl_info.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M net/ssl/ssl_info.cc View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +58 lines, -0 lines 0 comments Download

Messages

Total messages: 57 (10 generated)
dadrian
This should add CERT_STATUS_PKP_BYPASS and set it whenever pinning is bypassed due to a local ...
4 years, 7 months ago (2016-05-26 23:31:46 UTC) #3
svaldez
Sadly is_issued_by_known_roots continues to be full of lies, so the metrics won't be exact, but ...
4 years, 6 months ago (2016-05-27 18:27:45 UTC) #4
dadrian
https://codereview.chromium.org/2016143002/diff/1/net/socket/ssl_client_socket_unittest.cc File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/1/net/socket/ssl_client_socket_unittest.cc#newcode3306 net/socket/ssl_client_socket_unittest.cc:3306: TEST_F(SSLClientSocketTest, CertStatusPKPBypassed) { On 2016/05/27 18:27:44, svaldez wrote: > ...
4 years, 6 months ago (2016-05-27 22:03:16 UTC) #5
estark
I haven't done a full review yet, but sending one quick thing I noticed. https://codereview.chromium.org/2016143002/diff/20001/net/socket/ssl_client_socket_impl.cc ...
4 years, 6 months ago (2016-05-31 14:59:57 UTC) #6
dadrian
https://codereview.chromium.org/2016143002/diff/20001/net/socket/ssl_client_socket_impl.cc File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2016143002/diff/20001/net/socket/ssl_client_socket_impl.cc#newcode1359 net/socket/ssl_client_socket_impl.cc:1359: if (server_cert_verify_result_.is_issued_by_known_root) On 2016/05/31 14:59:57, estark wrote: > I ...
4 years, 6 months ago (2016-05-31 18:58:31 UTC) #7
svaldez
On 2016/05/31 18:58:31, dadrian wrote: > https://codereview.chromium.org/2016143002/diff/20001/net/socket/ssl_client_socket_impl.cc > File net/socket/ssl_client_socket_impl.cc (right): > > https://codereview.chromium.org/2016143002/diff/20001/net/socket/ssl_client_socket_impl.cc#newcode1359 > ...
4 years, 6 months ago (2016-05-31 19:05:06 UTC) #8
dadrian
Adding davidben@chromium.org: Before I go ahead and implement the required changes for SPDY, we're curious ...
4 years, 6 months ago (2016-06-01 00:18:03 UTC) #10
estark
On 2016/06/01 00:18:03, dadrian wrote: > Adding mailto:davidben@chromium.org: Before I go ahead and implement the ...
4 years, 6 months ago (2016-06-03 16:29:42 UTC) #11
davidben
Having a CertStatus flag for something PKP related (a higher level) seems kinda wonky. I ...
4 years, 6 months ago (2016-06-03 16:38:18 UTC) #12
estark
Chatted with davidben out-of-band. Sounds like he's okay with the cert status plan given that: ...
4 years, 6 months ago (2016-06-03 17:19:33 UTC) #13
dadrian
On 2016/06/03 17:19:33, estark wrote: > Chatted with davidben out-of-band. Sounds like he's okay with ...
4 years, 6 months ago (2016-06-03 17:36:56 UTC) #14
davidben
On 2016/06/03 17:19:33, estark wrote: > Chatted with davidben out-of-band. Sounds like he's okay with ...
4 years, 6 months ago (2016-06-03 17:37:52 UTC) #15
estark
On 2016/06/03 17:37:52, davidben (OOO until 5-31) wrote: > On 2016/06/03 17:19:33, estark wrote: > ...
4 years, 6 months ago (2016-06-03 17:58:11 UTC) #16
dadrian
On 2016/06/03 17:58:11, estark wrote: > On 2016/06/03 17:37:52, davidben (OOO until 5-31) wrote: > ...
4 years, 6 months ago (2016-06-04 00:37:43 UTC) #17
dadrian
On 2016/06/03 17:58:11, estark wrote: > On 2016/06/03 17:37:52, davidben (OOO until 5-31) wrote: > ...
4 years, 6 months ago (2016-06-04 00:37:44 UTC) #18
dadrian
On 2016/06/03 17:58:11, estark wrote: > On 2016/06/03 17:37:52, davidben (OOO until 5-31) wrote: > ...
4 years, 6 months ago (2016-06-04 00:37:44 UTC) #19
dadrian
On 2016/06/04 00:37:44, dadrian wrote: > On 2016/06/03 17:58:11, estark wrote: > > On 2016/06/03 ...
4 years, 6 months ago (2016-06-06 22:14:13 UTC) #20
estark
Thanks for doing another pass! I mostly just have nits. Also, can you update the ...
4 years, 6 months ago (2016-06-07 04:10:43 UTC) #21
svaldez
https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_verifier_chromium_test.cc File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_verifier_chromium_test.cc#newcode458 net/quic/crypto/proof_verifier_chromium_test.cc:458: // Test CERT_STATUS_PKP_BYPASSED is set when PKP is bypassed ...
4 years, 6 months ago (2016-06-07 14:12:48 UTC) #22
dadrian
I apparently internalized the opposite rule about periods. https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_info.cc File net/http/http_response_info.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_info.cc#newcode100 net/http/http_response_info.cc:100: // ...
4 years, 6 months ago (2016-06-07 17:48:23 UTC) #25
estark
Oh shoot, we still have to do something about the SPDY call at https://cs.chromium.org/chromium/src/net/spdy/spdy_session.cc?sq=package:chromium&l=663&rcl=1464678978, right? ...
4 years, 6 months ago (2016-06-07 18:38:32 UTC) #26
dadrian
On 2016/06/07 18:38:32, estark wrote: > Oh shoot, we still have to do something about ...
4 years, 6 months ago (2016-06-07 18:42:23 UTC) #27
estark
On 2016/06/07 18:42:23, dadrian wrote: > On 2016/06/07 18:38:32, estark wrote: > > Oh shoot, ...
4 years, 6 months ago (2016-06-07 18:47:29 UTC) #28
svaldez
On 2016/06/07 18:47:29, estark wrote: > On 2016/06/07 18:42:23, dadrian wrote: > > On 2016/06/07 ...
4 years, 6 months ago (2016-06-07 19:14:52 UTC) #29
dadrian
On 2016/06/07 19:14:52, svaldez wrote: > On 2016/06/07 18:47:29, estark wrote: > > On 2016/06/07 ...
4 years, 6 months ago (2016-06-07 21:07:40 UTC) #30
dadrian
https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_security_state_unittest.cc File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_security_state_unittest.cc#newcode1295 net/http/transport_security_state_unittest.cc:1295: // non-public root On 2016/06/07 18:38:32, estark wrote: > ...
4 years, 6 months ago (2016-06-07 21:07:53 UTC) #31
estark
lgtm davidben, svaldez, could you take another look when you have a chance? https://codereview.chromium.org/2016143002/diff/100001/net/spdy/spdy_session.cc File ...
4 years, 6 months ago (2016-06-08 17:09:29 UTC) #32
dadrian
https://codereview.chromium.org/2016143002/diff/100001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2016143002/diff/100001/net/spdy/spdy_session.cc#newcode666 net/spdy/spdy_session.cc:666: if (ssl_info.is_issued_by_known_root) On 2016/06/08 17:09:28, estark wrote: > optional ...
4 years, 6 months ago (2016-06-08 17:38:51 UTC) #33
svaldez
On 2016/06/08 17:38:51, dadrian wrote: > https://codereview.chromium.org/2016143002/diff/100001/net/spdy/spdy_session.cc > File net/spdy/spdy_session.cc (right): > > https://codereview.chromium.org/2016143002/diff/100001/net/spdy/spdy_session.cc#newcode666 > ...
4 years, 6 months ago (2016-06-09 14:38:47 UTC) #34
davidben
https://codereview.chromium.org/2016143002/diff/120001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2016143002/diff/120001/net/spdy/spdy_session.cc#newcode665 net/spdy/spdy_session.cc:665: if (!transport_security_state->CheckPublicKeyPins( I think it'd be better for this ...
4 years, 6 months ago (2016-06-09 15:56:32 UTC) #35
davidben
https://codereview.chromium.org/2016143002/diff/120001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2016143002/diff/120001/net/spdy/spdy_session.cc#newcode665 net/spdy/spdy_session.cc:665: if (!transport_security_state->CheckPublicKeyPins( On 2016/06/09 15:56:32, davidben wrote: > I ...
4 years, 6 months ago (2016-06-09 15:58:05 UTC) #36
dadrian
davidben: Do you want to change to an enum in this CL, or are you ...
4 years, 6 months ago (2016-06-09 16:39:40 UTC) #37
davidben
On 2016/06/09 16:39:40, dadrian wrote: > davidben: Do you want to change to an enum ...
4 years, 6 months ago (2016-06-09 18:27:54 UTC) #38
Ryan Sleevi
https://codereview.chromium.org/2016143002/diff/120001/net/cert/cert_verify_result.h File net/cert/cert_verify_result.h (right): https://codereview.chromium.org/2016143002/diff/120001/net/cert/cert_verify_result.h#newcode74 net/cert/cert_verify_result.h:74: bool pkp_bypassed; I believe this is a layering violation ...
4 years, 6 months ago (2016-06-09 19:17:32 UTC) #40
davidben
https://codereview.chromium.org/2016143002/diff/120001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2016143002/diff/120001/net/spdy/spdy_session.cc#newcode659 net/spdy/spdy_session.cc:659: return true; On 2016/06/09 19:17:32, Ryan Sleevi wrote: > ...
4 years, 6 months ago (2016-06-09 19:19:15 UTC) #41
Ryan Sleevi
On 2016/06/09 19:19:15, davidben wrote: > I believe this aligns with the old behavior, but ...
4 years, 6 months ago (2016-06-09 19:25:29 UTC) #42
estark
https://codereview.chromium.org/2016143002/diff/120001/net/cert/cert_verify_result.h File net/cert/cert_verify_result.h (right): https://codereview.chromium.org/2016143002/diff/120001/net/cert/cert_verify_result.h#newcode74 net/cert/cert_verify_result.h:74: bool pkp_bypassed; On 2016/06/09 19:17:32, Ryan Sleevi wrote: > ...
4 years, 6 months ago (2016-06-09 19:26:41 UTC) #43
Ryan Sleevi
On 2016/06/09 19:26:41, estark wrote: > That's only sort of true. The CertVerifier carries a ...
4 years, 6 months ago (2016-06-09 19:33:58 UTC) #44
dadrian
rsleevi@: So are we good leaving the modification of CertVerifier in this CL, and fixing ...
4 years, 6 months ago (2016-06-09 21:58:57 UTC) #45
Ryan Sleevi
> rsleevi@: So are we good leaving the modification of CertVerifier in this CL, > ...
4 years, 6 months ago (2016-06-09 22:09:05 UTC) #46
dadrian
|pkp_bypassed| is no longer in CertVerifyResult.
4 years, 6 months ago (2016-06-09 23:27:05 UTC) #47
Ryan Sleevi
I'm happy to defer on the enum handling, although I'd love to see it cleaned ...
4 years, 6 months ago (2016-06-09 23:32:22 UTC) #48
davidben
lgtm. (+1 to Ryan's comment below. We have a lot of code that predates it, ...
4 years, 6 months ago (2016-06-10 17:24:23 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016143002/160001
4 years, 6 months ago (2016-06-10 17:34:25 UTC) #52
commit-bot: I haz the power
Committed patchset #9 (id:160001)
4 years, 6 months ago (2016-06-10 18:49:13 UTC) #54
commit-bot: I haz the power
CQ bit was unchecked
4 years, 6 months ago (2016-06-10 18:49:16 UTC) #55
commit-bot: I haz the power
4 years, 6 months ago (2016-06-10 18:51:21 UTC) #57
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/df302c4d9de0bcd65378ecd44a4e3ec1199dea4c
Cr-Commit-Position: refs/heads/master@{#399242}

Powered by Google App Engine
This is Rietveld 408576698