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 ?
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.
Priority: -- → P1
Summary: New CRL import function → New CRL import and decode functions
Target Milestone: --- → 3.6
Created attachment 91740 [details] [diff] [review] 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 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!
Attachment #91740 - Flags: needs-work+
Created attachment 91890 [details] [diff] [review] 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
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
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.