Closed
Bug 1454504
Opened 6 years ago
Closed 6 years ago
switch CertUtils.checkCert away from calling nsIX509Cert.issuer repeatedly
Categories
(Toolkit :: Add-ons Manager, enhancement, P1)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | verified |
People
(Reporter: keeler, Assigned: keeler)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
nsIX509Cert.issuer performs synchronous certificate verification and isn't even guaranteed to return a verified result. Fortunately we should be able to use nsISSLStatus.succeededCertChain instead, which contains the already-verified certificate chain of the connection we're interested in. This should be a performance as well as architectural improvement.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8968350 -
Flags: review?(kmaglione+bmo)
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8968350 [details] bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert https://reviewboard.mozilla.org/r/237028/#review242852 ::: toolkit/modules/CertUtils.jsm:146 (Diff revision 1) > } > return; > } > > - var cert = > - aChannel.securityInfo.QueryInterface(Ci.nsISSLStatusProvider). > + let sslStatus = aChannel.securityInfo.QueryInterface(Ci.nsISSLStatusProvider) > + .SSLStatus.QueryInterface(Ci.nsISSLStatus); Nit: QI(nsISSLStatus) is completely unnecessary. ::: toolkit/modules/CertUtils.jsm:155 (Diff revision 1) > > - if (aAllowNonBuiltInCerts === true) > + if (aAllowNonBuiltInCerts === true) { > return; > + } > > - var issuerCert = cert; > + let certEnumerator = sslStatus.succeededCertChain.getEnumerator(); Random fact: nsISSLStatus.idl is super well-documented. ::: toolkit/modules/CertUtils.jsm:156 (Diff revision 1) > - while (issuerCert.issuer && !issuerCert.issuer.equals(issuerCert)) > - issuerCert = issuerCert.issuer; > + let issuerCert = null; > + while (certEnumerator.hasMoreElements()) { Nit: `for (issuerCert of XPCOMUtils.IterSimpleEnumerator(certEnumerator, Ci.nsIX509Cert))`
Attachment #8968350 -
Flags: review?(kmaglione+bmo) → review+
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8968350 [details] bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert https://reviewboard.mozilla.org/r/237028/#review243144 r+ with Kris's comments addressed.
Attachment #8968350 -
Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8968350 [details] bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert https://reviewboard.mozilla.org/r/237028/#review242852 Thanks for the reviews! > Nit: QI(nsISSLStatus) is completely unnecessary. Heh - whoops. Fixed. > Random fact: nsISSLStatus.idl is super well-documented. Indeed :( > Nit: `for (issuerCert of XPCOMUtils.IterSimpleEnumerator(certEnumerator, Ci.nsIX509Cert))` Cool - fixed.
Assignee | ||
Comment 6•6 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=389e57f8a8090aa6a7eaadedf09343dfc67546f5
Pushed by dkeeler@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a231670d9c66 use a more performant API to find a root certificate in CertUtils.checkCert r=kmag,mossop
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a231670d9c66
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 9•6 years ago
|
||
Please let us know if any testing is needed here or mark the bug as "qe-verify-"
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 10•6 years ago
|
||
If there are existing smoke tests that ensure that updating add-ons and the browser still works, it would be good to confirm that this change doesn't break that. Thanks!
Flags: needinfo?(dkeeler) → qe-verify?
Comment 11•6 years ago
|
||
We made a smoke testing around updates and everything worked as expected. Marking bug as verified.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•