299308 - Need additional APIs in the CRL cache for libpkix

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Julien Pierre]

This change is for NSS_LIBPKIX_BRANCH .

In the interest of performance, we want to allow libpkix to make use of the
existing NSS CRL cache. It will be used primarily as a content cache, for
decoded CRLs and hash table of entries. The validation of CRLs will happen
within libpkix.

Some new functionality was identified as a requirement. The new CRL cache
functionality will be private, as libpkix will be statically linked with libnss .

The changes are :

1) At the CRLDPCache level, there should be a function to return all the
CERTSignedCrl objects available for the particular issuer/DP. This function will
need to acquire/release a cache lock, and increment the refcount on all the
CERTSignedCrls . It should probably take an arena to allocate a NULL-termindated
array of CERTSignedCrl*.

2) Some DPCache state information should be made avaialble to libpkix .
The following errors are currently defined :

#define CRL_CACHE_INVALID_CRLS              0x0001 /* this state will be set
        if we have CRL objects with an invalid DER or signature. Can be
        cleared if the invalid objects are deleted from the token */
#define CRL_CACHE_LAST_FETCH_FAILED         0x0002 /* this state will be set
        if the last CRL fetch encountered an error. Can be cleared if a
        new fetch succeeds */

#define CRL_CACHE_OUT_OF_MEMORY             0x0004 /* this state will be set
        if we don't have enough memory to build the hash table of entries */

The first error is irrelevant to libpkix, since it will do its own CRL
validation. But it probably cares about the other errors .

3) There should be a function to return a CRL entry from a CERTSignedCrl* . A
DPCache lock will need to be held before entering this function. This will be
achieved by libpkix calling AcquireDPCache / ReleaseDPCache around it.
This function will first check that the CERTSignedCrl still exists in the
DPCache, and if it finds a CachedCrl for it, it will then use its hash table to
find the entry. Otherwise, it will fail with a new error
(SEC_ERROR_CRL_NO_LONGER_IN_CACHE ?), which should probably be a private code
for libpkix.

The reason this function shouldn't acquire the DPCache lock is for efficiency.
libpkix might check for entries from multiple CERTSignedCrl coming from the same
 DPCache. It shouldn't need to acquire/release the cache lock on the DPCache for
each lookup.

Comment 1

[Julien Pierre]

The new prototypes will be :

/* this function assumes the caller holds a read lock on the DPCache */
(I changed my mind and think it is cleaner to have the caller call
AcquireDPCache / ReleaseDPCache around this call too).
SECStatus DPCache_GetAllCRLs(CRLDPCache* dpc, PRArenaPool* arena,
                             CertSignedCrl*** crls, PRUint16* status)

/* this function assumes the caller holds a read lock on the DPCache */
SECStatus DPCache_GetCRLEntry(CRLDPCache* dpc, CertSignedCrl* crl, SECItem* sn,
                              CERTCrlEntry** returned)

Having the CRL verification completely decoupled from the CRL cache has an
unfortunate consequence . The CRL cache is now doing partial decoding of CRLs
when it fetches them from a token or gets them from an application. It decodes
them fully and builds the hash table of entries only after the CRL signature
validation succeeds, and the CRL has been determined to be the most recent, and
thus selected to be used for validations. Right now, only one hash table is ever
kept. The hash table for an old CRL is freed when a new CRL appears. The CRL
entries themselves cannot be freed once a CRL is fully decoded, because they are
allocated in the CRL's arena.

This behavior will need to change now, because the CRL cache won't know which
CRLs libpkix will need to access.

-  The logic for which CRLs to cache will need to be changed. When libpkix
requests an entry from a CRL, if it's not already been cached, a write lock will
need to be acquired, and the CRL will need to be fully decoded, and a table
created. This will be a significant loss in terms of memory footprint. It will
not be a performance loss otherwise, except for the 1-time per issuer cost of
building the tables. Proper scoping of this case should ensure that it doesn't
affect applications that use CERT_VerifyCert but none of the new libpkix APIs.

- the purging of CRL hash tables from old CRLs when a new CRL object appears
will need to be eliminated, since the CRL cache won't know when libpkix might no
longer need it. The hash table will still be purged when the cache detects that
the object has been removed from a token, or the application called
CERT_UncacheCRL .

Comment 2

[Julien Pierre]

Attachment: add new CRL cache functions

Hanfei,

I finished coding the new functions you need for libpkix. I have done no
runtime testing of them, so I need feedback to see if they work and if they
provided the expected increase in performance for the libpkix validation
function.

I checked in the patch to the NSS_LIBPKIX_BRANCH . It should also apply to a
3.10 tree; but not 3.9 .

Comment 3

[Julien Pierre]

There was a bug in DPCache_GetAllCrls in the attached patch. The fix is :

-        allcrls[index] = cachedcrl->crl;
+        allcrls[index] = SEC_DupCrl(cachedcrl->crl);
     }
+    *crls = allcrls;
     return SECSuccess;

I have checked it in to NSS_LIBPKIX_BRANCH .

Checking in crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.49.6.2; previous revision: 1.49.6.1

Comment 4

[Julien Pierre]

Attachment: define ReleaseDPCache in certi.h

This function in needed in order for anyone to be able to call the new APIs
defined in the previous patch .

As checked in on NSS_LIBPKIX_BRANCH .

Checking in certi.h;
/cvsroot/mozilla/security/nss/lib/certdb/certi.h,v  <--  certi.h
new revision: 1.16.6.2; previous revision: 1.16.6.1
done

Comment 5

[Julien Pierre]

Comment on attachment 188645 [details] [diff] [review]
add new CRL cache functions

Removing review, since the code has been tested and works.

Comment 6

[Julien Pierre]

This bug was fixed as part of the checkin for bug 358785 .