Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
433123003:
Centralize the logic for checking public key pins (Closed)
|
Created:
6 years, 4 months ago by Ryan Hamilton Modified:
6 years, 4 months ago CC:
cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionCentralize the logic for checking public key pins from ClientSocketNSS
and ProofVerifierChromium to TransportSecurityState::CheckPublicKeyPins.
This required adding an is_issued_by_known_root argument to this method.
In addition, CheckPublicKeyPins now only checks static pins if the
TransportSecurityState's enable_static_pins_ member is true. This defaults
to true only for official desktop builds. This also means that dynamic
pins are now checked on mobile and on non-official builds.
BUG=398925, 391033
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288435
Patch Set 1 #Patch Set 2 : add comment #
Total comments: 1
Patch Set 3 : Rebase #
Total comments: 15
Patch Set 4 : fix comments #
Total comments: 12
Patch Set 5 : fix comments from agl #
Total comments: 12
Patch Set 6 : Fix comments from sleevi #
Total comments: 14
Patch Set 7 : Fix more comments from sleevi #Patch Set 8 : small cleanup #Patch Set 9 : make IsBuildTimely and ReportUMAOnPinFailure static, as per wtc #
Total comments: 26
Patch Set 10 : git cl format #Patch Set 11 : more fixes #Patch Set 12 : final fixes? Hah! #
Total comments: 7
Patch Set 13 : fix final comments #Patch Set 14 : Rebase #Patch Set 15 : fewer friends #
Messages
Total messages: 37 (0 generated)
https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... net/http/transport_security_state.cc:696: if (!cert_verify_result.is_issued_by_known_root || I wonder if I should pass in the individual fields of cert_verify_result?
Review comments on patch set 3: Thank you very much for writing this CL! 1. After my review is finished, please ask agl,palmer,rsleevi to review this because they own TransportSecurityState. 2. I think TransportSecurityState is the wrong place for the VerifyPinning method. To TransportSecurityState, VerifyPinning is just a "convenience method" that wraps the HasPublicKeyPins and CheckPublicKeyPins method. I think the best solution is for CertVerifier to verify pinning automatically. This seems to require passing the "sni_available" and "transport_security_state" arguments to the CertVerifier::Verify method. We may be able to pass transport_security_state as an argument to the CertVerifier constructor, but the "sni_available" parameter seems necessary for CertVerifier:Verify. I suggest we first fix the nits in your current approach, and then ask the owners of TransportSecurityState to determine the right API. https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... net/http/transport_security_state.cc:674: Delete the blank line. https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... net/http/transport_security_state.cc:695: const CertStatus cert_status = cert_verify_result.cert_status; IMPORTANT: |cert_status| is not used. It seems that the policy of verifying pinning only when the cert either has no errors or has only minor errors, should also be centralized in this method. https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... net/http/transport_security_state.cc:696: if (!cert_verify_result.is_issued_by_known_root || On 2014/08/05 17:42:36, Ryan Hamilton wrote: > I wonder if I should pass in the individual fields of cert_verify_result? That seems like a good idea. I am also tempted to break this into two methods. 1. A policy function that determines whether we should verify pinning or not. 2. A function that verifies pinning. But that might be an overkill. https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... File net/http/transport_security_state.h (right): https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... net/http/transport_security_state.h:256: // |host|, or if the build it out of date, or if the root is unknown. 1. Typo: the build it out of date => the build is out of date 2. "the root is unknown" is confusing. We should say "the root is not a built-in trusted root". 3. Please document the output parameter pinning_failure_log, which is written to on failure. https://codereview.chromium.org/433123003/diff/40001/net/quic/crypto/proof_ve... File net/quic/crypto/proof_verifier_chromium.cc (right): https://codereview.chromium.org/433123003/diff/40001/net/quic/crypto/proof_ve... net/quic/crypto/proof_verifier_chromium.cc:239: const CertVerifyResult& cert_verify_result = Get rid of the |cert_verify_result| local variable. https://codereview.chromium.org/433123003/diff/40001/net/quic/crypto/proof_ve... net/quic/crypto/proof_verifier_chromium.cc:245: } BUG: delete this early return (lines 242-245). https://codereview.chromium.org/433123003/diff/40001/net/quic/crypto/proof_ve... net/quic/crypto/proof_verifier_chromium.cc:250: !transport_security_state_->VerifyPinning( Nit: please make these four lines look exactly the same as the corresponding four lines in ssl_client_socket_nss.cc.
https://codereview.chromium.org/433123003/diff/20001/net/quic/crypto/proof_ve... File net/quic/crypto/proof_verifier_chromium.h (right): https://codereview.chromium.org/433123003/diff/20001/net/quic/crypto/proof_ve... net/quic/crypto/proof_verifier_chromium.h:40: std::string pinning_failure_log; In my preferred API design, pinning_failure_log would become a member of CertVerifyResult.
Addressed your comments. Will ping other reviewers. https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... net/http/transport_security_state.cc:674: On 2014/08/05 18:16:24, wtc wrote: > > Delete the blank line. Done. https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... net/http/transport_security_state.cc:695: const CertStatus cert_status = cert_verify_result.cert_status; On 2014/08/05 18:16:24, wtc wrote: > > IMPORTANT: |cert_status| is not used. It seems that the policy of verifying > pinning only when the cert either has no errors or has only minor errors, should > also be centralized in this method. I thought about that, but it seemed confusing to me because the error code which the caller ends up with needs to be different if there were "real" errors vs, minor errors: The flow is: if (result is major_error) return result; if (!PinsVerify()) return ERR_CERT_PINNING_SOMETHING return OK; To move the first part of this logic into this method would require passing in result and possibly returning it :/ This seems ugly. What do you think? https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... net/http/transport_security_state.cc:696: if (!cert_verify_result.is_issued_by_known_root || On 2014/08/05 18:16:24, wtc wrote: > > On 2014/08/05 17:42:36, Ryan Hamilton wrote: > > I wonder if I should pass in the individual fields of cert_verify_result? > > That seems like a good idea. I am also tempted to break this into two methods. > > 1. A policy function that determines whether we should verify pinning or not. > > 2. A function that verifies pinning. > > But that might be an overkill. > OK, changed the signature but kept it as one method. https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... File net/http/transport_security_state.h (right): https://codereview.chromium.org/433123003/diff/40001/net/http/transport_secur... net/http/transport_security_state.h:256: // |host|, or if the build it out of date, or if the root is unknown. On 2014/08/05 18:16:24, wtc wrote: > > 1. Typo: the build it out of date => the build is out of date > > 2. "the root is unknown" is confusing. We should say "the root is not a built-in > trusted root". > > 3. Please document the output parameter pinning_failure_log, which is written to > on failure. Done. https://codereview.chromium.org/433123003/diff/40001/net/quic/crypto/proof_ve... File net/quic/crypto/proof_verifier_chromium.cc (right): https://codereview.chromium.org/433123003/diff/40001/net/quic/crypto/proof_ve... net/quic/crypto/proof_verifier_chromium.cc:239: const CertVerifyResult& cert_verify_result = On 2014/08/05 18:16:24, wtc wrote: > > Get rid of the |cert_verify_result| local variable. Discussed offline. Keeping. https://codereview.chromium.org/433123003/diff/40001/net/quic/crypto/proof_ve... net/quic/crypto/proof_verifier_chromium.cc:245: } On 2014/08/05 18:16:24, wtc wrote: > > BUG: delete this early return (lines 242-245). Done. https://codereview.chromium.org/433123003/diff/40001/net/quic/crypto/proof_ve... net/quic/crypto/proof_verifier_chromium.cc:250: !transport_security_state_->VerifyPinning( On 2014/08/05 18:16:24, wtc wrote: > > Nit: please make these four lines look exactly the same as the corresponding > four lines in ssl_client_socket_nss.cc. Done.
+agl,palmer,rsleevi: Can you guys take a look at this to figure out the right API for simplifying the pinning check API.
LGTM https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.cc:675: #if defined(OFFICIAL_BUILD) && !defined(OS_ANDROID) && !defined(OS_IOS) Now that this is it's own function, I think this would be better as: #if !defined(OFFICIAL_BUILD) || defined(OS_ANDROID) || defined(OS_IOS) return true; #endif https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.cc:697: } if (!is_issued_by_known_root || !TransportSecurityState::IsBuildTimely() || !HasPublicKeyPins(host, sni_available)) { return true; }
Will take a look. This is not an immediate rush, is it? If not, I'd like to take some time to make sure it's correct, because palmer@ and I just spent a few mega-CLs trying to cleanup TSS.
On 2014/08/07 18:36:49, Ryan Sleevi wrote: > Will take a look. This is not an immediate rush, is it? > > If not, I'd like to take some time to make sure it's correct, because palmer@ > and I just spent a few mega-CLs trying to cleanup TSS. It's a bit of a rush yes, because I am planning to use this CL (or whatever form it takes after you guys critique it) to re-enable pooling. I have a CL which does this here: https://codereview.chromium.org/425803014/ There is desire to get this all in beta, but we may run out of time for that, even if we rush this CL.
https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.cc:675: #if defined(OFFICIAL_BUILD) && !defined(OS_ANDROID) && !defined(OS_IOS) On 2014/08/07 18:32:59, agl wrote: > Now that this is it's own function, I think this would be better as: > > #if !defined(OFFICIAL_BUILD) || defined(OS_ANDROID) || defined(OS_IOS) > return true; > #endif Done. (Though I used "#else" instead of #endif because otherwise, I think we'll get unreachable code errors, right?) https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.cc:697: } On 2014/08/07 18:32:59, agl wrote: > if (!is_issued_by_known_root || > !TransportSecurityState::IsBuildTimely() || > !HasPublicKeyPins(host, sni_available)) { > return true; > } Done.
https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.cc:702: if (CheckPublicKeyPins(host, Does it make sense to just move the logic in this function into |CheckPublicKeyPins|? It seems odd to have both |VerifyPinning| (or |ValidatePinning| and |CheckPublicKeyPins|. https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... File net/http/transport_security_state.h (right): https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.h:255: // Returns true if |public_key_hashes| meets the pinning constrains of typo: "constraints" Can we call it |ValidatePins| or |ValidatePinning|? "Validate" is the verb I use in the HPKP spec and it'd be good to be consistent. https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.h:260: bool is_issued_by_known_root, If the caller knows |is_issued_by_known_root|, then they already know what value this function call will return. So does it make sense to pass it?
https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.cc:676: return true; // TODO(rsleevi): http://crbug.com/391035 - Enable dynamic PKP checks regardless of build type https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.cc:681: // end up with pins that cannot be easily updated. // TODO(rsleevi): http://crbug.com/391035 - Only disable static pins for non-official builds https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.cc:685: // certificate. This TODO(agl) no longer applies, does it? https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.cc:691: // error); This bullet is no longer correct - this is handled by a higher layer. https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.cc:713: TransportSecurityState::ReportUMAOnPinFailure(host); No need for ReportUMAOnPinFailure to be public static now, is there? https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... File net/http/transport_security_state.h (right): https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.h:258: // case of a pinning failure. 1) Comment wise, you're describing too much of the implementation, and the implementation will change. For example "build is out of date" 2) Typo -> constrains -> constraints 3) This belongs with the other 'check/verify' methods of line 164, which makes the difference between these two methods more ambiguous, and perhaps overlapping, but also highlights the difference in your parameter order.
While I work on other comments, I'm curious to get your input on this... https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.cc:702: if (CheckPublicKeyPins(host, On 2014/08/07 18:50:50, Chromium Palmer wrote: > Does it make sense to just move the logic in this function into > |CheckPublicKeyPins|? It seems odd to have both |VerifyPinning| (or > |ValidatePinning| and |CheckPublicKeyPins|. Hm. I'm totally happy to do this and it seems like a sensible idea. However there's one detail that I'm unclear how to best handle. As currently implemented, my ValidatePinning method simply returns true if it's not an official build. This complicates life for testing. Perhaps we could add an argument to the construct (or a setter) which controls how this method behaves. Tests would enable the checks, but the code which actually constructs the "real" TSS could disable it when not official?
https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.cc:702: if (CheckPublicKeyPins(host, On 2014/08/07 20:09:38, Ryan Hamilton wrote: > On 2014/08/07 18:50:50, Chromium Palmer wrote: > > Does it make sense to just move the logic in this function into > > |CheckPublicKeyPins|? It seems odd to have both |VerifyPinning| (or > > |ValidatePinning| and |CheckPublicKeyPins|. > > Hm. I'm totally happy to do this and it seems like a sensible idea. However > there's one detail that I'm unclear how to best handle. As currently > implemented, my ValidatePinning method simply returns true if it's not an > official build. This complicates life for testing. How so / what tests? Our tests don't have OFFICIAL_BUILD set. > > Perhaps we could add an argument to the construct (or a setter) which controls > how this method behaves. Tests would enable the checks, but the code which > actually constructs the "real" TSS could disable it when not official? If this is a merge target, probably not. I don't want you to be forced to fixing bugs, but as noted above, it's a BUG that we #ifdef these out (really, we should only be #ifdef'ing out the static pins) However, that's a bigger (different) CL, and though it's totally where we need to go, I'm not sure if it's worth blocking you on solving. In case it's ambiguous what it'd be: 1) We'd drop the #ifdefs entirely 2) We'd drop the IsBuildTimely check from 695 (it's only applicable to static pins, and GetStaticDomainState() already checks timeliness) 3) GetStaticDomainState() would have the #ifdef to disable it for OFFICIAL_BUILD If we wanted to enable it for builds, it'd be something like GetStaticDomainState(...) if (!StaticPinsEnabled()) return false; logic that looks at kNumPreloaded, etc bool StaticPinsEnabled(...) if (g_some_static_method_for_testing_was_called) return g_some_static_method_for_testing_was_called #if defined(OFFICIAL_BUILD) && ... return false; #else return true; #endif
OK, PTAL, I've reworked this CL slightly as per feedback from sleevi and palmer. https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.cc:702: if (CheckPublicKeyPins(host, On 2014/08/07 20:40:17, Ryan Sleevi wrote: > On 2014/08/07 20:09:38, Ryan Hamilton wrote: > > On 2014/08/07 18:50:50, Chromium Palmer wrote: > > > Does it make sense to just move the logic in this function into > > > |CheckPublicKeyPins|? It seems odd to have both |VerifyPinning| (or > > > |ValidatePinning| and |CheckPublicKeyPins|. > > > > Hm. I'm totally happy to do this and it seems like a sensible idea. However > > there's one detail that I'm unclear how to best handle. As currently > > implemented, my ValidatePinning method simply returns true if it's not an > > official build. This complicates life for testing. > > How so / what tests? Our tests don't have OFFICIAL_BUILD set. > > > > > Perhaps we could add an argument to the construct (or a setter) which controls > > how this method behaves. Tests would enable the checks, but the code which > > actually constructs the "real" TSS could disable it when not official? > > If this is a merge target, probably not. > > I don't want you to be forced to fixing bugs, but as noted above, it's a BUG > that we #ifdef these out (really, we should only be #ifdef'ing out the static > pins) > > However, that's a bigger (different) CL, and though it's totally where we need > to go, I'm not sure if it's worth blocking you on solving. > > In case it's ambiguous what it'd be: > 1) We'd drop the #ifdefs entirely > 2) We'd drop the IsBuildTimely check from 695 (it's only applicable to static > pins, and GetStaticDomainState() already checks timeliness) > 3) GetStaticDomainState() would have the #ifdef to disable it for OFFICIAL_BUILD > > If we wanted to enable it for builds, it'd be something like > GetStaticDomainState(...) > if (!StaticPinsEnabled()) > return false; > logic that looks at kNumPreloaded, etc > > bool StaticPinsEnabled(...) > if (g_some_static_method_for_testing_was_called) > return g_some_static_method_for_testing_was_called > #if defined(OFFICIAL_BUILD) && ... > return false; > #else > return true; > #endif Discussed offline. I *think* I've done what you proposed. https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... File net/http/transport_security_state.h (right): https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.h:255: // Returns true if |public_key_hashes| meets the pinning constrains of On 2014/08/07 18:50:50, Chromium Palmer wrote: > typo: "constraints" > > Can we call it |ValidatePins| or |ValidatePinning|? "Validate" is the verb I use > in the HPKP spec and it'd be good to be consistent. Acknowledged. https://codereview.chromium.org/433123003/diff/60001/net/http/transport_secur... net/http/transport_security_state.h:260: bool is_issued_by_known_root, On 2014/08/07 18:50:50, Chromium Palmer wrote: > If the caller knows |is_issued_by_known_root|, then they already know what value > this function call will return. So does it make sense to pass it? Personally, I think it does because it focuses the logic into this class. We'll end up with a few callers of this method, and I'd prefer to not duplicate any logic since that's just an opportunity to do it incorrectly :> This API forces the caller to figure out if the cert had a known root as opposed to simply ignoring this fact and causing problems later. https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.cc:676: return true; On 2014/08/07 18:58:41, Ryan Sleevi wrote: > // TODO(rsleevi): http://crbug.com/391035 - Enable dynamic PKP checks regardless > of build type Done. https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.cc:681: // end up with pins that cannot be easily updated. On 2014/08/07 18:58:41, Ryan Sleevi wrote: > // TODO(rsleevi): http://crbug.com/391035 - Only disable static pins for > non-official builds Done. https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.cc:685: // certificate. On 2014/08/07 18:58:41, Ryan Sleevi wrote: > This TODO(agl) no longer applies, does it? Done. https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.cc:691: // error); On 2014/08/07 18:58:41, Ryan Sleevi wrote: > This bullet is no longer correct - this is handled by a higher layer. Done. (As is the previous bullet) https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.cc:713: TransportSecurityState::ReportUMAOnPinFailure(host); On 2014/08/07 18:58:41, Ryan Sleevi wrote: > No need for ReportUMAOnPinFailure to be public static now, is there? Done. Same for IsBuildTimely() https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... File net/http/transport_security_state.h (right): https://codereview.chromium.org/433123003/diff/80001/net/http/transport_secur... net/http/transport_security_state.h:258: // case of a pinning failure. On 2014/08/07 18:58:41, Ryan Sleevi wrote: > 1) Comment wise, you're describing too much of the implementation, and the > implementation will change. For example "build is out of date" > > 2) Typo -> constrains -> constraints > > 3) This belongs with the other 'check/verify' methods of line 164, which makes > the difference between these two methods more ambiguous, and perhaps > overlapping, but also highlights the difference in your parameter order. I've removed this method in favor of palmer's suggestion of just exposing this via CheckPublicKeyPins. PTAL.
Mostly LG, some pedantry/comments I feel like you should at least add a test to make sure enable_static_pins_ works as expected - to the TransportSecurityState unittest That is enable_static_pins_ = false GetStaticDomainState for a known-pinned-host make sure it fails enable_static_pins_ = true GetStaticDomainState for a known-pinned host make sure it returns success https://codereview.chromium.org/433123003/diff/120001/net/http/http_security_... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/433123003/diff/120001/net/http/http_security_... net/http/http_security_headers_unittest.cc:504: state.enable_static_pinning_ = true; pedantry: enable_static_pins_ Also, feels like this is better on line 508, since it's set precisely because of the comment. https://codereview.chromium.org/433123003/diff/120001/net/http/http_security_... net/http/http_security_headers_unittest.cc:586: state.enable_static_pinning_ = true; I think this would be better moved to line 590, since that's *why* we're enabling it. https://codereview.chromium.org/433123003/diff/120001/net/http/http_security_... net/http/http_security_headers_unittest.cc:668: state.enable_static_pinning_ = true; ditto on the shuffle - 672 https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... net/http/transport_security_state.cc:159: bool TransportSecurityState::CheckPublicKeyPinsImpl( So, I know you did this for the diff, but move this function in the .cc to match the ordering in the .h before committing. https://codereview.chromium.org/433123003/diff/120001/net/quic/crypto/proof_v... File net/quic/crypto/proof_verifier_chromium.cc (right): https://codereview.chromium.org/433123003/diff/120001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium.cc:247: /* sni_available= */ true, nit: true, /* QUIC always supports SNI */ or at least nit: true, /* sni_available */ ;)
Thanks for pointing out transport_security_state_unittest.cc. I missed that it existed. I modified a number of tests which require static pinning to pass. https://codereview.chromium.org/433123003/diff/120001/net/http/http_security_... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/433123003/diff/120001/net/http/http_security_... net/http/http_security_headers_unittest.cc:504: state.enable_static_pinning_ = true; On 2014/08/07 22:19:07, Ryan Sleevi wrote: > pedantry: enable_static_pins_ > > Also, feels like this is better on line 508, since it's set precisely because of > the comment. Heh, I did that first and then switched it. Should have stuck with my gut. Done. https://codereview.chromium.org/433123003/diff/120001/net/http/http_security_... net/http/http_security_headers_unittest.cc:586: state.enable_static_pinning_ = true; On 2014/08/07 22:19:07, Ryan Sleevi wrote: > I think this would be better moved to line 590, since that's *why* we're > enabling it. Done. https://codereview.chromium.org/433123003/diff/120001/net/http/http_security_... net/http/http_security_headers_unittest.cc:668: state.enable_static_pinning_ = true; On 2014/08/07 22:19:07, Ryan Sleevi wrote: > ditto on the shuffle - 672 Done. https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... net/http/transport_security_state.cc:159: bool TransportSecurityState::CheckPublicKeyPinsImpl( On 2014/08/07 22:19:07, Ryan Sleevi wrote: > So, I know you did this for the diff, but move this function in the .cc to match > the ordering in the .h before committing. Will do. I'll leave it here until I get an LGTM so other reviewers do not have to read the diffs, but I'll move before landing. https://codereview.chromium.org/433123003/diff/120001/net/quic/crypto/proof_v... File net/quic/crypto/proof_verifier_chromium.cc (right): https://codereview.chromium.org/433123003/diff/120001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium.cc:247: /* sni_available= */ true, On 2014/08/07 22:19:07, Ryan Sleevi wrote: > nit: true, /* QUIC always supports SNI */ > > or at least > nit: true, /* sni_available */ > > ;) Done.
Patch set 6 LGTM. Please update ssl_client_socket_openssl.cc. https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... net/http/transport_security_state.cc:141: !TransportSecurityState::IsBuildTimely() || Nit: omit "TransportSecurityState::" https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... File net/http/transport_security_state.h (left): https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... net/http/transport_security_state.h:278: static void ReportUMAOnPinFailure(const std::string& host); Nit: "static" tells me that the method doesn't access any instance members. So it'd be good to keep it.
I think I would prefer to make the openssl changes in a different CL, which I'm happy to start on immediately, if that's OK with you. https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... net/http/transport_security_state.cc:141: !TransportSecurityState::IsBuildTimely() || On 2014/08/07 22:51:44, wtc wrote: > > Nit: omit "TransportSecurityState::" Done. https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... File net/http/transport_security_state.h (left): https://codereview.chromium.org/433123003/diff/120001/net/http/transport_secu... net/http/transport_security_state.h:278: static void ReportUMAOnPinFailure(const std::string& host); On 2014/08/07 22:51:44, wtc wrote: > > Nit: "static" tells me that the method doesn't access any instance members. So > it'd be good to keep it. Done.
https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state.cc:143: return true; Re-git-cl-format this? https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state.cc:811: return false; Ugh and crap. This breaks HSTS. And we didn't have any tests that caught it, presumably, which is double-plus-ungood. Or it just worked because of flipping the bool. I think it just means we move the enable_static_pinning_ check into HasPreload, and don't set the required hashes / excluded hashes (lines 611 onward, which modify out->pkp). With out->pkp empty, you should fall through to line 921, which allows any chain to be valid.
Patch set 9 LGTM. https://codereview.chromium.org/433123003/diff/180001/net/http/http_security_... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/433123003/diff/180001/net/http/http_security_... net/http/http_security_headers_unittest.cc:676: state.enable_static_pinning_ = true; Move this to line 671 to stay close to the comment "xxx has preloaded pins." https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state.h:297: static bool IsBuildTimely(); In the .cc file, these two methods are defined after CheckPublicKeyPinsImpl. Here they are declared before CheckPublicKeyPinsImpl. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state.h:326: bool enable_static_pinning_; Ryan asked you to rename this member "enable_static_pins_". https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:53: static void EnableStaticPinning(TransportSecurityState* state) { These methods (and the EnableStaticPinning test) should be renamed DisableStaticPins and EnableStaticPins if enable_static_pinning_ is renamed enable_static_pins_.
Further comment on the tests, which I think should be golden for this HSTS/HPKP split, with just a 'small' amount of shuffling. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:197: EnableStaticPinning(&state); Let's make this test more explicit about the STS bit vs the PKP part Delete this line, and remove line 203 https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:212: EnableStaticPinning(&state); Remove this https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:227: TransportSecurityStateTest::EnableStaticPinning(&state); Definitely removed - would have caught this bug :) https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:236: TransportSecurityStateTest::EnableStaticPinning(&state); This should be removed, I think. At least from the usage. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:269: EnableStaticPinning(&state); So, we should probably split this in two, with only the latter calling EnableStaticPinning which domains you need to transfer over "should" be easy - the OnlyPinningInStaticState / HasStaticPublicPins tests move over. I'm not sure how we got here, since BuiltinCertPins was meant to be this split (in that it only tests for pinning data). You should be able to do a superbowl shuffle of those few pinned bits into the BuiltinCertPins https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:567: // Disabled in order to help track down pinning failures --agl *Cough* Let's nuke this comment ;) https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:665: EnableStaticPinning(&state); This should be removed. This is an HSTS test.
https://codereview.chromium.org/433123003/diff/180001/net/http/http_security_... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/433123003/diff/180001/net/http/http_security_... net/http/http_security_headers_unittest.cc:676: state.enable_static_pinning_ = true; On 2014/08/07 23:39:12, wtc wrote: > > Move this to line 671 to stay close to the comment "xxx has preloaded pins." Done. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state.cc:143: return true; On 2014/08/07 23:31:19, Ryan Sleevi wrote: > Re-git-cl-format this? Done. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state.cc:811: return false; On 2014/08/07 23:31:19, Ryan Sleevi wrote: > Ugh and crap. > > This breaks HSTS. And we didn't have any tests that caught it, presumably, which > is double-plus-ungood. Or it just worked because of flipping the bool. > > I think it just means we move the enable_static_pinning_ check into HasPreload, > and don't set the required hashes / excluded hashes (lines 611 onward, which > modify out->pkp). With out->pkp empty, you should fall through to line 921, > which allows any chain to be valid. Done. (I hope :>) https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state.h:297: static bool IsBuildTimely(); On 2014/08/07 23:39:12, wtc wrote: > > In the .cc file, these two methods are defined after CheckPublicKeyPinsImpl. > Here they are declared before CheckPublicKeyPinsImpl. Done. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state.h:326: bool enable_static_pinning_; On 2014/08/07 23:39:12, wtc wrote: > > Ryan asked you to rename this member "enable_static_pins_". Done. Thanks, I missed that. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:53: static void EnableStaticPinning(TransportSecurityState* state) { On 2014/08/07 23:39:13, wtc wrote: > > These methods (and the EnableStaticPinning test) should be renamed > DisableStaticPins and EnableStaticPins if enable_static_pinning_ is renamed > enable_static_pins_. Done. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:197: EnableStaticPinning(&state); On 2014/08/07 23:48:40, Ryan Sleevi wrote: > Let's make this test more explicit about the STS bit vs the PKP part > > Delete this line, and remove line 203 Done. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:212: EnableStaticPinning(&state); On 2014/08/07 23:48:39, Ryan Sleevi wrote: > Remove this Done. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:227: TransportSecurityStateTest::EnableStaticPinning(&state); On 2014/08/07 23:48:39, Ryan Sleevi wrote: > Definitely removed - would have caught this bug :) Done. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:236: TransportSecurityStateTest::EnableStaticPinning(&state); On 2014/08/07 23:48:39, Ryan Sleevi wrote: > This should be removed, I think. At least from the usage. Done. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:269: EnableStaticPinning(&state); On 2014/08/07 23:48:40, Ryan Sleevi wrote: > So, we should probably split this in two, with only the latter calling > EnableStaticPinning > > which domains you need to transfer over "should" be easy - the > OnlyPinningInStaticState / HasStaticPublicPins tests move over. > > I'm not sure how we got here, since BuiltinCertPins was meant to be this split > (in that it only tests for pinning data). You should be able to do a superbowl > shuffle of those few pinned bits into the BuiltinCertPins Done. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:567: // Disabled in order to help track down pinning failures --agl On 2014/08/07 23:48:39, Ryan Sleevi wrote: > *Cough* Let's nuke this comment ;) Done. https://codereview.chromium.org/433123003/diff/180001/net/http/transport_secu... net/http/transport_security_state_unittest.cc:665: EnableStaticPinning(&state); On 2014/08/07 23:48:39, Ryan Sleevi wrote: > This should be removed. This is an HSTS test. Done.
LGTM. Please double check with Wan-Teh or Adam, given the modifications.
Patch set 12 LGTM. IMPORTANT: please update the CL's description to reflect the final code. 1. You didn't add a VerifyPinning method. 2. There are at least two behavior changes (CheckPublicKeyPins now checks the three preconditions, and only static pins are disabled in unofficial builds and mobile platforms). https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... net/http/transport_security_state.cc:132: // Perform pin validation if, and only if, all these conditions obtain: I think this comment block should be moved or copied to the header because it is no longer necessary to call IsBuildTimely() and HasPublicKeyPins() before calling this function. https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... net/http/transport_security_state.cc:137: // have some chance to recover). You should document the third condition: HasPublicKeyPins() returns true. https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... net/http/transport_security_state_unittest.cc:480: TEST_F(TransportSecurityStateTest, PreloadedPins) { Why is this test split from the "Preloaded" test?
https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... net/http/transport_security_state_unittest.cc:480: TEST_F(TransportSecurityStateTest, PreloadedPins) { On 2014/08/08 15:36:59, wtc wrote: > > Why is this test split from the "Preloaded" test? Note: I don't object to this change. I was just curious, so a short answer would suffice. Thanks.
Thanks for the review! https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... net/http/transport_security_state.cc:132: // Perform pin validation if, and only if, all these conditions obtain: On 2014/08/08 15:36:59, wtc wrote: > > I think this comment block should be moved or copied to the header because it is > no longer necessary to call IsBuildTimely() and HasPublicKeyPins() before > calling this function. I had a comment very similar to this on the VerifyPinning method that I added, but sleevi requested that I remove it because it was too descriptive of the implementation. I'm totally open to adding this back if you two agree. (Perhaps in a follow up CL so we can get this landed). https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... net/http/transport_security_state.cc:137: // have some chance to recover). On 2014/08/08 15:36:59, wtc wrote: > > You should document the third condition: HasPublicKeyPins() returns true. Done. https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/433123003/diff/150007/net/http/transport_secu... net/http/transport_security_state_unittest.cc:480: TEST_F(TransportSecurityStateTest, PreloadedPins) { On 2014/08/08 15:56:11, wtc wrote: > > On 2014/08/08 15:36:59, wtc wrote: > > > > Why is this test split from the "Preloaded" test? > > Note: I don't object to this change. I was just curious, so a short answer would > suffice. Thanks. Ah, right. So the main difference is that this test enables static pinning, but the other test does not. We wanted to make sure that if static pinning was disabled, the other static behavior still worked. (The initially proposed approach disabled *all* static state, which was undesirable).
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/433123003/290001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/39205) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/44447) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/39207) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/433123003/310001
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/433123003/330001
Message was sent while issue was closed.
Change committed as 288435 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
