383969 - PSM should not use OCSP while building cert manager display

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

See bug 383963 for the reasoning.
In short, PSM should not use OCSP while it collects the display strings for cert manager from a lot of certificates.

In the past PSM disabled OCSP globally, because there was no way around it.

I recently removed the code that disables OCSP.
But that was a bad decision.

I am running into regressions. This is especially bad when we have to wait for a lot of OCSP calls to be completed, and I also have some certs which have bogus OCSP URLs. I believe we have sufficient reasons to revert to the old style.


But we should no longer disable OCSP globally.
This is bad, as I explain in bug 383963.

In bug 383963 I am proposing to implement a new mechanism that would allow PSM to disable OCSP checking on a given thread only (and not affecting e.g. SSL on a different thread).


I'll attach a patch that changes PSM to make use of the new proposed NSS API functions.

In addition I'm adding back some code that gives the user a clue that cert manager listings have been produced without OCSP.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch to disable OCSP again for cert manager listings

Let's get this patch in ASAP.

We have a regression that causes cert manager to be completely broken and crash, because we do OCSP from the middle of a painting event, and because we yield to networking & UI while we wait for the OCSP result, causing recursive painting attempts...

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Additional Patch, this one depends on the new NSS functions proposed in bug 383963.

Changes PSM to temporarily disable OCSP on the UI thread only.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Kai, Based on bug 375759 comment 7, and on comment 0 above, I gather
that the Cert Manager is now broken on the trunk, always crashing on 
start up because of the change to enable OCSP while Cert Manager is 
running.  I submit that change was a regression, and that the appropriate
action is to back out the patch that caused the regression until it is fixed.

There are some issues to be resolved before the patch for bug 383963 will
be ready for checkin.  Beyond the cosmetic issues I raised, I think 
(speculate) that there may be some architectural issues with the proposed 
solution.  So, I would not suggest we rush to get that change checked in,
but rather, back out the change that caused the regression until it is 
ready to be properly fixed.

Comment 4

[Kai Engert (:KaiE:)]

(In reply to comment #3)
> Kai, Based on bug 375759 comment 7, and on comment 0 above, I gather
> that the Cert Manager is now broken on the trunk, always crashing on 
> start up because of the change to enable OCSP while Cert Manager is 
> running.  I submit that change was a regression, and that the appropriate
> action is to back out the patch that caused the regression until it is fixed.

Yes, i agree, and the patch that "backs it out" is the first attachment in this bug, attachment 267909 [details] [diff] [review], which is identical to what I propose to keep for the future.


But please note: By backing out the original change == by applying the first patch in this bug, every time you use cert manager, the OCSP cache will get cleared.

This is because NSS currently clears the cache whenever OCSP options get changed. Yes, we already discussed this could be changed in the future, and the cache contents could be kept on disable/enable, but that work has not been done yet.


> There are some issues to be resolved before the patch for bug 383963 will
> be ready for checkin.  Beyond the cosmetic issues I raised, I think 
> (speculate) that there may be some architectural issues with the proposed 
> solution.  So, I would not suggest we rush to get that change checked in,
> but rather, back out the change that caused the regression until it is 
> ready to be properly fixed.

I fully agree. This is why I attached two separate patches to this bug. The first attachment to get things working again, and the second attachment that will depend on what we do in bug 383963.

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 267909 [details] [diff] [review]
Patch to disable OCSP again for cert manager listings

This patch doesn't make sense if the workaround in bug 383963 is not acceptable.

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 267910 [details] [diff] [review]
Patch v2

This patch is obsolete because it depends on a new NSS feature proposed in bug 383963 which seems not acceptable.

Comment 7

[Kai Engert (:KaiE:)]

Attachment: Patch v3

I'm proposing this simple fix as a temporary workaround, until we are able to implement an improved solution, like the ones discussed in bug 383963.

This patch does the following to cert manager:

- it removes the "purposes" column from the "Your Certificates" tab
- note that "Your Certificates" already has an "Expires On" tab

- it removes the "purposes" column from the "Other People's" tab
- it adds a "Expires On" tab to the  "Other People's" tab

- it removes the "purposes" column from the "Web Sites" tab
- it adds a "Expires On" tab to the  "Web Sites" tab

This change will ensure that no cert verification and no OCSP requests will get triggered for the cert manager display.

All the information that was usually shown in the purposes column can still be reached by using the "view" button (which will trigger an async OCSP request and fill in the details in the windows once they are available).

Comment 8

[Robert Relyea]

Comment on attachment 268184 [details] [diff] [review]
Patch v3

r+ rrelyea

Comment 9

[Kai Engert (:KaiE:)]

Workaround checked in, marking fixed.

Improvement bug filed, see dependency.