Certificate Transparency: Code for unpacking EV cert hashes whitelist

Certificate Transparency: Code for unpacking EV cert hashes whitelist

The goal is to whitelist logged EV certificates so the requirement of
CT for EV certificates can be enabled without waiting for all EV certs
to be re-issued.

This change adds the code for unpacking the list of (truncated) hashes
of EV certificates. The compressed data format is the diff values
between the hashes, encoded using Golomb coding. This was suggested
by agl as an efficient encoding, since the hash values of the
EV certificates are uniformly distributed, so the differences between
them are geometrically distributed. See section 4 in:
http://algo2.iti.kit.edu/singler/publications/cacheefficientbloomfilters-wea2007.pdf

The code that generates the data can be found here:
https://github.com/google/certificate-transparency/blob/master/python/utilities/ev_whitelist/golomb_code.py#L27

Currently the code is not hooked into anything, but once the
compressed list would be fetched as a component update, we'll start
by logging statistics about known vs. unknown EV certs.

BUG=339128
Committed: https://crrev.com/743f614e8b1daec08613e6108a6b2b902d5b9b55
Cr-Commit-Position: refs/heads/master@{#293288}

[wtc, 2014-08-14 02:04:44]

Review comments on patch set 4:

I will have more time next week. I didn't review ct_ev_whitelist.cc carefully.
Let me send you some comments first.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist.cc
File net/cert/ct_ev_whitelist.cc (right):

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:23: uint64 kGolombMParameterBits = 47;  // 2^47

Add 'const' to these two constants.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:25: }

Add a comment:

}  // namespace

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist.h
File net/cert/ct_ev_whitelist.h (right):

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:12: #include "net/base/net_export.h"

Nit: Remove this header. Actually, I think we will need it because the
BitStreamReader class needs to be declared with NET_EXPORT_PRIVATE for the unit
tests.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:20: class BitStreamReader {

Nit: please add a short comment to summarize this class. It may be good to
explain why this class needs to be declared here (so that it can be tested).

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:31: uint64 BitsLeft();

Nit: Declare this method as const.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:40: const size_t length_;

Nit: To match this 'const', the |source_| member should be declared as follows:
  const char* const source_;

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:50: void SetEVWhitelistData(std::set<std::string>
ev_whitelist);

Nit: Should the input parameter be a const reference?

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:54: 

Nit: delete a blank line.

[Eran Messeri, 2014-08-14 11:55:08]

Addressed all comments, will wait for the more through review next week.
I'd like your opinion on one point of API design:
IsCertificateHashInWhitelist (in ct_ev_whitelist.h) takes a string, which has to
be created in a rather untidy way in ssl_client_socket_nss, since the data it
needs (in SHA256HashValue) is unsigned char*.

My question is: Is there a more suitable type for holding the raw hash bytes
inside the set, that would also be easy to create from SHA256HashValue?

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist.cc
File net/cert/ct_ev_whitelist.cc (right):

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:23: uint64 kGolombMParameterBits = 47;  // 2^47
On 2014/08/14 02:04:43, wtc wrote:
> 
> Add 'const' to these two constants.

Done.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:25: }
On 2014/08/14 02:04:43, wtc wrote:
> 
> Add a comment:
> 
> }  // namespace

Done.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist.h
File net/cert/ct_ev_whitelist.h (right):

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:12: #include "net/base/net_export.h"
On 2014/08/14 02:04:43, wtc wrote:
> 
> Nit: Remove this header. Actually, I think we will need it because the
> BitStreamReader class needs to be declared with NET_EXPORT_PRIVATE for the
unit
> tests.

Added NET_EXPORT & NET_EXPORT_PRIVATE where (I think) necessary.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:20: class BitStreamReader {
On 2014/08/14 02:04:43, wtc wrote:
> 
> Nit: please add a short comment to summarize this class. It may be good to
> explain why this class needs to be declared here (so that it can be tested).

Done.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:20: class BitStreamReader {
On 2014/08/14 02:04:43, wtc wrote:
> 
> Nit: please add a short comment to summarize this class. It may be good to
> explain why this class needs to be declared here (so that it can be tested).

Done.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:31: uint64 BitsLeft();
On 2014/08/14 02:04:43, wtc wrote:
> 
> Nit: Declare this method as const.

Done.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:40: const size_t length_;
On 2014/08/14 02:04:44, wtc wrote:
> 
> Nit: To match this 'const', the |source_| member should be declared as
follows:
>   const char* const source_;

Done.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:50: void SetEVWhitelistData(std::set<std::string>
ev_whitelist);
On 2014/08/14 02:04:44, wtc wrote:
> 
> Nit: Should the input parameter be a const reference?

No, because I'm using set.swap underneath. Will add a comment to clarify this.

https://codereview.chromium.org/462543002/diff/60001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:54: 
On 2014/08/14 02:04:43, wtc wrote:
> 
> Nit: delete a blank line.

Done.

[wtc, 2014-08-20 02:52:08]

Review comments on patch set 5:

I was on vacation yesterday. I reviewed ct_ev_whitelist.* carefully today but
didn't have time to review the rest of the CL. Let me send you my comments on
ct_ev_whitelist.* first.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist.cc
File net/cert/ct_ev_whitelist.cc (right):

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:33: uint64 res(0);

Nit: for this line and other similar lines, I'd prefer the conventional C-style
initialization:

  uint64 res = 0;

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:36: }

Nit: omit curly braces.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:58: }

Nit: omit curly braces.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:70: bool BitStreamReader::HasMoreBits() const {

Nit: this function doesn't seem very useful. I would probably just call
BitsLeft() directly.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:87: void SetEVWhitelistData(std::set<std::string>
ev_whitelist) {

Nit: in the .h file this function is declared after UncompressEVWhitelist. So in
thie file it should be defined after UncompressEVWhitelist.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:107: uint64 M = static_cast<uint64>(1) <<
kGolombMParameterBits;

Nit: this can be defined as a compile-time constant.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:123: uint64 next_hash = curr_hash + curr_diff;

Nit: we don't seem to need the |next_hash| variable. I think we can just do

  curr_hash += curr_diff;

here.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:157: g_current_ev_whitelist.Get().end();

Nit: store the return value of g_current_ev_whitelist.Get() in a local variable
so that it is called only once.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist.h
File net/cert/ct_ev_whitelist.h (right):

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:38: bool ReadBits(uint8 num_bits, uint64* out);

Please documents in which direction you are filling in the bits in |*out|. I
think it's from the most significant bit to the least significant bit.

Another thing you should document is where the unused bits are if |num_bits| is
less than 64. I think the unused bits are the most significant |64 - num_bits|
bits.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:45: // Reads a single bit.

Please document that within a byte, the bits are read from the most significant
bit to the least significant bit.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:52: int8 curr_bit_;

Nit: it would be nice to document these two members. I felt like I was given a
puzzle to solve. It's not too hard to figure out what they mean, but I was at
first puzzled by why curr_bit_ is initialized to 7 in the constructor.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:58: // false otherwise.

Nit: please reformat this comment block.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:65: // is using set::swap() to efficiently switch the
sets.

IMPORTANT: are you sure this is true? The |ev_whitelist| parameter is passed by
value. For this to be true, it needs to be passed as a reference or pointer.

Also, the "empty" part is only true if the old value is empty.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:66: NET_EXPORT_PRIVATE void
SetEVWhitelistData(std::set<std::string> ev_whitelist);

This function is only used by SetEVWhitelistFromFile and is not used by any unit
tests. So I think it can be an internal function in the .cc file and shouldn't
be declared here.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
File net/cert/ct_ev_whitelist_unittest.cc (right):

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:10: #include "base/strings/stringprintf.h"

I don't think we need these two headers.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:72:
ASSERT_EQ(static_cast<uint64>(0xd5e2afe5bb107cd1), v);

I think there is some GG_UINT64_C macro (or something like that) that allows you
to write a uint64 constant in a portable way.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:79: for (int i = 0; i < 9; ++i) {

There are still four 1 bits (the 'f' in 0xaf). Should we do i < 10 and see if
that is parsed as a 4?  (I assume the final '1111' is interpreted as 4 even
though it is missing the trailing 0 bit.)

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:83: << i;

Nit: I think ASSERT_EQ will automatically print the expected and actual values,
so the only thing missing is the index |i| where this fails.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:118:
internal::UncompressEVWhitelist(std::string(kWhitelistData, 21), &res));

Nit: it may be better to use sizeof(kWhitelistData) or arraysize(kWhitelistData)
instead of 21.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:125:
ASSERT_TRUE(res.find(std::string(kThirdHash, 8)) != res.end());

We should also verify that res.size() is equal to 3.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:132: }

These two tests are empty!

[wtc, 2014-08-20 19:04:22]

Review comments on patch set 5:

I reviewed the other files. They are all quite straightforward.

https://codereview.chromium.org/462543002/diff/80001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/462543002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3447: // fingerprint certificate

Nit: this comment can be omitted because it merely repeat the code.

[Eran Messeri, 2014-09-02 12:20:23]

Addressed all comments.
Wan-Teh, if you can, please take another look, otherwise agl@ will be the lucky
reviewer of this patch.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist.cc
File net/cert/ct_ev_whitelist.cc (right):

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:33: uint64 res(0);
On 2014/08/20 02:52:07, wtc wrote:
> 
> Nit: for this line and other similar lines, I'd prefer the conventional
C-style
> initialization:
> 
>   uint64 res = 0;

Done, throughout the file.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:36: }
On 2014/08/20 02:52:07, wtc wrote:
> 
> Nit: omit curly braces.

Done, throughout the file.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:58: }
On 2014/08/20 02:52:07, wtc wrote:
> 
> Nit: omit curly braces.

Done.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:70: bool BitStreamReader::HasMoreBits() const {
On 2014/08/20 02:52:07, wtc wrote:
> 
> Nit: this function doesn't seem very useful. I would probably just call
> BitsLeft() directly.

Done. Personally I find it more readable not having to read a condition every
time, but that's a very simple condition so I don't mind inlining it.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:87: void SetEVWhitelistData(std::set<std::string>
ev_whitelist) {
On 2014/08/20 02:52:07, wtc wrote:
> 
> Nit: in the .h file this function is declared after UncompressEVWhitelist. So
in
> thie file it should be defined after UncompressEVWhitelist.

Done.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:107: uint64 M = static_cast<uint64>(1) <<
kGolombMParameterBits;
On 2014/08/20 02:52:07, wtc wrote:
> 
> Nit: this can be defined as a compile-time constant.

Acknowledged, I prefer to leave it defined here though as the name (M) only
makes sense in this context.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:123: uint64 next_hash = curr_hash + curr_diff;
On 2014/08/20 02:52:07, wtc wrote:
> 
> Nit: we don't seem to need the |next_hash| variable. I think we can just do
> 
>   curr_hash += curr_diff;
> 
> here.

Done.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.cc:157: g_current_ev_whitelist.Get().end();
On 2014/08/20 02:52:07, wtc wrote:
> 
> Nit: store the return value of g_current_ev_whitelist.Get() in a local
variable
> so that it is called only once.

Done.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist.h
File net/cert/ct_ev_whitelist.h (right):

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:38: bool ReadBits(uint8 num_bits, uint64* out);
On 2014/08/20 02:52:07, wtc wrote:
> 
> Please documents in which direction you are filling in the bits in |*out|. I
> think it's from the most significant bit to the least significant bit.
> 
> Another thing you should document is where the unused bits are if |num_bits|
is
> less than 64. I think the unused bits are the most significant |64 - num_bits|
> bits.

Good points, done. Also added a class-level comment that the bitstream is read
MSB-first.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:45: // Reads a single bit.
On 2014/08/20 02:52:07, wtc wrote:
> 
> Please document that within a byte, the bits are read from the most
significant
> bit to the least significant bit.

Done.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:52: int8 curr_bit_;
On 2014/08/20 02:52:07, wtc wrote:
> 
> Nit: it would be nice to document these two members. I felt like I was given a
> puzzle to solve. It's not too hard to figure out what they mean, but I was at
> first puzzled by why curr_bit_ is initialized to 7 in the constructor.

Very valid point, code reviews should not be puzzles.
Documented both and added a comment explaining initialization.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:58: // false otherwise.
On 2014/08/20 02:52:07, wtc wrote:
> 
> Nit: please reformat this comment block.

Done, I blame git-cl format for that :)

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:65: // is using set::swap() to efficiently switch the
sets.
On 2014/08/20 02:52:07, wtc wrote:
> 
> IMPORTANT: are you sure this is true? The |ev_whitelist| parameter is passed
by
> value. For this to be true, it needs to be passed as a reference or pointer.
> 
> Also, the "empty" part is only true if the old value is empty.

My bad, it was supposed to be passed by (non-const) reference.  Fixed that and
amended the documentation.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist.h:66: NET_EXPORT_PRIVATE void
SetEVWhitelistData(std::set<std::string> ev_whitelist);
On 2014/08/20 02:52:07, wtc wrote:
> 
> This function is only used by SetEVWhitelistFromFile and is not used by any
unit
> tests. So I think it can be an internal function in the .cc file and shouldn't
> be declared here.

It's meant to be used from a test I did not write. It's now being used.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
File net/cert/ct_ev_whitelist_unittest.cc (right):

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:10: #include "base/strings/stringprintf.h"
On 2014/08/20 02:52:07, wtc wrote:
> 
> I don't think we need these two headers.

Done.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:72:
ASSERT_EQ(static_cast<uint64>(0xd5e2afe5bb107cd1), v);
On 2014/08/20 02:52:07, wtc wrote:
> 
> I think there is some GG_UINT64_C macro (or something like that) that allows
you
> to write a uint64 constant in a portable way.

Done, switched to using UINT64_C (GG_UINT64_C is deprecated; see
src/base/port.h)

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:79: for (int i = 0; i < 9; ++i) {
On 2014/08/20 02:52:08, wtc wrote:
> 
> There are still four 1 bits (the 'f' in 0xaf). Should we do i < 10 and see if
> that is parsed as a 4?  (I assume the final '1111' is interpreted as 4 even
> though it is missing the trailing 0 bit.)

Done. Adding this test case indeed uncovered the problem where the final 1 was
not accounted for, so I fixed the code.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:83: << i;
On 2014/08/20 02:52:08, wtc wrote:
> 
> Nit: I think ASSERT_EQ will automatically print the expected and actual
values,
> so the only thing missing is the index |i| where this fails.

Done.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:118:
internal::UncompressEVWhitelist(std::string(kWhitelistData, 21), &res));
On 2014/08/20 02:52:08, wtc wrote:
> 
> Nit: it may be better to use sizeof(kWhitelistData) or
arraysize(kWhitelistData)
> instead of 21.

Done.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:125:
ASSERT_TRUE(res.find(std::string(kThirdHash, 8)) != res.end());
On 2014/08/20 02:52:08, wtc wrote:
> 
> We should also verify that res.size() is equal to 3.

Done.

https://codereview.chromium.org/462543002/diff/80001/net/cert/ct_ev_whitelist...
net/cert/ct_ev_whitelist_unittest.cc:132: }
On 2014/08/20 02:52:08, wtc wrote:
> 
> These two tests are empty!

Eek! My bad, they're implemented in the new patchset.

https://codereview.chromium.org/462543002/diff/80001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/462543002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3447: // fingerprint certificate
On 2014/08/20 19:04:22, wtc wrote:
> 
> Nit: this comment can be omitted because it merely repeat the code.

Done.

[wtc, 2014-09-02 23:03:02]

Patch set 6 LGTM. Please fix the problems I identified.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
File net/cert/ct_ev_whitelist.cc (right):

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.cc:41: }

Nit: we should be able to simplify this (lines 37-41) as follows:

  while (BitsLeft() > 0 && ReadBit())
    res++;

But this code is very subtle, so I may have missed something.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.cc:69: DCHECK(BitsLeft() > 0);

Nit: you can use DCHECK_GT.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.cc:74: current_byte_ += 1;

Nit: use ++.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.cc:97: uint64 M = static_cast<uint64>(1) <<
kGolombMParameterBits;

What I meant is to declare this as either "const" or "static const":

  static const uint64 M = static_cast<uint64>(1) << kGolombMParameterBits;

We can even name it "kM" to suggest that it is a constant. But it should be OK
to make an exception to the constant naming rule in this case.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.cc:116: curr_hash += curr_diff;

What I meant is to replace lines 113-116 with the following:

  curr_hash += curr_diff;
  base::WriteBigEndian(hash_bytes, curr_hash);
  result.insert(std::string(hash_bytes, 8));

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelist.h
File net/cert/ct_ev_whitelist.h (right):

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.h:20: /**

Unless you are using a tool like javadoc or doxygen to generate documentation
from comments, we should just use C++ style comments ( // ...).

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.h:38: // bits are left unused. Returns true if the
stream had the requested

Nit: "left unused" doesn't say what values those bits have. You can say "unused
and left as zeros" or "set to zeros", for example.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.h:55: // from the MSB to the LSB, this value is
initialized to 7 and decreases

Nit: decreases => decremented

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.h:71: NET_EXPORT_PRIVATE void
SetEVWhitelistData(std::set<std::string>& ev_whitelist);

IMPORTANT: a non-const reference is usually disallowed by our Style Guide. I
think this is probably OK.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.h:72: }  // namespace internal

Please add a blank line before this line.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
File net/cert/ct_ev_whitelist_unittest.cc (right):

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist_unittest.cc:23: ASSERT_EQ(8u, reader.BitsLeft());

IMPORTANT: most of the ASSERT_xxx's in this file should be EXPECT_xxx's.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist_unittest.cc:143:
ASSERT_TRUE(IsCertificateHashInWhitelist(kFirstHash));

IMPORTANT: we should expect FALSE here, right?

https://codereview.chromium.org/462543002/diff/100001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/462543002/diff/100001/net/net.gypi#newcode1
net/net.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved.

IMPORTANT: you also need to add the new files to net/BUILD.gn, which is a new
build system.

[Eran Messeri, 2014-09-03 09:38:50]

eranm@chromium.org changed reviewers:
+ mpearson@chromium.org

[Eran Messeri, 2014-09-03 09:38:51]

Addressing wtc's comments, adding mpearson for histograms.xml review.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
File net/cert/ct_ev_whitelist.cc (right):

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.cc:41: }
On 2014/09/02 23:03:02, wtc wrote:
> 
> Nit: we should be able to simplify this (lines 37-41) as follows:
> 
>   while (BitsLeft() > 0 && ReadBit())
>     res++;
> 
> But this code is very subtle, so I may have missed something.

Done.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.cc:69: DCHECK(BitsLeft() > 0);
On 2014/09/02 23:03:02, wtc wrote:
> 
> Nit: you can use DCHECK_GT.

Done.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.cc:74: current_byte_ += 1;
On 2014/09/02 23:03:02, wtc wrote:
> 
> Nit: use ++.

Done.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.cc:97: uint64 M = static_cast<uint64>(1) <<
kGolombMParameterBits;
On 2014/09/02 23:03:02, wtc wrote:
> 
> What I meant is to declare this as either "const" or "static const":
> 
>   static const uint64 M = static_cast<uint64>(1) << kGolombMParameterBits;
> 
> We can even name it "kM" to suggest that it is a constant. But it should be OK
> to make an exception to the constant naming rule in this case.

Done.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.cc:116: curr_hash += curr_diff;
On 2014/09/02 23:03:01, wtc wrote:
> 
> What I meant is to replace lines 113-116 with the following:
> 
>   curr_hash += curr_diff;
>   base::WriteBigEndian(hash_bytes, curr_hash);
>   result.insert(std::string(hash_bytes, 8));

Done, good point.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelist.h
File net/cert/ct_ev_whitelist.h (right):

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.h:20: /**
On 2014/09/02 23:03:02, wtc wrote:
> 
> Unless you are using a tool like javadoc or doxygen to generate documentation
> from comments, we should just use C++ style comments ( // ...).

Done.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.h:38: // bits are left unused. Returns true if the
stream had the requested
On 2014/09/02 23:03:02, wtc wrote:
> 
> Nit: "left unused" doesn't say what values those bits have. You can say
"unused
> and left as zeros" or "set to zeros", for example.

Done.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.h:55: // from the MSB to the LSB, this value is
initialized to 7 and decreases
On 2014/09/02 23:03:02, wtc wrote:
> 
> Nit: decreases => decremented

Done.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.h:71: NET_EXPORT_PRIVATE void
SetEVWhitelistData(std::set<std::string>& ev_whitelist);
On 2014/09/02 23:03:02, wtc wrote:
> 
> IMPORTANT: a non-const reference is usually disallowed by our Style Guide. I
> think this is probably OK.

Acknowledged - Since this code is not performance-critical, I could change it to
copy the set, but copying ~1MB of data around just to adhere to the Style Guide
seems wasteful.
If you come to the conclusion that copying the data is better, I can change in a
follow-up CL, but IMHO this is one of the cases where an exception to the rules
is acceptable (especially since this API is only expected to be used in one
place).

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist.h:72: }  // namespace internal
On 2014/09/02 23:03:02, wtc wrote:
> 
> Please add a blank line before this line.

Done.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
File net/cert/ct_ev_whitelist_unittest.cc (right):

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist_unittest.cc:23: ASSERT_EQ(8u, reader.BitsLeft());
On 2014/09/02 23:03:02, wtc wrote:
> 
> IMPORTANT: most of the ASSERT_xxx's in this file should be EXPECT_xxx's.

Done.

https://codereview.chromium.org/462543002/diff/100001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist_unittest.cc:143:
ASSERT_TRUE(IsCertificateHashInWhitelist(kFirstHash));
On 2014/09/02 23:03:02, wtc wrote:
> 
> IMPORTANT: we should expect FALSE here, right?

Yes, my bad, I had a bug in the test (should have created the std::strings
explicitly by specifying the size, otherwise, since kFirstHash and kSecondHash
start with null, I'd get empty std::strings created from them).

https://codereview.chromium.org/462543002/diff/100001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/462543002/diff/100001/net/net.gypi#newcode1
net/net.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/09/02 23:03:02, wtc wrote:
> 
> IMPORTANT: you also need to add the new files to net/BUILD.gn, which is a new
> build system.

Not sure where to - net/BUILD.gn does not seem to have a list of all sources
under net/cert, but does exclude specific files on specific platforms. When I
build net_unittests my tests are definitely included.

[wtc, 2014-09-03 14:29:48]

Patch set 8 LGTM. I suggest an alternative solution to the initialization of
char arrays.

https://codereview.chromium.org/462543002/diff/100001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/462543002/diff/100001/net/net.gypi#newcode1
net/net.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved.

On 2014/09/03 09:38:51, Eran wrote:
> 
> Not sure where to - net/BUILD.gn does not seem to have a list of all sources
> under net/cert, but does exclude specific files on specific platforms.

Since your try build on the linux_chromium_gn_rel bot passed, I believe no
changes to net/BUILD.gn are needed. Perhaps the GN build system assumes all
source files in a directory should be compiled unless they are excluded?

https://codereview.chromium.org/462543002/diff/140001/net/cert/ct_ev_whitelis...
File net/cert/ct_ev_whitelist_unittest.cc (right):

https://codereview.chromium.org/462543002/diff/140001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist_unittest.cc:17: const uint8 kSomeData[] = {0xd5, 0xe2,
0xaf, 0xe5, 0xbb, 0x10, 0x7c, 0xd1};

A solution I used before is to initialize the char array with char constants
specified as hex escape sequences:

const char kSomeData[] = {'\xd5', '\xe2', '\xaf', ...};

https://codereview.chromium.org/462543002/diff/140001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist_unittest.cc:86: const std::string
kFirstHash(reinterpret_cast<const char*>(kFirstHashRaw), 8);

IMPORTANT: Chromium doesn't allow static objects. It may be OK to make an
exception here because this is just a test file, but it is best to avoid static
objects.

https://codereview.chromium.org/462543002/diff/140001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist_unittest.cc:135: whitelist_data.insert(kFirstHash);

IMPORTANT: you also need to fix this test to construct a std::string from
kFirstHash.

[Mark P, 2014-09-03 17:46:09]

https://codereview.chromium.org/462543002/diff/180001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/462543002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:17268: +    whitelist
Histogram descriptions should clearly state when the histogram is emitted
(profile open? network request received? etc.).

[Eran Messeri, 2014-09-03 20:27:10]

Thanks for the quick review, addressed all comments.
wtc, please take another look at the bottom of ct_ev_whitelist.h - I've added a
method to determine whether the client has a valid EV whitelist (i.e. the
component has been downloaded). That is to avoid falsely reporting EV certs as
non-present in the whitelist if the client does not have it yet.

https://codereview.chromium.org/462543002/diff/140001/net/cert/ct_ev_whitelis...
File net/cert/ct_ev_whitelist_unittest.cc (right):

https://codereview.chromium.org/462543002/diff/140001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist_unittest.cc:17: const uint8 kSomeData[] = {0xd5, 0xe2,
0xaf, 0xe5, 0xbb, 0x10, 0x7c, 0xd1};
On 2014/09/03 14:29:48, wtc wrote:
> 
> A solution I used before is to initialize the char array with char constants
> specified as hex escape sequences:
> 
> const char kSomeData[] = {'\xd5', '\xe2', '\xaf', ...};

Acknowledged - since I only use the data once (to create the string instance)
I'll keep it this way, with the cast, and will use your solution in the next
tests I write.

https://codereview.chromium.org/462543002/diff/140001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist_unittest.cc:86: const std::string
kFirstHash(reinterpret_cast<const char*>(kFirstHashRaw), 8);
On 2014/09/03 14:29:48, wtc wrote:
> 
> IMPORTANT: Chromium doesn't allow static objects. It may be OK to make an
> exception here because this is just a test file, but it is best to avoid
static
> objects.

Changed to use getter methods for the string representation of these values.

https://codereview.chromium.org/462543002/diff/140001/net/cert/ct_ev_whitelis...
net/cert/ct_ev_whitelist_unittest.cc:135: whitelist_data.insert(kFirstHash);
On 2014/09/03 14:29:48, wtc wrote:
> 
> IMPORTANT: you also need to fix this test to construct a std::string from
> kFirstHash.

Done.

https://codereview.chromium.org/462543002/diff/180001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/462543002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:17268: +    whitelist
On 2014/09/03 17:46:09, Mark P wrote:
> Histogram descriptions should clearly state when the histogram is emitted
> (profile open? network request received? etc.).

Done.

[Mark P, 2014-09-03 22:02:29]

histograms.xml lgtm

[Eran Messeri, 2014-09-04 10:35:14]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2014-09-04 10:36:03]

The CQ bit was unchecked by eranm@chromium.org

[Eran Messeri, 2014-09-04 10:36:03]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2014-09-04 10:36:11]

The CQ bit was unchecked by eranm@chromium.org

[Eran Messeri, 2014-09-04 10:36:18]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2014-09-04 10:36:52]

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

[commit-bot: I haz the power, 2014-09-04 11:56:40]

Committed patchset #11 (id:200001) as 65745f920ee45462bf164e2b4cef6e6d891b09a6

[Dmitry Lomov (no reviews), 2014-09-04 15:25:02]

dslomov@chromium.org changed reviewers:
+ dslomov@chromium.org

[Dmitry Lomov (no reviews), 2014-09-04 15:25:02]

Reverted in
https://chromium.googlesource.com/chromium/src/+/87bf0f714ed33c50a109dc587c6a...

[commit-bot: I haz the power, 2014-09-10 03:31:04]

Patchset 11 (id:??) landed as
https://crrev.com/743f614e8b1daec08613e6108a6b2b902d5b9b55
Cr-Commit-Position: refs/heads/master@{#293288}