1137538 - remove nsIIdentityInfo and nsNSSSocketInfo::GetPreviousCert

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

After bug 1136471, the only thing in nsIIdentityInfo will be isExtendedValidation, which is implemented in nsNSSCertificate. For the most part, it doesn't look like this API needs to be XPCOM - only C++ code uses it, and that code can be (mostly) easily changed to use the underlying nsNSSCertificate instead of nsIX509Cert. There may be a wrinkle in nsNSSCallbacks.cpp where we try to call SetServerCert using a nsIX509Cert we get from elsewhere (i.e. we don't have a handle on the underlying nsNSSCertificate), but it's not clear to me that the prevcert/newcert code is necessary anymore as a result of recent changes we've made. We should investigate further.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Here's what I've found:

nsIIdentityInfo.isExtendedValidation is only called in nsSSLStatus::SetServerCert, which is called from:
- AuthCertificate (where we have an nsNSSCertificate)
- TransportSecurityInfo::SetStatusErrorBits (where we have an nsIX509Cert, but this is only called from CreateCertErrorRunnable, where we have an nsNSSCertificate)
- HandshakeCallback (where in one case we have an nsIX509Cert)

The one case in HandshakeCallback where we have an nsIX509Cert (which technically isn't guaranteed to be backed by an nsNSSCertificate) is in the case where we've determined that the new certificate is equal to the certificate presented by the previous page viewed (via nsISecureBrowserUI). This functionality was added in bug 406999 as an optimization to not calculate EV status again when we already knew if a certificate had validated for EV. However, in the process of SetServerCert, we currently disregard the previous certificate's EV status, meaning this optimization is broken. It is also slow, because it involves a DispatchToMainThreadAndWait. Furthermore, it was introduced at a time when we relied on CERT_PKIXVerifyCert for EV verification, which was slow. mozilla::pkix, the current verification library, is much faster. Given that this optimization is broken, slow, and no longer as helpful as it once was, we should just remove it and the related infrastructure. This has the added benefit of making this area of code a bit more clear (for example, there's no explanation of why we're comparing the previous certificate to the current one - I had to do non-trivial code archeology to figure out why we were doing this).

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Honza, how does this look?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks for the review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06656d870a65
https://hg.mozilla.org/integration/mozilla-inbound/rev/7badc767cc25

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7badc767cc25