505559 - Need function to identify the one and only default internal private key slot.

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Robert Relyea]

NSS now has the ability to open more than one database in a given instance. This can happen either through hand tweaking the configuration strings for the database, or by calling the special function SECMOD_OpenUserDB.

This creates new NSS databases which look and act exactly like the main NSS database, including returning PR_TRUE for the call PK11_IsInternal(). Sometimes, though, we want to know not only is this *an* internal token, but is it *the* internal token returned from PK11_GetPrivateKeySlot(). In particular NSS uses this call to determine if it needs to add a slot prefix to the nickname (if no slot prefix is supplied, NSS assumes the nickname is a nickname in the slot returned from PK11_GetPrivateKeySlot().

In addition, NSS needs to use this test rather than the existing PK11_IsInternal() test when making this kind of decision.

This bug exists in the current code, but isn't prevalent because the current usage of multiple databases by NSS applications is low.

bob

Comment 1

[Robert Relyea]

Attachment: Implement PK11_IsPrivateKeySlot()

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 389785 [details] [diff] [review]
Implement PK11_IsPrivateKeySlot()

Preliminary review comments:
This patch changes 4 files.  

The changes to the first 3 files are a set, and must be taken all together.  
They seem fine. I have just one comment.  This patch declares the new function 
to be part of 3.12.5, but 3.12.4 has not been released (RTM) yet, and I would 
be happy to see this go into 3.12.4.  If you decide to make this part of a 3.12.5
API, you should not commit it until after 3.12.4 finally RTMs.  r+ for those 3 changes.  
I assume you'll do the right thing with the version number.

The change to the 4th file can be seen as a separate patch, dependent on the previous
3 changes, but not required to be committed with them.  I would like you to explain
why you want/need to make these changes in pki3hack.c.  What problem do they solve?
If these functions are called with an instance that is in the "generic" slot and not
in the DB slot, the old code and the new will treat it differently.  It's not 
immediately clear that the new code is correct for that case, or that the old code 
was wrong for that case, but I'd be happy to be convinced.  :)

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 389785 [details] [diff] [review]
Implement PK11_IsPrivateKeySlot()

I think maybe Bob didn't notice that I asked questions about the patch.  So, I'll give the patch r-.  Bob, when you've answered my questions, set it back to r?

Comment 4

[Robert Relyea]

re: 3.12.4: I was planning on pushing these to 3.12.5 to reduce risk, but I'll be happy to pull them into 3.12.4. I do want them for fedora, but I don't need the functions made public initially.

re pki3hack.c changes.: This is the initial bug that made me realize I needed the new functions. Without these changes, new database slots that are added will not display the correct nickname. Applications that use those nicknames will not then be able to find the given certs in those databases. This wasn't notices initially since the few apps that use SECMOD_OpenUserDB() access certs without the nicknames. I noticed it when I started making changes that caused certutil to open one of these new databases and listing the results.

bob

Comment 5

[Robert Relyea]

(+18/0) Lines changed.
When 	Who 	File 	Rev 	+/- 	Description
2009-07-29 17:37	rrelyea%redhat.com 	mozilla/security/nss/lib/nss/nss.def 	1.200 	1/0  	
Bug 505559 - Need function to identify the one and only default internal private key slot.
r=nelsonb
2009-07-29 17:37	rrelyea%redhat.com 	mozilla/security/nss/lib/pk11wrap/pk11pub.h 	1.30 	1/0 
2009-07-29 17:37	rrelyea%redhat.com 	mozilla/security/nss/lib/pk11wrap/pk11slot.c 	1.99 	16/0 


check in of the 3 approved files.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #4)
> re pki3hack.c changes.: This is the initial bug that made me realize I needed
> the new functions. Without these changes, new database slots that are added
> will not display the correct nickname. 

Nickname?  Do you mean token name?  or slot name?  
or the token name that NSS puts into cert nicknames?

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

oh!? 

Is it the case that these other softoken slots which are created by calls to
SECMOD_OpenUserDB cause 
PK11_IsInternal        to answer PR_TRUE, but cause 
PK11_IsInternalKeySlot to answer PR_FALSE?

If so, then I understand the last part of this patch.

Comment 8

[Robert Relyea]

> Nickname?  Do you mean token name?  or slot name?  
> or the token name that NSS puts into cert nicknames?

I mean full cert nicknames. For the private key slot, those are just bare CKA_LABELs for the certificate. For all other slots it's tokenname:CKA_LABEL. The new database slots should have certs with the former label, or when the cert is looked up by nickname, it won't be found.

> Is it the case that these other softoken slots which are created by calls to
> SECMOD_OpenUserDB cause 
> PK11_IsInternal        to answer PR_TRUE, but cause 
> PK11_IsInternalKeySlot to answer PR_FALSE?

Yes, exactly.

bob

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 389785 [details] [diff] [review]
Implement PK11_IsPrivateKeySlot()

r+=nelson
Thanks for the explanations.

Comment 10

[Robert Relyea]

Checking in pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.97; previous revision: 1.96
done