Add transport security state generator tests.

Add transport security state generator tests.

This CL adds tests for the transport security state generator and refactors
parts of the existing code to make it testable.

Changes to existing code:

  - The preloaded transport security state data source is made configurable for
    the tests. It was previously hardcoded to the data in
    transport_security_state_static.h

  - The Pinset struct definition is moved to a separate file to make it reusable
    between transport_security_state_static.h and the related unittests.

  - The build configuration is moved to its own BUILD.gn file.

  - The (trie) generation code was refactored to improve the handling of incorrect
    inputs. The existing code would CHECK or DCHECK where the old go script
    panic()'ed. This makes it hard to properly test this code. Most (D)CHECKs are
    removed in favor of boolean return values. Any existing return values are
    converted to out parameters.

  - The code responsible for parsing the input files is moved to a separate file
    (input_file_parsers.(h|cc)) and out of the anonymous namespace to make them
    testable.

    - The error handling of this code is also improved by replacing std::cerr
      output with a vector. The vector will contain all errors that were
      encountered during the parsing of the inputs files*.

Tests:

  - This CL adds unittests for the transport security state generator.
    These tests are run as part of the net_unittests target.

  - On the Chromium side, unittests are added to test the decoding of the
    generated data. The input data for these tests is generated during the build.

* This currently is always, at most, 1 error but the plan is to change that when
issues 677294 and 568378 are addressed.

BUG=595493

[martijnc, 2017-01-30 21:19:48]

Patchset #2 (id:20001) has been deleted

[martijnc, 2017-02-05 22:00:40]

Patchset #3 (id:60001) has been deleted

[martijnc, 2017-02-06 20:58:06]

Patchset #2 (id:40001) has been deleted

[martijnc, 2017-02-06 20:58:11]

Patchset #2 (id:80001) has been deleted

[martijnc, 2017-02-06 21:39:28]

Description was changed from

==========
[WIP] Add TSS generator tests.

BUG=595493
==========

to

==========
Add transport security state generator tests.

This CL adds tests for the transport security state generator and refactors
parts of the existing code to make it testable.

Changes to existing code:

  - The preloaded transport security state data source is made configurable for
    the tests. It was previously hardcoded to the data in
    transport_security_state_static.h

  - The Pinset struct definition is moved to a new file to make it reusable
    between transport_security_state_static.h and the related unittest files.

  - The build configuration is moved to its own BUILD.gn file.

  - The (trie) generation code is reworked to improve the handling of incorrect
    inputs. The existing code would CHECK or DCHECK where the old go script
    panic()'ed. This makes it hard to properly test this code. Most (D)CHECKs
are
    removed in favor of boolean return values. Any existing return values are
    converted to out parameters.

  - The code responsible for parsing the input files is moved to a separate file
    (input_file_parsers.(h|cc)) and out of the anonymous namespace to make them
    testable.

    - The error handling of this code is also improved by replacing std::cerr
      output with a vector. The vector will contain all errors that were
      encountered during the parsing of the inputs files*.

Tests:

  - This CL adds a lot of unittests for the transport security state generator.
    These tests become part of the net_unittests target.

  - On the Chromium side, unittests are added to test the decoding of the
    generated data. The input data is generated during the build.

* This currently is always, at most, 1 error but the plan is to change that when
issues 677294 and 568378 are addressed.

BUG=595493
==========

[martijnc, 2017-02-06 21:39:29]

martijn@martijnc.be changed reviewers:
+ lgarron@chromium.org, rsleevi@chromium.org

[martijnc, 2017-02-07 21:23:51]

Description was changed from

==========
Add transport security state generator tests.

This CL adds tests for the transport security state generator and refactors
parts of the existing code to make it testable.

Changes to existing code:

  - The preloaded transport security state data source is made configurable for
    the tests. It was previously hardcoded to the data in
    transport_security_state_static.h

  - The Pinset struct definition is moved to a new file to make it reusable
    between transport_security_state_static.h and the related unittest files.

  - The build configuration is moved to its own BUILD.gn file.

  - The (trie) generation code is reworked to improve the handling of incorrect
    inputs. The existing code would CHECK or DCHECK where the old go script
    panic()'ed. This makes it hard to properly test this code. Most (D)CHECKs
are
    removed in favor of boolean return values. Any existing return values are
    converted to out parameters.

  - The code responsible for parsing the input files is moved to a separate file
    (input_file_parsers.(h|cc)) and out of the anonymous namespace to make them
    testable.

    - The error handling of this code is also improved by replacing std::cerr
      output with a vector. The vector will contain all errors that were
      encountered during the parsing of the inputs files*.

Tests:

  - This CL adds a lot of unittests for the transport security state generator.
    These tests become part of the net_unittests target.

  - On the Chromium side, unittests are added to test the decoding of the
    generated data. The input data is generated during the build.

* This currently is always, at most, 1 error but the plan is to change that when
issues 677294 and 568378 are addressed.

BUG=595493
==========

to

==========
Add transport security state generator tests.

This CL adds tests for the transport security state generator and refactors
parts of the existing code to make it testable.

Changes to existing code:

  - The preloaded transport security state data source is made configurable for
    the tests. It was previously hardcoded to the data in
    transport_security_state_static.h

  - The Pinset struct definition is moved to a separate file to make it reusable
    between transport_security_state_static.h and the related unittests.

  - The build configuration is moved to its own BUILD.gn file.

  - The (trie) generation code was refactored to improve the handling of
incorrect
    inputs. The existing code would CHECK or DCHECK where the old go script
    panic()'ed. This makes it hard to properly test this code. Most (D)CHECKs
are
    removed in favor of boolean return values. Any existing return values are
    converted to out parameters.

  - The code responsible for parsing the input files is moved to a separate file
    (input_file_parsers.(h|cc)) and out of the anonymous namespace to make them
    testable.

    - The error handling of this code is also improved by replacing std::cerr
      output with a vector. The vector will contain all errors that were
      encountered during the parsing of the inputs files*.

Tests:

  - This CL adds unittests for the transport security state generator.
    These tests are run as part of the net_unittests target.

  - On the Chromium side, unittests are added to test the decoding of the
    generated data. The input data for these tests is generated during the
build.

* This currently is always, at most, 1 error but the plan is to change that when
issues 677294 and 568378 are addressed.

BUG=595493
==========

[martijnc, 2017-02-07 21:51:58]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 21:52:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 22:04:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 22:04:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[martijnc, 2017-02-07 22:21:35]

Patchset #3 (id:120001) has been deleted

[martijnc, 2017-02-08 18:07:30]

Patchset #5 (id:180001) has been deleted

[martijnc, 2017-02-08 18:46:51]

Patchset #4 (id:160001) has been deleted

[martijnc, 2017-02-08 19:19:57]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 19:20:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 19:38:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 19:38:20]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[martijnc, 2017-02-08 19:46:23]

Patchset #3 (id:140001) has been deleted

[martijnc, 2017-02-08 19:46:28]

Patchset #2 (id:100001) has been deleted

[martijnc, 2017-02-08 19:46:45]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 19:47:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[martijnc, 2017-02-08 20:44:07]

Patchset #2 (id:200001) has been deleted

[martijnc, 2017-02-08 20:58:22]

Apologies for the delay, I had hoped to get this done sooner.

Although most changes are either new tests or moved code, the combination has
resulted in a large CL. I can split it up into smaller patches if you want.

Can you take a look? Thanks!

https://codereview.chromium.org/2660793002/diff/220001/net/http/transport_sec...
File net/http/transport_security_state_static.h (left):

https://codereview.chromium.org/2660793002/diff/220001/net/http/transport_sec...
net/http/transport_security_state_static.h:779: -struct Pinset {
This is moved to a separate file because the unittests have their own
(generated) data and need to define Pinsets but don't have access to the
definition here.

https://codereview.chromium.org/2660793002/diff/220001/net/http/transport_sec...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/2660793002/diff/220001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:45: namespace test1 {
I tried to mix several cases (common prefix, no common prefix, partial common
prefix, entry as prefix of other entries) to get coverage for all possible
structures in the trie.

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
File net/tools/transport_security_state_generator/bit_writer.h (left):

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
net/tools/transport_security_state_generator/bit_writer.h:42: // Returns the
minimum number of bits needed to represent |input|.
This wasn't used.

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
File net/tools/transport_security_state_generator/huffman/huffman_builder.cc
(left):

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
net/tools/transport_security_state_generator/huffman/huffman_builder.cc:125: if
(item.second > 0) {
This condition is always true because if the value is never used it won't be in
|counts_| and any value that is used will have a count of 1 or higher.

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
File net/tools/transport_security_state_generator/huffman/huffman_builder.cc
(right):

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
net/tools/transport_security_state_generator/huffman/huffman_builder.cc:131: //
At least 2 nodes are required for everything to work properly. Add
ToVector() expects at least 2 nodes.

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
File net/tools/transport_security_state_generator/preloaded_state_generator.cc
(right):

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
net/tools/transport_security_state_generator/preloaded_state_generator.cc:270:
output.append(kIndent);
There are tests without Expect-CT and Expect-Staple report URIs which throw
compiler errors because these arrays would be empty in those cases.

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
File net/tools/transport_security_state_generator/trie/trie_writer.cc (right):

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
net/tools/transport_security_state_generator/trie/trie_writer.cc:177: if (pin_id
> 15) {
The check was incorrect before, 4 bits can encode the values 0-15 so 16 would
overflow.

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
File net/tools/transport_security_state_generator/trie/trie_writer_unittest.cc
(right):

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
net/tools/transport_security_state_generator/trie/trie_writer_unittest.cc:146:
EXPECT_THAT(trie_writer->bytes(),
I've verified that these expectations are correct (like the one above) and tried
to write down the reasoning but that got very complicated and hard to follow.
The encoded bytes here don't align with the bytes in the expectations so you
have to constantly juggle bits. There are also 'jumps' from position to position
(within bytes) which complicates things further.

Are there better ways to define these expectations?

[Ryan Sleevi, 2017-02-08 21:45:29]

Going to be very slow on reviews ,unfortunately, and this is a huge review.

I would definitely encourage you to break it into smaller reviews where
possible; I think it's useful to have this CL as you've done so we can see and
look back at "This is the overall direction we're going towards", but then see
if you can't break it into smaller sequential CLs that each do just enough to
make a consistent and holistic step forward, without being overwhelming.

Does that work?

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
File net/tools/transport_security_state_generator/trie/trie_writer_unittest.cc
(right):

https://codereview.chromium.org/2660793002/diff/220001/net/tools/transport_se...
net/tools/transport_security_state_generator/trie/trie_writer_unittest.cc:146:
EXPECT_THAT(trie_writer->bytes(),
On 2017/02/08 20:58:21, martijnc wrote:
> Are there better ways to define these expectations?

That's a really tough question. You're effectively measuring pristine input.

I suppose the question here is: What invariants of the (public) API are you
trying to test? That is, in thinking about "Test the interface, not the
implementation" - what is the contract that the interface of TrieWriter
guarantees? Can we change the implementation at all?

For example, if we changed the entry/endOfTableValue or something like that, how
should that work?

In thinking about those questions and how to answer them, one suggestion would
be "Is the interface such that TrieWriter outputs something that returns the
same input via [TrieReader/whatever?]"? If so, then you could write the test
such that it does a round-trip - that you get the same output for input.

However, if the interface is such that you output these exact bytes, then I
suppose that's valid. However, it does seem to make it more likely that anytime
someone changes the implementation, they'll have these tests fail, then say
"Ugh, how do I regenerate and why are these tests here", and just copy the
'actual' value to the 'expected' value and call it a day.

[commit-bot: I haz the power, 2017-02-08 21:45:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 21:45:54]

Dry run: This issue passed the CQ dry run.

[martijnc, 2017-02-09 21:23:17]

On 2017/02/08 at 21:45:29, rsleevi wrote:
> Going to be very slow on reviews ,unfortunately, and this is a huge review.
> 
> I would definitely encourage you to break it into smaller reviews where
possible; I think it's useful to have this CL as you've done so we can see and
look back at "This is the overall direction we're going towards", but then see
if you can't break it into smaller sequential CLs that each do just enough to
make a consistent and holistic step forward, without being overwhelming.
> 
> Does that work?

Yes, I can probably split this up into 4 or 5 smaller CLs. I'll leave this one
up to provide an overview of the changes.