311483 - exposing includeCertChain as a parameter to SEC_PKCS12AddCertAndKey

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P3)

Description

[Pavel Pavlov]

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

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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.  

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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.  

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Implement new function, delete several dead functions

Here's my first attempt to implement this RFE as an addition to 3.11.

Comment 6

[Wan-Teh Chang]

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.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

(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! :-)

Comment 8

[Wan-Teh Chang]

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 9

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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 11

[Wan-Teh Chang]

Comment on attachment 340209 [details] [diff] [review]
Remove dead functions (checked in)

r=wtc.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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 13

[Wan-Teh Chang]

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.

Comment 14

[Wan-Teh Chang]

(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 15

[Nelson Bolyard (seldom reads bugmail)]

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 16

[Nelson Bolyard (seldom reads bugmail)]

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