Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2205403002:
Add production Cast CRL certificate. (Closed)
DescriptionAdd production Cast CRL certificate.
Modify tests to support testing the production CRLs.
BUG=618463
Committed: https://crrev.com/e0e3a6c65f43c70856bf100eadc1cdaed6adacd1
Cr-Commit-Position: refs/heads/master@{#409933}
Patch Set 1 #
Total comments: 4
Patch Set 2 : TrustStore as explicit dependency for testing verify functions. #
Total comments: 34
Patch Set 3 : Addresses comments #
Total comments: 6
Patch Set 4 : Addresses comments #
Messages
Total messages: 25 (13 generated)
Description was changed from ========== Add production Cast CRL certificate. Modify tests to support testing the production CRLs. BUG=618463 ========== to ========== Add production Cast CRL certificate. Modify tests to support testing the production CRLs. BUG=618463 ==========
ryanchung@chromium.org changed reviewers: + sheretov@chromium.org
lgtm
ryanchung@chromium.org changed reviewers: + eroman@chromium.org
+eroman PTAL thanks.
https://codereview.chromium.org/2205403002/diff/1/components/cast_certificate... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/2205403002/diff/1/components/cast_certificate... components/cast_certificate/cast_cert_validator.cc:58: static void Reinitialize() { GetInstance()->Initialize(); } How about ReinitializeForTest() ? Or make a comment regarding thread-safety, or that this should not ordinarily be called. https://codereview.chromium.org/2205403002/diff/1/components/cast_certificate... File components/cast_certificate/cast_crl_unittest.cc (right): https://codereview.chromium.org/2205403002/diff/1/components/cast_certificate... components/cast_certificate/cast_crl_unittest.cc:93: ResetCRLTrustAnchorForTest(); This seems problematic, in that running the tests can pollute global state for subsequent tests. The trust anchor is not necessarily reset here upon completion of the test (just upon entry). Can this be solved by one of these solutions, in order of preference. (a) Make the trust store an explicit dependency -- this could be done by introducing a *ForTest() flavor of the verification function that takes a TrustStore dependency. The advantage of this approach is tests to not mutate a singleton, and mutation of the singleton no longer needs to be exposed. I think this is my preferred approach. (b) Keep the current organization but instead of exposing a "Set/Reset*" method for test, use a scoped setter. This ensures that override of the trust anchor is short-lived to particular test cases or test fixtures. (c) Call reset in the tear down method for test fixture, so after running the test global state is reset.
https://codereview.chromium.org/2205403002/diff/1/components/cast_certificate... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/2205403002/diff/1/components/cast_certificate... components/cast_certificate/cast_cert_validator.cc:58: static void Reinitialize() { GetInstance()->Initialize(); } On 2016/08/03 22:47:58, eroman wrote: > How about ReinitializeForTest() ? > > Or make a comment regarding thread-safety, or that this should not ordinarily be > called. Removed this code. It's no longer needed due to the change of approach. https://codereview.chromium.org/2205403002/diff/1/components/cast_certificate... File components/cast_certificate/cast_crl_unittest.cc (right): https://codereview.chromium.org/2205403002/diff/1/components/cast_certificate... components/cast_certificate/cast_crl_unittest.cc:93: ResetCRLTrustAnchorForTest(); On 2016/08/03 22:47:58, eroman wrote: > This seems problematic, in that running the tests can pollute global state for > subsequent tests. > The trust anchor is not necessarily reset here upon completion of the test (just > upon entry). > > Can this be solved by one of these solutions, in order of preference. > > (a) Make the trust store an explicit dependency -- this could be done by > introducing a *ForTest() flavor of the verification function that takes a > TrustStore dependency. The advantage of this approach is tests to not mutate a > singleton, and mutation of the singleton no longer needs to be exposed. I think > this is my preferred approach. > > (b) Keep the current organization but instead of exposing a "Set/Reset*" method > for test, use a scoped setter. This ensures that override of the trust anchor is > short-lived to particular test cases or test fixtures. > > (c) Call reset in the tear down method for test fixture, so after running the > test global state is reset. Done. Went with solution a). Thanks!
Thanks for making those changes -- I am happy to see mutation of that singleton gone! LGTM after this round of comments. Note that I am rubber-stamping the trust anchor addition itself, since I don't know the process through which that key-pair was created and stored. I am deferring that part of the review to sheretov, and assuming the usual rigors were followed :) https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:261: net::TrustStore& trust_store) { Please pass this as a pointer (or const reference if that works) https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:343: net::TrustStore& trust_store) { Same comment as elsewhere (pointer or const-reference) https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:356: bool SetTrustAnchorForTest(const std::string& cert) { Can this be deleted now? https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.h (right): https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.h:65: // Verifies a cast device certficate given a chain of DER-encoded certificates. How about adding: ", using the built in Cast trust anchors" https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.h:111: net::TrustStore& trust_store) WARN_UNUSED_RESULT; Same comment as elsewhere (pointer or const-reference) https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.h:125: bool SetTrustAnchorForTest(const std::string& cert) WARN_UNUSED_RESULT; Can this be deleted now? https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... File components/cast_certificate/cast_crl.cc (right): https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl.cc:103: net::TrustStore& trust_store, Please pass this as a pointer instead (or non-const reference if that works) https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl.cc:305: net::TrustStore& trust_store) { Same comment as elsewhere (pointer or const-reference) https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... File components/cast_certificate/cast_crl.h (right): https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl.h:47: // Cast device certificates. How about adding: ", using the built in Cast CRL trust anchors" https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl.h:64: net::TrustStore& trust_store); Same comment as elsewhere (pointer or const-reference) https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl.h:72: bool SetCRLTrustAnchorForTest(const std::string& cert) WARN_UNUSED_RESULT; Can this be deleted now? https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... File components/cast_certificate/cast_crl_unittest.cc (right): https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:18: void AddTrustAnchor(net::TrustStore& trust_store, const std::string& path) { style nit: Make the out parameter a pointer type rather than reference, and make it the last parameter. In fact based on a subsequent comment, I suggest prototyping this as: std::unique_ptr<net::TrustStore> CreateTrustStoreFromFile(const std::string& path) { ... } https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:45: bool use_test_trust_anchors, Instead of the boolean parameter, how about an optional pointer? (where nullptr is interpreted as meaning "use the production trust anchors"): net::TrustStore* cast_trust_store https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:46: net::TrustStore& cast_trust_store) { see comment above. Style-wise should use pointer rather than reference https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:72: bool use_test_trust_anchors, Same comment as above (how about a single parameter). There are advantages to two parameters as you have it (can verify that don't accidentally pass nulptr), but I think based on these tests that is a less useful assertion than the simpler interface. WDYT? https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:99: bool use_test_trust_anchors, same comment https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:136: bool use_test_trust_anchors = test_case.use_test_trust_anchors(); If you follow my recommendation from above, then you would probably want to change this to something like: std::unique_ptr<net::TrustStore> crl_trust_store; std::unique_ptr<net::TrustStore> cast_trust_store; if (test_case.use_test_trust_anchors()) { crl_trust_store = CreateTrustStoreFromFile("certificates/cast_crl_test_root_ca.pem"); cast_trust_store = CreateTrustStoreFromFile("certificates/cast_test_root_ca.pem"); EXPECT_TRUE(crl_trust_store.get()); EXPECT_TRUE(cast_trust_store.get()); } Then the rest of the code proceeds by just passing crl_trst_store.get() and cast_trust_store.get(), since nullptr is interpreted as "use default". WDYT?
The CQ bit was checked by ryanchung@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...
Thanks! https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.cc (right): https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:261: net::TrustStore& trust_store) { On 2016/08/04 19:28:52, eroman wrote: > Please pass this as a pointer (or const reference if that works) Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:343: net::TrustStore& trust_store) { On 2016/08/04 19:28:52, eroman wrote: > Same comment as elsewhere (pointer or const-reference) Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.cc:356: bool SetTrustAnchorForTest(const std::string& cert) { On 2016/08/04 19:28:52, eroman wrote: > Can this be deleted now? Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.h (right): https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.h:65: // Verifies a cast device certficate given a chain of DER-encoded certificates. On 2016/08/04 19:28:52, eroman wrote: > How about adding: ", using the built in Cast trust anchors" Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.h:111: net::TrustStore& trust_store) WARN_UNUSED_RESULT; On 2016/08/04 19:28:52, eroman wrote: > Same comment as elsewhere (pointer or const-reference) Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_cert_validator.h:125: bool SetTrustAnchorForTest(const std::string& cert) WARN_UNUSED_RESULT; On 2016/08/04 19:28:52, eroman wrote: > Can this be deleted now? Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... File components/cast_certificate/cast_crl.cc (right): https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl.cc:103: net::TrustStore& trust_store, On 2016/08/04 19:28:53, eroman wrote: > Please pass this as a pointer instead (or non-const reference if that works) Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl.cc:305: net::TrustStore& trust_store) { On 2016/08/04 19:28:53, eroman wrote: > Same comment as elsewhere (pointer or const-reference) Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... File components/cast_certificate/cast_crl.h (right): https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl.h:47: // Cast device certificates. On 2016/08/04 19:28:53, eroman wrote: > How about adding: ", using the built in Cast CRL trust anchors" Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl.h:64: net::TrustStore& trust_store); On 2016/08/04 19:28:53, eroman wrote: > Same comment as elsewhere (pointer or const-reference) Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl.h:72: bool SetCRLTrustAnchorForTest(const std::string& cert) WARN_UNUSED_RESULT; On 2016/08/04 19:28:53, eroman wrote: > Can this be deleted now? Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... File components/cast_certificate/cast_crl_unittest.cc (right): https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:18: void AddTrustAnchor(net::TrustStore& trust_store, const std::string& path) { On 2016/08/04 19:28:53, eroman wrote: > style nit: Make the out parameter a pointer type rather than reference, and make > it the last parameter. > > In fact based on a subsequent comment, I suggest prototyping this as: > > std::unique_ptr<net::TrustStore> CreateTrustStoreFromFile(const std::string& > path) { > ... > } Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:45: bool use_test_trust_anchors, On 2016/08/04 19:28:53, eroman wrote: > Instead of the boolean parameter, how about an optional pointer? (where nullptr > is interpreted as meaning "use the production trust anchors"): > > net::TrustStore* cast_trust_store Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:46: net::TrustStore& cast_trust_store) { On 2016/08/04 19:28:53, eroman wrote: > see comment above. Style-wise should use pointer rather than reference Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:72: bool use_test_trust_anchors, On 2016/08/04 19:28:53, eroman wrote: > Same comment as above (how about a single parameter). > > There are advantages to two parameters as you have it (can verify that don't > accidentally pass nulptr), but I think based on these tests that is a less > useful assertion than the simpler interface. WDYT? Done. Sounds good. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:99: bool use_test_trust_anchors, On 2016/08/04 19:28:53, eroman wrote: > same comment Done. https://codereview.chromium.org/2205403002/diff/20001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:136: bool use_test_trust_anchors = test_case.use_test_trust_anchors(); On 2016/08/04 19:28:53, eroman wrote: > If you follow my recommendation from above, then you would probably want to > change this to something like: > > std::unique_ptr<net::TrustStore> crl_trust_store; > std::unique_ptr<net::TrustStore> cast_trust_store; > > if (test_case.use_test_trust_anchors()) { > crl_trust_store = > CreateTrustStoreFromFile("certificates/cast_crl_test_root_ca.pem"); > cast_trust_store = > CreateTrustStoreFromFile("certificates/cast_test_root_ca.pem"); > > EXPECT_TRUE(crl_trust_store.get()); > EXPECT_TRUE(cast_trust_store.get()); > } > > Then the rest of the code proceeds by just passing crl_trst_store.get() and > cast_trust_store.get(), since nullptr is interpreted as "use default". > > WDYT? Done. Sounds good.
lgtm https://codereview.chromium.org/2205403002/diff/40001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.h (right): https://codereview.chromium.org/2205403002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.h:66: // using the built in Cast trust anchors. nit: "built in" --> "built-in" https://codereview.chromium.org/2205403002/diff/40001/components/cast_certifi... File components/cast_certificate/cast_crl_unittest.cc (right): https://codereview.chromium.org/2205403002/diff/40001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:16: // Adds a trust anchor to the provided trust store. The input is the path of How about: Creates a trust store using the test roots encoded in the PEM file at |path| https://codereview.chromium.org/2205403002/diff/40001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:23: EXPECT_EQ(crl_test_root.size(), 1u); [optional] Seems reasonable to allow more than 1 certificate, and just loop over them below.
https://codereview.chromium.org/2205403002/diff/40001/components/cast_certifi... File components/cast_certificate/cast_cert_validator.h (right): https://codereview.chromium.org/2205403002/diff/40001/components/cast_certifi... components/cast_certificate/cast_cert_validator.h:66: // using the built in Cast trust anchors. On 2016/08/04 22:03:12, eroman wrote: > nit: "built in" --> "built-in" Done. https://codereview.chromium.org/2205403002/diff/40001/components/cast_certifi... File components/cast_certificate/cast_crl_unittest.cc (right): https://codereview.chromium.org/2205403002/diff/40001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:16: // Adds a trust anchor to the provided trust store. The input is the path of On 2016/08/04 22:03:12, eroman wrote: > How about: > > Creates a trust store using the test roots encoded in the PEM file at |path| Done. https://codereview.chromium.org/2205403002/diff/40001/components/cast_certifi... components/cast_certificate/cast_crl_unittest.cc:23: EXPECT_EQ(crl_test_root.size(), 1u); On 2016/08/04 22:03:12, eroman wrote: > [optional] Seems reasonable to allow more than 1 certificate, and just loop over > them below. Done.
The CQ bit was checked by ryanchung@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ryanchung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sheretov@chromium.org, eroman@chromium.org Link to the patchset: https://codereview.chromium.org/2205403002/#ps60001 (title: "Addresses comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add production Cast CRL certificate. Modify tests to support testing the production CRLs. BUG=618463 ========== to ========== Add production Cast CRL certificate. Modify tests to support testing the production CRLs. BUG=618463 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add production Cast CRL certificate. Modify tests to support testing the production CRLs. BUG=618463 ========== to ========== Add production Cast CRL certificate. Modify tests to support testing the production CRLs. BUG=618463 Committed: https://crrev.com/e0e3a6c65f43c70856bf100eadc1cdaed6adacd1 Cr-Commit-Position: refs/heads/master@{#409933} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e0e3a6c65f43c70856bf100eadc1cdaed6adacd1 Cr-Commit-Position: refs/heads/master@{#409933} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
