The default bug view has changed. See this FAQ.

exposing includeCertChain as a parameter to SEC_PKCS12AddCertAndKey

RESOLVED FIXED in 3.12.2

Status

NSS
Libraries
P3
enhancement
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Pavel Pavlov, Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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
(Reporter)

Updated

12 years ago
Blocks: 311486

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

12 years ago
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?
Priority: -- → P3
Hardware: PC → All
Version: unspecified → 3.10
(Assignee)

Comment 2

12 years ago
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.
(Assignee)

Comment 3

12 years ago
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.  
(Assignee)

Comment 4

12 years ago
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.  
(Assignee)

Comment 5

12 years ago
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.
Attachment #199695 - Flags: review?(wtchang)

Comment 6

12 years ago
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.
Attachment #199695 - Flags: review?(wtchang) → review+
(Assignee)

Comment 7

12 years ago
(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! :-)
Assignee: wtchang → nelson

Comment 8

12 years ago
I like SEC_PKCS12AddCertOrChainAndKey.  If
we add a function that takes an includeCertChain
parameter, we don't need to add a
SEC_PKCS12AddCertAndKeyWithoutChain function.
(Assignee)

Comment 9

11 years ago
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.
Attachment #199695 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
QA Contact: jason.m.reid → libraries
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.
Attachment #340209 - Flags: review?(wtc)

Comment 11

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

r=wtc.
Attachment #340209 - Flags: review?(wtc) → review+
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.
Attachment #340218 - Flags: review?(wtc)

Comment 13

9 years ago
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.
Attachment #340218 - Flags: review?(wtc) → review+

Comment 14

9 years ago
(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
Attachment #340209 - Attachment description: Remove dead functions → Remove dead functions (checked in)
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
Attachment #340218 - Attachment description: Add new function SEC_PKCS12AddCertOrChainAndKey → Add new function SEC_PKCS12AddCertOrChainAndKey (checked in)
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12.2
You need to log in before you can comment on or make changes to this bug.