remove nsIIdentityInfo and nsNSSSocketInfo::GetPreviousCert

RESOLVED FIXED in Firefox 39

Status

()

Core
Security: PSM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

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.
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).
(Assignee)

Updated

3 years ago
See Also: → bug 406999
Summary: investigate removing nsIIdentityInfo entirely → remove nsIIdentityInfo and nsNSSSocketInfo::GetPreviousCert
(Assignee)

Updated

3 years ago
Assignee: nobody → dkeeler
Created attachment 8571576 [details] [diff] [review]
patch

Honza, how does this look?
Attachment #8571576 - Flags: review?(honzab.moz)
Attachment #8571576 - Flags: review?(honzab.moz) → review+
Thanks for the review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06656d870a65
https://hg.mozilla.org/integration/mozilla-inbound/rev/7badc767cc25
https://hg.mozilla.org/mozilla-central/rev/7badc767cc25
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Updated

2 years ago
Duplicate of this bug: 679144
You need to log in before you can comment on or make changes to this bug.