Last Comment Bug 554369 - Patches for CERT_CacheOCSPResponseFromSideChannel
: Patches for CERT_CacheOCSPResponseFromSideChannel
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12.6
: All All
: P1 normal (vote)
: 3.15
Assigned To: Adam Langley
:
:
Mentors:
Depends on:
Blocks: 360420 700701
  Show dependency treegraph
 
Reported: 2010-03-23 10:40 PDT by Wan-Teh Chang
Modified: 2013-04-12 07:41 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
change the caching semantics of side channel responses (by Adam Langley) (9.27 KB, patch)
2010-03-23 10:40 PDT, Wan-Teh Chang
kaie: review+
Details | Diff | Splinter Review
fix bug that I noticed in my travels (by Adam Langley) (1.08 KB, patch)
2010-03-23 10:44 PDT, Wan-Teh Chang
kaie: review+
Details | Diff | Splinter Review

Description Wan-Teh Chang 2010-03-23 10:40:50 PDT
Created attachment 434271 [details] [diff] [review]
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 2010-03-23 10:44:18 PDT
Created attachment 434274 [details] [diff] [review]
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) (on vacation) 2012-04-27 09:57:28 PDT
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) (on vacation) 2012-04-27 10:17:53 PDT
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) (on vacation) 2013-01-14 08:16:29 PST
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) (on vacation) 2013-01-14 08:16:47 PST
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 2013-01-31 19:07:30 PST
Marking as resolved, based on Kai's comment #4 and bug 360420
Comment 7 Kai Engert (:kaie) (on vacation) 2013-02-15 10:08:00 PST
checked in, see bug 360420 comment 160

Note You need to log in before you can comment on or make changes to this bug.