User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.11) Gecko/20050728 Build Identifier: NSS_3_10 I am trying to modify the pk12util tool add a new option to not export the full certificate chain. While following this I found that SEC_PKCS12AddCertAndKey invokes SEC_PKCS12AddCert by always passing TRUE for includeCertChain. I want this option to be exposed as a parameter to the SEC_PKCS12AddCertAndKey function. Reproducible: Always
Since SEC_PKCS12AddCertAndKey is a public function, already exposed to callers, we cannot change its signature now. In a situation such as this, where we want to add an argument to a function, we typically create a new function with a new name that takes the desired arguments, and then reimplement the existing function as a call to that new one. If we do that to SEC_PKCS12AddCertAndKey then we should also do that to SEC_PKCS12ExportCertificateAndKeyUsingPassword; that is, there should also be a new version of SEC_PKCS12ExportCertificateAndKeyUsingPassword (with a shorter name :-) that also takes an includeCertChain argument. Neil, would you like to take this bug?
While we're looking at this file, it appears that function SEC_PKCS12AddDERCertAndEncryptedKey is dead code. According to http://lxr.mozilla.org/security/search?string=PKCS12AddDERCertAndEncryp it is not exposed through any .def file and is not used within NSS itself anywhere. IMO, we should eliminate dead code like this.
Likewise, according to http://lxr.mozilla.org/security/search?string=SEC_PKCS12AddEncryptedKey function SEC_PKCS12AddEncryptedKey is not exposed in any .def file, and is only called by the (dead) function SEC_PKCS12AddDERCertAndEncryptedKey, so both of those functions can be eliminated at the same time.
According to http://lxr.mozilla.org/security/search?string=PKCS12ExportCertificateAn function SEC_PKCS12ExportCertificateAndKeyUsingPassword is not declared in any header file, nor exported in any .def file, nor called from any where in NSS.
Created attachment 199695 [details] [diff] [review] Implement new function, delete several dead functions Here's my first attempt to implement this RFE as an addition to 3.11.
Comment on attachment 199695 [details] [diff] [review] Implement new function, delete several dead functions r=wtc. In pkcs12/p12e.c, the first line of this comment block needs to be updated to say SEC_PKCS12AddCertChainAndKey instead: > /* SEC_PKCS12AddCertAndKey > * Add a certificate and key pair to be exported. > * >- * p12ctxt - the export context >- * certSafe - the safeInfo where the cert is stored >+ * p12ctxt - the export context >+ * certSafe - the safeInfo where the cert is stored > * certNestedDest - the nested safeContents to store the cert ... >+ * includeCertChain - also add certs from chain to bag. > */ > SECStatus >-SEC_PKCS12AddCertAndKey(SEC_PKCS12ExportContext *p12ctxt, >+SEC_PKCS12AddCertChainAndKey(SEC_PKCS12ExportContext *p12ctxt, It is unfortunate that the new function's name and the existing function's name are opposite to what they do. SEC_PKCS12AddCertAndKey always adds the cert chain, but SEC_PKCS12AddCertChainAndKey may not add the cert chain. Perhaps adding a new function SEC_PKCS12AddCertAndKeyWithoutChain (or NoChain) or SEC_PKCS12AddCertOnlyAndKey, without a boolean includeCertChain argument, and make SEC_PKCS12AddCertChainAndKey an internal function would be less confusing.
(In reply to comment #6) > It is unfortunate that the new function's name and the existing > function's name are opposite to what they do. > Perhaps adding a new function SEC_PKCS12AddCertAndKeyWithoutChain > (or NoChain) or SEC_PKCS12AddCertOnlyAndKey, without a boolean > includeCertChain argument, and make SEC_PKCS12AddCertChainAndKey > an internal function would be less confusing. I am quite willing to add a AddCertAndKeyWithoutChain function, to complement SEC_PKCS12AddCertAndKey, but I believe that the function that takes an argument specifying the addition of the chain (or not) should also be public. I am open to a better name for it, and invite suggestions. SEC_PKCS12AddCertAndKeyOptionalChain ? SEC_PKCS12AddCertAndKeyAndMaybeChain ? SEC_PKCS12AddKeyAndOneOrMoreCerts ? SEC_PKCS12AddKeyAndCertOrChain ? SEC_PKCS12AddCertAndKeyEx (Aaargh! :-)
I like SEC_PKCS12AddCertOrChainAndKey. If we add a function that takes an includeCertChain parameter, we don't need to add a SEC_PKCS12AddCertAndKeyWithoutChain function.
Comment on attachment 199695 [details] [diff] [review] Implement new function, delete several dead functions Marking obsolete to remind myself that the name(s) of the new functions need to be changed before this can be committed.
Created attachment 340209 [details] [diff] [review] Remove dead functions (checked in) The previous patch for this bug did two separate things: a) it removed lots of dead code, and b) it added a new function, which was controversial. The work stalled because of the issue with the new function. So, I have broken the old patch into two patches, one to remove the dead code, and another to add the new function. This is the patch to remove the dead code. Wan-Teh, please review this patch, which only removes dead code. When this gets r+, I will remove this dead code right away, separately from adding the other new function. I will attach the patch for the new function to this bug later.
Comment on attachment 340209 [details] [diff] [review] Remove dead functions (checked in) r=wtc.
Created attachment 340218 [details] [diff] [review] Add new function SEC_PKCS12AddCertOrChainAndKey (checked in) This patch is essentially the same as before, except that the function name I have used this time is the one Wan-Teh previously suggested. Wan-Teh, please review.
Comment on attachment 340218 [details] [diff] [review] Add new function SEC_PKCS12AddCertOrChainAndKey (checked in) r=wtc. I no longer remember why I liked this name. It is still a little unclear, but ... > extern SECStatus >+SEC_PKCS12AddCertOrChainAndKey(SEC_PKCS12ExportContext *p12ctxt, >+ void *certSafe, void *certNestedDest, >+ CERTCertificate *cert, CERTCertDBHandle *certDb, >+ void *keySafe, void *keyNestedDest, PRBool shroudKey, >+ SECItem *pwitem, SECOidTag algorithm, >+ PRBool includeCertChain); >+ >+ Nit: align the parameters with the opening parenthesis. > /* SEC_PKCS12AddCertAndKey > * Add a certificate and key pair to be exported. "SEC_PKCS12AddCertAndKey" should be changed to the new function name.
(In reply to comment #7) I'm beginning to like SEC_PKCS12AddCertAndKeyEx better now. I'm afraid that in trying to avoid the Windowism "Ex", we may come up with function names that are wordier and less clear. Anyway, I leave the decision up to you :-)
Comment on attachment 340209 [details] [diff] [review] Remove dead functions (checked in) Dead functions removed. Checking in p12.h; new revision: 1.11; previous revision: 1.10 Checking in p12e.c; new revision: 1.19; previous revision: 1.18
Comment on attachment 340218 [details] [diff] [review] Add new function SEC_PKCS12AddCertOrChainAndKey (checked in) pkcs12/p12.h; new revision: 1.12; previous revision: 1.11 pkcs12/p12e.c; new revision: 1.20; previous revision: 1.19 smime/smime.def; new revision: 1.35; previous revision: 1.34