158005 - New CRL import and decode functions

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Julien Pierre]

Currently, there are two CRL import functions in NSS :

1) SEC_NewCRL, which merely decodes the CRL before storing it in softoken
2) CERT_ImportCRL, which decodes it, and does a number of checks on the CRL,
such as the presence of the issuer certificate, its key usage, and the signature
of the CRL, before storing it in softoken

The following limitations exist with both functions :
1) they always make a full copy of the DER into the structure
2) they always store the CRL in softoken . No slot is provided as input

The existing functions should be consolidated into a single function.
This function would take three additional arguments :
- PK11SlotInfo* - the slot to import the CRL to . This just needs to be passed
in to the internal function crl_storeCRL
- PRBool checks . This would select whether or not to perform the checks on the
CRL before importing it
- PRArena* arena . This would be the optional arena that the DER was allocated
on, and on which the CRL should be allocated. The CRL would then own that arena.

I'm not sure what the name of this new function would be - how about
PK11_ImportCRL ?

Comment 1

[Julien Pierre]

CERT_DecodeDERCrl already takes a PRArenaPool* as its first argument. It is
currently used only to select to allocate the CERTSignedCrl structure from that
arena.

Neither CERT_ImportCrl nor SEC_NewCrl currently take an arena .

Bob said we should have an additional bool called "copyDER" to select whether to
copy the DER or keep a copy.

Of course, this requires the application to know what it's doing if passing
PR_FALSE. The recommended default would be PR_TRUE.

Comment 2

[Julien Pierre]

Attachment: patch to implement PK11_ImportCRL and CERT_DecodeDERCrlExtended

This extensive patches improves upon the following areas :
1) allows not copying the DER when decoding the CRL by means of a PRBool
copyDER argument
2) adds a PRBool doChecks to enable or disable checks on the CRL
3) the new PK11_ImportCRL API has an input slot so the CRL can be saved into
any token instead of just softoken

Please review ASAP as this patch will conflict with my other CRL patches (the
same files are affected), and I'd like to check this one in first.

Comment 3

[Wan-Teh Chang]

Comment on attachment 91740 [details] [diff] [review]
patch to implement PK11_ImportCRL and CERT_DecodeDERCrlExtended

Here is a quick summary of my review comments.

1. Add comments to explain what caution the caller should
take if he chooses to not copy DER.

2. Consider using a bitflag argument for options like
copyDER and bypassChecks.  This will make it easier to
extend this function in the future.

3. Use 'Ex' instead of 'Extended' to be consistent with
the existing function SECMOD_AddNewModuleEx.  Another
style we have used is 'WithFlags'.

4. Using the PK11_ prefix for the new, general ImportCRL
function: please check this with Bob and Nelson.  It
seems reasonable to me.

5. The 'handle' argument to CERT_ImportCRL is now unused.
PK11_ImportCRL always uses CERT_GetDefaultCertDB().  We
should assert that handle == CERT_GetDefaultCertDB() or
make PK11_ImportCRL take a 'handle' argument.

Thank you, Julien!

Comment 4

[Julien Pierre]

Attachment: new patch

This patch takes Wan-Teh's comments into account.

1. I have added comments both in certdb.h and crl.c
2. I have added two bitflags, one for the decode options for
CERT_DecodeDERCrlEx, and another for the import options in PK11_ImportCRL . The
import functions takes both bitflags as inputs, since it also decodes first
3. Changed CERT_DecodeDERCrlExtended to CERT_DecodeDERCrlEx
4. I think PK11_ is OK because of the use of PK11SlotInfo as an input argument.
It doesn't belong in the CERT calls for that reason
5. I think dropping CERTCertDBHandle is OK because none of the other PK11_
functions take one

Comment 5

[Julien Pierre]

checked in to the tip.

Checking in certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.18; previous revision: 1.17
done
Checking in certdb/crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.12; previous revision: 1.11
done
Checking in certhigh/certhigh.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.23; previous revision: 1.22
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.73; previous revision: 1.72
done
Checking in pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.89; previous revision: 1.88
done
Checking in pk11wrap/pk11func.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11func.h,v  <--  pk11func.h
new revision: 1.31; previous revision: 1.30
done