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)

enhancement

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.
Attachment #8968350 - Flags: review?(kmaglione+bmo)
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 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 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.
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
https://hg.mozilla.org/mozilla-central/rev/a231670d9c66
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Please let us know if any testing is needed here or mark the bug as "qe-verify-"
Flags: needinfo?(dkeeler)
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?
We made a smoke testing around updates and everything worked as expected. Marking bug as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.