142868 - CA certificates are imported with NULL nicknames

Status: VERIFIED FIXED

(NSS :: Libraries, defect)

Description

[Julien Pierre]

As discussed in bug 136920, CERT_SaveImportCerts in 3.4 no longer generates a 
nickname for CA certificates when saving them into the database. As a result 
they are getting saved with NULL nicknames. This creates many application 
propblems. This is a regression from NSS 3.3 .

Comment 1

[Julien Pierre]

The quick fix for this would be to restore the old behavior and put the nickname 
generation code back into CERT_ImportCerts.

However, it can be argued that logically, we should really do this at a 
higher-level, independent of the token that the CA certificate is getting stored 
into. Unfortunately, the API to do that doesn't appear to be exposed, which is 
the subject of bug 142866. Ian has mentioned there is a function to import a 
certificate into an NSSToken, but that sounds like a Stan API rather than 3.x.

It is not clear that the two fixes are mutually exclusive because 
CERT_ImportCerts bypass the PKCS#11 softoken layer, and accesses the database 
directly. So it is unlikely that the code to generate the nickname would be 
called more than once, if we add the other token-independent import API.

My suggestion is just to roll back the old code into 3.4 . Bob, is there any 
reason not to do that ?

Comment 2

[Julien Pierre]

Attachment: patch that solves nickname generation for CA certificate import

This is a bit of a hack. I have to create a temporary certificate, then call
the old function CERT_MakeCANickname on it, then I destroy the temporary
certificate. It solves the problem, however.

Comment 3

[Julien Pierre]

FYI : the reason I did this at the PKCS#12 level rather than inside 
CERT_ImportCert is because I plan to switch to PK11_ImportCert. See bug 142889 .

Comment 4

[Julien Pierre]

Attachment: patch to generate nickname for CA certs in CERT_SaveImportedCerts

This patch restores the old NSS 3.3 behavior in the cert code. It only solves
the problem for importing CA certs to the cert db . This works as long as the
PKCS#11 layer keeps calling CERT_ImportCert .

Comment 5

[Julien Pierre]

Comment on attachment 82720 [details] [diff] [review]
patch that solves nickname generation for CA certificate import

forget about this one, too hackish.

Comment 6

[Julien Pierre]

I have attached patch  
8272 - http://bugzilla.mozilla.org/attachment.cgi?id=82726&action=view . This is 
just restoring the old code from 3.3 that creates the nickname during 
CERT_SaveImportedCert. It also solves the problem, and unlike the previous 
patch, does not interfere with any other patch I'm working on for the PKCS#12 
code.

Comment 7

[Robert Relyea]

In NSS 3.4 NONE of the code bypasses the token. CERT_ImportCert eventually calls
the STAN import cert code.

I think the correct thing to do is to set the nickname on
all the CA's. I think PSM needs to be tolerant of NULL nicknamed CA's and certs,
but NSS should to it best not to generate such things.

Right now I'm most worried about PSM saved certs, so catching the CERT_Save
functions seem to be the best way to deal with this. Pulling the original code
from 3.3 seems fine as long as we're careful not to pull direct to database
calls (which the patch does not.

Comment 8

[Julien Pierre]

Hmm. Never mind, this patch does not actually work. I had two patches in my tree 
and the other was causing it to work. The nickname in CERT_SaveImportedCerts is 
unused. So the 3.3 code rollback approach has to be ruled out, unless we can 
restore the nickname setting step somehow.

Comment 9

[Julien Pierre]

Comment on attachment 82726 [details] [diff] [review]
patch to generate nickname for CA certs in CERT_SaveImportedCerts

does not work - nickname at this level is unused

Comment 10

[Julien Pierre]

Attachment: third time is the charm

This one really works and does the nickname computation at the proper level, in
CERT_ImportCerts. It tries to emulate exactly the same behavior as before, by
computing and setting the nickname only for CA certs, and if no other nickname
is passed in. I have tested it successfully with no other patches needed.

Comment 11

[Julien Pierre]

Bob, could you please review this patch ?

Comment 12

[Robert Relyea]

Comment on attachment 82744 [details] [diff] [review]
third time is the charm

Looks good. The only thing I would change is freeNickname is redundant. you can
use canickname != NULL in the free case and remove the freeNickname altogether.

This is a style issue, not a functional issue.

r=relyea

Comment 13

[Julien Pierre]

Checked in to the tip (NSS 3.5).

Checking in certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.32; previous revision: 1.31
done

Comment 14

[Wan-Teh Chang]

Set target milestone to NSS 3.5.

Comment 15

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in
asap. thanks! 

Comment 16

[Charles Rosendahl]

.