Add scripts to generate simple test data for certificate verification.

Add scripts to generate simple test data for certificate verification.

The intent is to complement other path validation test suites (i.e PKITS). Certainly there is overlap, but the advantage is easier manipulation and addition of test cases.

This introduces some python scripts and helpers that shell out to "openssl req" and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) are used as test data by the follow-up CL (https://codereview.chromium.org/1414923007)

BUG=410574
Committed: https://crrev.com/e461b9127d35010e41e47cbb9f07a4bc25fc0866
Cr-Commit-Position: refs/heads/master@{#365922}

[eroman, 2015-10-28 02:00:19]

Description was changed from

==========
Add scripts to generate simple test data for certificate verification.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) will be used as
test data for a forthcoming changelist on certificate chain verification.

BUG=410574
==========

to

==========
Add scripts to generate simple test data for certificate verification.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) will be used as
test data for a forthcoming changelist on certificate chain verification
(https://codereview.chromium.org/1414923007)

BUG=410574
==========

[eroman, 2015-10-28 02:00:35]

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

[mattm, 2015-10-29 01:47:17]

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

[mattm, 2015-10-29 01:47:18]

Nice. Mostly nit-picky comments.

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

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:19: sys.path.insert(0,
os.path.dirname(__file__))
This is unnecessary, can import files in the same dir by default.

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:42: g_invoking_script_name
= None
You could use sys.argv[0] to get it instead of having each script pass their
__file__ value in. Though I guess that would break if you ever wanted to make
generate-all a python script that imported each of the generation scripts
instead of executing them separately. Dunno if that's worth worrying about.

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:204: 
nit: extra line

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:359: verify_result =
'SUCCESS'
I found it confusing to overwrite the original argument. Could use a conditional
in the DataToPem call (eg: DataToPem('VERIFY_RESULT', 'SUCCESS' if verify_result
else 'FAIL'), or just a different variable name.

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:369: if len(parts) > 0:
should be > 1

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:370: stripped_script_name =
parts[1]
Or you could just do:
stripped_script_name = g_invoking_script_name.split('generate-', 1)[-1]
But maybe that's less obvious. Whichever.

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:395: os.makedirs('out')
no need for os.makedirs here, use os.mkdir?

Actually don't need to do this at all since the os.makedirs below would create
it.

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
File net/data/verify_certificate_chain_unittest/generate-all.sh (right):

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/generate-all.sh:6: for script in $(ls
generate-*.py) ; do
don't need the $(ls), just:
for script in generate-*.py ; do

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/generate-all.sh:7: python $script
Add quotes around $script

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
File
net/data/verify_certificate_chain_unittest/generate-basic-constraints-pathlen-0-self-issued.py
(right):

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/generate-basic-constraints-pathlen-0-self-issued.py:12:
description = """Certificate chain with 2 intermediaries. The first
Could use a regular doc string, and __doc__ to access it.

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

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/openssl_conf.py:25: Would be a
property where:
I wasn't clear if this is just "property" in general or referring to the
Property class.
On my initial read it seemed that maybe should capitalize Property here, or even
just say:
Would be a Property('baseConstraints', 'critical, CA:false').
But maybe that was intentional.
(Similar comment for Section below.)

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/openssl_conf.py:53: adds it"""
Should define how this (and RemoveProperty) handle the case of duplicates.

[eroman, 2015-10-31 00:34:25]

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

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:19: sys.path.insert(0,
os.path.dirname(__file__))
On 2015/10/29 01:47:18, mattm (OOO oct 30) wrote:
> This is unnecessary, can import files in the same dir by default.

Done.

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:42: g_invoking_script_name
= None
On 2015/10/29 01:47:18, mattm (OOO oct 30) wrote:
> You could use sys.argv[0] to get it instead of having each script pass their
> __file__ value in. Though I guess that would break if you ever wanted to make
> generate-all a python script that imported each of the generation scripts
> instead of executing them separately. Dunno if that's worth worrying about.

Done (good idea, simplifies the boilerplate for each generator script)

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:204: 
On 2015/10/29 01:47:17, mattm (OOO oct 30) wrote:
> nit: extra line

Done.

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:359: verify_result =
'SUCCESS'
On 2015/10/29 01:47:17, mattm (OOO oct 30) wrote:
> I found it confusing to overwrite the original argument. Could use a
conditional
> in the DataToPem call (eg: DataToPem('VERIFY_RESULT', 'SUCCESS' if
verify_result
> else 'FAIL'), or just a different variable name. 

Done (I didn't know that is how you did tertiary operator in python!)

Now it is:

  verify_result_string = 'SUCCESS' if verify_result else 'FAIL'
  test_data += '\n' + DataToPem('VERIFY_RESULT', verify_result_string)

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:369: if len(parts) > 0:
On 2015/10/29 01:47:17, mattm (OOO oct 30) wrote:
> should be > 1

No longer relevant (in fact I merged this logic into Init())

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:370: stripped_script_name =
parts[1]
On 2015/10/29 01:47:18, mattm (OOO oct 30) wrote:
> Or you could just do:
> stripped_script_name = g_invoking_script_name.split('generate-', 1)[-1]
> But maybe that's less obvious. Whichever.

I opted for this:

  # Strip the leading 'generate-'
  if out_name.startswith('generate-'):
    out_name = out_name[9:] 

But I can switch to the split version if you prefer, since 9: is kind of yuck.

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/common.py:395: os.makedirs('out')
On 2015/10/29 01:47:17, mattm (OOO oct 30) wrote:
> no need for os.makedirs here, use os.mkdir?
> 
> Actually don't need to do this at all since the os.makedirs below would create
> it.

Done (just keeping the on call to mkdirs)

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
File net/data/verify_certificate_chain_unittest/generate-all.sh (right):

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/generate-all.sh:6: for script in $(ls
generate-*.py) ; do
On 2015/10/29 01:47:18, mattm (OOO oct 30) wrote:
> don't need the $(ls), just:
> for script in generate-*.py ; do

Done.

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/generate-all.sh:7: python $script
On 2015/10/29 01:47:18, mattm (OOO oct 30) wrote:
> Add quotes around $script

Done.

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
File
net/data/verify_certificate_chain_unittest/generate-basic-constraints-pathlen-0-self-issued.py
(right):

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/generate-basic-constraints-pathlen-0-self-issued.py:12:
description = """Certificate chain with 2 intermediaries. The first
On 2015/10/29 01:47:18, mattm (OOO oct 30) wrote:
> Could use a regular doc string, and __doc__ to access it.

Done (neato)

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

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/openssl_conf.py:25: Would be a
property where:
On 2015/10/29 01:47:18, mattm (OOO oct 30) wrote:
> I wasn't clear if this is just "property" in general or referring to the
> Property class.
> On my initial read it seemed that maybe should capitalize Property here, or
even
> just say:
> Would be a Property('baseConstraints', 'critical, CA:false').
> But maybe that was intentional.
> (Similar comment for Section below.)

Done. Changed to:

 25   Could be represented by a Property where:
 26     name = 'baseConstraints'
 27     value = 'critical, CA:false'

And for Section I changed to:

 44   Could be represented by a Section where:
 45     name = 'CA_root'       
 46     properties = [Property('preserve', 'true')]

https://codereview.chromium.org/1414393008/diff/1/net/data/verify_certificate...
net/data/verify_certificate_chain_unittest/openssl_conf.py:53: adds it"""
On 2015/10/29 01:47:18, mattm (OOO oct 30) wrote:
> Should define how this (and RemoveProperty) handle the case of duplicates.

Done, good point.

I have documented that duplicates are not allowed for SetProperties() and for
RemoveProperty(), and enforced it in the code (rather than doing something
arbitrary like changing the first property found):


 63   def SetProperty(self, name, value):
 64     """Replaces, adds, or removes a Property from the Section:
 65 
 66       - If |value| is None, then this is equivalent to calling
 67         RemoveProperty(name)
 68       - If there is an existing property matching |name| then its value is
 69         replaced with |value|
 70       - If there are no properties matching |name| then a new one is added
at
 71         the end of the section
 72       
 73     It is expected that there is AT MOST 1 property with the given name. If
 74     that is not the case then this function will raise an error."""

[mattm, 2015-11-02 19:40:39]

lgtm

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

https://codereview.chromium.org/1414393008/diff/40001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/common.py:47: g_out_pem = None;
unnecessary semicolons

https://codereview.chromium.org/1414393008/diff/40001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/openssl_conf.py (right):

https://codereview.chromium.org/1414393008/diff/40001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/openssl_conf.py:58: count += 1
If you want to get into python tricks:
count = sum(1 for prop in self.properties if prop.name == name)

[eroman, 2015-11-02 20:51:43]

I also updated the naming style for function names (output from script I ran):

replacing EnsurePropertyNameNotDuplicated with
ensure_property_name_not_duplicated ...
replacing CreateSelfSignedRootCertificate with
create_self_signed_root_certificate ...
replacing CreateIntermediaryCertificate with create_intermediary_certificate ...
replacing CreateEndEntityCertificate with create_end_entity_certificate ...
replacing WriteStringToFile with write_string_to_file ...
replacing SetValidityRange with set_validity_range ...
replacing SetSignatureHash with set_signature_hash ...
replacing GetDatabasePath with get_database_path ...
replacing GenerateRsaKey with generate_rsa_key ...
replacing RemoveProperty with remove_property ...
replacing WriteTestFile with write_test_file ...
replacing GetSerialPath with get_serial_path ...
replacing GetConfigPath with get_config_path ...
replacing GenerateEcKey with generate_ec_key ...
replacing GetExtensions with get_extensions ...
replacing WriteToFile with write_to_file ...
replacing GetCertPath with get_cert_path ...
replacing SetProperty with set_property ...
replacing AddProperty with add_property ...
replacing GetKeyPath with get_key_path ...
replacing GetCsrPath with get_csr_path ...
replacing GetCertPem with get_cert_pem ...
replacing InitConfig with init_config ...
replacing GetSection with get_section ...
replacing DataToPem with data_to_pem ...
replacing Finalize with finalize ...
replacing WriteTo with write_to ...
replacing WriteTo with write_to ...
replacing GetPath with get_path ...
replacing Init with init ...

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

https://codereview.chromium.org/1414393008/diff/40001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/common.py:47: g_out_pem = None;
On 2015/11/02 19:40:39, mattm wrote:
> unnecessary semicolons

Done.

https://codereview.chromium.org/1414393008/diff/40001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/openssl_conf.py (right):

https://codereview.chromium.org/1414393008/diff/40001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/openssl_conf.py:58: count += 1
On 2015/11/02 19:40:39, mattm wrote:
> If you want to get into python tricks:
> count = sum(1 for prop in self.properties if prop.name == name)

Thanks, I decided not to use that (since I find it harder to read as a python
n00b).

[eroman, 2015-11-02 21:12:35]

eroman@chromium.org changed reviewers:
- rsleevi@chromium.org

[eroman, 2015-11-02 21:12:35]

Removed Ryan as a reviewer.

And adding David as FYI in case he wants to comment.

[davidben, 2015-11-19 21:21:28]

On 2015/11/02 21:12:35, eroman wrote:
> Removed Ryan as a reviewer.
> 
> And adding David as FYI in case he wants to comment.

(Er, my silence should be taken as not having any opinions, particularly because
it's Python code and shelling out to OpenSSL, none of which I know that much of.
If mattm is happy, go for it.)

[eroman, 2015-12-03 03:18:32]

I added two new tests to this change:

target-has-keycertsign-but-not-ca
target-has-pathlen-but-not-ca

[mattm, 2015-12-17 02:40:31]

still lgtm

[eroman, 2015-12-17 19:51:51]

Description was changed from

==========
Add scripts to generate simple test data for certificate verification.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) will be used as
test data for a forthcoming changelist on certificate chain verification
(https://codereview.chromium.org/1414923007)

BUG=410574
==========

to

==========
Add scripts to generate simple test data for certificate verification.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) will be used as
test data for a forthcoming changelist on certificate chain verification
(https://codereview.chromium.org/1414923007)

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap. The advantage is ease of manipulation and addition
of new test cases.

BUG=410574
==========

[eroman, 2015-12-17 19:52:38]

Description was changed from

==========
Add scripts to generate simple test data for certificate verification.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) will be used as
test data for a forthcoming changelist on certificate chain verification
(https://codereview.chromium.org/1414923007)

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap. The advantage is ease of manipulation and addition
of new test cases.

BUG=410574
==========

to

==========
Add scripts to generate simple test data for certificate verification.

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap, but the advantage is easier manipulation and
addition of new test cases.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) will be used as
test data for a forthcoming changelist on certificate chain verification
(https://codereview.chromium.org/1414923007)

BUG=410574
==========

[eroman, 2015-12-17 19:53:19]

Description was changed from

==========
Add scripts to generate simple test data for certificate verification.

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap, but the advantage is easier manipulation and
addition of new test cases.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) will be used as
test data for a forthcoming changelist on certificate chain verification
(https://codereview.chromium.org/1414923007)

BUG=410574
==========

to

==========
Add scripts to generate simple test data for certificate verification.

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap, but the advantage is easier manipulation and
addition of new test cases.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) are used as test
data by the follow-up CL (https://codereview.chromium.org/1414923007)

BUG=410574
==========

[eroman, 2015-12-17 19:53:40]

Description was changed from

==========
Add scripts to generate simple test data for certificate verification.

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap, but the advantage is easier manipulation and
addition of new test cases.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) are used as test
data by the follow-up CL (https://codereview.chromium.org/1414923007)

BUG=410574
==========

to

==========
Add scripts to generate simple test data for certificate verification.

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap, but the advantage is easier manipulation and
addition of test cases.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) are used as test
data by the follow-up CL (https://codereview.chromium.org/1414923007)

BUG=410574
==========

[eroman, 2015-12-17 19:53:51]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-12-17 19:53:51]

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/1414393008/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-17 19:56:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414393008/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414393008/200001

[commit-bot: I haz the power, 2015-12-17 22:38:10]

Description was changed from

==========
Add scripts to generate simple test data for certificate verification.

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap, but the advantage is easier manipulation and
addition of test cases.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) are used as test
data by the follow-up CL (https://codereview.chromium.org/1414923007)

BUG=410574
==========

to

==========
Add scripts to generate simple test data for certificate verification.

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap, but the advantage is easier manipulation and
addition of test cases.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) are used as test
data by the follow-up CL (https://codereview.chromium.org/1414923007)

BUG=410574
==========

[commit-bot: I haz the power, 2015-12-17 22:38:11]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-12-17 22:39:11]

Description was changed from

==========
Add scripts to generate simple test data for certificate verification.

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap, but the advantage is easier manipulation and
addition of test cases.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) are used as test
data by the follow-up CL (https://codereview.chromium.org/1414923007)

BUG=410574
==========

to

==========
Add scripts to generate simple test data for certificate verification.

The intent is to complement other path validation test suites (i.e PKITS).
Certainly there is overlap, but the advantage is easier manipulation and
addition of test cases.

This introduces some python scripts and helpers that shell out to "openssl req"
and "openssl ca" to generate certificate chains with certain properties.

These certificate chains (and associated data for verification) are used as test
data by the follow-up CL (https://codereview.chromium.org/1414923007)

BUG=410574

Committed: https://crrev.com/e461b9127d35010e41e47cbb9f07a4bc25fc0866
Cr-Commit-Position: refs/heads/master@{#365922}
==========

[commit-bot: I haz the power, 2015-12-17 22:39:12]

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