969985 - certificate generation for test_certificate_usages.js is written in Perl (security/manager/ssl/tests/unit/test_certificate_usages/generate.pl)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

All the other certificate generation is written in Python, which is the scripting language Mozilla uses for almost everything. We created a python library to help with certificate generation; see security/manager/ssl/tests/unit/psm_common_py/CertUtils.py. We should replace this script with one that uses CertUtils.py.

Comment 1

[Maksim Ivanov]

Hi there! I would like to take this on.

Comment 2

[Camilo Viecco (:cviecco)]

Hello Maksim, that is great. Once you have a patch ready, mark me for feedback. If you have issues/questions please mark them here on this bug.

Comment 3

[Maksim Ivanov]

How can I test certificate generation?

Comment 4

[Camilo Viecco (:cviecco)]

The output certificates should match the properties that are being tests. For this test we are interested in the Key usage and Extended Key usage. A single CA is generated and then we generate loop to generate 4 intermedates; also, for each intermediate we loop to generate different combinations of end entity certs. Once the new certs are generates the test case
'./mach xpcshell-test security/manager/ssl/tests/unit/test_certificate_usages.js' should pass.

Comment 5

[Mike Hoye [:mhoye]]

Hi, Maksim - Are you making progress on this bug?

Comment 6

[Mohamed Zenadi [:zeapo]]

(In reply to Camilo Viecco (:cviecco) from comment #4)
> The output certificates should match the properties that are being tests.
> For this test we are interested in the Key usage and Extended Key usage. A
> single CA is generated and then we generate loop to generate 4 intermedates;
> also, for each intermediate we loop to generate different combinations of
> end entity certs. Once the new certs are generates the test case
> './mach xpcshell-test
> security/manager/ssl/tests/unit/test_certificate_usages.js' should pass.

Do you mean that generate.pl should be replaced by a generate.py that uses CertUtils.py to generate exactly the same certificates as the perl code does?

Comment 7

[Camilo Viecco (:cviecco)]

(In reply to Mohamed Zenadi [:zeapo] from comment #6)
> (In reply to Camilo Viecco (:cviecco) from comment #4)
> > The output certificates should match the properties that are being tests.
> > For this test we are interested in the Key usage and Extended Key usage. A
> > single CA is generated and then we generate loop to generate 4 intermedates;
> > also, for each intermediate we loop to generate different combinations of
> > end entity certs. Once the new certs are generates the test case
> > './mach xpcshell-test
> > security/manager/ssl/tests/unit/test_certificate_usages.js' should pass.
> 
> Do you mean that generate.pl should be replaced by a generate.py that uses
> CertUtils.py to generate exactly the same certificates as the perl code does?

yes OR

create a new generate.py and modify test_certificate_usages.js so that we can have the same coverage.

My goal for this patch is to remove the perl code, so either option is fine.

Comment 8

[Ronald Kwan [:rkwan]]

Hello, could I be assigned to this bug? I am currently working on a patch for it.

Comment 9

[Ronald Kwan [:rkwan]]

Attachment: patch: rewrite generate.pl to generate.py in test_certificate_usages

This script is essentially a direct translation from the Perl version, so it could be written a little more nicely, but it does pass the test.

Comment 10

[Camilo Viecco (:cviecco)]

Comment on attachment 8460779 [details] [diff] [review]
patch: rewrite generate.pl to generate.py in test_certificate_usages

Review of attachment 8460779 [details] [diff] [review]:
-----------------------------------------------------------------

This is a great start, but rewriting in perl is just the first part. The patch should use certutils to generate the certificates so that all the openssl/certutil logic is centralized. 
You should look at security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints/generate.py as inspiration on how to append the different key usages. You will need to look at the openssl documentation to map the nss values to openssl values. You should probably keep the same looping logic.

Comment 11

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Another drawback of this generator file is that it's difficult to add additional test-cases. Because the output is of the form ee-{i}-ca-{j}.der where i and j are indices without semantic meaning, changing the order of the output changes what certificate corresponds to which test.

Comment 12

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Whenever this gets fixed, it might be nice to clean up test_certificate_usages.js as well. For example, the documentation will have to be updated, we should use 'let', not 'var', and line 42 is unused: 'var basicEndEntityUsagesWithObjectSigner = basicEndEntityUsages + ",Object Signer"' (it also doesn't end with a semicolon).

Comment 13

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: MozReview Request: bug 969985 - cleanup of test_certificate_usages.js - see the rest of this commit message

bug 969985 - cleanup of test_certificate_usages.js - see the rest of this commit message

Converts test_certificate_usages.js to generate certificates at build time.
Also does miscellaneous cleanup to use modern JS practices.
Since the test_cert_eku-* suite of tests covers the extended key usage extension,
removes superfluous testcases involving EKU.
Finally, renames test_certificate_usages.js to test_cert_keyUsage.js for a more
consistent naming scheme.

Comment 14

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

The requirements of this bug have changed as of bug 1166976 landing.

Comment 15

[:Cykesiopka]

https://reviewboard.mozilla.org/r/9897/#review8781

::: security/manager/ssl/tests/unit/test_cert_keyUsage/moz.build:30
(Diff revision 1)
> +    props.inputs = [input_file, TOPOBJDIR + '/config']

Just a friendly reminder that this needs to change to avoid another Bug 1170431 - apologies for not catching this in the Bug 1166976 review.

Comment 16

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Thanks for the reminder - I pushed an updated to the patch.

Comment 17

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8614326 [details]
MozReview Request: bug 969985 - cleanup of test_certificate_usages.js - see the rest of this commit message

https://reviewboard.mozilla.org/r/9897/#review8931

Looks good to me.

::: security/manager/ssl/tests/unit/test_cert_keyUsage/moz.build:30
(Diff revision 1)
> +    props.inputs = [input_file, TOPOBJDIR + '/config']

As per Cykesiopka's comment, you probably want something like:

props.inputs = [input_file, '!/config/buildid']

Comment 18

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Thanks!
Try run on OS X, for good measure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2267e97aa8f5

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7720cbbe62e

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c7720cbbe62e