Refactor how net/data/verify_certificate_chain_unittest/*

Refactor how net/data/verify_certificate_chain_unittest/* data is generated.

* Use directory structure to group the generators
* Separate test inputs/expectations from chain generation
* Use a simple CERTIFICATE sequence for chains, since test file carries other information.

This decoupling makes it easier to re-use the chain generation code elsewhere, as well as to have tests that use the same chain but with different input parameters to certificate verification.

Review-Url: https://codereview.chromium.org/2805213004
Cr-Commit-Position: refs/heads/master@{#468857}
Committed: https://chromium.googlesource.com/chromium/src/+/266bda9e1395873338822a426948bf355696ee52

[eroman, 2017-04-08 00:38:38]

eroman@chromium.org changed reviewers:
+ mattm@chromium.org

[eroman, 2017-04-08 00:38:39]

WDYT of this approach?

(It should make it easier to smuggle in new parameters in the future, and more
closely matches the abstraction on C++ side).

I translated two files as a test (ie. this is incomplete, just sending it out
for high level feedback).

If you think this is worthwhile I can finish the patch.

[mattm, 2017-04-11 00:25:48]

hm, I guess another option would be to make use of python named keyword
arguments (you can say write_to_file(description="blah", chain=[a,b,c],
trust_anchor=anchor, etc))

That would also be more obvious which arguments are non-optional from looking at
the function definition.

With python that would also safer against typos (eg, you could typo the name of
some optional member like test_case.optoinal_member=foo, and not get an error
unless you notice that the generated file doesn't have the correct thing set.
Unless you use __slots__. I don't know if using __slots__ for that is considered
good style or not.)

[eroman, 2017-04-11 16:48:15]

Thinking more about this, more, how about this more radical change in test
structure. Rather than having generate-XXX.py and XXX.pem and keys/XXX/*.key, I
can split tests into this directory structure:

....verify_certificate_chain_unittest/
........XXX/
............generate.py
............chain.pem
............roots.pem
............keys/
............verify_inputs.txt
............verify_expectation.txt

generate.py would now be responsible ONLY for generating the first 3 files
(chain.pem, roots.pem, keys/).

The other inputs and outputs would be edited directly via the .txt file and
hence not need to be part of the python generation.

This simplifies a couple things:
 * Having chain.pem vs roots.pem makes it possible to feed the test cases
directly to cert_verify_tool (which takes a --roots file of CERTIFICATE)
 * Having expectations in a separate file simplifies rebase_errors.py, as it
doesn't need to rewrite the generator script (just needs to replace text in the
expectations file)
  * Having expectations in a separate file makes the scripts without those
expectations less of an oddity. Also makes it easier to re-use of the chain in
other places and provide different inputs/expectations.

WDYT?

[eroman, 2017-04-14 23:37:54]

WDYT of the approach in patchset 2? (not ready for a real review, just design
review).

The organization is:

....verify_certificate_chain_unittest/
........XXX/
............generate.py
............chain.pem
............main.test
............keys/


generate.py --> writes out chain.pem

main.test --> lists the test inputs and expectations (including the path to
chain.pem)

[eroman, 2017-05-02 02:18:05]

Description was changed from

==========
Refactor how test case parameters are set in the generate-*.py

Use a structure to better hide default parameters, and avoid positional
ambiguity in long parameter lists.
==========

to

==========
Refactor how net/data/verify_certificate_chain/* test data is generated.

* Use directory structure to group the generators
* Separate test inputs/expectations from chain generation
* Use a simple CERTIFICATE sequence for chains, since test file carries other
information.

This decoupling makes it easier to re-use the chain generation code elsewhere,
as well as to have tests that use the same chain but with different input
parameters to certificate verification.
==========

[eroman, 2017-05-02 02:24:39]

ping

[mattm, 2017-05-02 06:43:47]

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/path_...
File net/cert/internal/path_builder_verify_certificate_chain_unittest.cc
(right):

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/path_...
net/cert/internal/path_builder_verify_certificate_chain_unittest.cc:52:
EXPECT_EQ(test.expected_errors.empty(), result.HasValidPath());
This is fine, but I wonder if maybe it should have a comment?

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/test_...
File net/cert/internal/test_helpers.cc (right):

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:185: std::string chain_path = file_path_ascii;
initialized value here isn't used.

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:233: test->expected_errors =
file_data.substr(errors_start + 18);
I guess "\nexpected_errors:\n" could be a constant so you could get the length
of that instead of hardcoding 18 here

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:235: }
should it error on unrecognized lines?

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/README (right):

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/README:39: utc_time: <string of the
UTC time to do verification at>
key_purpose should be mentioned here

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/README:40: errors: "" | "\n" <errors>
expected_errors

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/common.py (right):

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/common.py:157: #Save keys to CWD /
keys / < generate - script - name >/*.key
indentation, space after #

are the internal spaces intentional?

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/rebase-errors.py (right):

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/rebase-errors.py:98: #Read the input
either from a file, or from stdin.
indentation/spacing

[eroman, 2017-05-02 19:19:47]

Description was changed from

==========
Refactor how net/data/verify_certificate_chain/* test data is generated.

* Use directory structure to group the generators
* Separate test inputs/expectations from chain generation
* Use a simple CERTIFICATE sequence for chains, since test file carries other
information.

This decoupling makes it easier to re-use the chain generation code elsewhere,
as well as to have tests that use the same chain but with different input
parameters to certificate verification.
==========

to

==========
Refactor how net/data/verify_certificate_chain_unittest/* data is generated.

* Use directory structure to group the generators
* Separate test inputs/expectations from chain generation
* Use a simple CERTIFICATE sequence for chains, since test file carries other
information.

This decoupling makes it easier to re-use the chain generation code elsewhere,
as well as to have tests that use the same chain but with different input
parameters to certificate verification.
==========

[eroman, 2017-05-02 19:20:23]

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/path_...
File net/cert/internal/path_builder_verify_certificate_chain_unittest.cc
(right):

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/path_...
net/cert/internal/path_builder_verify_certificate_chain_unittest.cc:52:
EXPECT_EQ(test.expected_errors.empty(), result.HasValidPath());
On 2017/05/02 06:43:46, mattm wrote:
> This is fine, but I wonder if maybe it should have a comment?

I have tried to address this by adding a helper HasHighSeverityErrors().

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/test_...
File net/cert/internal/test_helpers.cc (right):

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:185: std::string chain_path = file_path_ascii;
On 2017/05/02 06:43:46, mattm wrote:
> initialized value here isn't used.

Done.

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:233: test->expected_errors =
file_data.substr(errors_start + 18);
On 2017/05/02 06:43:46, mattm wrote:
> I guess "\nexpected_errors:\n" could be a constant so you could get the length
> of that instead of hardcoding 18 here

Done.

https://codereview.chromium.org/2805213004/diff/30001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:235: }
On 2017/05/02 06:43:46, mattm wrote:
> should it error on unrecognized lines?

Done.

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/README (right):

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/README:39: utc_time: <string of the
UTC time to do verification at>
On 2017/05/02 06:43:46, mattm wrote:
> key_purpose should be mentioned here

Done.

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/README:40: errors: "" | "\n" <errors>
On 2017/05/02 06:43:46, mattm wrote:
> expected_errors

Done.

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/common.py (right):

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/common.py:157: #Save keys to CWD /
keys / < generate - script - name >/*.key
On 2017/05/02 06:43:46, mattm wrote:
> indentation, space after #
> 
> are the internal spaces intentional?

Done. 
(I think these lines got messed up by some formatter)

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/rebase-errors.py (left):

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/rebase-errors.py:89: def
fixup_pem_file(path, actual_errors):
Turns out this is still used by data/parse_certificate_unittest - I moved this
to its own rebaser.

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/rebase-errors.py (right):

https://codereview.chromium.org/2805213004/diff/30001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/rebase-errors.py:98: #Read the input
either from a file, or from stdin.
On 2017/05/02 06:43:46, mattm wrote:
> indentation/spacing

Done.

[eroman, 2017-05-02 19:32:19]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 19:32:59]

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-05-02 20:04:51]

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

[commit-bot: I haz the power, 2017-05-02 20:04:54]

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...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[eroman, 2017-05-02 22:07:17]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 22:07:40]

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-05-02 22:44:53]

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

[commit-bot: I haz the power, 2017-05-02 22:44:56]

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...)

[eroman, 2017-05-02 23:29:39]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 23:30:13]

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-05-03 00:33:22]

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

[commit-bot: I haz the power, 2017-05-03 00:33:26]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-05-03 00:55:16]

lgtm

(I didn't check every test data file)

https://codereview.chromium.org/2805213004/diff/70001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/generate-all.sh (right):

https://codereview.chromium.org/2805213004/diff/70001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/generate-all.sh:10: cd $dir
cd "$dir"

[eroman, 2017-05-03 00:57:51]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2017-05-03 00:58:26]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-05-03 01:02:22]

Thanks!

> (I didn't check every test data file)

FWIW I used a script to do the moves and updating the generator python files --
so if a sampling of them looks good, then should have high confidence that the
rest are the same.

The only exceptions are these ones, which I translated manually:

  * issuer-and-subject-not-byte-for-byte-equal/
  * key-rollover/

[commit-bot: I haz the power, 2017-05-03 01:28:12]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1493773071003560,
"parent_rev": "4575e0c18c3fea30e4f59ccc4c00fca6e6014698", "commit_rev":
"266bda9e1395873338822a426948bf355696ee52"}

[commit-bot: I haz the power, 2017-05-03 01:28:26]

Description was changed from

==========
Refactor how net/data/verify_certificate_chain_unittest/* data is generated.

* Use directory structure to group the generators
* Separate test inputs/expectations from chain generation
* Use a simple CERTIFICATE sequence for chains, since test file carries other
information.

This decoupling makes it easier to re-use the chain generation code elsewhere,
as well as to have tests that use the same chain but with different input
parameters to certificate verification.
==========

to

==========
Refactor how net/data/verify_certificate_chain_unittest/* data is generated.

* Use directory structure to group the generators
* Separate test inputs/expectations from chain generation
* Use a simple CERTIFICATE sequence for chains, since test file carries other
information.

This decoupling makes it easier to re-use the chain generation code elsewhere,
as well as to have tests that use the same chain but with different input
parameters to certificate verification.

Review-Url: https://codereview.chromium.org/2805213004
Cr-Commit-Position: refs/heads/master@{#468857}
Committed:
https://chromium.googlesource.com/chromium/src/+/266bda9e1395873338822a426948...
==========

[commit-bot: I haz the power, 2017-05-03 01:28:28]

Committed patchset #7 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/266bda9e1395873338822a426948...