554369 - Patches for CERT_CacheOCSPResponseFromSideChannel

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: change the caching semantics of side channel responses (by Adam Langley)

This bug continues the work of bug 542538 on caching stapled OCSP responses.

The first patch was originally attached in bug 542538 comment 27.

Comment 1

[Wan-Teh Chang]

Attachment: fix bug that I noticed in my travels (by Adam Langley)

This patch was originally attached by Adam Langley in bug 542538 comment 28,
with the following description:

I don't believe that cert_RememberOCSPProcessingFailure will currently work in
all cases. It calls ocsp_CreateOrUpdateCacheEntry with single==NULL and that
causes ocsp_CreateOrUpdateCacheEntry to set the missingResponseError on any
cache entry that it finds.

However, if there is already a cache entry, and that cache entry has a
certStatusArena, then setting missingResponseError is not sufficient. It's the
non-NULLness of certStatusArena that is checked in
ocsp_GetCachedOCSPResponseStatusIfFresh

Comment 2

[Kai Engert (:KaiE:)]

I'm really glad I just found this bug report and these patches!!!

With the current state of NSS in CVS,
OCSP stapling / side channel data will never be stored in the OCSP cache,
if the OCSP response has a valid signature and says revoked cert.

I agree with this patch to change this behaviour,
and always cache such information, even if it came from a side channel.

This change of behaviour is necessary for my work in bug 700701,
where I want to enhance the tstclnt tool to report the information retrieved
via the side channel.

Comment 3

[Kai Engert (:KaiE:)]

These patches no longer apply cleanly, but luckily,
it's only a matter of context.

Luckily this means, 
it's still fine to review the patches that are attached in this bug.


Unfortunately the context between your patch and the patch in bug 360420 is too close.

I merged our patches, and there are just two lines of context between some of the changes, so it's impossible to create separate unified patches, not even with cvs -u2

I think we should get the code in this bug reviewed first and checked in,
then I will attach an updated patch to bug 360420.

Until then I'll carry the merged version of your patch in my tree and 
include it in future patches to bug 360420.

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 434271 [details] [diff] [review]
change the caching semantics of side channel responses (by Adam Langley)

I agree with these patches. I've merged them into the "injection" patches in bug 360420, since patches are overlapping.

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 434274 [details] [diff] [review]
fix bug that I noticed in my travels (by Adam Langley)

r=kaie on both patches

Comment 6

[Ryan Sleevi]

Marking as resolved, based on Kai's comment #4 and bug 360420

Comment 7

[Kai Engert (:KaiE:)]

checked in, see bug 360420 comment 160