Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2587243002:
Do not do Expect-Staple when OCSPVerifyResult has not been populated (Closed)
|
Created:
4 years ago by estark Modified:
3 years, 12 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not do Expect-Staple when OCSPVerifyResult has not been populated
The OCSPVerifyResult is not always populated on a connection, for example
when a certificate error has been bypassed. This CL adds a new response
status that allows us to distinguish whether or not OCSP details have been
checked on the connection, and we no longer send reports when they haven't
been checked.
This CL also adds a test that reports are not sent when there is a
certificate error, as is the case when first encountering a cert error,
before it has been bypassed.
BUG=654127
Committed: https://crrev.com/13e0b315a37468b305d5504caab8f3dd3691f3ba
Cr-Commit-Position: refs/heads/master@{#440547}
Patch Set 1 #
Total comments: 18
Patch Set 2 : sleevi comments; revert to not sending reports on cert errors #
Total comments: 1
Messages
Total messages: 28 (16 generated)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Description was changed from ========== Tweak Expect-Staple behavior when there are certificate errors There are two tweaks in this CL related to Expect-Staple in the presence of cert errors: 1. Do not send reports when the OCSPVerifyResult has not been populated, as in the case where a certificate error has been bypassed. A new UNKNOWN response status allows us to distinguish whether OCSP details have been checked on the connection, so that we can distinguish whether or not OCSPVerifyResult has been populated. 2. If the OCSPVerifyResult has been populated, do send a report even if there is a certificate error on the connection. Such reports can be useful, e.g. if the error was itself caused by a bad stapled OCSP response. BUG=654127 ========== to ========== Tweak Expect-Staple behavior when there are certificate errors There are two tweaks in this CL related to Expect-Staple in the presence of cert errors: 1. Do not send reports when the OCSPVerifyResult has not been populated, as in the case where a certificate error has been bypassed. A new UNKNOWN response status allows us to distinguish whether or not OCSP details have been checked on the connection. 2. If the OCSPVerifyResult has been populated, do send a report even if there is a certificate error on the connection. Such reports can be useful, e.g. if the error was itself caused by a bad stapled OCSP response. BUG=654127 ==========
estark@chromium.org changed reviewers: + rsleevi@chromium.org
rsleevi, PTAL?
I still can't convince myself that it's the right thing to do to, to send reports on an error, especially because in an error scenario, we don't know whether or not it's a local trust anchor or otherwise contains PII / sensitive info. Do we have a request for this? Can we just say "No"? :) https://codereview.chromium.org/2587243002/diff/1/net/cert/ocsp_verify_result.h File net/cert/ocsp_verify_result.h (right): https://codereview.chromium.org/2587243002/diff/1/net/cert/ocsp_verify_result... net/cert/ocsp_verify_result.h:31: UNKNOWN, The risk of "Unknown" here is that it might become more overloaded. Any reason not to call this something like NOT_CHECKED ? (I can see arguments both for & against, just noting the asymmetry between this and the other response codes) https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... net/http/transport_security_state.cc:669: return std::string(); nit: Any reason there's not a NOTREACHED() here as well? https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... net/http/transport_security_state.cc:814: ssl_info.ocsp_result.revocation_status == OCSPRevocationStatus::GOOD)) { suggestion: I would actually break these into two conditionals, since it's logically two different criteria, even if it's the same behaviour // No report needed if OCSP details were not checked on this connection. if (ssl_info.ocsp_result.response_status == OCSPVerifyResult::UNKNOWN) return; // No report needed if a stapled OCSP response was provided and it was valid. if (...) return; (The above nit is a comment on the comment - it's not just stapling - it's a valid stapled response) https://codereview.chromium.org/2587243002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2587243002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_impl.cc:1324: SSLInfo ssl_info; Might by worth including a comment here in the code explaining the intentional subtlety here (about sending reports on bad certificates) https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:9633: // but not provide any OCSP information. The MockCertVerifier doesn't accept this certificate - at least, not per line 9639. The comment seems to conflict with 9638. // Set up a MockCertVerifier to report an error for the certificate and indicate no OCSP information was provided. (Or something better worded, I'm awful here.) You could also likely delete 9638 with that. https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:9642: verify_result.ocsp_result.response_status = OCSPVerifyResult::MISSING; Is this really an accurate simulation? Your description suggests that an error code from a CertVerifier would result in the SSLSocket not checking the OCSPVerifyResult, rather than saying its missing (at least, judging by the comment in response_status) https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:9646: // Catch the Expect-Staple report. This comment reads a little weird, because this code block isn't catching anything, it's just setting up the code to listen for the report. https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:9653: MockHostResolver host_resolver; Logically, it seems like there should be a newline here between 9653 and 9654 - because 9654+ has no relation to what the comment describes. Comment wise, it seems less ideal to describe what MockHostResolver does (implementation wise) than it is to describe what you're doing. // Force |kExpectStapleStaticHostname| to resolve to |https_test_server|. https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:9662: // Now send a request to trigger the violation. What violation? What is it violating? That's the first usage in this context, and it's not clear. // Make a connection to |kExpectStapleStaticHostname|. Because the |verify_result| used with // the |cert_verifier| will report a stapled OCSP response was missing, an Expect-Staple report // should be sent using |mock_report_sender|.
Oh, and to be clear: I definitely think we want to not send reports on bypassed errors on the second connection. That part totally makes sense :)
(no code changes yet, want to first hash out whether we should send reports on errors) On 2016/12/21 01:35:29, Ryan Sleevi wrote: > I still can't convince myself that it's the right thing to do to, to send > reports on an error, especially because in an error scenario, we don't know > whether or not it's a local trust anchor or otherwise contains PII / sensitive > info. > > Do we have a request for this? Can we just say "No"? :) Uuuurgh, I forgot that we don't know about local trust anchors if it's a cert error. That does indeed seem bad. This came up in looking through some of Dropbox's reports. My worry is that by not sending reports on cert errors, we're missing out on a good chunk of the cases that we actually want to report. That is, if a server staples, say, an expired response, on most platforms we pass that into the cert verifier which would barf on the expired OCSP response which would then cause us to not send a report. Am I understanding the state of things correctly? > > https://codereview.chromium.org/2587243002/diff/1/net/cert/ocsp_verify_result.h > File net/cert/ocsp_verify_result.h (right): > > https://codereview.chromium.org/2587243002/diff/1/net/cert/ocsp_verify_result... > net/cert/ocsp_verify_result.h:31: UNKNOWN, > The risk of "Unknown" here is that it might become more overloaded. Any reason > not to call this something like NOT_CHECKED ? > > (I can see arguments both for & against, just noting the asymmetry between this > and the other response codes) > > https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... > File net/http/transport_security_state.cc (right): > > https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... > net/http/transport_security_state.cc:669: return std::string(); > nit: Any reason there's not a NOTREACHED() here as well? > > https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... > net/http/transport_security_state.cc:814: ssl_info.ocsp_result.revocation_status > == OCSPRevocationStatus::GOOD)) { > suggestion: I would actually break these into two conditionals, since it's > logically two different criteria, even if it's the same behaviour > > // No report needed if OCSP details were not checked on this connection. > if (ssl_info.ocsp_result.response_status == OCSPVerifyResult::UNKNOWN) > return; > > // No report needed if a stapled OCSP response was provided and it was valid. > if (...) > return; > > > (The above nit is a comment on the comment - it's not just stapling - it's a > valid stapled response) > > https://codereview.chromium.org/2587243002/diff/1/net/socket/ssl_client_socke... > File net/socket/ssl_client_socket_impl.cc (right): > > https://codereview.chromium.org/2587243002/diff/1/net/socket/ssl_client_socke... > net/socket/ssl_client_socket_impl.cc:1324: SSLInfo ssl_info; > Might by worth including a comment here in the code explaining the intentional > subtlety here (about sending reports on bad certificates) > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > File net/url_request/url_request_unittest.cc (right): > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > net/url_request/url_request_unittest.cc:9633: // but not provide any OCSP > information. > The MockCertVerifier doesn't accept this certificate - at least, not per line > 9639. The comment seems to conflict with 9638. > > // Set up a MockCertVerifier to report an error for the certificate and indicate > no OCSP information was provided. > > (Or something better worded, I'm awful here.) > > You could also likely delete 9638 with that. > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > net/url_request/url_request_unittest.cc:9642: > verify_result.ocsp_result.response_status = OCSPVerifyResult::MISSING; > Is this really an accurate simulation? Your description suggests that an error > code from a CertVerifier would result in the SSLSocket not checking the > OCSPVerifyResult, rather than saying its missing (at least, judging by the > comment in response_status) > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > net/url_request/url_request_unittest.cc:9646: // Catch the Expect-Staple report. > This comment reads a little weird, because this code block isn't catching > anything, it's just setting up the code to listen for the report. > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > net/url_request/url_request_unittest.cc:9653: MockHostResolver host_resolver; > Logically, it seems like there should be a newline here between 9653 and 9654 - > because 9654+ has no relation to what the comment describes. > > Comment wise, it seems less ideal to describe what MockHostResolver does > (implementation wise) than it is to describe what you're doing. > > // Force |kExpectStapleStaticHostname| to resolve to |https_test_server|. > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > net/url_request/url_request_unittest.cc:9662: // Now send a request to trigger > the violation. > What violation? What is it violating? That's the first usage in this context, > and it's not clear. > > // Make a connection to |kExpectStapleStaticHostname|. Because the > |verify_result| used with > // the |cert_verifier| will report a stapled OCSP response was missing, an > Expect-Staple report > // should be sent using |mock_report_sender|.
On 2016/12/21 01:45:51, estark wrote: > (no code changes yet, want to first hash out whether we should send reports on > errors) > > On 2016/12/21 01:35:29, Ryan Sleevi wrote: > > I still can't convince myself that it's the right thing to do to, to send > > reports on an error, especially because in an error scenario, we don't know > > whether or not it's a local trust anchor or otherwise contains PII / sensitive > > info. > > > > Do we have a request for this? Can we just say "No"? :) > > Uuuurgh, I forgot that we don't know about local trust anchors if it's a cert > error. That does indeed seem bad. > > This came up in looking through some of Dropbox's reports. My worry is that by > not sending reports on cert errors, we're missing out on a good chunk of the > cases that we actually want to report. That is, if a server staples, say, an > expired response, on most platforms we pass that into the cert verifier which > would barf on the expired OCSP response which would then cause us to not send a > report. Am I understanding the state of things correctly? Oh wait, apparently I have the memory of a goldfish, because I ALSO forgot that we turned off Expect-Staple reporting for private roots because it was causing a lot of noise. So we don't have to worry about sending PII, but that also means that what I've done in this CL won't really fix the problem. On a cert error, |is_issued_by_known_root| will (usually?) be false so we won't send a report whether we're ignoring the cert error or not. Hm. > > > > > > https://codereview.chromium.org/2587243002/diff/1/net/cert/ocsp_verify_result.h > > File net/cert/ocsp_verify_result.h (right): > > > > > https://codereview.chromium.org/2587243002/diff/1/net/cert/ocsp_verify_result... > > net/cert/ocsp_verify_result.h:31: UNKNOWN, > > The risk of "Unknown" here is that it might become more overloaded. Any reason > > not to call this something like NOT_CHECKED ? > > > > (I can see arguments both for & against, just noting the asymmetry between > this > > and the other response codes) > > > > > https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... > > File net/http/transport_security_state.cc (right): > > > > > https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... > > net/http/transport_security_state.cc:669: return std::string(); > > nit: Any reason there's not a NOTREACHED() here as well? > > > > > https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... > > net/http/transport_security_state.cc:814: > ssl_info.ocsp_result.revocation_status > > == OCSPRevocationStatus::GOOD)) { > > suggestion: I would actually break these into two conditionals, since it's > > logically two different criteria, even if it's the same behaviour > > > > // No report needed if OCSP details were not checked on this connection. > > if (ssl_info.ocsp_result.response_status == OCSPVerifyResult::UNKNOWN) > > return; > > > > // No report needed if a stapled OCSP response was provided and it was valid. > > if (...) > > return; > > > > > > (The above nit is a comment on the comment - it's not just stapling - it's a > > valid stapled response) > > > > > https://codereview.chromium.org/2587243002/diff/1/net/socket/ssl_client_socke... > > File net/socket/ssl_client_socket_impl.cc (right): > > > > > https://codereview.chromium.org/2587243002/diff/1/net/socket/ssl_client_socke... > > net/socket/ssl_client_socket_impl.cc:1324: SSLInfo ssl_info; > > Might by worth including a comment here in the code explaining the intentional > > subtlety here (about sending reports on bad certificates) > > > > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > > File net/url_request/url_request_unittest.cc (right): > > > > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > > net/url_request/url_request_unittest.cc:9633: // but not provide any OCSP > > information. > > The MockCertVerifier doesn't accept this certificate - at least, not per line > > 9639. The comment seems to conflict with 9638. > > > > // Set up a MockCertVerifier to report an error for the certificate and > indicate > > no OCSP information was provided. > > > > (Or something better worded, I'm awful here.) > > > > You could also likely delete 9638 with that. > > > > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > > net/url_request/url_request_unittest.cc:9642: > > verify_result.ocsp_result.response_status = OCSPVerifyResult::MISSING; > > Is this really an accurate simulation? Your description suggests that an error > > code from a CertVerifier would result in the SSLSocket not checking the > > OCSPVerifyResult, rather than saying its missing (at least, judging by the > > comment in response_status) > > > > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > > net/url_request/url_request_unittest.cc:9646: // Catch the Expect-Staple > report. > > This comment reads a little weird, because this code block isn't catching > > anything, it's just setting up the code to listen for the report. > > > > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > > net/url_request/url_request_unittest.cc:9653: MockHostResolver host_resolver; > > Logically, it seems like there should be a newline here between 9653 and 9654 > - > > because 9654+ has no relation to what the comment describes. > > > > Comment wise, it seems less ideal to describe what MockHostResolver does > > (implementation wise) than it is to describe what you're doing. > > > > // Force |kExpectStapleStaticHostname| to resolve to |https_test_server|. > > > > > https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... > > net/url_request/url_request_unittest.cc:9662: // Now send a request to trigger > > the violation. > > What violation? What is it violating? That's the first usage in this context, > > and it's not clear. > > > > // Make a connection to |kExpectStapleStaticHostname|. Because the > > |verify_result| used with > > // the |cert_verifier| will report a stapled OCSP response was missing, an > > Expect-Staple report > > // should be sent using |mock_report_sender|.
On 2016/12/21 01:45:51, estark wrote: > This came up in looking through some of Dropbox's reports. My worry is that by > not sending reports on cert errors, we're missing out on a good chunk of the > cases that we actually want to report. That is, if a server staples, say, an > expired response, on most platforms we pass that into the cert verifier which > would barf on the expired OCSP response which would then cause us to not send a > report. Am I understanding the state of things correctly? I don't think so - not on Windows (which would go out and fetch a response) or OS X (which ignores the stapled response). So it'd only be Linux+NSS, and ... yeah. Plus I thought (some of) the point of dadrian@'s code was to give us insight into the stapled response here. So I think we're fine with the current state of the world? If not, you can always... test it :D
On 2016/12/21 07:25:17, Ryan Sleevi wrote: > On 2016/12/21 01:45:51, estark wrote: > > This came up in looking through some of Dropbox's reports. My worry is that by > > not sending reports on cert errors, we're missing out on a good chunk of the > > cases that we actually want to report. That is, if a server staples, say, an > > expired response, on most platforms we pass that into the cert verifier which > > would barf on the expired OCSP response which would then cause us to not send > a > > report. Am I understanding the state of things correctly? > > I don't think so - not on Windows (which would go out and fetch a response) or > OS X (which ignores the stapled response). So it'd only be Linux+NSS, and ... > yeah. Ah, ok. I didn't know Windows would go out and fetch a response if the stapled one is bad. I can do some testing and corroborate with Dropbox to make sure there's no problem in that respect, but in the meantime, I'll change this CL to just fix the thing with bypassed errors. Will update when that's ready to review. Thanks! > > Plus I thought (some of) the point of dadrian@'s code was to give us insight > into the stapled response here. > > So I think we're fine with the current state of the world? If not, you can > always... test it :D
Description was changed from ========== Tweak Expect-Staple behavior when there are certificate errors There are two tweaks in this CL related to Expect-Staple in the presence of cert errors: 1. Do not send reports when the OCSPVerifyResult has not been populated, as in the case where a certificate error has been bypassed. A new UNKNOWN response status allows us to distinguish whether or not OCSP details have been checked on the connection. 2. If the OCSPVerifyResult has been populated, do send a report even if there is a certificate error on the connection. Such reports can be useful, e.g. if the error was itself caused by a bad stapled OCSP response. BUG=654127 ========== to ========== Do not do Expect-Staple when OCSPVerifyResult has not been populated The OCSPVerifyResult is not always populated on a connection, for example when a certificate error has been bypassed. This CL adds a new response status that allows us to distinguish whether or not OCSP details have been checked on the connection, and we no longer send reports when they haven't been checked. BUG=654127 ==========
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Do not do Expect-Staple when OCSPVerifyResult has not been populated The OCSPVerifyResult is not always populated on a connection, for example when a certificate error has been bypassed. This CL adds a new response status that allows us to distinguish whether or not OCSP details have been checked on the connection, and we no longer send reports when they haven't been checked. BUG=654127 ========== to ========== Do not do Expect-Staple when OCSPVerifyResult has not been populated The OCSPVerifyResult is not always populated on a connection, for example when a certificate error has been bypassed. This CL adds a new response status that allows us to distinguish whether or not OCSP details have been checked on the connection, and we no longer send reports when they haven't been checked. This CL also adds a test that reports are not sent when there is a certificate error, as is the case when first encountering a cert error, before it has been bypassed. BUG=654127 ==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated code and CL description; PTAL. I reverted the SSLClientSocketImpl change so that we continue to *not* send reports on cert errors. https://codereview.chromium.org/2587243002/diff/1/net/cert/ocsp_verify_result.h File net/cert/ocsp_verify_result.h (right): https://codereview.chromium.org/2587243002/diff/1/net/cert/ocsp_verify_result... net/cert/ocsp_verify_result.h:31: UNKNOWN, On 2016/12/21 01:35:29, Ryan Sleevi wrote: > The risk of "Unknown" here is that it might become more overloaded. Any reason > not to call this something like NOT_CHECKED ? > > (I can see arguments both for & against, just noting the asymmetry between this > and the other response codes) NOT_CHECKED seems reasonable, done. https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... net/http/transport_security_state.cc:669: return std::string(); On 2016/12/21 01:35:29, Ryan Sleevi wrote: > nit: Any reason there's not a NOTREACHED() here as well? Not that I can see, added one. https://codereview.chromium.org/2587243002/diff/1/net/http/transport_security... net/http/transport_security_state.cc:814: ssl_info.ocsp_result.revocation_status == OCSPRevocationStatus::GOOD)) { On 2016/12/21 01:35:29, Ryan Sleevi wrote: > suggestion: I would actually break these into two conditionals, since it's > logically two different criteria, even if it's the same behaviour > > // No report needed if OCSP details were not checked on this connection. > if (ssl_info.ocsp_result.response_status == OCSPVerifyResult::UNKNOWN) > return; > > // No report needed if a stapled OCSP response was provided and it was valid. > if (...) > return; > > > (The above nit is a comment on the comment - it's not just stapling - it's a > valid stapled response) Done. https://codereview.chromium.org/2587243002/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2587243002/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_impl.cc:1324: SSLInfo ssl_info; On 2016/12/21 01:35:29, Ryan Sleevi wrote: > Might by worth including a comment here in the code explaining the intentional > subtlety here (about sending reports on bad certificates) Ack, no longer applicable (reverted this change) https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:9633: // but not provide any OCSP information. On 2016/12/21 01:35:29, Ryan Sleevi wrote: > The MockCertVerifier doesn't accept this certificate - at least, not per line > 9639. The comment seems to conflict with 9638. > > // Set up a MockCertVerifier to report an error for the certificate and indicate > no OCSP information was provided. > > (Or something better worded, I'm awful here.) > > You could also likely delete 9638 with that. Done. https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:9642: verify_result.ocsp_result.response_status = OCSPVerifyResult::MISSING; On 2016/12/21 01:35:29, Ryan Sleevi wrote: > Is this really an accurate simulation? Your description suggests that an error > code from a CertVerifier would result in the SSLSocket not checking the > OCSPVerifyResult, rather than saying its missing (at least, judging by the > comment in response_status) This test is for the first connection that hits a cert error, not the second connection that bypasses it. On the first connection everything is populated as normal (OCSPVerifyResult gets populated in CertVerifyProc regardless of whether there's an error). It's the second connection where we populate CertVerifyStatus with just the cached cert status from allowed_bad_certs when OCSPVerifyResult isn't populated. (https://cs.chromium.org/chromium/src/net/socket/ssl_client_socket_impl.cc?sq=...) https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:9646: // Catch the Expect-Staple report. On 2016/12/21 01:35:29, Ryan Sleevi wrote: > This comment reads a little weird, because this code block isn't catching > anything, it's just setting up the code to listen for the report. Changed to "Set up a mock report sender to..." https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:9653: MockHostResolver host_resolver; On 2016/12/21 01:35:29, Ryan Sleevi wrote: > Logically, it seems like there should be a newline here between 9653 and 9654 - > because 9654+ has no relation to what the comment describes. > > Comment wise, it seems less ideal to describe what MockHostResolver does > (implementation wise) than it is to describe what you're doing. > > // Force |kExpectStapleStaticHostname| to resolve to |https_test_server|. > Done. https://codereview.chromium.org/2587243002/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:9662: // Now send a request to trigger the violation. On 2016/12/21 01:35:29, Ryan Sleevi wrote: > What violation? What is it violating? That's the first usage in this context, > and it's not clear. > > // Make a connection to |kExpectStapleStaticHostname|. Because the > |verify_result| used with > // the |cert_verifier| will report a stapled OCSP response was missing, an > Expect-Staple report > // should be sent using |mock_report_sender|. Done. https://codereview.chromium.org/2587243002/diff/40001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/2587243002/diff/40001/net/url_request/url_req... net/url_request/url_request_unittest.cc:9622: // Tests that Expect-Staple reports are not sent for connections on which there Just in case this is confusing: In PS #1, this was a test that reports *are* sent when encountering a cert error. In PS #2, I reverted the SSLClientSocketImpl change to go back to the behavior where reports are *not* sent when encountering a cert error. So this is now a test that reports are *not* sent (which was the existing behavior, but previously untested).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM! Thanks for adding the tests, and definitely think we should keep exploring on how to make this most useful to site operators, especially when things are 'hinky' :)
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482445935023270,
"parent_rev": "6342d10b42f6591b932c5e5008de94e75bc6b2ba", "commit_rev":
"86921c869f1f45321248c0dd8f26143c58cd135f"}
Message was sent while issue was closed.
Description was changed from ========== Do not do Expect-Staple when OCSPVerifyResult has not been populated The OCSPVerifyResult is not always populated on a connection, for example when a certificate error has been bypassed. This CL adds a new response status that allows us to distinguish whether or not OCSP details have been checked on the connection, and we no longer send reports when they haven't been checked. This CL also adds a test that reports are not sent when there is a certificate error, as is the case when first encountering a cert error, before it has been bypassed. BUG=654127 ========== to ========== Do not do Expect-Staple when OCSPVerifyResult has not been populated The OCSPVerifyResult is not always populated on a connection, for example when a certificate error has been bypassed. This CL adds a new response status that allows us to distinguish whether or not OCSP details have been checked on the connection, and we no longer send reports when they haven't been checked. This CL also adds a test that reports are not sent when there is a certificate error, as is the case when first encountering a cert error, before it has been bypassed. BUG=654127 Review-Url: https://codereview.chromium.org/2587243002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Do not do Expect-Staple when OCSPVerifyResult has not been populated The OCSPVerifyResult is not always populated on a connection, for example when a certificate error has been bypassed. This CL adds a new response status that allows us to distinguish whether or not OCSP details have been checked on the connection, and we no longer send reports when they haven't been checked. This CL also adds a test that reports are not sent when there is a certificate error, as is the case when first encountering a cert error, before it has been bypassed. BUG=654127 Review-Url: https://codereview.chromium.org/2587243002 ========== to ========== Do not do Expect-Staple when OCSPVerifyResult has not been populated The OCSPVerifyResult is not always populated on a connection, for example when a certificate error has been bypassed. This CL adds a new response status that allows us to distinguish whether or not OCSP details have been checked on the connection, and we no longer send reports when they haven't been checked. This CL also adds a test that reports are not sent when there is a certificate error, as is the case when first encountering a cert error, before it has been bypassed. BUG=654127 Committed: https://crrev.com/13e0b315a37468b305d5504caab8f3dd3691f3ba Cr-Commit-Position: refs/heads/master@{#440547} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/13e0b315a37468b305d5504caab8f3dd3691f3ba Cr-Commit-Position: refs/heads/master@{#440547} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
