216117 - X509Certificate.findCertByNickName should store slot instance

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect)

Description

[Julien Pierre]

As discussed in bug 74822, CERTCertificate only has one slot pointer, even
though the certificate may exist in multiple instances.

When this is the case, NSS has to make a decision for the "default" slot in the
CERTCertificate. This slot may not match the slot requested in the
"token:nickname" input string.

If the application wants to reference a specific instance of the certificate in
a particular token, for example to find the private key, it can maintain its own
separate slot pointer that matches the string, by using the function
PK11_FindSlotByName . Extensive C++ code samples were provided in
http://bugzilla.mozilla.org/show_bug.cgi?id=74822#c22 . They should be easily
adapted to JSS .

In summary, the change is to :
1. extract the token name from the input string
2. convert that token name to a PK11SlotInfo using PK11_FindSlotByNAme
3. save this slot pointer inside X509Certificate
4. when doing further operations on that certificate, reference
X509Certificate.slot instead of X509Certificate.CERTCertificate.slot .

Comment 1

[Wan-Teh Chang]

Attachment: Preliminary patch (incomplete)

This patch adds a tokenProxy member to the PK11Cert class.
We store the slot pointer in tokenProxy.  Right now the
slot pointer is always cert->slot, so there is no behavior
change yet.

I'd like to submit the changes I have so far for review to
make sure I am going in the right direction.

Comment 2

[Wan-Teh Chang]

Attachment: Preliminary patch (just enough to meet customer requirements)

This patch implements just enough to meet our customer's
requirements.  I will ask the customer to test it.

To complete the work, I'll need to review all the current
calls to JSS_PK11_wrapCert and JSS_PK11_getCertPtr and
see if they should be replaced by JSS_PK11_wrapCertAndSlot
and JSS_PK11_getCertSlotPtr.

Comment 3

[Wan-Teh Chang]

Comment on attachment 130035 [details] [diff] [review]
Preliminary patch (just enough to meet customer requirements)

Jamie, please review this patch.  I can sit down with you
to explain this patch to you tomorrow morning.

Comment 4

[Julien Pierre]

Wan-Teh,

I briefly reviewed your patch.

Two comments :

1. It's not necessary to call PK11_FindCertFromNickname and fall back to
CERT_FindCertFromNickname if it fails. I believe this is old code from pre-NSS
3.4 . Nowadays onlthe first call is necessary.

2. In JSS_PK11_findCertAndSlotFromNickname, it might be useful to add an extra
check after calling PK11_FindSlotByName to ensure that the cert really exists on
that slot. You would call PK11_FindCertInSlot against the cert and slot you just
got from the string.

The reason for that is that there may be two tokens with the same name but the
cert you are looking for only exists in one. In that case, PK11_FindSlotByName
may not return the correct slot pointer.

To truly fix this 100%, we would need to expose the CERTCertificate's slot
instances, perhaps in the way proposed by Ian McGreer ...

Comment 5

[Jamie Nicolson]

Comment on attachment 130035 [details] [diff] [review]
Preliminary patch (just enough to meet customer requirements)

Patch 130035 looks good.
r=nicolson

Comment 6

[Wan-Teh Chang]

Attachment: Proposed patch

This is the full patch.  I reviewed every place in JSS where we
create a PK11Cert object.  Where it is important to have the
correct slot or the slot info is readily available, I store the
slot in the PK11Cert object.

Julien, Jamie, please review this patch.  Thanks.

Comment 7

[Wan-Teh Chang]

Attachment: Proposed patch v1.1

I decided not to fix the getCerts function in PK11Finder.c
in the bug.  Until that is fixed (bug 216918),
CryptoManager.getCACerts and CryptoManager.getPermCerts
will continue to return certs that may not have the correct
slot pointers.	Thomas, I hope this is OK.

I also changed all PK11_FindKeyByAnyCert calls in JSS to
PK11_FindPrivateKeyFromCert, which allows you to specify
the slot.

Comment 8

[Wan-Teh Chang]

Attachment: Proposed patch v1.2

In this version of the patch, I handle the possibility
that the slot pointer of a temporary cert is null.

Comment 9

[Wan-Teh Chang]

Patch v1.2 has been checked into the JSS tip.  We
should have JSS 3.4.2 Beta 1 available for testing
on Friday.

Comment 10

[Wan-Teh Chang]

Taking bug.

Comment 11

[Wan-Teh Chang]

Marked the bug fixed.