Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1454993002:
QUIC - Code to verify SCT tag with certificate transparency verifier (Closed)
|
Created:
5 years, 1 month ago by ramant (doing other things) Modified:
5 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Eran Messeri Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionQUIC - Code to verify SCT tag with certificate transparency verifier
(CTVerifier). SCT tag is verified only if it exists.
Added unittests that verifies a known SCT (a string returned
by ct::GetTestSignedCertificateTimestamp()) that are
similar to MultiLogCTVerifierTest. Used the same certs
as SCT unittests.
TODO: Enable code that requests SCT tag from server.
R=rch@chromium.org, eranm@chromium.org
Committed: https://crrev.com/052774e6549b4b224e4678110c35e16cbb75ae49
Cr-Commit-Position: refs/heads/master@{#361439}
Patch Set 1 : #
Total comments: 13
Patch Set 2 : Fix comments and add tests for SCTList #
Total comments: 8
Patch Set 3 : rebase with TOT - use scoped_refptr<const CTLogVerifier> #Depends on Patchset: Messages
Total messages: 61 (31 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== QUIC - Code to verify SCT tag with certificate transparency verifier (CTVerifier). SCT tag is verified only if it exists. Added a FakeCTVerifier that verifies a known SCT (a string returned by ct::GetTestSignedCertificateTimestamp()). TODO: Enable code that requests SCT tag from server. Decide if we should propogate the error to the UI. ========== to ========== QUIC - Code to verify SCT tag with certificate transparency verifier (CTVerifier). SCT tag is verified only if it exists. Added a FakeCTVerifier that verifies a known SCT (a string returned by ct::GetTestSignedCertificateTimestamp()). TODO: Enable code that requests SCT tag from server. ==========
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993002/80001
Description was changed from ========== QUIC - Code to verify SCT tag with certificate transparency verifier (CTVerifier). SCT tag is verified only if it exists. Added a FakeCTVerifier that verifies a known SCT (a string returned by ct::GetTestSignedCertificateTimestamp()). TODO: Enable code that requests SCT tag from server. ========== to ========== QUIC - Code to verify SCT tag with certificate transparency verifier (CTVerifier). SCT tag is verified only if it exists. Added a FakeCTVerifier that verifies a known SCT (a string returned by ct::GetTestSignedCertificateTimestamp()). TODO: Enable code that requests SCT tag from server. R=rch@chromium.org ==========
rtenneti@chromium.org changed reviewers: + rch@chromium.org
Hi Ryan and Eran, This is a first cut verification of SCT. + If a non-empty SCT is sent from server, then will perform certificate transparency verification (CTVerifier.Verify()). + An error from CTVerfier is not fatal (ala openssl/nss). + The result from CTVerifier is sent to policy_enforcer. + the SCT list returned in CTVerifierResult is copied into SSLInfo object (ala openssl/nss). Will submit another CL that deletes the duplicated code between openssl/nss by moving it into a new ssl_util.cc file. PTAL. thanks raman
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eranm@chromium.org changed reviewers: + eranm@chromium.org
LGTM from CT perspective - thanks a lot for doing this so quickly! A few minor comments about tests. https://codereview.chromium.org/1454993002/diff/80001/net/quic/crypto/proof_t... File net/quic/crypto/proof_test.cc (right): https://codereview.chromium.org/1454993002/diff/80001/net/quic/crypto/proof_t... net/quic/crypto/proof_test.cc:147: // We don't generate errors for corrupt SCT. Can you / should you dig into the ProofVerifyDetails to verify that the ct_verify_result contain the SCT as an invalid SCT in this case, and valid one in other cases? I noticed that ct_verify_result was only added to the ChromiumProofVerifyDetails, not ProofVerifyDetails. https://codereview.chromium.org/1454993002/diff/80001/net/quic/crypto/proof_v... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/1454993002/diff/80001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:150: TEST(ProofVerifierChromiumTest, FailsIfCertFails) { Should there be another test case with non-null CTVerifier to make sure what it returns get stored in the ProofVerifyDetails?
Also, raullenchai suggested I file a tracking bug for this work, so I've filed https://code.google.com/p/chromium/issues/detail?id=557632.
Thanks for doing this! https://codereview.chromium.org/1454993002/diff/80001/net/quic/crypto/proof_v... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/1454993002/diff/80001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:150: TEST(ProofVerifierChromiumTest, FailsIfCertFails) { On 2015/11/18 11:14:00, Eran Messeri wrote: > Should there be another test case with non-null CTVerifier to make sure what it > returns get stored in the ProofVerifyDetails? Agreed. https://codereview.chromium.org/1454993002/diff/80001/net/quic/quic_chromium_... File net/quic/quic_chromium_client_session.cc (right): https://codereview.chromium.org/1454993002/diff/80001/net/quic/quic_chromium_... net/quic/quic_chromium_client_session.cc:505: // TODO(rtenneti): Move the following code into ssl_util.cc a new file. +1. Or, it could be a static method of SSLInfo: SSLInfo::UpdateSignedCertificateTimestamps(info, ct_verify_result_); https://codereview.chromium.org/1454993002/diff/80001/net/quic/quic_chromium_... net/quic/quic_chromium_client_session.cc:508: iter != ct_verify_result_->verified_scts.end(); ++iter) { Can you use a c++11 range based for loop here: for (const auto& sct : ct_verify_result_->verified_scts) { ssl_info->signed_certificate_timestamps.push_back( SignedCertificateTimestampAndStatus(sct, ct::SCT_STATUS_OK)); } https://codereview.chromium.org/1454993002/diff/80001/net/quic/test_tools/cry... File net/quic/test_tools/crypto_test_utils.h (right): https://codereview.chromium.org/1454993002/diff/80001/net/quic/test_tools/cry... net/quic/test_tools/crypto_test_utils.h:155: static CTVerifier* CTVerifierTesting(); Since this code is shared, I'm not sure this method belongs here. https://codereview.chromium.org/1454993002/diff/80001/net/tools/quic/quic_cli... File net/tools/quic/quic_client_bin.cc (right): https://codereview.chromium.org/1454993002/diff/80001/net/tools/quic/quic_cli... net/tools/quic/quic_client_bin.cc:245: cert_verifier.get(), nullptr, transport_security_state.get(), nullptr); Can we use a real CTVerifier?
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993002/180001
Description was changed from ========== QUIC - Code to verify SCT tag with certificate transparency verifier (CTVerifier). SCT tag is verified only if it exists. Added a FakeCTVerifier that verifies a known SCT (a string returned by ct::GetTestSignedCertificateTimestamp()). TODO: Enable code that requests SCT tag from server. R=rch@chromium.org ========== to ========== QUIC - Code to verify SCT tag with certificate transparency verifier (CTVerifier). SCT tag is verified only if it exists. Added unittests that verifies a known SCT (a string returned by ct::GetTestSignedCertificateTimestamp()) that are similar to MultiLogCTVerifierTest. Used the same certs as SCT unittests. TODO: Enable code that requests SCT tag from server. R=rch@chromium.org, eranm@chromium.org ==========
Hi Ryan and Eran, Sorry it took so long. Added unit tests for SCTList based on unittests in net/ct area. Used the same certificates as those tests for testing SCT (another approach is to write these unittests is to figure out the SCT data in quic certificates and build SCT list based on that). Undid all my changes to common code. All these changes are chromium specific only. PTAL. thanks raman https://codereview.chromium.org/1454993002/diff/80001/net/quic/crypto/proof_t... File net/quic/crypto/proof_test.cc (right): https://codereview.chromium.org/1454993002/diff/80001/net/quic/crypto/proof_t... net/quic/crypto/proof_test.cc:147: // We don't generate errors for corrupt SCT. On 2015/11/18 11:14:00, Eran Messeri wrote: > Can you / should you dig into the ProofVerifyDetails to verify that the > ct_verify_result contain the SCT as an invalid SCT in this case, and valid one > in other cases? > I noticed that ct_verify_result was only added to the > ChromiumProofVerifyDetails, not ProofVerifyDetails. Hi Eran, This is shared code with internal server. Reverted changes to this file and added tests to chromium version of these tests. https://codereview.chromium.org/1454993002/diff/80001/net/quic/crypto/proof_v... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/1454993002/diff/80001/net/quic/crypto/proof_v... net/quic/crypto/proof_verifier_chromium_test.cc:150: TEST(ProofVerifierChromiumTest, FailsIfCertFails) { On 2015/11/18 20:57:26, Ryan Hamilton wrote: > On 2015/11/18 11:14:00, Eran Messeri wrote: > > Should there be another test case with non-null CTVerifier to make sure what > it > > returns get stored in the ProofVerifyDetails? > > Agreed. Good point. Added tests. Done. https://codereview.chromium.org/1454993002/diff/80001/net/quic/quic_chromium_... File net/quic/quic_chromium_client_session.cc (right): https://codereview.chromium.org/1454993002/diff/80001/net/quic/quic_chromium_... net/quic/quic_chromium_client_session.cc:505: // TODO(rtenneti): Move the following code into ssl_util.cc a new file. On 2015/11/18 20:57:26, Ryan Hamilton wrote: > +1. Or, it could be a static method of SSLInfo: > > SSLInfo::UpdateSignedCertificateTimestamps(info, ct_verify_result_); > Done. https://codereview.chromium.org/1454993002/diff/80001/net/quic/quic_chromium_... net/quic/quic_chromium_client_session.cc:508: iter != ct_verify_result_->verified_scts.end(); ++iter) { On 2015/11/18 20:57:26, Ryan Hamilton wrote: > Can you use a c++11 range based for loop here: > > for (const auto& sct : ct_verify_result_->verified_scts) { > ssl_info->signed_certificate_timestamps.push_back( > SignedCertificateTimestampAndStatus(sct, ct::SCT_STATUS_OK)); > } Done. https://codereview.chromium.org/1454993002/diff/80001/net/quic/test_tools/cry... File net/quic/test_tools/crypto_test_utils.h (right): https://codereview.chromium.org/1454993002/diff/80001/net/quic/test_tools/cry... net/quic/test_tools/crypto_test_utils.h:155: static CTVerifier* CTVerifierTesting(); On 2015/11/18 20:57:26, Ryan Hamilton wrote: > Since this code is shared, I'm not sure this method belongs here. Undid all chromium specific changes to common code. Thanks for the above. Done. https://codereview.chromium.org/1454993002/diff/80001/net/tools/quic/quic_cli... File net/tools/quic/quic_client_bin.cc (right): https://codereview.chromium.org/1454993002/diff/80001/net/tools/quic/quic_cli... net/tools/quic/quic_client_bin.cc:245: cert_verifier.get(), nullptr, transport_security_state.get(), nullptr); On 2015/11/18 20:57:27, Ryan Hamilton wrote: > Can we use a real CTVerifier? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm
Still LGTM, a few minor comments regarding testing. I wouldn't mind that merging the common test code be done in a follow-up change. https://codereview.chromium.org/1454993002/diff/180001/net/quic/crypto/proof_... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/1454993002/diff/180001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium_test.cc:124: static const char kTestCert[] = "quic_test.example.com.crt"; Nit: If you use the SCTList I've emailed you and the test key obtained by ct::GetTestPublicKey, you should be able to call CheckForSingleVerifiedSCTInResult(true) in test cases where you expect the QuicProofVerifier tu succeed with the quic_test.example.com.crt https://codereview.chromium.org/1454993002/diff/180001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium_test.cc:224: EXPECT_EQ(1U, ct_verify_result.unknown_logs_scts.size()); Nit: It depends how you corrupt the SCT - corrupting the log ID would make the SCT get into the unknown_logs_scts list while corrupting the signature would get the SCT in the unverified list. https://codereview.chromium.org/1454993002/diff/180001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium_test.cc:261: DummyProofVerifierCallback* callback = new DummyProofVerifierCallback; Nit: Any reason a scoped_ptr can't be used here? https://codereview.chromium.org/1454993002/diff/180001/net/quic/quic_chromium... File net/quic/quic_chromium_client_session.cc (right): https://codereview.chromium.org/1454993002/diff/180001/net/quic/quic_chromium... net/quic/quic_chromium_client_session.cc:806: ct::CTVerifyResult* ct_verify_result_copy = new ct::CTVerifyResult; Nit: In the past the use of scoped_ptr was recommended to me, any reason not to: scoped_ptr<ct::CTVerifyResult> ct_verify_result_copy(new ct::CTVerifyResult); *ct_verify_result_copy = verify_details_chromium->ct_verify_result; ct_verify_result_ = ct_verify_result_copy.Pass(); ? (even better if the CTVerifyResult had a copy c'tor).
Thanks Eran. Will make your suggestions to use scoped_ptr and then call Pass with CTVerifyResult and CertVerifyResult and in unittests in the following CL and will upload for review. https://codereview.chromium.org/1454993002/diff/180001/net/quic/crypto/proof_... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/1454993002/diff/180001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium_test.cc:124: static const char kTestCert[] = "quic_test.example.com.crt"; On 2015/11/23 17:35:19, Eran Messeri wrote: > Nit: If you use the SCTList I've emailed you and the test key obtained by > ct::GetTestPublicKey, you should be able to call > CheckForSingleVerifiedSCTInResult(true) in test cases where you expect the > QuicProofVerifier tu succeed with the quic_test.example.com.crt Will make this change in the next CL. thanks much. https://codereview.chromium.org/1454993002/diff/180001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium_test.cc:224: EXPECT_EQ(1U, ct_verify_result.unknown_logs_scts.size()); On 2015/11/23 17:35:19, Eran Messeri wrote: > Nit: It depends how you corrupt the SCT - corrupting the log ID would make the > SCT get into the unknown_logs_scts list while corrupting the signature would get > the SCT in the unverified list. Acknowledged. Will add that change in the next CL. https://codereview.chromium.org/1454993002/diff/180001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium_test.cc:261: DummyProofVerifierCallback* callback = new DummyProofVerifierCallback; On 2015/11/23 17:35:19, Eran Messeri wrote: > Nit: Any reason a scoped_ptr can't be used here? Could be done and we could do a Pass (in this test as well as in other tests). Will make that change in the next CL. ProofVerifier takes the ownership of the callback. " In this case, the ProofVerifier will take ownership of |callback|." https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/pr... https://codereview.chromium.org/1454993002/diff/180001/net/quic/quic_chromium... File net/quic/quic_chromium_client_session.cc (right): https://codereview.chromium.org/1454993002/diff/180001/net/quic/quic_chromium... net/quic/quic_chromium_client_session.cc:806: ct::CTVerifyResult* ct_verify_result_copy = new ct::CTVerifyResult; On 2015/11/23 17:35:19, Eran Messeri wrote: > Nit: In the past the use of scoped_ptr was recommended to me, any reason not to: > scoped_ptr<ct::CTVerifyResult> ct_verify_result_copy(new ct::CTVerifyResult); > *ct_verify_result_copy = verify_details_chromium->ct_verify_result; > ct_verify_result_ = ct_verify_result_copy.Pass(); > > ? > > (even better if the CTVerifyResult had a copy c'tor). Acknowledged. Will modify CTVerifyResult and CertVerifyResult to use scoped_ptr and then call Pass(). (I am guessing the previous reviewer preferred the current style of coding. Will switch to using scoped_ptr here and the unittests).
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Heads up - you mail get a merge conflict with https://codereview.chromium.org/1440643002/ as I've made CTLogVerifier instances const in that change, which I'm about to push. If that's the case, references to scoped_refptr<CTLogVerifier> should be converted to scoped_refptr<const CTLogVerifier>
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/11/23 19:53:00, Eran Messeri wrote: > Heads up - you mail get a merge conflict with > https://codereview.chromium.org/1440643002/ as I've made CTLogVerifier instances > const in that change, which I'm about to push. > If that's the case, references to scoped_refptr<CTLogVerifier> should be > converted to scoped_refptr<const CTLogVerifier> Thanks much. Merged with TOT and made the above change.
The CQ bit was checked by rtenneti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org, eranm@chromium.org Link to the patchset: https://codereview.chromium.org/1454993002/#ps200001 (title: "rebase with TOT - use scoped_refptr<const CTLogVerifier>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993002/200001
Message was sent while issue was closed.
Committed patchset #3 (id:200001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/052774e6549b4b224e4678110c35e16cbb75ae49 Cr-Commit-Position: refs/heads/master@{#361439}
Message was sent while issue was closed.
On 2015/11/24 21:00:50, commit-bot: I haz the power wrote: > Patchset 3 (id:??) landed as > https://crrev.com/052774e6549b4b224e4678110c35e16cbb75ae49 > Cr-Commit-Position: refs/heads/master@{#361439} Try and try again till you succeed. :-). |
