Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(162)

Issue 2182533002: Adds a VerifyAuditProof method to CTLogVerifier (Closed)

Created:
4 years, 5 months ago by Rob Percival
Modified:
4 years, 1 month ago
CC:
chromium-reviews, rsleevi+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, 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.

Description

Adds a VerifyAuditProof method to CTLogVerifier This allows audit (aka. inclusion) proofs to be verified, which helps check that CT logs are behaving correctly. BUG=631087 Committed: https://crrev.com/89f010d82559c73e7d23cbf067137ceb1df698df Cr-Commit-Position: refs/heads/master@{#430264}

Patch Set 1 #

Total comments: 1

Patch Set 2 : Rebase #

Total comments: 5

Patch Set 3 : Switch from flag to bool constant #

Total comments: 2

Patch Set 4 : Removes hash cache #

Total comments: 20

Patch Set 5 : Rebase #

Patch Set 6 : Addresses some of Ryan Sleevi's comments #

Total comments: 16

Patch Set 7 : Rebase #

Patch Set 8 : Addresses some minor comments #

Patch Set 9 : More extensive refactoring #

Total comments: 4

Patch Set 10 : Update RFC6962-bis link #

Total comments: 15

Patch Set 11 : Addresses David's comments #

Total comments: 4

Patch Set 12 : Better documentation for MerkleAuditProof #

Patch Set 13 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+577 lines, -252 lines) Patch
M net/cert/ct_log_verifier.h View 1 2 3 4 5 6 2 chunks +10 lines, -3 lines 0 comments Download
M net/cert/ct_log_verifier.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +58 lines, -0 lines 0 comments Download
M net/cert/ct_log_verifier_unittest.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +499 lines, -248 lines 0 comments Download
M net/cert/merkle_audit_proof.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -0 lines 0 comments Download
M net/cert/merkle_audit_proof.cc View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 62 (36 generated)
Rob Percival
PTAL https://codereview.chromium.org/2182533002/diff/20001/net/cert/ct_log_verifier_unittest.cc File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2182533002/diff/20001/net/cert/ct_log_verifier_unittest.cc#newcode213 net/cert/ct_log_verifier_unittest.cc:213: // Test vectors use a 1-based leaf index, ...
4 years, 5 months ago (2016-07-25 16:29:37 UTC) #5
Ryan Sleevi
Please don't combine the caching CL with this, or at least, it doesn't seem related ...
4 years, 5 months ago (2016-07-25 19:41:41 UTC) #9
Ryan Sleevi
https://codereview.chromium.org/2182533002/diff/40001/net/cert/ct_log_verifier.cc File net/cert/ct_log_verifier.cc (right): https://codereview.chromium.org/2182533002/diff/40001/net/cert/ct_log_verifier.cc#newcode261 net/cert/ct_log_verifier.cc:261: uint64_t sn = proof.tree_size - 1; DESIGN: From this ...
4 years, 5 months ago (2016-07-25 19:44:54 UTC) #10
Rob Percival
https://codereview.chromium.org/2182533002/diff/40001/net/cert/ct_log_verifier_unittest.cc File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2182533002/diff/40001/net/cert/ct_log_verifier_unittest.cc#newcode36 net/cert/ct_log_verifier_unittest.cc:36: const char* kNoHashCacheFlag = "no-hash-cache"; On 2016/07/25 19:41:41, Ryan ...
4 years, 4 months ago (2016-07-26 14:24:29 UTC) #11
Eran Messeri
overall looks good, I'll wait until https://codereview.chromium.org/2178153003/ is reviewed and submitted before thoroughly reviewing the ...
4 years, 4 months ago (2016-07-26 15:52:41 UTC) #12
Ryan Sleevi
https://codereview.chromium.org/2182533002/diff/100001/net/cert/ct_log_verifier.cc File net/cert/ct_log_verifier.cc (right): https://codereview.chromium.org/2182533002/diff/100001/net/cert/ct_log_verifier.cc#newcode11 net/cert/ct_log_verifier.cc:11: #include <vector> Unnecessary https://codereview.chromium.org/2182533002/diff/100001/net/cert/ct_log_verifier_unittest.cc File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/2182533002/diff/100001/net/cert/ct_log_verifier_unittest.cc#newcode13 net/cert/ct_log_verifier_unittest.cc:13: ...
4 years, 4 months ago (2016-07-26 19:33:45 UTC) #15
Rob Percival
https://codereview.chromium.org/2182533002/diff/60001/net/cert/ct_log_verifier.cc File net/cert/ct_log_verifier.cc (right): https://codereview.chromium.org/2182533002/diff/60001/net/cert/ct_log_verifier.cc#newcode252 net/cert/ct_log_verifier.cc:252: // https://tools.ietf.org/html/draft-ietf-trans-rfc6962-bis-16#section-10.4.1. On 2016/07/26 15:52:40, Eran Messeri wrote: > ...
4 years, 4 months ago (2016-07-28 05:18:47 UTC) #16
Ryan Sleevi
https://codereview.chromium.org/2182533002/diff/100001/net/cert/ct_log_verifier.cc File net/cert/ct_log_verifier.cc (right): https://codereview.chromium.org/2182533002/diff/100001/net/cert/ct_log_verifier.cc#newcode11 net/cert/ct_log_verifier.cc:11: #include <vector> On 2016/07/28 05:18:47, Rob Percival wrote: > ...
4 years, 4 months ago (2016-07-28 14:54:49 UTC) #17
Ryan Sleevi
https://codereview.chromium.org/2182533002/diff/100001/net/cert/ct_log_verifier.cc File net/cert/ct_log_verifier.cc (right): https://codereview.chromium.org/2182533002/diff/100001/net/cert/ct_log_verifier.cc#newcode252 net/cert/ct_log_verifier.cc:252: // https://tools.ietf.org/html/draft-ietf-trans-rfc6962-bis-17#section-10.4.1. Omit the period at the end, it's ...
4 years, 4 months ago (2016-07-28 16:40:47 UTC) #18
Ryan Sleevi
Is this ready for re-review?
4 years, 4 months ago (2016-07-29 20:38:23 UTC) #19
Rob Percival
https://codereview.chromium.org/2182533002/diff/100001/net/cert/ct_log_verifier.cc File net/cert/ct_log_verifier.cc (right): https://codereview.chromium.org/2182533002/diff/100001/net/cert/ct_log_verifier.cc#newcode252 net/cert/ct_log_verifier.cc:252: // https://tools.ietf.org/html/draft-ietf-trans-rfc6962-bis-17#section-10.4.1. On 2016/07/28 16:40:46, Ryan Sleevi (slow) wrote: ...
4 years, 3 months ago (2016-08-25 17:44:59 UTC) #20
Ryan Sleevi
Since performance came up in the other CL, should we be removing HexToBytes and switching ...
4 years, 3 months ago (2016-08-25 18:56:24 UTC) #25
Rob Percival
PTAL. I've renamed some things, reworded some of the documentation and gotten rid of the ...
4 years, 2 months ago (2016-10-03 17:39:26 UTC) #30
Rob Percival
Hi David, mind reviewing this? I'm drowning Ryan in reviews right now and Eran says ...
4 years, 2 months ago (2016-10-05 12:30:47 UTC) #37
Eran Messeri
drive-by review, overall looks good. https://codereview.chromium.org/2182533002/diff/220001/net/cert/ct_log_verifier.cc File net/cert/ct_log_verifier.cc (right): https://codereview.chromium.org/2182533002/diff/220001/net/cert/ct_log_verifier.cc#newcode253 net/cert/ct_log_verifier.cc:253: // https://tools.ietf.org/html/draft-ietf-trans-rfc6962-bis-17#section-10.4.1 nit: Update ...
4 years, 2 months ago (2016-10-05 13:17:54 UTC) #38
Rob Percival
https://codereview.chromium.org/2182533002/diff/220001/net/cert/ct_log_verifier.cc File net/cert/ct_log_verifier.cc (right): https://codereview.chromium.org/2182533002/diff/220001/net/cert/ct_log_verifier.cc#newcode253 net/cert/ct_log_verifier.cc:253: // https://tools.ietf.org/html/draft-ietf-trans-rfc6962-bis-17#section-10.4.1 On 2016/10/05 13:17:54, Eran Messeri wrote: > ...
4 years, 2 months ago (2016-10-05 13:34:51 UTC) #40
davidben
Seems I'm not much more timely than Ryan is. :-( Sorry about that! This looks ...
4 years, 2 months ago (2016-10-18 22:18:57 UTC) #41
Rob Percival
No worries, thanks for looking it over. https://codereview.chromium.org/2182533002/diff/260001/net/cert/ct_log_verifier.cc File net/cert/ct_log_verifier.cc (right): https://codereview.chromium.org/2182533002/diff/260001/net/cert/ct_log_verifier.cc#newcode253 net/cert/ct_log_verifier.cc:253: // https://tools.ietf.org/html/draft-ietf-trans-rfc6962-bis-19#section-10.4.1 ...
4 years, 2 months ago (2016-10-20 13:51:47 UTC) #42
davidben
lgtm
4 years, 2 months ago (2016-10-20 14:32:13 UTC) #45
Eran Messeri
lgtm % two minor comments. https://codereview.chromium.org/2182533002/diff/280001/net/cert/merkle_audit_proof.h File net/cert/merkle_audit_proof.h (right): https://codereview.chromium.org/2182533002/diff/280001/net/cert/merkle_audit_proof.h#newcode36 net/cert/merkle_audit_proof.h:36: // Number of leaves ...
4 years, 2 months ago (2016-10-23 16:18:06 UTC) #48
Rob Percival
https://codereview.chromium.org/2182533002/diff/280001/net/cert/merkle_audit_proof.h File net/cert/merkle_audit_proof.h (right): https://codereview.chromium.org/2182533002/diff/280001/net/cert/merkle_audit_proof.h#newcode36 net/cert/merkle_audit_proof.h:36: // Number of leaves in the log's tree. On ...
4 years, 1 month ago (2016-11-07 09:39:49 UTC) #49
Rob Percival
On 2016/11/07 09:39:49, Rob Percival wrote: > https://codereview.chromium.org/2182533002/diff/280001/net/cert/merkle_audit_proof.h > File net/cert/merkle_audit_proof.h (right): > > https://codereview.chromium.org/2182533002/diff/280001/net/cert/merkle_audit_proof.h#newcode36 ...
4 years, 1 month ago (2016-11-07 13:48:18 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2182533002/320001
4 years, 1 month ago (2016-11-07 13:48:36 UTC) #57
commit-bot: I haz the power
Committed patchset #13 (id:320001)
4 years, 1 month ago (2016-11-07 13:53:19 UTC) #59
commit-bot: I haz the power
Patchset 13 (id:??) landed as https://crrev.com/89f010d82559c73e7d23cbf067137ceb1df698df Cr-Commit-Position: refs/heads/master@{#430264}
4 years, 1 month ago (2016-11-07 13:55:58 UTC) #61
davidben
4 years, 1 month ago (2016-11-07 15:51:26 UTC) #62
Message was sent while issue was closed.
> Eran has requested that this be committed now so that he can use it
immediately.
> Since it's been 2 months since Ryan's last comments, I'm going to go ahead and
> commit it with 2 out of 3 LGTMs. Ryan, if you have any further comments, feel
> free to make them here and I'll address them in a follow-up CL.

I think Ryan probably meant for me to review in his stead anyway. :-P

Powered by Google App Engine
This is Rietveld 408576698