Save the private keys used by generated verify_certificate_chain tests.

Save the private keys used by generated verify_certificate_chain tests.

This way re-generating test data can use the same key and is stable.

...At least for the majority of the test data. The ECDSA-signed
certificates will still result in a different signature each time you
run, because it uses some un-mockable randomness.

Review-Url: https://codereview.chromium.org/2797303006
Cr-Commit-Position: refs/heads/master@{#463511}
Committed: https://chromium.googlesource.com/chromium/src/+/2ec3bdfff0f3a33971998380fc2bfe179d69fa72

[eroman, 2017-04-06 23:47:32]

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

[commit-bot: I haz the power, 2017-04-06 23:48:25]

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

[eroman, 2017-04-06 23:51:19]

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

[eroman, 2017-04-06 23:51:20]

WDYT? I got annoyed at all the diffs in the last CL I sent you, so figured I
would make it easer to re-run things in the future.

Note that the .pem files were all auto-generated, I don't expect you to review
them all.

[commit-bot: I haz the power, 2017-04-07 00:58:12]

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

[commit-bot: I haz the power, 2017-04-07 00:58:13]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-04-07 22:01:27]

https://codereview.chromium.org/2797303006/diff/1/net/data/verify_certificate...
File net/data/verify_certificate_chain_unittest/common.py (right):

https://codereview.chromium.org/2797303006/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:129: if not
file_contents.startswith(generation_arguments_str):
This could just match part of the generation args in the file if it has a common
prefix. Should have generation_arguments_str include the newline, or equality
compare against the first line of the file (ex, using splitlines()[0]).
I guess the splitlines approach would be safe against different line endings.

https://codereview.chromium.org/2797303006/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:132: 'requested
parameters:\n%s.\n') % (path, generation_arguments_str))
might be helpful to mention you can delete the key file to have a new one
generated

https://codereview.chromium.org/2797303006/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:155: return
get_unique_path_id('keys/' + g_script_name + '_' + base_name) + '.key'
This may work a bit less well for other scripts that use common.py to generate a
bunch of certs directly rather than using the write_test_file interface. (eg
net/data/cert_issuer_source_aia_unittest/generate-certs.py,
net/data/cert_issuer_source_static_unittest/generate-certs.py)

Looks like it does work, but you end up with keys like "certs_target.key",
"certs_target_1.key", "certs_target_2.key", etc which don't correspond to the
names the cert is actually saved as. And would probably get re-associated with a
different cert if you added/removed/re-ordered the cert creation.

Those scripts currently save a cert like:
common.write_string_to_file(target.get_cert_pem(), 'target_one_aia.pem')        
                                                                                
          

I guess we could have a helper method that takes the target file name, and uses
it both to generate the key(if it hasn't already had one manually set with
set_key) and output the cert file.

wdyt?

[eroman, 2017-04-07 23:34:56]

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

[commit-bot: I haz the power, 2017-04-07 23:35:37]

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

[eroman, 2017-04-07 23:36:33]

https://codereview.chromium.org/2797303006/diff/1/net/data/verify_certificate...
File net/data/verify_certificate_chain_unittest/common.py (right):

https://codereview.chromium.org/2797303006/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:129: if not
file_contents.startswith(generation_arguments_str):
On 2017/04/07 22:01:27, mattm wrote:
> This could just match part of the generation args in the file if it has a
common
> prefix. Should have generation_arguments_str include the newline, or equality
> compare against the first line of the file (ex, using splitlines()[0]).
> I guess the splitlines approach would be safe against different line endings.

Good point! Done.

https://codereview.chromium.org/2797303006/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:132: 'requested
parameters:\n%s.\n') % (path, generation_arguments_str))
On 2017/04/07 22:01:27, mattm wrote:
> might be helpful to mention you can delete the key file to have a new one
> generated

Done. I also print out the contradiction explicitly now.

https://codereview.chromium.org/2797303006/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:155: return
get_unique_path_id('keys/' + g_script_name + '_' + base_name) + '.key'
On 2017/04/07 22:01:27, mattm wrote:
> This may work a bit less well for other scripts that use common.py to generate
a
> bunch of certs directly rather than using the write_test_file interface. (eg
> net/data/cert_issuer_source_aia_unittest/generate-certs.py,
> net/data/cert_issuer_source_static_unittest/generate-certs.py)
> 
> Looks like it does work, but you end up with keys like "certs_target.key",
> "certs_target_1.key", "certs_target_2.key", etc which don't correspond to the
> names the cert is actually saved as. And would probably get re-associated with
a
> different cert if you added/removed/re-ordered the cert creation.
> 
> Those scripts currently save a cert like:
> common.write_string_to_file(target.get_cert_pem(), 'target_one_aia.pem')      
 
>                                                                               
 
>           
> 
> I guess we could have a helper method that takes the target file name, and
uses
> it both to generate the key(if it hasn't already had one manually set with
> set_key) and output the cert file.
> 
> wdyt?

I made a few changes which hopefully address those concerns:

 (1) Changed to a directory grouping under keys/ to make the names less terrible
 (2) For the specific test you hilighted, I extracted 3 named keys
 (3) [bugfix] added code to automatically create the keys/ directory when
missing

[commit-bot: I haz the power, 2017-04-07 23:43:12]

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

[commit-bot: I haz the power, 2017-04-07 23:43:13]

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

[eroman, 2017-04-08 00:03:18]

Patchset #5 (id:40001) has been deleted

[eroman, 2017-04-08 00:06:19]

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

[commit-bot: I haz the power, 2017-04-08 00:06:43]

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-04-08 00:12:56]

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

[commit-bot: I haz the power, 2017-04-08 00:12:57]

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

[eroman, 2017-04-08 00:54:02]

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

[commit-bot: I haz the power, 2017-04-08 00:54:20]

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-04-08 02:08:33]

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

[commit-bot: I haz the power, 2017-04-08 02:08:35]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-04-10 23:13:00]

lgtm

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

https://codereview.chromium.org/2797303006/diff/60001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/README:23: The keys used/generated by
test XXX. The privat keys shouldn't be needed to run
private

[eroman, 2017-04-10 23:56:30]

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

https://codereview.chromium.org/2797303006/diff/60001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/README:23: The keys used/generated by
test XXX. The privat keys shouldn't be needed to run
On 2017/04/10 23:13:00, mattm wrote:
> private

Done.

[eroman, 2017-04-10 23:56:48]

The CQ bit was checked by eroman@chromium.org

[eroman, 2017-04-10 23:56:49]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2797303006/#ps70001
(title: "Fix comment")

[commit-bot: I haz the power, 2017-04-10 23:57:24]

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-04-11 03:12:42]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1491868608747790,
"parent_rev": "4044498152ad71d05c57aee559cba6a97db82375", "commit_rev":
"2ec3bdfff0f3a33971998380fc2bfe179d69fa72"}

[commit-bot: I haz the power, 2017-04-11 03:14:03]

Description was changed from

==========
Save the private keys used by generated verify_certificate_chain tests.

This way re-generating test data can use the same key and is stable.

...At least for the majority of the test data. The ECDSA-signed
certificates will still result in a different signature each time you
run, because it uses some un-mockable randomness.
==========

to

==========
Save the private keys used by generated verify_certificate_chain tests.

This way re-generating test data can use the same key and is stable.

...At least for the majority of the test data. The ECDSA-signed
certificates will still result in a different signature each time you
run, because it uses some un-mockable randomness.

Review-Url: https://codereview.chromium.org/2797303006
Cr-Commit-Position: refs/heads/master@{#463511}
Committed:
https://chromium.googlesource.com/chromium/src/+/2ec3bdfff0f3a33971998380fc2b...
==========

[commit-bot: I haz the power, 2017-04-11 03:14:05]

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