Last Comment Bug 216117 - X509Certificate.findCertByNickName should store slot instance
: X509Certificate.findCertByNickName should store slot instance
Status: RESOLVED FIXED
:
Product: JSS
Classification: Components
Component: Library (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: 3.4.2
Assigned To: Wan-Teh Chang
: Jamie Nicolson
Mentors:
Depends on:
Blocks: 216918
  Show dependency treegraph
 
Reported: 2003-08-13 18:56 PDT by Julien Pierre
Modified: 2007-02-22 17:51 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Preliminary patch (incomplete) (7.31 KB, patch)
2003-08-15 17:15 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Preliminary patch (just enough to meet customer requirements) (19.48 KB, patch)
2003-08-18 18:15 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Proposed patch (42.71 KB, patch)
2003-08-20 19:16 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Proposed patch v1.1 (55.05 KB, patch)
2003-08-21 16:56 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Proposed patch v1.2 (51.03 KB, patch)
2003-09-24 15:15 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review

Description Julien Pierre 2003-08-13 18:56:57 PDT
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 2003-08-15 17:15:29 PDT
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.
Comment 2 Wan-Teh Chang 2003-08-18 18:15:33 PDT
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 3 Wan-Teh Chang 2003-08-18 18:18:12 PDT
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 2003-08-18 18:35:35 PDT
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 2003-08-19 13:25:55 PDT
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 2003-08-20 19:16:41 PDT
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.
Comment 7 Wan-Teh Chang 2003-08-21 16:56:05 PDT
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.
Comment 8 Wan-Teh Chang 2003-09-24 15:15:54 PDT
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.
Comment 9 Wan-Teh Chang 2003-09-24 15:21:19 PDT
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 2003-10-02 15:42:55 PDT
Taking bug.
Comment 11 Wan-Teh Chang 2007-02-22 17:51:21 PST
Marked the bug fixed.

Note You need to log in before you can comment on or make changes to this bug.