Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
282873003:
Handle max-age in HPKP. (Closed)
|
Created:
6 years, 7 months ago by palmer Modified:
6 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHandle max-age in HPKP.
BUG=243865
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270716
Patch Set 1 #
Total comments: 1
Patch Set 2 : Test the current "exact of a preload wins" policy. #
Total comments: 2
Patch Set 3 : Test bad hashes to force a pin validation failure. #
Total comments: 7
Patch Set 4 : Fix nits. #
Messages
Total messages: 19 (0 generated)
PTAL. Thanks!
https://codereview.chromium.org/282873003/diff/1/net/http/http_security_heade... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/282873003/diff/1/net/http/http_security_heade... net/http/http_security_headers_unittest.cc:631: } More testing: Good that you test the static/dynamic state, but what's the expected result for CheckPublicKeyPins and the like? Expected: Disabled (that's what we said in the spec, right?)
> More testing: Good that you test the static/dynamic state, but what's the > expected result for CheckPublicKeyPins and the like? > > Expected: Disabled (that's what we said in the spec, right?) Actually, no; the spec is "completely implementation-defined" as of now (per previous mailing list agreement). Currently, our implementation expects for exact hostname matches for preloads to always win. Changing that policy is a bit of a big deal and might have more far-reaching consequences that we should consider first. For this CL, I concern myself with only dynamic state, to at least get us to where we *can* consider any more far-reaching policy.
On 2014/05/13 21:23:24, Chromium Palmer wrote: > > More testing: Good that you test the static/dynamic state, but what's the > > expected result for CheckPublicKeyPins and the like? > > > > Expected: Disabled (that's what we said in the spec, right?) > > Actually, no; the spec is "completely implementation-defined" as of now (per > previous mailing list agreement). So then, what is our present policy, and can you add a test to make sure we're consistent with that policy until we decide to change said policy? :) > > Currently, our implementation expects for exact hostname matches for preloads to > always win. Changing that policy is a bit of a big deal and might have more > far-reaching consequences that we should consider first. > > For this CL, I concern myself with only dynamic state, to at least get us to > where we *can* consider any more far-reaching policy.
> So then, what is our present policy, and can you add a test to make sure we're > consistent with that policy until we decide to change said policy? :) Added a test.
https://codereview.chromium.org/282873003/diff/20001/net/http/http_security_h... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/282873003/diff/20001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:640: EXPECT_EQ(0UL, failure_log.length()); So, this doesn't seem like it actually tests the right thing. Because you did max-age = 0, this would also 'succeed' if static pins were disabled (and I would expect 621 would continue to succeed) Instead, seems like you should test something that is explicitly bad. If it fails, it's because static pins still apply. If it succeeds, it's because no pins apply (ergo dynamic state winning)
https://codereview.chromium.org/282873003/diff/20001/net/http/http_security_h... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/282873003/diff/20001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:640: EXPECT_EQ(0UL, failure_log.length()); > So, this doesn't seem like it actually tests the right thing. Because you did > max-age = 0, this would also 'succeed' if static pins were disabled (and I would > expect 621 would continue to succeed) It *should* still succeed if static pins were disabled — that would happen because we decided we should disable static pins for some reason. > Instead, seems like you should test something that is explicitly bad. If it > fails, it's because static pins still apply. If it succeeds, it's because no > pins apply (ergo dynamic state winning) I don't think I understand. The current/historical policy is that static pins for an exact domain match always apply. So how could I formulate such a test? Try one with a wildcard in the static policy and a specific subdomain in the dynamic policy? Is that what you mean?
On 2014/05/14 00:23:51, Chromium Palmer wrote: > https://codereview.chromium.org/282873003/diff/20001/net/http/http_security_h... > File net/http/http_security_headers_unittest.cc (right): > > https://codereview.chromium.org/282873003/diff/20001/net/http/http_security_h... > net/http/http_security_headers_unittest.cc:640: EXPECT_EQ(0UL, > failure_log.length()); > > So, this doesn't seem like it actually tests the right thing. Because you did > > max-age = 0, this would also 'succeed' if static pins were disabled (and I > would > > expect 621 would continue to succeed) > > It *should* still succeed if static pins were disabled — that would happen > because we decided we should disable static pins for some reason. Let me phrase it differently If we said dynamic max-age=0 should disable static pins, this test would succeed. If we said static pins should still apply, this test would succeed. Thus it doesn't really test the policy. What we've said is dynamic max-age=0 should NOT disable static pins. The only way to test that static pins are NOT disabled is to test something that will only return a result if static pins are still enabled. That is, testing a BAD hash will show static pins ARE enabled, because it will be rejected. Testing a GOOD hash does not provide any new information, since a GOOD hash will succeed if static pins are enabled and a GOOD hash will succeed if static pins are disabled. > > > Instead, seems like you should test something that is explicitly bad. If it > > fails, it's because static pins still apply. If it succeeds, it's because no > > pins apply (ergo dynamic state winning) > > I don't think I understand. The current/historical policy is that static pins > for an exact domain match always apply. So how could I formulate such a test? > Try one with a wildcard in the static policy and a specific subdomain in the > dynamic policy? Is that what you mean?
> What we've said is dynamic max-age=0 should NOT disable static pins. > > The only way to test that static pins are NOT disabled is to test something that > will only return a result if static pins are still enabled. > > That is, testing a BAD hash will show static pins ARE enabled, because it will > be rejected. Testing a GOOD hash does not provide any new information, since a > GOOD hash will succeed if static pins are enabled and a GOOD hash will succeed > if static pins are disabled. Testing bad hashes now.
LGTM https://codereview.chromium.org/282873003/diff/40001/net/http/http_security_h... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/282873003/diff/40001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:582: EXPECT_TRUE( ASSERT_TRUE? (I.e. the test is boned if this is false, right?) https://codereview.chromium.org/282873003/diff/40001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:602: domain, sni_enabled, &new_static_domain_state)); EXPECT_EQ(saved_hashes.size(), new_static_domain_state.pkp.spki_hashes.size()); https://codereview.chromium.org/282873003/diff/40001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:622: domain, sni_enabled, &new_static_domain_state2)); ditto about checking the length too. https://codereview.chromium.org/282873003/diff/40001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:639: new_static_domain_state2.pkp.spki_hashes[0].data()[0] = 0; ^= 0x80 rather than setting the byte to zero as setting a byte fails to damage the hash ~0.5% of the time.
https://codereview.chromium.org/282873003/diff/40001/net/http/http_security_h... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/282873003/diff/40001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:582: EXPECT_TRUE( On 2014/05/15 00:01:53, agl wrote: > ASSERT_TRUE? (I.e. the test is boned if this is false, right?) Done. https://codereview.chromium.org/282873003/diff/40001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:602: domain, sni_enabled, &new_static_domain_state)); On 2014/05/15 00:01:53, agl wrote: > EXPECT_EQ(saved_hashes.size(), new_static_domain_state.pkp.spki_hashes.size()); Done. https://codereview.chromium.org/282873003/diff/40001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:622: domain, sni_enabled, &new_static_domain_state2)); On 2014/05/15 00:01:53, agl wrote: > ditto about checking the length too. Done.
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/palmer@chromium.org/282873003/60001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/palmer@chromium.org/282873003/60001
Message was sent while issue was closed.
Change committed as 270716 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
