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 .
Created attachment 129889 [details] [diff] [review] 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.
Created attachment 130035 [details] [diff] [review] 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 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.
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 on attachment 130035 [details] [diff] [review] Preliminary patch (just enough to meet customer requirements) Patch 130035 looks good. r=nicolson
Created attachment 130150 [details] [diff] [review] 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.
Created attachment 130203 [details] [diff] [review] 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.
Created attachment 132096 [details] [diff] [review] Proposed patch v1.2 In this version of the patch, I handle the possibility that the slot pointer of a temporary cert is null.
Patch v1.2 has been checked into the JSS tip. We should have JSS 3.4.2 Beta 1 available for testing on Friday.
Marked the bug fixed.