149834 - Enhance PSM speed by using new NSS API CERT_VerifyCertificateNow

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P3)

Description

[Julien Pierre]

PSM makes a lot of CERT_VerifyCert, multiple calls per certificate in order to 
check cert usage. When validation is enabled, this causes a huge performance 
problem.

For each certificate :
1) if a CRL is installed and matches the issuer, that CRL will be linearly 
searched (bug 149784 ) for each of the 12 calls to CERT_VerifyCert ( bug 149832 
)
2) if OCSP is enabled, there will be a new OCSP connection for each of the 12 
calls to CERT_VerifyCert (again, bug 149832)

As a test, I created a new profile with the Mozilla 1.0 release, and got myself 
a brand-new Thawte cert, on my home dual CPU 1.5 GHz Athlon machine on OS/2. 
Without validation, cert manager was fast, <1s delay to open cert manager or 
view the cert. 

I then installed the Thawte CRL. At that point, cert manager became basically 
unusable. It took 7s to view my certificate. Once I deleted the CRL, performance 
was back to normal.

Comment 1

[Julien Pierre]

Attachment: patch to use the new CERT_VerifyCertificate and reduce number of cert verification calls

This patch reduces the number of calls, but there is still room for
improvement, as separate verification calls are made for each of the column
fields in the certmanager. You should be able to optimize this down to one call
per cert (ie. per row) but I wasn't able to because I don't know the PSM GUI
code enough.

This patch divides the number of calls by 8.
Once you eliminate the redundant calls per column, you'll reduce it another
factor of 5.

Comment 2

[Kai Engert (:KaiE:)]

PSM calls CERT_VerifyCertNow multiple times, because it is checking the validity
for multiple usages. It does so, because it wants to report a list of valid
usages in the UI.

If NSS is internally doing the verification each time within that function,
maybe we should create an optimized interface.

CERT_VerifyCert has separate logic for each usage, so we should probably
continue to call the function multiple times.

But maybe you could provide a stripped down version of the function, that allows
one to omit the revocation checks? PSM could do the revocation check separately,
by calling a Cert_VerifyCertRevocation and only if it succeeds calls a usage
verification check like Cert_VerifyCertIgnoreRevocationStatus.

Comment 3

[Stephane Saux]

Kai. Take a look at NSM which combines the verified and purposes columns into
one. This makes total sense since what's in verified can always be deduced from
what's in the purposes column (i.e., its redundant information which takes a
long time to compute.

Comment 4

[Julien Pierre]

Kai,

I am well aware that the CERT_VerifyCert API was inefficient in that it only
allowed one usage to be checked at a time. However, a new API has been created
in NSS 3.6, CERT_VerifyCertificate. See bug 149832.

The patch for PSM that I attached uses that new API so that all 8 usages can be
checked in one CERT_VerifyCertificate call instead of 8 CERT_VerifyCert calls.

However, the GetPurposes() method is getting called more than once per cert, in
fact, 5 times, once for each column in the cert manager. This seems to happen at
each repaint time (ie. when resizing the dialog). This can also be optimized
down to once per cert, but I didn't want to mess with the GUI code.

Comment 5

[Julien Pierre]

Attachment: actual intended patch

I had uploaded the wrong file before.
This is the one that calls CERT_VerifyCertificate instead of CERT_VerifyCert .

Comment 6

[Kai Engert (:KaiE:)]

Oops. I have changed the very same source locations with my patch to bug 132589 :(

Since this patch is smaller, please delay this bug until 132589 has landed
(which will happen soon). Making this bug dependent. Julien, I will attach a
collision free version of your patch once that is done.

Comment 7

[Kai Engert (:KaiE:)]

Hmm, in addition I realize that your patch depends on NSS 3.6, so we need to
delay this bug, anyway.

Do you know when you want to land NSS 3.6 as the NSS_CLIENT_TAG?

Maybe we could file a bug for the proposed landing of NSS 3.6 and make it block
this bug? So we will get notified when we are ready to work on this bug again.

Comment 8

[Stephane Saux]

Kai. Can you open the following bugs:
- Move NSS_CLIENT_TAG (but check whether it should be an NSS bug in which case
work with the nss team and have a corresponding PSM tracking bug).
- Update the PSM version to 2.4 (when we start using 3.6 we should up the version).
- bug against bugzilla to have a PSM 2.4 version and target milestone.

Comment 9

[Kai Engert (:KaiE:)]

Bugs filed. Adding dependency.

I assume the change suggested in this bug is something that depends on 3.6, but
it can be done after 3.6 (i.e. it is not required to make this change at the
same time).

Comment 10

[Julien Pierre]

Correct. The old CERT_VerifyCert will still work, but no faster than before. You
will be able to call the new CERT_VerifyCertificate at any time after you move
to 3.6 .

Comment 11

[Julien Pierre]

FYI, there is now a CRL cache in NSS. So CRL checks that occur as part of the
cert verification will only be expensive the first time a cert of a given issure
is verified. Afterwards the CRL is cached and the CRL lookup will take less than
100 microseconds on even a slow machine.

Comment 12

[John Unruh]

Works for me. Even with 8 certs using OCSP, (out of a total of 20 personal certs
installed), and three CRLs installed, the Cert Manager opens very quickly.

Comment 13

[Julien Pierre]

Kai,

Are you now using the new function CERT_VerifyCertificateNow ?

Comment 14

[Kai Engert (:KaiE:)]

Reopening. Also changing summary that probbably caused confusion and motivated
John to close the bug.

I'll work on this right after bugs 168450 and 177260 have both been checked in.

Comment 15

[John Unruh]

Adding depends on bug 168450 and bug 177260 

Comment 16

[John G. Myers]

Mass reassign ssaux bugs to nobody

Comment 17

[John G. Myers]

Attachment: Revised fix

Comment 18

[Kai Engert (:KaiE:)]

Comment on attachment 144417 [details] [diff] [review]
Revised fix

ssaux is no longer at aol, removing review request

Comment 19

[Kai Engert (:KaiE:)]

Attachment: Same patch as before, just updated to apply cleanly to the trunk

r=kengert

Comment 20

[Kai Engert (:KaiE:)]

Checked in to trunk.

Comment 21

[Kai Engert (:KaiE:)]

This bug introduced regression bug 351873.

Comment 22

[Julien Pierre]

Kai,

OCSP checking should still be done in CERT_VerifyCertificateNow . It just will get done one time instead of x times for each usage if calling CERT_VerifyCertNow x times. Are you sure that's the cause of the regression ?

Comment 23

[Kai Engert (:KaiE:)]

(In reply to comment #22)
> 
> OCSP checking should still be done in CERT_VerifyCertificateNow . It just will
> get done one time instead of x times for each usage if calling
> CERT_VerifyCertNow x times. Are you sure that's the cause of the regression ?


Yes, I feel confident this is the cause of the regression.

As soon as the application code asks to verify usage "status responder", NSS will suppress OCSP.

 if ( (! (requiredUsages & certificateUsageStatusResponder)) &&
    statusConfig != NULL) {
    if (statusConfig->statusChecker != NULL) {
       rv = (* statusConfig->statusChecker)(handle, cert,
                                                    t, wincx);

In the past, we had multiple calls, so only the check for the responder usage suppressed ocsp.

As a work around in the application, I propose that PSM makes two separate calls to CERT_VerifyCertNow. One asking for the responder usage, and another call for all the other usages.

I will attached such a woraround patch to bug 351871. In my tests that patch fixed the issue.

Comment 24

[Julien Pierre]

Kai,

Actually, I think this may be considered an NSS bug in CERT_VerifyCertificate / CERT_VerifyCertificateNow . It should skip the OCSP check only if the status responder usage is the only usage requested to be checked.