205406 - (ocspcache) Need a local OCSP cache

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Wan-Teh Chang]

This RFE is related to bug 91532, which requests an
*offline* OCSP cache that can be used when the machine
is not connected to a network.

This RFE requests a local OCSP cache for recent OCSP
responses.  The purpose of this simpler OCSP cache is
to improve performance by reducing the number of OCSP
requests going to responder over the network.  Offline
use is *not* a requirement.  Therefore, in this RFE we
do not need to consider the issue of persistent storage
of OCSP responses across application sessions.

Comment 1

[Wan-Teh Chang]

Two issues we need to resolve when implementing the OCSP cache.

1. The expiration of cache entries.

2. The "freshness" check of an OCSP response.

Comment 2

[Julien Pierre]

Which release of NSS do we want to target this feature to ?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

We probably should not turn on OCSP by default in the browser until this 
is implemented.

Comment 4

[Kai Engert (:KaiE:)]

Nelson, do you still think we should implement an OCSP cache for recent queries, before turning on OCSP by default?

Has there been any work on this feature already?

Shouldn't that be very simple to do?

I suppose we'd store pairs of {response-bytes, expiration-time} and the primary key is {cert-issuer, serial number}

The in-memory-cache would then be limited to, say, 100 entries. It would be sorted by expiration time. Whenever the list grows larger than the max, we delete the oldest entry.

The cache should hold the entries in a hash table.

Other requirements, ideas, suggestions?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #4)
> Nelson, do you still think we should implement an OCSP cache for recent
> queries, before turning on OCSP by default?
Yes.

> Has there been any work on this feature already?
No code has been written, AFAIK.

> Shouldn't that be very simple to do?
I wouldn't say "very simple", but it's straightforward for someone who 
already understands all the issues. 

> I suppose we'd store pairs of {response-bytes, expiration-time} and the 
> primary key is {cert-issuer, serial number}
I agree about primary key.  The value we would store might be something
much simpler than the previous OCSP response.  It might be the expiration
time plus a single byte (or word) containing the answer for this cert.  
No point in revalidating the signature on the previously validated OCSP
response, for example.  We should strive to avoid doing that.

> The in-memory-cache would then be limited to, say, 100 entries. It would be
> sorted by expiration time. Whenever the list grows larger than the max, we
> delete the oldest entry.
100 is too small, IMO.  I'd say it should be bounded by age of the responses,
not by number of responses.  We should save all the responses that are not
older than some settable amount of elapsed time.  If we need to bound the 
number, perhaps 1000 should be the upper bound.

> The cache should hold the entries in a hash table.
> 
> Other requirements, ideas, suggestions?
Needs to be a publicly callable way to flush this cache.  
Appliation needs to flush it whenever the OCSP configuration changes
and IMO also whenever the list of trusted CAs changes.

Needs to be a way to shut the cache down (free it all), and that needs to 
be callable at NSS shutdown time.  NSS now has a way to register functions
to be called wheh NSS shuts down.  This OCSP cache shutdown function 
would need to be registered to be shut down at NSS shutdown time. 
Ideally this would be done automatically by NSS itself, rather than 
putting this burden on the appliation.  Some care is needed to make 
that idempotent.

Cache needs to be used by ALL of NSS's OCSP clients.  I think (not sure) 
that we have two, the old one, and a new one in libPKIX.  

We need a way to "preload" the cache with responses obtained through 
other means than the usual OCSP request/response protocol.  I am thinking
specifically of "stapled" OCSP responses, which are responses for TLS 
server certs that are (optionally) sent by the server, along with the 
server cert, during the TLS handshake.  The idea is that the client would
recieve both the cert and the "stapled" OCSP response, and would preload
the response into the OCSP cache before validating the cert, so that when
the cert validation got to the point where it wanted to check the cert 
validity with OCSP, it would find the answer already in the cache.  Of 
course, the stapled response itself must be validated before being cached.
OCSP response stapling is now defined in an RFC, and some think this will
be as vital for OCSP scalability as OCSP caching will be, eventually (soon).

There might conceivably be a desire to disable the cache for a certain 
OCSP responder, so that some responders' results are cached and others'
are not.  I'd say we should design with that in mind, so that it is easy
to add later if we so choose, but that we should not provide that feature
on day one.  We should wait for demand for it before implementing it. 
The default should be that all OCSP responses are cached.  

Regarding the question of OCSP cache entry lifetime, I don't recall whether
OCSP responses have a lifetime encoded in them or not.  I know that CRLs 
have a "next update" time, after which it is undesirable to use an old 
cached copy, and getting a new copy is preferred (but not required).  
I don't know if OCSP entries have a similar time limit.  If so, we should
honor that.  If not, we should apply our own.  It should be appliation 
settable, but we should choose a reasonable default so that the application
is not required to set it.  I'm initially thinking 24 hours is a reasonable
default, but we should strive for consensus on that.

That's all I can think of off the top of my head.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

One more though on what we would store in the cache.  
We might want to store a triple (OCSP response, simple result, expiration time).
That is, store the encoded OCSP response, and the decoded essential results.
or alternatively, store (hash of OCSP response, simple result, expiration time).
This way, we don't have to redecode and revalidate the response each time,
but we have the ability to discern between duplicate responses and new/modified
ones.  Not sure this is necessary.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #0)
> This RFE is related to bug 91532, which requests an
> *offline* OCSP cache that can be used when the machine
> is not connected to a network.

bug 91532 was/is a duplicate of bug 48597.
But I don't understand the difference between those bugs and this one.
I think this bug and those are all duplicates.

How is an "offline" cache different from a "local" cache?
What difference does it make to the cache whether the appliation is 
online or offline?  How does the cache behave differently?

I think perhaps the "offline" cache request is a request that the cache
contents be preserved when the browser goes offline, and even when it 
shuts down, so that it is possible to start the browser offline and still
have old OCSP response values to rely on until it goes online again.

Perhaps the answer to my question above ("how does the cache behave differently?") is that, when "offline", all cache entries live on in 
perpetuity, never expiring, until it goes online again.  

If there is a requirement for an offline cache, the design of the new 
cache should encompass those requirements.  

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Kai, please note my responses to your questions, in the preceeding comments.

Comment 9

[Kai Engert (:KaiE:)]

(In reply to comment #7)
> bug 91532 was/is a duplicate of bug 48597.
> But I don't understand the difference between those bugs and this one.
> I think this bug and those are all duplicates.

No, I think there is indeed a difference, see also comment 0 in this bug.

> How is an "offline" cache different from a "local" cache?
> What difference does it make to the cache whether the appliation is 
> online or offline?  How does the cache behave differently?

For an "offline cache" as explained for the "in plane" scenario, you will need on-disk persistent storage of the cache.

However, this bug simply proposes to implement an in-memory-cache, that will speed up things, but have the cache contents go away on application shutdown.


> If there is a requirement for an offline cache, the design of the new 
> cache should encompass those requirements.  

IMHO, the when the application is offline, no OCSP should be attempted. And https content is most likely not available from the cache anyway, because it must not be stored. I'd prefer a different solution, like a clear UI indication that OCSP is not in effect, because you're offline. I think we should ignore the persistent idea at this time. It should still be possible to extend such a cache into a persistent one at a later time, if its really necessary.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Patch v1

First patch.
I want to take a look at Nelson's comments 5-7 and might attach an updated patch soon.

This patch is based on our recent discussion by email, and a proposal from Nelson. I hope I got the behavior right, but we can certainly fine tune the "freshness" behavior, which is a small portion of this patch only.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 252525 [details] [diff] [review]
Patch v1

Some initial not-full-review comments on this patch.
Some of the code in this patch follows the NSS coding style, but much does not.  Some issues I see include:
a) lines longer than 80 columns,
b) indentation should be 4 characters.
c)  "} else {" should be one line.
d) case and default sohuld be at the same level of indentation as switch.  
e) multi-line comments hsould have an asterisk on each line.  The asterisks should be in the same column.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Patch v2

This patch addresses Nelson's requests, hope I got them all.

Comment 13

[Kai Engert (:KaiE:)]

Here are some replies to comment 5, which triggered to me do some changes to the patch.

(In reply to comment #5)
> I agree about primary key.  The value we would store might be something
> much simpler than the previous OCSP response.  It might be the expiration
> time plus a single byte (or word) containing the answer for this cert.  
> No point in revalidating the signature on the previously validated OCSP
> response, for example.  We should strive to avoid doing that.

We are caching the contents of existing structure ocspCertStatus, after verification succeeded, which contains the guts of the response only. If we failed to do networking, we store a flag that no ocspCertStatus is available.


> 100 is too small, IMO.  I'd say it should be bounded by age of the responses,
> not by number of responses.  We should save all the responses that are not
> older than some settable amount of elapsed time.  If we need to bound the 
> number, perhaps 1000 should be the upper bound.

As we had recently discussed, we are using a combination of max number of entries with a LRU list.

> Needs to be a publicly callable way to flush this cache.  

I added such a function.


> Appliation needs to flush it whenever the OCSP configuration changes
> and IMO also whenever the list of trusted CAs changes.

In the proposed patch I'm flushing the cache from within NSS, whenever the default responder changes or gets disabled, and when OCSP gets turned off. Is that reasonable?

Good point regarding the list of changed CAs. I wonder if we could/should detect this from within NSS. But I'm ok to do this in PSM.


> Needs to be a way to shut the cache down (free it all), and that needs to 
> be callable at NSS shutdown time.  

Added.



> NSS now has a way to register functions
> to be called wheh NSS shuts down.  This OCSP cache shutdown function 
> would need to be registered to be shut down at NSS shutdown time. 
> Ideally this would be done automatically by NSS itself, rather than 
> putting this burden on the appliation.  Some care is needed to make 
> that idempotent.

I added a call to do a cache shutdown to function NSS_Shutdown, right next to a call that shuts down the CRL cache. I agree we don't need to make use of the dynamic shutdown function registration mechanism, but let NSS care of that on its own.


> Cache needs to be used by ALL of NSS's OCSP clients.  I think (not sure) 
> that we have two, the old one, and a new one in libPKIX.  

When I worked on the OCSP cache, I figured something which is a bit unfortunate. Part of the exported API are several OCSP functions that do work well with the idea to have a cache. I had come to the following decision, in order to get something started, to keep the current API. It sufficient for all OCSP requests that originate from within NSS to go through the cache.

My decision, the proposal is:
- Cache the OCSP activity that is triggered from within 
  function CERT_CheckOCSPStatus, that is always acting on a single cert.
- Do not attempt to cache the OCSP activity that operates on lists of
  certs, that operates on generic requests (which might ask for multiple
  certs). This seems much more work.
  I would like to postpone supporting that.
  
> We need a way to "preload" the cache with responses obtained through 
> other means than the usual OCSP request/response protocol.  I am thinking
> specifically of "stapled" OCSP responses, which are responses for TLS 
> server certs that are (optionally) sent by the server, along with the 
> server cert, during the TLS handshake.  The idea is that the client would
> recieve both the cert and the "stapled" OCSP response, and would preload
> the response into the OCSP cache before validating the cert, so that when
> the cert validation got to the point where it wanted to check the cert 
> validity with OCSP, it would find the answer already in the cache.  Of 
> course, the stapled response itself must be validated before being cached.
> OCSP response stapling is now defined in an RFC, and some think this will
> be as vital for OCSP scalability as OCSP caching will be, eventually (soon).

Once we implement stapling, it should be straightforward to use NSS internal functions to add entries to the cache.


> There might conceivably be a desire to disable the cache for a certain 
> OCSP responder, so that some responders' results are cached and others'
> are not.  I'd say we should design with that in mind, so that it is easy
> to add later if we so choose, but that we should not provide that feature
> on day one.  We should wait for demand for it before implementing it. 
> The default should be that all OCSP responses are cached.  

I believe it will be straightforward to implement a filtering based on OCSP responder once we need it.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Patch v3 - Reviewer version - More Context - Ignores Whitespace

Review comments welcome! Thanks.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 252608 [details] [diff] [review]
Patch v3 - Reviewer version - More Context - Ignores Whitespace

Kai, I suspect that you developed this patch against the NSS_3_11_BRANCH, not the trunk.  Yet it seems to be intended for NSS 3.12, which will come from the trunk, not the 3_11 branch.  The trunk already has a 3.12 section in NSS.def, for example.  There are now a lot of differences between the trunk and the branch in OCSP code, IINM, so I suspect you will need to develop a different patch for the trunk.

Comment 17

[Kai Engert (:KaiE:)]

Nelson, you are right, I'm currently coding against the Firefox 2.0 branch, which works with NSS 3.11, in order to have a stable environment while testing.

Before I attached the patch, I had already ensured that the patch applies cleanly to the trunk of NSS, too!

I just compared revisions of file ocsp.c from NSS 3.11 branch and NSS trunk. While there are some differences, I don't see any conflicts with my changes.

Comment 18

[Kai Engert (:KaiE:)]

Well, of course we will need to adjust the change to nss.def not later than checking in the final version of the patch. But I see that as a minor and obvious issue, the change is listed to remind us these new functions need to get exported, the other portions of the patch are more tricky to review, and should work on both branch and trunk.

Comment 19

[Kai Engert (:KaiE:)]

I want to ensure that OCSP still behaves correctly with the OCSP cache in place.

The critical test, of course, is to verify that we still reject revoked certs, even if we are going to the cache only.

As I'm not aware of a public test site that uses a revoked cert which specifies an OCSP service URL, I went the extra mile and set it up myself.

Page with links and explanations:
  http://kuix.de/misc/test-ocsp/

The above link should be all you need, but let me elaborate:

This uses a mini Test-Only CA available here:
  http://kuix.de/misc/test-ocsp/ocsp-test-ca.php
It connects to a mini OCSP responder here:
  http://kuix.de:8359/
In order to test a connection to a secure site that uses a still-valid cert:
  https://valid.kuix.de:8351/
An here is a secure site that uses a revoked cert:
  https://revoked.kuix.de:8352/

My testing so far gave me good results.

Comment 20

[Kai Engert (:KaiE:)]

Attachment: trace logfile

This logfile was produced using SeaMonkey, so I could make use of
profile switching and test NSS shutdown / re-init.

Comment 21

[Kai Engert (:KaiE:)]

Attachment: Incremental patch v3 -> v4

I have implemented a new graceful OCSP failure mode in NSS.

I will shortly attach a patch v4, however, should you have already started to review v3, this patch helps you to easily see the differences.

In this proposed new patch, NSS' current OCSP failure behaviour did not change. NSS will still fail hard if OCSP is enabled but there is a failure in OCSP networking or response processing. The reasonsing for this approach: Be backwards compatible.

The patch introduces a new function to control the failure mode, and Mozilla applications will probably enable that new mode by default.

I will also attach a patch with proposed application changes to bug 110161.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: Patch v4

New full patch v4, combines v3 and the incremental patch.

Comment 23

[Kai Engert (:KaiE:)]

Adding dependency for reference purposes, as bug 338986 also changes a lot of OCSP code. Luckily, there are currently no conflicts. Hopefully it will stay that way.

Comment 24

[Robert Relyea]

Comment on attachment 253391 [details] [diff] [review]
Patch v4

OK, I'm about halfway through, but I do have some comments that need work..

General comments:
1) The code seems to grab the global lock early and hang onto it for a long time. Monitors are better for that type of coding style.  I'm a little worried that our lock latency and (worse) contention will be fairly high. It might be better to try to break down the locking into finer grain units. (lock the cache to get an item, lock the item itself, etc).

2) linked lists are the some of the trickiest and hardest to review portions of the code. You could simplify your code to make an entry 'most recently used' to be "remove from list" and "add to top". You already need the latter (used in your remove an item), and the former becomes trivial for a new element. That would make the code easier to read and remove the funny corner case (the element is on the queue, but the only element).

3) Most of your functions to deal with the cache should take a cache pointer, rather than grabbing the cache from the global data structure.

3)  Your new ocsp_CreateSingleRequestList()

is basically 3 different functions welded together with if's. It might be cleaner to just split the 2 functions and use a single if where you call it to select the appropriate function.

4) CERT_SetOCSPFailureMode should take an emum or use #defines rather than '0' or '1' as a parameter (the value can still be '0' or '1', it just needs a friendly define for the caller).

I'm most worried about #1. It could change the way the code is put together.

Comment 25

[Kai Engert (:KaiE:)]

ocsp_MakeCacheEntryMostRecent
  only asserts and assignments

ocsp_IsCacheDisabled
  comparison

ocsp_FindCacheEntry
  PL_HashTableLookup

ocsp_FreeCacheItem
  free

ocsp_RemoveCacheItem
  asserts
  assignments
  PL_HashTableRemove

ocsp_CheckCacheSize
  mostly single removal
  potentially loop of calling remove

ocsp_ClearCache
  loop of calling remove

ocsp_CreateCacheItem_and_ConsumeCertID
  alloc
  PL_HashTableAdd

ocsp_CopyCertStatus
  arena-dup
  alloc
  copyitem

ocsp_SetCacheItemResponse
  free
  alloc
  DER_GeneralizedTimeToTime

ocsp_FreshenCacheItemNextFetchAttemptTime
  now
  simple math
  assignments

ocsp_IsCacheItemFresh
  now
  comparison

ocsp_CreateOrUpdateCacheEntry
  asserts
  assignments

CERT_OCSPCacheSettings
  assignments
  might clear cache

ocsp_GetCachedOCSPResponseStatusIfFresh
  series of above function

Comment 26

[Kai Engert (:KaiE:)]

(In reply to comment #24)
> 1) The code seems to grab the global lock early and hang onto it for a long
> time. 

This is of course subjective, but I tend to disagree that the lock is being kept for a long time.

We never hold the lock while going to the network.
We never hold the lock while calling other NSS functionality (besides allocation, hash table access and time conversion).

I went through all the new functions and put together a list of the actions that happen while the current patch holds the lock. I listed it in the previous comment.

In my personal opinion, this is always limited to a series of simple operations:
- allocation
- freeing
- 1 or 2 hash table operations
- simple math (no math functions)
- comparison
- assertions
- DER_GeneralizedTimeToTime
- PR_Now

Holding the lock is strictly limited to updating the cache data structures and performing the above operations.

There is only one possibility where a loop can occur while holding the lock:
If we need to clear the cache.

My subjective feeling is that introducing additional locks is not necessary and will overly complicate the code. This is based on my assumption, that a series of the above simple operations is "fine" while holding the lock.

But maybe you'll say "even these operations are consuming too much time"?

Would it be sufficient to relax the loop, and release the lock after each removal of one item?


> Monitors are better for that type of coding style.

IIUC, a Monitor is a reference counted lock. The only added benefit is: Coding mistakes can't result in a deadlock when accidentally locking twice on the same thread, right?

I think I have carefully avoided that possibility, but if you suggest we should introduce the added protection, I'm ok to convert to using a Monitor.


> I'm a little worried
> that our lock latency and (worse) contention will be fairly high.

This can only ever happen when two threads require OCSP cache data at the same time. For example, it will still be possible to have two OCSP network requests running in parallel.


> It might be
> better to try to break down the locking into finer grain units. (lock the cache
> to get an item, lock the item itself, etc).

I would prefer to avoid that additional complexity.

This is why I am trying to convince you with the above comments.

However, if I failed to convince you, just say so, and I will work on implementing finer grained locking!

It would require to work on cases like:
- ensure no two threads are accessing the same cache item at the same time
- take care of one thread removing an item, while the other wants to modify it
- introduce reference counting for cache items, so they can destroy themselves if the last reference goes away

Comment 27

[Robert Relyea]

The danger of holding locks for 'a long period of time' is that it creates potential lock contention issues. Holding the lock while going out to the network is an obvious no-no, but for certain kinds of applications, even holding the lock while doing a memory allocation is a bad idea.

Where there is no lock contention, it really doesn't matter, but if the function is called frequently, then it becomes more of an issue. In softoken, for instance, you will almost never see a lock held over a function call. Locks are usually used to protect hash tables, cues and reference counts. In server environments where you may be making frequent concurrent calls, long locks will kill your throughput (which is why softoken doesn't have any).

For functions that are less frequently called, the contention issue may not be as bit. In an SSL connection, softoken can be called dozens of times on full handshakes and dozens of times on restarts. OCSP will be called maybe 3-4 times on full handshakes and never on restarts (and only on client auth). In this case OCSP may not have the lock contention issues that softoken has.

At this point, let's switch to PR_Monitors() (this will remind us that there are longer term locks around). We should then measure the performance of client auth connections with the cache turned on versus with the cache turned off using strsclnt and selfserv and see if we have any issues (the cert processing code may give us some problems).

bob

Comment 28

[Kai Engert (:KaiE:)]

I made OCSP_Global.lock a monitor, and changed lock/unlock to enter/exitmonitor.

At every place I had a comment "caller needs to lock" I added pairs of enter/exitmonitor.

I changed all internal sub-functions that accessed OCSP_Global.cache to take a cache pointer parameter.

Your request to split that one if'd function was a good one. The new code is really easier to read. And it helped me to further simplify and get rid of a signerCert parameter, because I figured this is only being used for an initial check.

I isolated some common init code into a new function ocsp_prepareEmptyOCSPRequest.

I also like your request to introduce an enum for the failure modes. I used that new enum to replace the boolean variable I had in OCSP_Global, so our stored preference is now consistent with the function parameter.

I like your smart proposal to use funtions AddToLinkedList and RemoveFromLinkedList, and have the MakeMostRecent call them both. This indeed simplified the code. I wish I have had this idea myself, it would have saved me some headaches. Only problem is that I had to change the already tested code, and I will have to test again. But that's life.

Comment 29

[Kai Engert (:KaiE:)]

Attachment: Patch v5

Comment 30

[Kai Engert (:KaiE:)]

Attachment: v5 in colored html

Experimental HTML version of the patch that uses syntax highlighting. Might be easier to review. Let me know if you like it!

Comment 31

[Kai Engert (:KaiE:)]

Attachment: v5 without whitespace changes in colored html

Note that I'm using an editor that replaces all hard tabs with spaces when I touch the code. So patch v5 lists some bogus differences. This is a version of the patch that ignores any change in whitespace. Again as syntax highlighting html.

Comment 32

[Kai Engert (:KaiE:)]

Comment on attachment 254621 [details] [diff] [review]
Patch v5

Bob, I know you have already started to review this patch.

I have seen there is the possibility that InitOCSPGlobal gets called twice, while NSS makes attempts to init. (I ran into this while forcing an NSS init failure.)

With the current patch this will trigger an assertion. I currently have this code in the patch:

+    PORT_Assert(OCSP_Global.cache.entries == NULL);
+
+    OCSP_Global.ocspFailureMode = ocspMode_FailureIsVerificationFailure;
+    OCSP_Global.cache.number_of_entries = 0;
+    OCSP_Global.cache.most_recently_used_cache_item = NULL;
+    OCSP_Global.cache.least_recently_used_cache_item = NULL;
+
+    OCSP_Global.cache.entries = 
+        PL_NewHashTable(0, 
+                        ocsp_CacheKeyHashFunction, 
+                        ocsp_CacheKeyCompareFunction, 
+                        PL_CompareValues, 
+                        NULL, 
+                        NULL);


I would like to change that to:


+    if (!OCSP_Global.cache.entries) {
+        OCSP_Global.cache.entries = 
+            PL_NewHashTable(0, 
+                            ocsp_CacheKeyHashFunction, 
+                            ocsp_CacheKeyCompareFunction, 
+                            PL_CompareValues, 
+                            NULL, 
+                            NULL);
+
+        OCSP_Global.ocspFailureMode = ocspMode_FailureIsVerificationFailure;
+        OCSP_Global.cache.number_of_entries = 0;
+        OCSP_Global.cache.most_recently_used_cache_item = NULL;
+        OCSP_Global.cache.least_recently_used_cache_item = NULL;
+    } else {
+        /*
+         * NSS might call this function twice while attempting to init.
+         * But it's not allowed to call this again after any activity.
+         */
+        PORT_Assert(OCSP_Global.cache.number_of_entries == 0);
+    }

Comment 33

[Robert Relyea]

Comment on attachment 254621 [details] [diff] [review]
Patch v5

Much better factoring.

There is one bug that warrents the r-:

In ocsp_RemoveCacheItemFromLinkedList

+    if (!item->less_recent && !item->more_recent) {
+        PR_ExitMonitor(OCSP_Global.monitor);
+        return;
+    }

This test should be moved to after the if (item == cache->least_recently_used_cache_item  test, otherwise you will never be able to remove the last entry from the table.


Other general comments:
I forgot to meantion the #ifdef DEBUG_kaie before.  It seems we should include a regular trace option in the DEBUG code. We should probably look at util to see if we have a general trace mechanism (or use NSPR tracing).

The locks feel like the should be part of the cache to me, but they are clearly protecting other fields in the global context, so I wouldn't require you to move it.

There are lots places where you have

PR_Lock()
global.value = new_value
PR_Unlock()

It might be better to use PR_AtomicSet() for these, as long as the types work (I'm sure the enum is fine, the function pointers, on the other hand, may not be doable unless there's a pr_atomic call that sets a void *.

Fix the first one is a MUST.

Comment 34

[Kai Engert (:KaiE:)]

(In reply to comment #33)
> In ocsp_RemoveCacheItemFromLinkedList
> 
> +    if (!item->less_recent && !item->more_recent) {
> +        PR_ExitMonitor(OCSP_Global.monitor);
> +        return;
> +    }
> 
> This test should be moved to after the if (item ==
> cache->least_recently_used_cache_item  test, otherwise you will never be able
> to remove the last entry from the table.


Thanks for catching this issue.
I rearranged this test and handling of the one-item-in-list case.
I also changed the code that adds a new entry to no longer call MakeMostRecent but only AddToLinkedList.
While invalid removal attempts should no longer happen, I guess it's still a good idea to have this code fail gracefully.


> I forgot to meantion the #ifdef DEBUG_kaie before.  It seems we should include
> a regular trace option in the DEBUG code. We should probably look at util to
> see if we have a general trace mechanism (or use NSPR tracing).

It seems there is currently not yet a global trace mechanism in NSS.
The only thing I found is in libssl, controlled by environment variables - which is currently specific to SSL.
I re-used code from the SSL tracing, but I was not up to introducing a new OCSP_TRACE environment variable - but I could if you want me to.
I agree it would be good to consolidate and have global trace flags.
I take your words we can postpone this.


> The locks feel like the should be part of the cache to me, but they are clearly
> protecting other fields in the global context, so I wouldn't require you to
> move it.

Yeah, indeed. Those other fields are other global OCSP settings, and they are just a few. I didn't want to introduce yet another lock, because when those options change, we'll likely also have to change the cache.


> There are lots places where you have
> PR_Lock()
> global.value = new_value
> PR_Unlock()
> It might be better to use PR_AtomicSet() for these

?
- I'm using monitors now, no longer locks
- there is no AtomicSet for 64 bit values
- are you proposing to no longer lock, but using AtomicSet only? This proposal would apply only for 2 occurrences in the code. I would rather see all accessses to our variables use the same locking mechanism, as a matter of consistency.

And my biggest confusion is: While AtomicSet ensures that read-decide-update is an atomic operation - it does not protect us from another one doing lock-read-DoSomethingElseRequiredForDecision-update-unlock

Comment 35

[Kai Engert (:KaiE:)]

Attachment: Patch v6

This new patch has the following differences from the previous one:

- changed ocsp_RemoveCacheItemFromLinkedList as requested

- ocsp_CreateCacheItem_and_ConsumeCertID no longer calls 
  ocsp_MakeCacheEntryMostRecent, but ocsp_AddCacheItemToLinkedList

- changed InitOCSPGlobal as explained in comment 32

Comment 36

[Kai Engert (:KaiE:)]

Bob, you had asked me to do performance testing:


(In reply to comment #27)
> At this point, let's switch to PR_Monitors() (this will remind us that there
> are longer term locks around). We should then measure the performance of client
> auth connections with the cache turned on versus with the cache turned off
> using strsclnt and selfserv and see if we have any issues (the cert processing
> code may give us some problems).


My testing is with two cert db's, one has ca and server cert, another one ca and client cert.
I ran the tests on an AMD Athlon 64 X2 dual core 4400+ machine.
The commands I used:

selfserv -d . -n localhost -p 1234 -r -r
(require client auth)

/usr/bin/time strsclnt -d . -p 1234 localhost -t 10 -D -N -c 10000 2>&1 |grep -v "SSL: Server Certificate Validated"
(10 threads, 10000 iterations)

I ran the test with and without OCSP cache patch applied.
OCSP was disabled.
Each test gave me this general output on the client:
  strsclnt: 0 cache hits; 8 cache misses, 0 cache not reusable
  strsclnt: 0 cache hits; 10000 cache misses, 0 cache not reusable
  strsclnt: NoReuse - 10000 server certificates tested.

To my surprise the new code seems to be *slightly* faster...?
I ran each test 3 times.


test output using current 3.11 branch (no patch):

21.00user 2.28system 0:19.63elapsed 118%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+16145minor)pagefaults 0swaps
21.04user 2.21system 0:19.69elapsed 118%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+11748minor)pagefaults 0swaps
21.07user 2.15system 0:19.79elapsed 117%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+19250minor)pagefaults 0swaps



test output with the OCSP cache patch applied:

20.98user 2.22system 0:19.56elapsed 118%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+10425minor)pagefaults 0swaps
20.96user 2.21system 0:19.53elapsed 118%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+8642minor)pagefaults 0swaps
20.96user 2.21system 0:19.68elapsed 117%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+18577minor)pagefaults 0swaps


Is this the test you wanted?
Would you like me to run another test with different parameters?

Comment 37

[Robert Relyea]

Comment on attachment 255051 [details] [diff] [review]
Patch v6

OK, this patch looks good to me.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 255051 [details] [diff] [review]
Patch v6

Here is some minor whining over cosmetic/style issues.  
This is not yet a full review.  I'm working on it.
I'm also asking Alexei for "additional review", which is optional.

1.. indentation

>+#ifdef DEBUG_kaie
>+
>+    #define OCSP_TRACE(msg) ocsp_Trace msg
>+    #define OCSP_TRACE_TIME(msg, time) ocsp_dumpStringWithTime(msg, time)
>+    #define OCSP_TRACE_CERT(cert) dumpCertificate(cert)

All the code in this ifdef is indented 4 spaces more than normal.
Please don't do that.  Please use normal NSS indentation.
(I have no objection to committing this debug code.)

2. too many blank lines.

The new code in this patch has many many blank lines.  Something like 1/3 
of the new lines are blank lines.  This causes "vertical exaggeration" of
code, making functions appear much longer than they are.  

When a function has only ~30 non-blank lines of code, it ought to be 
possible to see the entire function in a window, all at once.  Being able to
see a whole function in a window without scrolling really helps readability
and reviewing.  But with the number of blank lines in this patch, in some 
places, that's just not possible.  Perhaps we ought to set a standard that 
no more than 20% of the lines in a function may be blank lines.

3. I really prefer that all struct declarations, enum declarations, and typedefs and 
most #defines and global variables in a .c file come at the beginning of the file, before the functions.  Maybe that's old fashioned.  I don't know if there's any 
consensus on this.  If not, feel free to ignore this.

Comment 39

[Kai Engert (:KaiE:)]

I can certainly address your whitespace concerns and move declarations up to the beginning of the file.

I'll wait for your full review before I attach a new patch.

I'm glad to see you have started reviewing. So I'll not yet commit this on the trunk, but wait a bit more.

Comment 40

[Kai Engert (:KaiE:)]

Nelson, during out meeting today, I mentioned that there are OCSP functions which won't go through the cache, at least not (yet) with this patch. Here is my full explanation.

When OCSP has been enabled using function CERT_EnableOCSPChecking
then NSS uses its internal function CERT_CheckOCSPStatus for verification.
This is the only entry point used from within the NSS library to call OCSP.

The above function is what I enhanced to use a cache,
so all OCSP requests triggered from within NSS, as part of
cert verification, go through the cache.

It was simple to do so, because function CERT_CheckOCSPStatus
takes a single cert as its argument, and returns the verification
status.

CERT_CheckOCSPStatus is not (yet) an exported function.

In addition NSS offers an external API for OCSP (exported in def files).
It consists of functions that operate on OCSP requests
and OCSP responses, which deal with lists of certificates.

The functions are :
    CERT_CreateOCSPRequest
    CERT_EncodeOCSPRequest
    CERT_DecodeOCSPResponse
    CERT_GetOCSPResponseStatus
    CERT_GetOCSPStatusForCertID
    CERT_VerifyOCSPResponseSignature

I consider these functions "raw worker functions", because
they directly reflect the data structure that is being sent 
out to the network or received from the network.

When we brainstormed about the OCSP cache a while ago,
we expressed the desire that we do not want to cache the
response signatures, and that we do not want a repetitive
verification of signatures.

So the design of the above worker functions, which uses a 
OCSP-repsonse data structure, which must contain a valid
signature, seems contrary to our goal.

Because of that, and because of the requirement to keep
the NSS API backwards compatible, I propose we do not change
the behaviour of above worker functions, but indeed declare
them as pure OCSP worker functions.

Should we require an export API that can verify multiple
certificates at once and go through the cache,
I propose that we introduce a new higher level API.

FYI, the proposed caching implementation of CERT_CheckOCSPStatus makes use of those worker functions.

Comment 41

[Kai Engert (:KaiE:)]

During yesterday's meeting, Nelson also raised the question, what about the new libpkix code?

libpkix currently uses the worker functions
    CERT_CreateOCSPRequest
    CERT_EncodeOCSPRequest
    CERT_DecodeOCSPResponse
    CERT_GetOCSPResponseStatus
    CERT_GetOCSPStatusForCertID
    CERT_VerifyOCSPResponseSignature
too.

Although the worker functions can operate on a list of cert,
libpkix always calls them with a single as the query argument.

Both the current production NSS code and the new libpkix code
use a single entry point for their OCSP verification.
(CERT_CheckOCSPStatus vs. pkix_OcspChecker_Check)

pkix_OcspChecker_Check has explicit calls to verify-signature, something we want to avoid when using a cached response.

My proposal for OCSP-cache-enabling libpkix is:
Change pkix_OcspChecker_Check to match the behaviour of CERT_CheckOCSPStatus as its being done in patch v6.
In other words, add the cache lookup to the implementation of pkix_OcspChecker_Check.

Comment 42

[Kai Engert (:KaiE:)]

I looked at the libpkix implementation a bit more.

I'm very surprised to see, the implementation of pkix_OcspChecker_Check (and dependent libpkix helper functions) does not offer a "valid at time" parameter, and the implementation of pkix_OcspChecker_Check is using the hardcoded parameter NOW. Shall I file a separate bug on this? Doesn't that mean the libpkix API is missing an important parameter?

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 42,  
IINM, OCSP is only capable of answering the question for time NOW.  
Julien believed that the API should not let the caller ask a question 
that the function cannot answer (such as the validity on some time
other than NOW).  I think I agree.

I think this means that we should not invoke OCSP for revocation checking
if the target verification time is other than NOW.

Comment 44

[Kai Engert (:KaiE:)]

(In reply to comment #43)
> IINM, OCSP is only capable of answering the question for time NOW.  

Hmm, but the data returned by the OCSP server contains a field "revocation-time".

The current OCSP code in NSS compares the given valid-at-time parameter with the revocation-time.

Comment 45

[Robert Relyea]

Right, I don't think it is the case that OCSP is only capable of answering the question for time NOW. I think CRLv1.0 has that problem (though 2.0 have revocation time stamps).

There is an issue that most infrastructures loose revocation information after the certificate expires, but that is an infrastructure decision, not necessarily a standards decision.

bob

Comment 46

[Julien Pierre]

Bob,

Technically, RFC3280 and RFC2560 only define an algorithm to check for validity of a cert at the current time. NSS has tried to go beyond that in the past, and we should continue to do so with libpkix, for compatibility reasons if nothing else, but it is not a standardized method.

Both CRLs and OCSP have the same problem in that no information is available for dates before the notBefore of the certificate. There are infrastructure decisions as you point out, but in the general case there is no guarantee it is available so it is not a given that we should accept all dates. Some recent extensions in the certificate can indicate whether the revocation information for old certs is being archived by the CA and for how long (sorry, I don't remember the specific extension names). However those extensions are not supported by libpkix yet and they are not defined in RFC3280. It would be really good to add them especially to tie them with S/MIME support and verification of old messages.

Comment 47

[Kai Engert (:KaiE:)]

I talked to Nelson by email. Because he was not yet able to do a full review of the patch, he proposes we go ahead and start by landing it on the trunk first.

However, I had promised to address Nelson's "minor whining" from comment 38. I'm doing this now, here are my replies, and I'm attaching a new patch.


(In reply to comment #38)
> 1.. indentation
> 
> >+#ifdef DEBUG_kaie
> >+
> >+    #define OCSP_TRACE(msg) ocsp_Trace msg
> >+    #define OCSP_TRACE_TIME(msg, time) ocsp_dumpStringWithTime(msg, time)
> >+    #define OCSP_TRACE_CERT(cert) dumpCertificate(cert)
> 
> All the code in this ifdef is indented 4 spaces more than normal.
> Please don't do that.  Please use normal NSS indentation.

Ok, I will remove the single additional nesting level I had added here. I thought I was a good idea, to make it clear that this is debugging-only code...


> 2. too many blank lines.
> 
> The new code in this patch has many many blank lines.  Something like 1/3 
> of the new lines are blank lines.  

I used "grep -c" to count.
We have 1890 non-blank + lines in the patch, and 217 blank ones.
217 / (1890+217) => 10.3 %

The existing file ocsp.c has 4577 non-blank lines and 701 blank ones.
701 / (4577+701) => 13.3 %

My code introduces less blank lines than are being used in the existing code.
I believe the patch uses blank lines in a reasonable way to enhance the readability by grouping lines. But I will have removed some lines in the patch I'm going to attach...


> 3. I really prefer that all struct declarations, enum declarations, and
> typedefs and 
> most #defines and global variables in a .c file come at the beginning of the
> file, before the functions.

done

Comment 48

[Kai Engert (:KaiE:)]

Attachment: Patch v7

After applying the changes explained in the previous patch, doing some minor cleanup and also changing nss.def file to change the existing version 3.12 section, I got this Patch v7

I am marking this Patch as r+ rrelyea, because no real code got changed.

I will check this patch in to NSS trunk now.

As this patch might eventually go into the 3.11 branch, I am asking for Nelson's and/or Alexei's second review.

Comment 49

[Kai Engert (:KaiE:)]

Checked in to trunk, resolving bug as fixed.

If changes are requested for 3.11 branch landing, we should reopen this bug and add incremental patches.

(I might file spin-off bugs for the other requests.)

Checking in certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.37; previous revision: 1.36
done
Checking in certhigh/ocsp.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.8; previous revision: 1.7
done
Checking in certhigh/ocspi.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspi.h,v  <--  ocspi.h
new revision: 1.3; previous revision: 1.2
done
Checking in certhigh/ocspt.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspt.h,v  <--  ocspt.h
new revision: 1.7; previous revision: 1.6
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.169; previous revision: 1.168
done
Checking in nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.76; previous revision: 1.75
done

Comment 50

[Kai Engert (:KaiE:)]

I filed bug 375019: Cache-enable pkix_OcspChecker_Check
I filed bug 375020: Support "is valid at time" OCSP query in libPKIX

Comment 51

[Kai Engert (:KaiE:)]

Attachment: Patch v8

Compiling on Windows uncovered 3 problems with the patch:
- ocsp_CacheKeyHashFunction now uses a (unsigned char*) instead of (void*)
- in ocsp_CreateCacheItem_and_ConsumeCertID variable item was not declared at the beginning of the function
- ocsp_CreateCacheItem_and_ConsumeCertID returned an uninitialized variable on failure (oops)

This Patch v8 addresses the above issues and fixes the initial tinderbox failure on Windows.

Checking in lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.39; previous revision: 1.38
done
Checking in lib/certhigh/ocsp.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.10; previous revision: 1.9
done
Checking in lib/certhigh/ocspi.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspi.h,v  <--  ocspi.h
new revision: 1.5; previous revision: 1.4
done
Checking in lib/certhigh/ocspt.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspt.h,v  <--  ocspt.h
new revision: 1.9; previous revision: 1.8
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.171; previous revision: 1.170
done
Checking in lib/nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.78; previous revision: 1.77
done

Comment 52

[Nelson Bolyard (seldom reads bugmail)]

I finally reviewed this.  Will attach review comments shortly.

Comment 53

[Nelson Bolyard (seldom reads bugmail)]

Attachment: review comments, sr-

These are my review comments about patch v8.  sr-

Comment 54

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 259389 [details] [diff] [review]
Patch v8

see review comments in attachment 260939 [details].

Comment 55

[Wan-Teh Chang]

Comment on attachment 260939 [details]
review comments, sr-

Nelson wrote:
>9. use of LL_ macros.   IIRC, we've stopped using LL_ macros in other 
>parts of NSS.  That is, NSS now assumes that every supported compiler
>now implements a 64-bit type and arithmetic on that type, so LL_ macros
>are no longer necessary.  LL_ macros are not wrong, just not necessary,
>and may create code that is more difficult to understand.   I should 
>double-check this with Wan-Teh.

I confirm that LL_ macros are no longer necessary.

Comment 56

[Kai Engert (:KaiE:)]

Attachment: please ignore. backup. patches for various branches

Please ignore this comment and attachment. I'm in a bit of merging h*ll, because I need to work on 4 branches at the same time, for development and for producing test builds...

NSS trunk:
  A p32-nss-trunk-incremental-minimal

NSS 3.11:
  B p31-nss311-minimal-ocspcache
  A p32-nss-trunk-incremental-minimal

moz 1.8 branch / firefox 2 / seamonkey 1.1
  C p31-moztrunk-minimal-ocspcache
  A p32-nss-trunk-incremental-minimal
  D p3-psm-minimal-ocspcache

moz trunk / firefox 3 / seamonkey 1.5
  C p31-moztrunk-minimal-ocspcache
  A p32-nss-trunk-incremental-minimal
  E p3-trunk-psm-minimal-ocspcache

Comment 57

[Kai Engert (:KaiE:)]

Attachment: incremental Patch v9

Comment 58

[Kai Engert (:KaiE:)]

In order to simplify the review process, I created an incremental patch.

Nelson, could you please review incremental patch v9, which addresses nearly all of your comments? (please await the next bug comment)

Trunk needs: incremental patch v9

Branch needs: patch v8 + patch v9

Comment 59

[Kai Engert (:KaiE:)]

Nelson, thanks a lot for your review.
I addressed all your requests, except number 3 (which was NOT a must fix).

Items 1, 2, 6-13 were all rather simple chance requests about style, simplifying code, adding comments, unnecessary macros. I addressed them all.

I addressed your request number 4, which was about enhancing our trace code.
I can now be controlled by an environment varilable (in a debug build) and will direct its output to the standard log. I tested that it works on Windows.

I addressed your comment 5, which is about a mem leak in tracing code (only). Thanks for catching it, I fixed it. But please note, I had copied this code from existing dbck.c, which has the bug, too. We should probably fix it as well (in another bug).

I would like to skip your request number 3, which is about linked lists.
You are arguing that we should avoid reinventing the wheel and avoid having to debug again.
However, the debugging already happened. It's done and it seems to work. Changing it at this late time would introduce a new risk and make my earlier testing obsolete. And the use of the proposed macros would require code reordering. And another disadvantage is: The current nice "more recent" and "less recent" wordings would be replaced by a less obvious "next" and "prev". So I would like to pledge to leave this code as is. Thanks.

Comment 60

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 261312 [details] [diff] [review]
incremental Patch v9

r=nelson for trunk.
requesting SR from Julien for branch

Comment 61

[Kai Engert (:KaiE:)]

Comment on attachment 261312 [details] [diff] [review]
incremental Patch v9

Bob, could you please review this incremental patch?

(Nelson and Julien agreed in today's conf call that Bob's review will be sufficient.)

Comment 62

[Robert Relyea]

Comment on attachment 261312 [details] [diff] [review]
incremental Patch v9

r+=relyea

Comment 63

[Kai Engert (:KaiE:)]

Incremental patch v9 checked in to trunk, marking as fixed again.

The combination of v8+v9 has full review from Nelson and Bob, so we are technically ready to check it in.

However, I produced engineering builds and asked several people to help testing and I propose we wait a bit more for feedback.

If no regressions show up I propose to land this on the NSS 3.11.x branch in about one week.

At the time I check it in, I will adjust nss.def (on both branch and trunk) to list the actual first version of our 3 new exported functions, which is (probably) 3.11.7.

Comment 64

[Kai Engert (:KaiE:)]

trunk checkin: 

Checking in certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.40; previous revision: 1.39
done
Checking in certhigh/ocspi.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspi.h,v  <--  ocspi.h
new revision: 1.6; previous revision: 1.5
done
Checking in nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.79; previous revision: 1.78
done

Comment 65

[Kai Engert (:KaiE:)]

I checked in patch v8 + v9 to the NSS_3_11_BRANCH.
As announced in comment 67, I adjusted nss.def to export the 3 new functions as of version 3.11.7

Checking in certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.21.2.15; previous revision: 1.21.2.14
done
Checking in certhigh/ocsp.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.6.28.2; previous revision: 1.6.28.1
done
Checking in certhigh/ocspi.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspi.h,v  <--  ocspi.h
new revision: 1.2.2.3; previous revision: 1.2.2.2
done
Checking in certhigh/ocspt.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspt.h,v  <--  ocspt.h
new revision: 1.4.28.3; previous revision: 1.4.28.2
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.158.2.6; previous revision: 1.158.2.5
done
Checking in nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.69.2.7; previous revision: 1.69.2.6
done


I did the same to the trunk version of nss.def:

Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.172; previous revision: 1.171
done


Setting target milestone to NSS 3.11.7