Closed Bug 969985 Opened 7 years ago Closed 5 years ago

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

Categories

(Core :: Security: PSM, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: briansmith, Assigned: keeler)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Hi there! I would like to take this on.
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.
How can I test certificate generation?
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.
Hi, Maksim - Are you making progress on this bug?
(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?
(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.
Mentor: cviecco
Whiteboard: [good first bug][mentor=cviecco] → [good first bug]
Hello, could I be assigned to this bug? I am currently working on a patch for it.
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.
Attachment #8460779 - Flags: review?(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.
Attachment #8460779 - Flags: review?(cviecco) → review-
Assignee: nobody → rkwan
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.
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).
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.
Attachment #8614326 - Flags: review?(mgoodwin)
The requirements of this bug have changed as of bug 1166976 landing.
Assignee: rkwan → dkeeler
Mentor: cviecco
Depends on: 1166976
Whiteboard: [good first bug]
Attachment #8460779 - Attachment is obsolete: true
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.
Thanks for the reminder - I pushed an updated to the patch.
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']
Attachment #8614326 - Flags: review?(mgoodwin) → review+
https://hg.mozilla.org/mozilla-central/rev/c7720cbbe62e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.