Closed
Bug 1077926
Opened 10 years ago
Closed 10 years ago
RSA Keypair generation makes mozilla::pkix unit tests too slow
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
Attachments
(1 file, 2 obsolete files)
40.22 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
Most of our tests don't care at all whether a certificate's keypair is distinct from another certificates' keypair. Often, we don't even use the keypair except to extract the encoded SubjectPublicKeyInfo from it. Hoewver, the keypair generation is VERY slow--easily the slowest part of running the test suite. This patch introduces a function that returns the same keypair every time it is called. It can be used anywhere we need a keypair but where the uniqueness/distinctiveness of the keypair doesn't matter.
Attachment #8500089 -
Flags: review?(dkeeler)
Comment on attachment 8500089 [details] [diff] [review] CloneReusedKey.patch Review of attachment 8500089 [details] [diff] [review]: ----------------------------------------------------------------- Cool - LGTM. ::: security/pkix/test/lib/pkixtestnss.cpp @@ +37,5 @@ > > namespace mozilla { namespace pkix { namespace test { > > namespace { > + nit: trailing whitespace
Attachment #8500089 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 2•10 years ago
|
||
Thanks for the review. I fixed the nit before pushing: https://hg.mozilla.org/integration/mozilla-inbound/rev/124b04c01c71
Comment 3•10 years ago
|
||
sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=8847ac2e2a53 since one of this 3 checkins made XPCshell tests more failing frequently like the issues in bug 964673 as example
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #3) > sorry had to back this out in > https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla- > inbound&revision=8847ac2e2a53 since one of this 3 checkins made XPCshell > tests more failing frequently like the issues in bug 964673 as example I had already discovered that issue on Try and wrote a patch that resolves that issue in bug 1078108, but I forgot that this bug depends on that one. Sorry for the inconvenience.
Depends on: 1078108
Assignee | ||
Comment 5•10 years ago
|
||
The first implementation had a problem. Although it made some tests that previously called GenerateKeyPair more than once faster, it made every other test slower because the reused keypair was generated by anything that used pkixtestnss, which is basically everything. In particular, it made the xpcshell-tests slower, which caused bug 1079436 to be triggered more frequently. In this version, we only generate the reused keypair if/when something actually calls CloneReusedKeyPair. On tryserver, I verified that bug 1079436 gets triggered much less frequently with this new version: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cffede9bb940
Attachment #8500089 -
Attachment is obsolete: true
Attachment #8501515 -
Flags: review?(dkeeler)
Assignee | ||
Updated•10 years ago
|
Attachment #8501515 -
Attachment is obsolete: true
Attachment #8501515 -
Flags: review?(dkeeler)
Comment on attachment 8501928 [details] [diff] [review] reused-keypair.patch [v3] Review of attachment 8501928 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with comments addressed. Bug 1058812 is currently in inbound, so this may require rebasing again - sorry. ::: security/pkix/test/gtest/pkixcert_extension_tests.cpp @@ +42,5 @@ > ByteString issuerDER(CNToDERName(subjectCN)); > EXPECT_FALSE(ENCODING_FAILED(issuerDER)); > ByteString subjectDER(CNToDERName(subjectCN)); > EXPECT_FALSE(ENCODING_FAILED(subjectDER)); > + subjectKey = CloneReusedKeyPair(); subjectKey doesn't look like it needs to be an out parameter anymore (the calling code never uses it, as far as I can tell). ::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp @@ +238,5 @@ > > ByteString CreateEncodedOCSPSuccessfulResponse( > OCSPResponseContext::CertStatus certStatus, > const CertID& certID, > + /*optional*/ const char* signerName, nit: not sure this line needs to change (probably a rebasing issue) @@ +721,3 @@ > ByteString signerDER(CreateEncodedCertificate( > + 1, sha256WithRSAEncryption, subCAName, > + oneDayBeforeNow, oneDayAfterNow, signerName, nit: trailing whitespace @@ +761,3 @@ > ByteString subCADER(CreateEncodedCertificate( > + ++rootIssuedCount, sha256WithRSAEncryption, rootName, > + oneDayBeforeNow, oneDayAfterNow, subCAName, nit: trailing whitespace
Attachment #8501928 -
Flags: review?(dkeeler) → review+
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b4d10d3e9131
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•