1174292 - convert test_cert_version.js to generate certificates at build time

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Convert test_cert_version.js to generate certificates at build time using pycert.py.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: MozReview Request: bug 1174292 - convert test_cert_version.js to generate certificates at build time

bug 1174292 - convert test_cert_version.js to generate certificates at build time

Also remove redundant test-cases.

Comment 2

[:Cykesiopka]

Comment on attachment 8623343 [details]
MozReview Request: bug 1174292 - convert test_cert_version.js to generate certificates at build time

https://reviewboard.mozilla.org/r/11491/#review10253

Sorry for the delay. Mostly LGTM, just a few comments.

> Also remove redundant test-cases.
Just to double check whether I'm understanding correctly: This patch removes checking for v1, v2 or v4 roots - the same logic is already tested by using v1-v4 intermediates.

::: security/manager/ssl/tests/unit/test_cert_version/generate.py:46
(Diff revision 1)
> -  name = "v1_int_bc-" + ca_name;
> +for versionStr, versionVal in versions.iteritems():

Nit: Is it necessary to repeat the following three lines of code here and below?

Adding comments as delimiters seems to achieve the same thing, assuming the purpose here is to partition the different classes of certs.

::: security/manager/ssl/tests/unit/test_cert_version.js:7
(Diff revision 1)
>  

Optional: Add a description for the various types of things being tested here.

::: security/manager/ssl/tests/unit/test_cert_version/generate.py:23
(Diff revision 1)
> -def generate_ee_family(db_dir, dest_dir, noise_file, ca_name):
> +    filename = '%s-%s.pem.certspec' % (subject, issuer)

Optional: Maybe use underscores to delimit the EE, intermediate and root?

The basicConstraintsTypes keys use hyphens as well, which I find makes the cert file names more difficult to mentally parse.

Not a big deal though - it might just be me.

::: security/manager/ssl/tests/unit/pycert.py:13
(Diff revision 1)
> +[version:<{1,2,3,4}>]

Minor nit: Grouping this with the other optional fields might look nicer.

::: security/manager/ssl/tests/unit/test_cert_version/generate.py:44
(Diff revision 1)
> -  generate_child_cert(db, srcdir, noise_file, name, ca_name, 1, False, False)
> +        writeCertspec(intermediateName, 'ee', [])

Optional: Maybe change |intermediateName| to |intermediateName + '_ca'|?

I keep getting confused between the EE certs issued via an intermediate and those signed directly by the root. Adding '_ca' might make it clearer.

Again, not a big deal.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://reviewboard.mozilla.org/r/11491/#review10559

Thanks for the review!
I figured we didn't need to generate those other roots since we could mark the intermediates as trust anchors and achieve the same effect.

> Minor nit: Grouping this with the other optional fields might look nicer.

My original thinking was that it should be in the same order as how certificates are encoded, but I don't think that's actually important since pycert doesn't care about the order anyway.

> Optional: Add a description for the various types of things being tested here.

Good call.

> Optional: Maybe use underscores to delimit the EE, intermediate and root?
> 
> The basicConstraintsTypes keys use hyphens as well, which I find makes the cert file names more difficult to mentally parse.
> 
> Not a big deal though - it might just be me.

I converted them to underscores and documented the naming convention.

> Optional: Maybe change |intermediateName| to |intermediateName + '_ca'|?
> 
> I keep getting confused between the EE certs issued via an intermediate and those signed directly by the root. Adding '_ca' might make it clearer.
> 
> Again, not a big deal.

I'm hoping the added documentation will make this clearer.

> Nit: Is it necessary to repeat the following three lines of code here and below?
> 
> Adding comments as delimiters seems to achieve the same thing, assuming the purpose here is to partition the different classes of certs.

Good call - I used comments instead.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c5fae7fa49c

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/470858b4d3bc

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/470858b4d3bc