Certificate Transparency: Switch to using a generated CT log list

Certificate Transparency: Switch to using a CT log list that is generated from a signed, published list.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279435

[Eran Messeri, 2014-06-13 00:19:59]

I expect the first question to be "where is the list published?"
It's not published yet, but that would be the output of the code generation
utility if it were.
I'm fine with waiting for it to be published before this change is pushed.

[agl, 2014-06-13 00:32:06]

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.cc
File net/cert/ct_known_logs.cc (right):

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.cc#ne...
net/cert/ct_known_logs.cc:19: const char* log_key;
The keys are expected to be the same length, right? If so, const char
log_key[92] saves a relocation record for each one.

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.cc#ne...
net/cert/ct_known_logs.cc:21: std::string log_name;
This looks like a static initialiser, so should probably be a const char * or
const char log_name[32];

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h
File net/cert/ct_known_logs.h (right):

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h#new...
net/cert/ct_known_logs.h:19: // Meant to be consumed by the caller.
// CreateLogVerifiersForKnownLogs returns a vector of CT logs for all the known
and trusted logs.

https://codereview.chromium.org/337603003/diff/1/net/cert/multi_log_ct_verifi...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/337603003/diff/1/net/cert/multi_log_ct_verifi...
net/cert/multi_log_ct_verifier.cc:70: std::vector<linked_ptr<CTLogVerifier> >
log_verifiers) {
const & for the vector?

https://codereview.chromium.org/337603003/diff/1/net/cert/multi_log_ct_verifi...
net/cert/multi_log_ct_verifier.cc:78: 
extra space?

[Ryan Sleevi, 2014-06-17 01:02:04]

The description needs updating. This is not auto-generated (as part of Chrome
build process) :)

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h
File net/cert/ct_known_logs.h (right):

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h#new...
net/cert/ct_known_logs.h:19: // Meant to be consumed by the caller.
On 2014/06/13 00:32:05, agl wrote:
> // CreateLogVerifiersForKnownLogs returns a vector of CT logs for all the
known
> and trusted logs.

This comment doesn't really provide any description. All return values are meant
to be consumed by the caller.

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h#new...
net/cert/ct_known_logs.h:20: NET_EXPORT std::vector<linked_ptr<CTLogVerifier> >
Use ScopedVector instead -
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h#new...
net/cert/ct_known_logs.h:21: CreateLogVerifiersForKnownLogs();
four spaces

https://codereview.chromium.org/337603003/diff/1/net/cert/multi_log_ct_verifi...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/337603003/diff/1/net/cert/multi_log_ct_verifi...
net/cert/multi_log_ct_verifier.h:36: void
AddLogs(std::vector<linked_ptr<CTLogVerifier> > log_verifiers);
Feels like too much coupling here. I dislike the linked_ptr<> bleeding in here.

It should, presumably, be a const-ref arg if passed as vector, although if you
go ScopedVector, and you do intend to ownership transfer, you'd continue
by-value and use the .Pass() semantics (aka C++11 move emulation)

[Eran Messeri, 2014-06-17 17:31:41]

Thanks all for the quick reviews, PTAL.

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.cc
File net/cert/ct_known_logs.cc (right):

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.cc#ne...
net/cert/ct_known_logs.cc:19: const char* log_key;
On 2014/06/13 00:32:05, agl wrote:
> The keys are expected to be the same length, right? If so, const char
> log_key[92] saves a relocation record for each one.

For now, for our logs, yes. May change if (hopefully when!) we add other logs.
The original intention was future-proofing this bit. I've set a constant size
for the log keys as you suggested, but if you think future-proofing here makes
sense I'll revert this change.

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.cc#ne...
net/cert/ct_known_logs.cc:21: std::string log_name;
On 2014/06/13 00:32:05, agl wrote:
> This looks like a static initialiser, so should probably be a const char * or
> const char log_name[32];

Done.

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h
File net/cert/ct_known_logs.h (right):

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h#new...
net/cert/ct_known_logs.h:19: // Meant to be consumed by the caller.
On 2014/06/13 00:32:05, agl wrote:
> // CreateLogVerifiersForKnownLogs returns a vector of CT logs for all the
known
> and trusted logs.

Done.

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h#new...
net/cert/ct_known_logs.h:19: // Meant to be consumed by the caller.
On 2014/06/17 01:02:03, Ryan Sleevi wrote:
> On 2014/06/13 00:32:05, agl wrote:
> > // CreateLogVerifiersForKnownLogs returns a vector of CT logs for all the
> known
> > and trusted logs.
> 
> This comment doesn't really provide any description. All return values are
meant
> to be consumed by the caller.

Done.

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h#new...
net/cert/ct_known_logs.h:20: NET_EXPORT std::vector<linked_ptr<CTLogVerifier> >
On 2014/06/17 01:02:03, Ryan Sleevi wrote:
> Use ScopedVector instead -
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

Done.

https://codereview.chromium.org/337603003/diff/1/net/cert/ct_known_logs.h#new...
net/cert/ct_known_logs.h:21: CreateLogVerifiersForKnownLogs();
On 2014/06/17 01:02:04, Ryan Sleevi wrote:
> four spaces

Done.

https://codereview.chromium.org/337603003/diff/1/net/cert/multi_log_ct_verifi...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/337603003/diff/1/net/cert/multi_log_ct_verifi...
net/cert/multi_log_ct_verifier.cc:70: std::vector<linked_ptr<CTLogVerifier> >
log_verifiers) {
On 2014/06/13 00:32:05, agl wrote:
> const & for the vector?

Switched to using ScopedVector, see if that makes sense.
As a side note, the reason for creating the CTLogVerifier instances in one place
and taking ownership of them here is that (1) their creation is separated from
the implementation of the MultiLogCTVerifier and (2) should these classes grow
to contain additional log information (like latest tree head), there could be
multiple instances of them in different parts of Chrome that do not share state
(I understand it's good practice not to mix state between the different profiles
and the system context).

https://codereview.chromium.org/337603003/diff/1/net/cert/multi_log_ct_verifi...
net/cert/multi_log_ct_verifier.cc:78: 
On 2014/06/13 00:32:05, agl wrote:
> extra space?

Done, removed.

https://codereview.chromium.org/337603003/diff/1/net/cert/multi_log_ct_verifi...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/337603003/diff/1/net/cert/multi_log_ct_verifi...
net/cert/multi_log_ct_verifier.h:36: void
AddLogs(std::vector<linked_ptr<CTLogVerifier> > log_verifiers);
On 2014/06/17 01:02:04, Ryan Sleevi wrote:
> Feels like too much coupling here. I dislike the linked_ptr<> bleeding in
here.
> 
> It should, presumably, be a const-ref arg if passed as vector, although if you
> go ScopedVector, and you do intend to ownership transfer, you'd continue
> by-value and use the .Pass() semantics (aka C++11 move emulation)

Good point. I've switched to using ScopedVector and take ownership of the
pointers inside the AddLogs method.

[Ryan Sleevi, 2014-06-17 19:48:55]

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.cc
File net/cert/ct_known_logs.cc (right):

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.c...
net/cert/ct_known_logs.cc:21: const char* log_name;
const char* const;

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.c...
net/cert/ct_known_logs.cc:24: #include "net/cert/ct_known_logs_static.h"
Why isn't the definition for the structure (CTLogInfo) part of the static list?
the ct_known_logs_static.h is tightly coupled to the structural definition here.

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.c...
net/cert/ct_known_logs.cc:30: for (int i = 0; i < kNumKnownCTLogs; ++i) {
s/int/size_t;

Seems like you should be able to use arraysize(kCTLogList), because it will be a
statically initialized list, rather than rely on kNumKnownCTLogs (which might
someday get out of sync)

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.c...
net/cert/ct_known_logs.cc:32: base::StringPiece key(log.log_key, kLogKeyLength -
1);
Ditto here; Seems like arraysize(log.log_key) avoids any sync issues.

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs_s...
File net/cert/ct_known_logs_static.h (right):

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs_s...
net/cert/ct_known_logs_static.h:5: // This file is automatically generated by
print_log_list.py
print_log_list.py isn't being checked in here.

s/automatically//

https://codereview.chromium.org/337603003/diff/20001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/337603003/diff/20001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:81: }
You could avoid the .release() with the use of .weak_clear(), thus avoiding the
intermediate std::vector

[Eran Messeri, 2014-06-18 14:37:57]

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.cc
File net/cert/ct_known_logs.cc (right):

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.c...
net/cert/ct_known_logs.cc:21: const char* log_name;
On 2014/06/17 19:48:54, Ryan Sleevi wrote:
> const char* const;

Done.

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.c...
net/cert/ct_known_logs.cc:24: #include "net/cert/ct_known_logs_static.h"
On 2014/06/17 19:48:54, Ryan Sleevi wrote:
> Why isn't the definition for the structure (CTLogInfo) part of the static
list?
> the ct_known_logs_static.h is tightly coupled to the structural definition
here.

Good point, done. That makes the generated header file much more useful.

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.c...
net/cert/ct_known_logs.cc:30: for (int i = 0; i < kNumKnownCTLogs; ++i) {
On 2014/06/17 19:48:54, Ryan Sleevi wrote:
> s/int/size_t;
> 
> Seems like you should be able to use arraysize(kCTLogList), because it will be
a
> statically initialized list, rather than rely on kNumKnownCTLogs (which might
> someday get out of sync)

Done, although kNumKnownCTLogs is generated together with kCTLogList.

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.c...
net/cert/ct_known_logs.cc:32: base::StringPiece key(log.log_key, kLogKeyLength -
1);
On 2014/06/17 19:48:54, Ryan Sleevi wrote:
> Ditto here; Seems like arraysize(log.log_key) avoids any sync issues.

For some reason arraysize was not happy with it:
../../net/cert/ct_known_logs.cc: In function ‘ScopedVector<net::CTLogVerifier>
net::ct::CreateLogVerifiersForKnownLogs()’:
../../net/cert/ct_known_logs.cc:22:40: error: no matching function for call to
‘ArraySizeHelper(const char* const&)’
../../net/cert/ct_known_logs.cc:22:40: note: candidates are:
../../base/macros.h:64:8: note: template<class T, long unsigned int N> char (&
ArraySizeHelper(T (&)[N]))[N]
../../base/macros.h:71:8: note: template<class T, long unsigned int N> char (&
ArraySizeHelper(const T (&)[N]))[N]

So I've switched to using a constant that is defined in the generated file,
which should not get out-of-sync as it is automatically calculated.
I could use ARRAYSIZE_UNSAFE, though.

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs_s...
File net/cert/ct_known_logs_static.h (right):

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs_s...
net/cert/ct_known_logs_static.h:5: // This file is automatically generated by
print_log_list.py
On 2014/06/17 19:48:54, Ryan Sleevi wrote:
> print_log_list.py isn't being checked in here.
> 
> s/automatically//

Done.

https://codereview.chromium.org/337603003/diff/20001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/337603003/diff/20001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:81: }
On 2014/06/17 19:48:54, Ryan Sleevi wrote:
> You could avoid the .release() with the use of .weak_clear(), thus avoiding
the
> intermediate std::vector

Done.

[Eran Messeri, 2014-06-18 14:59:51]

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.cc
File net/cert/ct_known_logs.cc (right):

https://codereview.chromium.org/337603003/diff/20001/net/cert/ct_known_logs.c...
net/cert/ct_known_logs.cc:32: base::StringPiece key(log.log_key, kLogKeyLength -
1);
By defining the size of the log_key array in the LogInfo type definition, that
now works and the  the constant is no longer needed.

[Ryan Sleevi, 2014-06-19 00:34:08]

Still need a description update, but otherwise, LGTM.

That is: This is not automatically generated

https://codereview.chromium.org/337603003/diff/80001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/337603003/diff/80001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:7: #include <vector>
Do you actually still need vector? Seems like no.

https://codereview.chromium.org/337603003/diff/80001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:80: log_verifiers.weak_clear();
nit: add comment
// Ownership of the pointers in |log_verifiers| is transferred to |logs_|

[Eran Messeri, 2014-06-19 08:28:38]

Amended description, added a reviewer for chrome/browser/io_thread.cc.

https://codereview.chromium.org/337603003/diff/80001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/337603003/diff/80001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:7: #include <vector>
On 2014/06/19 00:34:08, Ryan Sleevi wrote:
> Do you actually still need vector? Seems like no.

std::vector is actually used in a different method, I did not include it
properly beforehand.

https://codereview.chromium.org/337603003/diff/80001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:80: log_verifiers.weak_clear();
On 2014/06/19 00:34:08, Ryan Sleevi wrote:
> nit: add comment
> // Ownership of the pointers in |log_verifiers| is transferred to |logs_|

Done.

[Ben Laurie (Google), 2014-06-19 10:36:33]

https://codereview.chromium.org/337603003/diff/100001/net/cert/ct_known_logs_...
File net/cert/ct_known_logs_static.h (right):

https://codereview.chromium.org/337603003/diff/100001/net/cert/ct_known_logs_...
net/cert/ct_known_logs_static.h:10: const char log_key[92];
Shouldn't this be uint8?

[Eran Messeri, 2014-06-24 15:33:09]

Ping - final review is needed for chrome/browser/io_thread.cc

https://codereview.chromium.org/337603003/diff/100001/net/cert/ct_known_logs_...
File net/cert/ct_known_logs_static.h (right):

https://codereview.chromium.org/337603003/diff/100001/net/cert/ct_known_logs_...
net/cert/ct_known_logs_static.h:10: const char log_key[92];
On 2014/06/19 10:36:33, Ben Laurie (Google) wrote:
> Shouldn't this be uint8?

I think not, since it's used by base::StringPiece.

[Ryan Sleevi, 2014-06-24 16:41:08]

Ping clarification: That's for you thestig@ :)

[Lei Zhang, 2014-06-24 17:30:11]

lgtm

[commit-bot: I haz the power, 2014-06-24 17:31:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eranm@chromium.org/337603003/100001

[commit-bot: I haz the power, 2014-06-24 17:47:58]

Change committed as 279435