|
|
DescriptionAdd |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
Messages
Total messages: 57 (10 generated)
Description was changed from ========== Add CERT_STATUS_PKP_BYPASS This is the first step in exposing in devtools when pinning is bypassed due to a local trust anchor. BUG=566144 ========== to ========== Add CERT_STATUS_PKP_BYPASS This is the first step in exposing in devtools when pinning is bypassed due to a local trust anchor. BUG=566144 ==========
dadrian@google.com changed reviewers: + estark@chromium.org, svaldez@chromium.org
This should add CERT_STATUS_PKP_BYPASS and set it whenever pinning is bypassed due to a local trust anchor. Adding estark@ and svaldez@ for initial review.
Sadly is_issued_by_known_roots continues to be full of lies, so the metrics won't be exact, but I don't think we get any worse results with this change. In general we should probably be using EmbeddedTestServer for new tests that don't requires hacks into SSL, but its probably fine to keep using SpawnedTestServer in ssl_client_socket tests, until we move the rest of them over. https://codereview.chromium.org/2016143002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_unittest.cc:3306: TEST_F(SSLClientSocketTest, CertStatusPKPBypassed) { Possibly add a test to make sure that the connection fails without is_issued_by_known_root being set to false?
https://codereview.chromium.org/2016143002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_unittest.cc:3306: TEST_F(SSLClientSocketTest, CertStatusPKPBypassed) { On 2016/05/27 18:27:44, svaldez wrote: > Possibly add a test to make sure that the connection fails without > is_issued_by_known_root being set to false? Done.
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_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2016143002/diff/20001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:1359: if (server_cert_verify_result_.is_issued_by_known_root) I think we need to apply the same logic to the corresponding check for QUIC connections: https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/pr... We probably also need to think about what to do for the SPDY pinning check here: https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_sess... That SPDY check is for whether or not a request can be pooled with an existing connection: in order to be allowed, the connection must not violate pins for the request's host. I suppose what we want to do there is continue to return true if |is_issued_by_known_root| is false...?
https://codereview.chromium.org/2016143002/diff/20001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2016143002/diff/20001/net/socket/ssl_client_s... 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 think we need to apply the same logic to the corresponding check for QUIC > connections: > https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/pr... > > We probably also need to think about what to do for the SPDY pinning check here: > https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_sess... > > That SPDY check is for whether or not a request can be pooled with an existing > connection: in order to be allowed, the connection must not violate pins for the > request's host. I suppose what we want to do there is continue to return true if > |is_issued_by_known_root| is false...? I implemented the same logic for QUIC, but I'm worried this CL is getting a little out of hand. I'm taking what was unified logic in TransportSecurityState and reimplementing it in three places. It does seem like the most straightforward way, but I'm anticipating net owners won't be happy.
On 2016/05/31 18:58:31, dadrian wrote: > https://codereview.chromium.org/2016143002/diff/20001/net/socket/ssl_client_s... > File net/socket/ssl_client_socket_impl.cc (right): > > https://codereview.chromium.org/2016143002/diff/20001/net/socket/ssl_client_s... > 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 think we need to apply the same logic to the corresponding check for QUIC > > connections: > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/pr... > > > > We probably also need to think about what to do for the SPDY pinning check > here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_sess... > > > > That SPDY check is for whether or not a request can be pooled with an existing > > connection: in order to be allowed, the connection must not violate pins for > the > > request's host. I suppose what we want to do there is continue to return true > if > > |is_issued_by_known_root| is false...? > > I implemented the same logic for QUIC, but I'm worried this CL is getting a > little out of hand. I'm taking what was unified logic in TransportSecurityState > and reimplementing it in three places. It does seem like the most > straightforward way, but I'm anticipating net owners won't be happy. One possibility as adding a new helper method around CheckPublicKeyPins that takes the cert_verify_result and returns a bool, internally dealing with setting cert_status.
dadrian@google.com changed reviewers: + davidben@chromium.org
Adding davidben@chromium.org: Before I go ahead and implement the required changes for SPDY, we're curious what your thoughts are on the current direction of the CL, and how CertStatus gets modified by each socket implementation.
On 2016/06/01 00:18:03, dadrian wrote: > Adding mailto:davidben@chromium.org: Before I go ahead and implement the required > changes for SPDY, we're curious what your thoughts are on the current direction > of the CL, and how CertStatus gets modified by each socket implementation. Friendly ping to davidben@
Having a CertStatus flag for something PKP related (a higher level) seems kinda wonky. I think this might make more sense as a flag on the SSLInfo. Although, don't we have this information already? There's is_issued_by_known_root. I suppose PKP_BYPASS is ANDed with the pin not working. Would you not want the former anyway though? If I'm a site on some enterprise-installed local root and I try to use HPKP, the pin will probably pass, but I probably wish to know that it won't do much useful anyway. (I suppose it does lock out all public roots. Yeah, I'm not sure.)
Chatted with davidben out-of-band. Sounds like he's okay with the cert status plan given that: 1. there's already a cert status flag about pinning (PINNED_KEY_MISSING), and pinning is not entirely unrelated to certificates 2. other non-error conditions are already recorded in this way (e.g. CT_COMPLIANCE_FAILED) 3. putting a flag on SSLInfo would require us to modify the disk cache format in order for it to appear on cached responses David (B), is that an accurate summary?
On 2016/06/03 17:19:33, estark wrote: > Chatted with davidben out-of-band. Sounds like he's okay with the cert status > plan given that: > 1. there's already a cert status flag about pinning (PINNED_KEY_MISSING), and > pinning is not entirely unrelated to certificates > 2. other non-error conditions are already recorded in this way (e.g. > CT_COMPLIANCE_FAILED) > 3. putting a flag on SSLInfo would require us to modify the disk cache format in > order for it to appear on cached responses > David (B), is that an accurate summary? So to confirm: continue down the same path we're currently on?
On 2016/06/03 17:19:33, estark wrote: > Chatted with davidben out-of-band. Sounds like he's okay with the cert status > plan given that: > 1. there's already a cert status flag about pinning (PINNED_KEY_MISSING), and > pinning is not entirely unrelated to certificates > 2. other non-error conditions are already recorded in this way (e.g. > CT_COMPLIANCE_FAILED) > 3. putting a flag on SSLInfo would require us to modify the disk cache format in > order for it to appear on cached responses > David (B), is that an accurate summary? I think (3) is mostly a non-issue since adding a CertStatus also counts as modifying the format. You'd just adding a single flag here instead. https://cs.chromium.org/chromium/src/net/http/http_response_info.cc?rcl=0&l=38 But, yeah, there's already (1) and (2), so.... Meh. Whatever. :-) Though (1) is kind of weird. The CL that added it also thought bad DH keys made sense as a CertStatus which is nonsense.
On 2016/06/03 17:37:52, davidben (OOO until 5-31) wrote: > On 2016/06/03 17:19:33, estark wrote: > > Chatted with davidben out-of-band. Sounds like he's okay with the cert status > > plan given that: > > 1. there's already a cert status flag about pinning (PINNED_KEY_MISSING), and > > pinning is not entirely unrelated to certificates > > 2. other non-error conditions are already recorded in this way (e.g. > > CT_COMPLIANCE_FAILED) > > 3. putting a flag on SSLInfo would require us to modify the disk cache format > in > > order for it to appear on cached responses > > David (B), is that an accurate summary? > > I think (3) is mostly a non-issue since adding a CertStatus also counts as > modifying the format. You'd just adding a single flag here instead. > https://cs.chromium.org/chromium/src/net/http/http_response_info.cc?rcl=0&l=38 > > But, yeah, there's already (1) and (2), so.... Meh. Whatever. :-) Though (1) is > kind of weird. The CL that added it also thought bad DH keys made sense as a > CertStatus which is nonsense. I also feel "Meh. Whatever" about CertStatus-vs-HttpResponseInfo now that I know adding fields to the disk cache is not really a big undertaking. Soooo David (A), I think you should decide. :P
On 2016/06/03 17:58:11, estark wrote: > On 2016/06/03 17:37:52, davidben (OOO until 5-31) wrote: > > On 2016/06/03 17:19:33, estark wrote: > > > Chatted with davidben out-of-band. Sounds like he's okay with the cert > status > > > plan given that: > > > 1. there's already a cert status flag about pinning (PINNED_KEY_MISSING), > and > > > pinning is not entirely unrelated to certificates > > > 2. other non-error conditions are already recorded in this way (e.g. > > > CT_COMPLIANCE_FAILED) > > > 3. putting a flag on SSLInfo would require us to modify the disk cache > format > > in > > > order for it to appear on cached responses > > > David (B), is that an accurate summary? > > > > I think (3) is mostly a non-issue since adding a CertStatus also counts as > > modifying the format. You'd just adding a single flag here instead. > > https://cs.chromium.org/chromium/src/net/http/http_response_info.cc?rcl=0&l=38 > > > > But, yeah, there's already (1) and (2), so.... Meh. Whatever. :-) Though (1) > is > > kind of weird. The CL that added it also thought bad DH keys made sense as a > > CertStatus which is nonsense. > > I also feel "Meh. Whatever" about CertStatus-vs-HttpResponseInfo now that I know > adding fields to the disk cache is not really a big undertaking. Soooo David > (A), I think you should decide. :P After a little bit of poking around, it looks like HTTPResponseInfo will be slightly nicer, and shouldn't take too long to transfer over to. From what I can tell, the steps are: 1. Add a bool SSLINfo::pkp_bypassed 2. Add a bitflag to HTTPResponseInfo 3. Update HTTPResponseInfo::Persist and HTTPResponseInfo::InitFromPickle accordingly 4. Make sure SSLInfo::pkp_bypassed is set in every socket
On 2016/06/03 17:58:11, estark wrote: > On 2016/06/03 17:37:52, davidben (OOO until 5-31) wrote: > > On 2016/06/03 17:19:33, estark wrote: > > > Chatted with davidben out-of-band. Sounds like he's okay with the cert > status > > > plan given that: > > > 1. there's already a cert status flag about pinning (PINNED_KEY_MISSING), > and > > > pinning is not entirely unrelated to certificates > > > 2. other non-error conditions are already recorded in this way (e.g. > > > CT_COMPLIANCE_FAILED) > > > 3. putting a flag on SSLInfo would require us to modify the disk cache > format > > in > > > order for it to appear on cached responses > > > David (B), is that an accurate summary? > > > > I think (3) is mostly a non-issue since adding a CertStatus also counts as > > modifying the format. You'd just adding a single flag here instead. > > https://cs.chromium.org/chromium/src/net/http/http_response_info.cc?rcl=0&l=38 > > > > But, yeah, there's already (1) and (2), so.... Meh. Whatever. :-) Though (1) > is > > kind of weird. The CL that added it also thought bad DH keys made sense as a > > CertStatus which is nonsense. > > I also feel "Meh. Whatever" about CertStatus-vs-HttpResponseInfo now that I know > adding fields to the disk cache is not really a big undertaking. Soooo David > (A), I think you should decide. :P After a little bit of poking around, it looks like HTTPResponseInfo will be slightly nicer, and shouldn't take too long to transfer over to. From what I can tell, the steps are: 1. Add a bool SSLINfo::pkp_bypassed 2. Add a bitflag to HTTPResponseInfo 3. Update HTTPResponseInfo::Persist and HTTPResponseInfo::InitFromPickle accordingly 4. Make sure SSLInfo::pkp_bypassed is set in every socket
On 2016/06/03 17:58:11, estark wrote: > On 2016/06/03 17:37:52, davidben (OOO until 5-31) wrote: > > On 2016/06/03 17:19:33, estark wrote: > > > Chatted with davidben out-of-band. Sounds like he's okay with the cert > status > > > plan given that: > > > 1. there's already a cert status flag about pinning (PINNED_KEY_MISSING), > and > > > pinning is not entirely unrelated to certificates > > > 2. other non-error conditions are already recorded in this way (e.g. > > > CT_COMPLIANCE_FAILED) > > > 3. putting a flag on SSLInfo would require us to modify the disk cache > format > > in > > > order for it to appear on cached responses > > > David (B), is that an accurate summary? > > > > I think (3) is mostly a non-issue since adding a CertStatus also counts as > > modifying the format. You'd just adding a single flag here instead. > > https://cs.chromium.org/chromium/src/net/http/http_response_info.cc?rcl=0&l=38 > > > > But, yeah, there's already (1) and (2), so.... Meh. Whatever. :-) Though (1) > is > > kind of weird. The CL that added it also thought bad DH keys made sense as a > > CertStatus which is nonsense. > > I also feel "Meh. Whatever" about CertStatus-vs-HttpResponseInfo now that I know > adding fields to the disk cache is not really a big undertaking. Soooo David > (A), I think you should decide. :P After a little bit of poking around, it looks like HTTPResponseInfo will be slightly nicer, and shouldn't take too long to transfer over to. From what I can tell, the steps are: 1. Add a bool SSLINfo::pkp_bypassed 2. Add a bitflag to HTTPResponseInfo 3. Update HTTPResponseInfo::Persist and HTTPResponseInfo::InitFromPickle accordingly 4. Make sure SSLInfo::pkp_bypassed is set in every socket
On 2016/06/04 00:37:44, dadrian wrote: > On 2016/06/03 17:58:11, estark wrote: > > On 2016/06/03 17:37:52, davidben (OOO until 5-31) wrote: > > > On 2016/06/03 17:19:33, estark wrote: > > > > Chatted with davidben out-of-band. Sounds like he's okay with the cert > > status > > > > plan given that: > > > > 1. there's already a cert status flag about pinning (PINNED_KEY_MISSING), > > and > > > > pinning is not entirely unrelated to certificates > > > > 2. other non-error conditions are already recorded in this way (e.g. > > > > CT_COMPLIANCE_FAILED) > > > > 3. putting a flag on SSLInfo would require us to modify the disk cache > > format > > > in > > > > order for it to appear on cached responses > > > > David (B), is that an accurate summary? > > > > > > I think (3) is mostly a non-issue since adding a CertStatus also counts as > > > modifying the format. You'd just adding a single flag here instead. > > > > https://cs.chromium.org/chromium/src/net/http/http_response_info.cc?rcl=0&l=38 > > > > > > But, yeah, there's already (1) and (2), so.... Meh. Whatever. :-) Though (1) > > is > > > kind of weird. The CL that added it also thought bad DH keys made sense as a > > > CertStatus which is nonsense. > > > > I also feel "Meh. Whatever" about CertStatus-vs-HttpResponseInfo now that I > know > > adding fields to the disk cache is not really a big undertaking. Soooo David > > (A), I think you should decide. :P > > After a little bit of poking around, it looks like HTTPResponseInfo will be > slightly nicer, and shouldn't take too long to transfer over to. From what I can > tell, the steps are: > > 1. Add a bool SSLINfo::pkp_bypassed > 2. Add a bitflag to HTTPResponseInfo > 3. Update HTTPResponseInfo::Persist and HTTPResponseInfo::InitFromPickle > accordingly > 4. Make sure SSLInfo::pkp_bypassed is set in every socket I've reworked this CL to use SSLInfo and HttpResponseInfo instead of CertStatus.
Thanks for doing another pass! I mostly just have nits. Also, can you update the CL description to mention that it's in the HttpResponseInfo instead of a cert status flag now? https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_... File net/http/http_response_info.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_... net/http/http_response_info.cc:100: // trust anchor nit: period at the end https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_... net/http/http_response_info.cc:303: ssl_info.pkp_bypassed = (flags & RESPONSE_INFO_PKP_BYPASSED); nit: != 0 (for consistency with the lines above, plus I think some compiler will complain without this... maybe?) https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_... File net/http/http_response_info_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_... net/http/http_response_info_unittest.cc:55: TEST_F(HttpResponseInfoTest, PKPBypassPersisted) { Can you add a similar test for when |pkp_bypassed| is false? https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:420: // Test that PKP is enforced for certificates that chain up to known roots also, nit (sorry): add a period Believe it or not, it's actually in the style guide: https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:420: // Test that PKP is enforced for certificates that chain up to known roots So the pinning check is supposed to fail here, right? Is there any more specific way to test for that other than just QUIC_FAILURE? https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:455: EXPECT_NE("", verify_details->pinning_failure_log); Perhaps check that |pkp_bypassed| is false too.
https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:458: // Test CERT_STATUS_PKP_BYPASSED is set when PKP is bypassed due to a local Name. https://codereview.chromium.org/2016143002/diff/60001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:3306: TEST_F(SSLClientSocketTest, CertStatusPKPBypassed) { Update name. https://codereview.chromium.org/2016143002/diff/60001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/2016143002/diff/60001/net/ssl/ssl_info.h#newc... net/ssl/ssl_info.h:97: // True if pinning was bypassed on this connection nit: .
Description was changed from ========== Add CERT_STATUS_PKP_BYPASS This is the first step in exposing in devtools when pinning is bypassed due to a local trust anchor. BUG=566144 ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
I apparently internalized the opposite rule about periods. https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_... File net/http/http_response_info.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_... net/http/http_response_info.cc:100: // trust anchor On 2016/06/07 04:10:43, estark wrote: > nit: period at the end Done. https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_... net/http/http_response_info.cc:303: ssl_info.pkp_bypassed = (flags & RESPONSE_INFO_PKP_BYPASSED); On 2016/06/07 04:10:43, estark wrote: > nit: != 0 > (for consistency with the lines above, plus I think some compiler will complain > without this... maybe?) Wow, I totally missed the `!= 0` on the other lines. Done. https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_... File net/http/http_response_info_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/http/http_response_... net/http/http_response_info_unittest.cc:55: TEST_F(HttpResponseInfoTest, PKPBypassPersisted) { On 2016/06/07 04:10:43, estark wrote: > Can you add a similar test for when |pkp_bypassed| is false? Done. https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:420: // Test that PKP is enforced for certificates that chain up to known roots On 2016/06/07 04:10:43, estark wrote: > also, nit (sorry): add a period > Believe it or not, it's actually in the style guide: > https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... Done. https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:420: // Test that PKP is enforced for certificates that chain up to known roots On 2016/06/07 04:10:43, estark wrote: > So the pinning check is supposed to fail here, right? Is there any more specific > way to test for that other than just QUIC_FAILURE? Effectively, this is only exposed through strings: the |error_details_| string, and the |pinning_failure_log| in VerifyDetailsChromium. I do check that the |pinning_failure_log| is non-empty, but I'm not sure we can do much better than that, beyond grepping through |error_details_| for ERR_SSL_PINNED_KEY_NOT_IN_CHAIN. https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:455: EXPECT_NE("", verify_details->pinning_failure_log); On 2016/06/07 04:10:43, estark wrote: > Perhaps check that |pkp_bypassed| is false too. Done. https://codereview.chromium.org/2016143002/diff/60001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:458: // Test CERT_STATUS_PKP_BYPASSED is set when PKP is bypassed due to a local On 2016/06/07 14:12:48, svaldez wrote: > Name. Done. https://codereview.chromium.org/2016143002/diff/60001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_unittest.cc:3306: TEST_F(SSLClientSocketTest, CertStatusPKPBypassed) { On 2016/06/07 14:12:48, svaldez wrote: > Update name. Done. https://codereview.chromium.org/2016143002/diff/60001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/2016143002/diff/60001/net/ssl/ssl_info.h#newc... net/ssl/ssl_info.h:97: // True if pinning was bypassed on this connection On 2016/06/07 14:12:48, svaldez wrote: > nit: . Done.
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:chro..., right? In particular, I thiiiink that we should only be returning false there if |is_issued_by_known_root| is true. https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:1295: // non-public root nit: period
On 2016/06/07 18:38:32, estark wrote: > 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:chro..., > right? In particular, I thiiiink that we should only be returning false there if > |is_issued_by_known_root| is true. > > https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... > File net/http/transport_security_state_unittest.cc (right): > > https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... > net/http/transport_security_state_unittest.cc:1295: // non-public root > nit: period I think that's just controlling connection pooling, but to keep the same behavior as before, I think we would have to always return true whenever |is_issued_by_known_root| is false, which as I'm typing I realize is exactly what you said.
On 2016/06/07 18:42:23, dadrian wrote: > On 2016/06/07 18:38:32, estark wrote: > > 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:chro..., > > right? In particular, I thiiiink that we should only be returning false there > if > > |is_issued_by_known_root| is true. > > > > > https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... > > File net/http/transport_security_state_unittest.cc (right): > > > > > https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... > > net/http/transport_security_state_unittest.cc:1295: // non-public root > > nit: period > > I think that's just controlling connection pooling, but to keep the same > behavior as before, I think we would have to always return true whenever > |is_issued_by_known_root| is false, which as I'm typing I realize is exactly > what you said. One other thought. I wonder if it's worth considering a slight tweak to this, where CheckPublicKeyPins() returns an enum (PINNING_VIOLATION, PINNING_VIOLATION_BYPASSED, NO_PINNING_VIOLATION... or something like that). That way only TSS would have to know that |is_issued_by_known_root| certificates are special, instead of both TSS and the CheckPublicKeyPins() callers knowing that. What do you all think?
On 2016/06/07 18:47:29, estark wrote: > On 2016/06/07 18:42:23, dadrian wrote: > > On 2016/06/07 18:38:32, estark wrote: > > > 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:chro..., > > > right? In particular, I thiiiink that we should only be returning false > there > > if > > > |is_issued_by_known_root| is true. > > > > > > > > > https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... > > > File net/http/transport_security_state_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... > > > net/http/transport_security_state_unittest.cc:1295: // non-public root > > > nit: period > > > > I think that's just controlling connection pooling, but to keep the same > > behavior as before, I think we would have to always return true whenever > > |is_issued_by_known_root| is false, which as I'm typing I realize is exactly > > what you said. > > One other thought. I wonder if it's worth considering a slight tweak to this, > where CheckPublicKeyPins() returns an enum (PINNING_VIOLATION, > PINNING_VIOLATION_BYPASSED, NO_PINNING_VIOLATION... or something like that). > That way only TSS would have to know that |is_issued_by_known_root| certificates > are special, instead of both TSS and the CheckPublicKeyPins() callers knowing > that. What do you all think? Yeah, connection pooling should keep the previous semantics. Generally I think callers that set DISABLE_PIN_REPORTS should not be affected by whether the root is locally trusted. Switching it to an enum will probably make callers slightly more complicated, though probably correctly hides the details of what is considered in order to bypass pinning. On the other hand, we'll need to ensure callers are consistent in using '== NO_PINNING_VIOLATION' versus '!= PINNING_VIOLATION' at the appropriate places. Though that might be fine.
On 2016/06/07 19:14:52, svaldez wrote: > On 2016/06/07 18:47:29, estark wrote: > > On 2016/06/07 18:42:23, dadrian wrote: > > > On 2016/06/07 18:38:32, estark wrote: > > > > 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:chro..., > > > > right? In particular, I thiiiink that we should only be returning false > > there > > > if > > > > |is_issued_by_known_root| is true. > > > > > > > > > > > > > > https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... > > > > File net/http/transport_security_state_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... > > > > net/http/transport_security_state_unittest.cc:1295: // non-public root > > > > nit: period > > > > > > I think that's just controlling connection pooling, but to keep the same > > > behavior as before, I think we would have to always return true whenever > > > |is_issued_by_known_root| is false, which as I'm typing I realize is exactly > > > what you said. > > > > One other thought. I wonder if it's worth considering a slight tweak to this, > > where CheckPublicKeyPins() returns an enum (PINNING_VIOLATION, > > PINNING_VIOLATION_BYPASSED, NO_PINNING_VIOLATION... or something like that). > > That way only TSS would have to know that |is_issued_by_known_root| > certificates > > are special, instead of both TSS and the CheckPublicKeyPins() callers knowing > > that. What do you all think? > > Yeah, connection pooling should keep the previous semantics. Generally I think > callers that set DISABLE_PIN_REPORTS should not be affected by whether the root > is locally trusted. Switching it to an enum will probably make callers slightly > more complicated, though probably correctly hides the details of what is > considered in order to bypass pinning. On the other hand, we'll need to ensure > callers are consistent in using '== NO_PINNING_VIOLATION' versus '!= > PINNING_VIOLATION' at the appropriate places. Though that might be fine. I reverted to the old SPDY semantics. Changing the return type of CheckPublicKeyPins() sounds like a reasonable idea for a different CL :)
https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/80001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:1295: // non-public root On 2016/06/07 18:38:32, estark wrote: > nit: period Done.
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 net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2016143002/diff/100001/net/spdy/spdy_session.... net/spdy/spdy_session.cc:666: if (ssl_info.is_issued_by_known_root) optional nit: we might as well bypass CheckPublicKeyPins() all together if |is_issued_by_known_root| is false, right?
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.... net/spdy/spdy_session.cc:666: if (ssl_info.is_issued_by_known_root) On 2016/06/08 17:09:28, estark wrote: > optional nit: we might as well bypass CheckPublicKeyPins() all together if > |is_issued_by_known_root| is false, right? Done.
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.... > net/spdy/spdy_session.cc:666: if (ssl_info.is_issued_by_known_root) > On 2016/06/08 17:09:28, estark wrote: > > optional nit: we might as well bypass CheckPublicKeyPins() all together if > > |is_issued_by_known_root| is false, right? > > Done. lgtm, I'm still not completely convinced that the Spdy pooling is DTRT generally, but since we're keeping the same semantics as before here, I don't think that needs to be figured out for this CL.
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.... net/spdy/spdy_session.cc:665: if (!transport_security_state->CheckPublicKeyPins( I think it'd be better for this to return an enum. From all three callers, it looks like it's never correct for the caller of CheckPublicKeyPins to not special-case is_issued_by_known_root which means this function effective returns a tri-state. An enum means we're less likely to accidentally omit the check somewhere. (In fact, it's a little fishy that this function significantly changed the semantics of CheckPublicKeyPins from "is this stuff okay for this name?" to "are the pins valid, independent of whether we'd care" without changing the documentation of the function.)
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.... net/spdy/spdy_session.cc:665: if (!transport_security_state->CheckPublicKeyPins( On 2016/06/09 15:56:32, davidben wrote: > I think it'd be better for this to return an enum. From all three callers, it > looks like it's never correct for the caller of CheckPublicKeyPins to not > special-case is_issued_by_known_root which means this function effective returns > a tri-state. An enum means we're less likely to accidentally omit the check > somewhere. > > (In fact, it's a little fishy that this function significantly changed the > semantics of CheckPublicKeyPins from "is this stuff okay for this name?" to "are > the pins valid, independent of whether we'd care" without changing the > documentation of the function.) Another possibility is maybe this returns the old bool (since that's the important high-order bit. "May I proceed?") and then has some other output parameter for the diagnostic "PS: it's okay, but it's because I bypassed stuff" info.
davidben: Do you want to change to an enum in this CL, or are you OK with that happening in a future CL? 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.... net/spdy/spdy_session.cc:665: if (!transport_security_state->CheckPublicKeyPins( On 2016/06/09 15:56:32, davidben wrote: > I think it'd be better for this to return an enum. From all three callers, it > looks like it's never correct for the caller of CheckPublicKeyPins to not > special-case is_issued_by_known_root which means this function effective returns > a tri-state. An enum means we're less likely to accidentally omit the check > somewhere. > > (In fact, it's a little fishy that this function significantly changed the > semantics of CheckPublicKeyPins from "is this stuff okay for this name?" to "are > the pins valid, independent of whether we'd care" without changing the > documentation of the function.) It looks like the current documentation already matches the new behavior. It implies that the function only looks at key hashes.
On 2016/06/09 16:39:40, dadrian wrote: > davidben: Do you want to change to an enum in this CL, or are you OK with that > happening in a future CL? > > 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.... > net/spdy/spdy_session.cc:665: if (!transport_security_state->CheckPublicKeyPins( > On 2016/06/09 15:56:32, davidben wrote: > > I think it'd be better for this to return an enum. From all three callers, it > > looks like it's never correct for the caller of CheckPublicKeyPins to not > > special-case is_issued_by_known_root which means this function effective > returns > > a tri-state. An enum means we're less likely to accidentally omit the check > > somewhere. > > > > (In fact, it's a little fishy that this function significantly changed the > > semantics of CheckPublicKeyPins from "is this stuff okay for this name?" to > "are > > the pins valid, independent of whether we'd care" without changing the > > documentation of the function.) > > It looks like the current documentation already matches the new behavior. It > implies that the function only looks at key hashes. Hrm, yeah. It doesn't seem to say much useful about what is_known_root does, actually. :-) *shrug* Separate CL seems fine since this is all ready I guess. Though maybe tweak the comment for now to call attention to the fact that the caller does need to handle the !is_known_root => ignore pin check bit themselves. A priori I would expect a CheckPublicKeyPins function which takes an is_known_root parameter to include is_known_root processing.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/2016143002/diff/120001/net/cert/cert_verify_r... File net/cert/cert_verify_result.h (right): https://codereview.chromium.org/2016143002/diff/120001/net/cert/cert_verify_r... net/cert/cert_verify_result.h:74: bool pkp_bypassed; I believe this is a layering violation - the CertVerifier does not know about PKP at present. https://codereview.chromium.org/2016143002/diff/120001/net/http/transport_sec... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2016143002/diff/120001/net/http/transport_sec... net/http/transport_security_state.cc:680: } STYLE: No braces on single-line conditionals https://codereview.chromium.org/2016143002/diff/120001/net/quic/crypto/proof_... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/2016143002/diff/120001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium_test.cc:412: HashValueVector MakeHashValueVector(uint8_t tag) { NAMING: This is confusing with HashValueTag (which is SHA1 or SHA256) https://codereview.chromium.org/2016143002/diff/120001/net/socket/ssl_client_... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/2016143002/diff/120001/net/socket/ssl_client_... net/socket/ssl_client_socket_unittest.cc:3266: HashValueVector MakeHashValueVector(const std::string& pin) { DESIGN: Why coerce to string when you only care about const char*? SECURITY: Why assume that |pin| is 32 bytes (line 3269) if you're taking a string 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.... net/spdy/spdy_session.cc:659: return true; FWIW, and for better or worse, it was intentional this was omitted, but that should have been documented. The thinking at the time was that it's OK for things to be slower if the user is MITM'ing themselves; that is, you don't get "the performance of pooling for pinned hosts" I'm also a little nervous about this conditional being before the pinning check, rather than being a part of it. This conditional makes it too easy to short-circuit security checks added later (e.g. as part of line 672) https://codereview.chromium.org/2016143002/diff/120001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/2016143002/diff/120001/net/ssl/ssl_info.h#new... net/ssl/ssl_info.h:98: bool pkp_bypassed; Totally OK with this here, FWIW
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.... net/spdy/spdy_session.cc:659: return true; On 2016/06/09 19:17:32, Ryan Sleevi wrote: > FWIW, and for better or worse, it was intentional this was omitted, but that > should have been documented. > > The thinking at the time was that it's OK for things to be slower if the user is > MITM'ing themselves; that is, you don't get "the performance of pooling for > pinned hosts" > > I'm also a little nervous about this conditional being before the pinning check, > rather than being a part of it. This conditional makes it too easy to > short-circuit security checks added later (e.g. as part of line 672) I believe this aligns with the old behavior, but I agree I really don't like having callers need to care about this. My suggestion (below) was that CheckPublicKeyPins should either return an enum or return the same boolean as before and then have a separate output parameter for the diagnositic information.
On 2016/06/09 19:19:15, davidben wrote: > I believe this aligns with the old behavior, but I agree I really don't like > having callers need to care about this. My suggestion (below) was that > CheckPublicKeyPins should either return an enum or return the same boolean as > before and then have a separate output parameter for the diagnositic > information. Right, it was dadrian@ filing the bug that got my attention to this :) My inclination is we've found a design edge that we should fix, and perhaps split into two CLs: 1) Switch TSS to return the enum, taking in |is_issued_by_known_root| as a flag, and returns an enum a) Design Question: What should TSS think of the variable? Should it think of it as "issued by known root" or "ok to bypass"? My inclination is explicitly calling it out as known root, since calling it "ok to bypass" argues for the current design (namely, callers should care about bypass) 2) Add it to the SSLInfo (and thus, by proxy, the HTTP cache) - Note: FWIW, I totally agree with your remarks about stuff that "shouldn't have been a CertStatus" and "affects the serialization anyways". That mistake is my fault for encouraging short-cuts, and since then, every time I look at some of our CertStatus flags I sort of go "Huh, wtf was I thinking"
https://codereview.chromium.org/2016143002/diff/120001/net/cert/cert_verify_r... File net/cert/cert_verify_result.h (right): https://codereview.chromium.org/2016143002/diff/120001/net/cert/cert_verify_r... net/cert/cert_verify_result.h:74: bool pkp_bypassed; On 2016/06/09 19:17:32, Ryan Sleevi wrote: > I believe this is a layering violation - the CertVerifier does not know about > PKP at present. That's only sort of true. The CertVerifier carries a CertStatus which knows about PKP (CERT_STATUS_PINNED_KEY_MISSING). It sounds like we need to get rid of that flag somehow.
On 2016/06/09 19:26:41, estark wrote: > That's only sort of true. The CertVerifier carries a CertStatus which knows > about PKP (CERT_STATUS_PINNED_KEY_MISSING). It sounds like we need to get rid of > that flag somehow. Right, I think of that more as a #3 - cleanup that weird case. The CertVerifier itself doesn't know about pinning, and that status smuggling is functionally unused (that is, code is generally and correctly looking at the //net error code, ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN, rather than the CertStatus). It looks like it was done just to get the right SSL error page to show and that's... gross :) But I don't view that as an item that should block this CL or be more pressing - just a cleanup for consistency :)
rsleevi@: So are we good leaving the modification of CertVerifier in this CL, and fixing the whole thing up some other time. What began as a "starter bug" to add a string to devtools is slowly evolving into a refactoring of how certificate verification interacts with the rest of the net stack. 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.... net/spdy/spdy_session.cc:659: return true; On 2016/06/09 19:17:32, Ryan Sleevi wrote: > FWIW, and for better or worse, it was intentional this was omitted, but that > should have been documented. > > The thinking at the time was that it's OK for things to be slower if the user is > MITM'ing themselves; that is, you don't get "the performance of pooling for > pinned hosts" > > I'm also a little nervous about this conditional being before the pinning check, > rather than being a part of it. This conditional makes it too easy to > short-circuit security checks added later (e.g. as part of line 672) Done.
> rsleevi@: So are we good leaving the modification of CertVerifier in this CL, > and fixing the whole thing up some other time. What began as a "starter bug" to > add a string to devtools is slowly evolving into a refactoring of how > certificate verification interacts with the rest of the net stack. No. I left concrete advice below on how to avoid that in this CL without having to refactor anything. https://codereview.chromium.org/2016143002/diff/140001/net/quic/crypto/proof_... File net/quic/crypto/proof_verifier_chromium.cc (right): https://codereview.chromium.org/2016143002/diff/140001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium.cc:341: verify_details_->cert_verify_result.pkp_bypassed = true; Move this to be on ProofVerifyDetailsChromium (e.g. verify_details_->pkp_bypassed), not on verify_details_->cert_verify_result https://codereview.chromium.org/2016143002/diff/140001/net/quic/quic_chromium... File net/quic/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2016143002/diff/140001/net/quic/quic_chromium... net/quic/quic_chromium_client_session.cc:557: ssl_info->pkp_bypassed = cert_verify_result_->pkp_bypassed; Here you would add a new local member and update it on OnProofVerifyDetailsAvailable from the ProofVerifyDetailsChromium https://codereview.chromium.org/2016143002/diff/140001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2016143002/diff/140001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:1358: server_cert_verify_result_.pkp_bypassed = true; Here, it'd be a local member that then gets populated to the ssl_info above
|pkp_bypassed| is no longer in CertVerifyResult.
I'm happy to defer on the enum handling, although I'd love to see it cleaned up. My concerns are all addressed so LG, but I'll let David (Ben) ack if he's happy https://codereview.chromium.org/2016143002/diff/160001/net/quic/crypto/proof_... File net/quic/crypto/proof_verifier_chromium.h (right): https://codereview.chromium.org/2016143002/diff/160001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium.h:50: bool pkp_bypassed; If you added lines 34-36 to default initialize to false, you can just use C++11 here bool pkp_bypassed = false; Saves ya some pain :)
lgtm. (+1 to Ryan's comment below. We have a lot of code that predates it, but C++11 is great. :) ) https://codereview.chromium.org/2016143002/diff/160001/net/quic/crypto/proof_... File net/quic/crypto/proof_verifier_chromium.h (right): https://codereview.chromium.org/2016143002/diff/160001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium.h:50: bool pkp_bypassed; +1
The CQ bit was checked by dadrian@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org, svaldez@chromium.org Link to the patchset: https://codereview.chromium.org/2016143002/#ps160001 (title: "Make CertVerifyResult Great Again.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016143002/160001
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/df302c4d9de0bcd65378ecd44a4e3ec1199dea4c Cr-Commit-Position: refs/heads/master@{#399242} |