strsclnt should be able to reuse token after it is unplugged and plugged back in

RESOLVED FIXED in 3.9

Status

NSS
Tools
P2
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

15 years ago
When using strsclnt to login to a server that requests client auth and
unplugging the token that contains the client cert after the beginning of the
test, strsclnt reports an error that the token has been removed. So far so good.

However, if the token is plugged back in, the application does not try to log
back in to the token and use the cert on it again. It just gives up. Further
handshakes just don't do the client auth.

In the interest of stress testing PKCS#11 driver code, it is valuable to have a
test program that has the ability to recover in these unplug/plug back
situations. I nominate strsclnt as this test program.
(Assignee)

Comment 1

15 years ago
Created attachment 122718 [details] [diff] [review]
Add features

- add code to support logging back to the token after it is removed. If the
token isn't reinserted within a minute, strsclnt will exit

- put back the code to do automatic cert selection . I tested it with a
hardware token

- add -S option to skip server cert verification . This is useful when using
servers with test certs that aren't trusted
(Assignee)

Comment 2

15 years ago
Comment on attachment 122718 [details] [diff] [review]
Add features

Bob, Nelson,

Could you please review this before I go on sabbatical ? Thanks.
Attachment #122718 - Flags: superreview?(relyea)
Attachment #122718 - Flags: review?(nelsonb)
(Assignee)

Updated

15 years ago
Priority: -- → P2
Target Milestone: --- → 3.9
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED

Comment 3

15 years ago
Comment on attachment 122718 [details] [diff] [review]
Add features

Minor nit: LoggedIn should verify that cert->slot is not NULL. It may be
possible to have a cert structure without a slot. I think it is probably
sufficient to just test of the key slot is logged in.

Other note: The reason for having separate RSA and Fortezza cert slots is
because fortezza ciphers must have fortezza certs when doing client auth. Since
this is clearly broken in the root tree, and I doubt we've actually tested a
Fortezza cipher since the turn of the century (OK it sounds longer than it
is;), removing the effectively dead code should be OK.
Attachment #122718 - Flags: superreview?(relyea) → superreview-
(Assignee)

Comment 4

15 years ago
Created attachment 123200 [details] [diff] [review]
update

check key slot & cert slot
Attachment #122718 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #123200 - Flags: superreview?(relyea)
Attachment #123200 - Flags: review?(nelsonb)
Bob, regarding the separate -n and -f options, There are numerous SSL/TLS
ciphersuites that need specific types of certs to work.  In addition to RSA
certs and fortezza certs, there are DSA certs and EC certs.  Rather than 
having a separate option letter for each one, I think it is sufficient to 
either:
a) just test with one type at a time and let -n work for all types, or 
b) allow -n to be specified more than once, and 
c) determine the cert type by examining the cert rather than by the option.
Bob, regarding the LoggedIn function, is there any valid circumstance _in_
_strsclnt_ in which the cert slot and the key slot can be different? 
If not, I think this code should assert that they are the same.

Comment 7

15 years ago
Actually it is possible. If you have the cert and key in the internal token, and
just the cert in the smart card, then the cert->slot would be in the smart card
and the key->slot would be in the internal token.

For this particular case, I believe we really only need to make sure the key's
slot is logged in. At this point it doesn't matter if the cert's slot is logged
in or not since we already have a copy of the cert in our cache.

bob

Comment 8

15 years ago
re comment 5 I think Julien's code is fine the way it is. In general the client
auth cert is independent of the key exchange used. The only case where this was
not the case was Fortezza, and now that I think about it, it was only Fortezza
V1 (Fortezza V2 separated the signing from encryption). Since I doubt there
exists a Fortezza V1 cert today that isn't expired, I don't think we really need
to worry about that case. It's perfectly valid to sign an RSA key exchange with
a DSA signature cert.

Allowing multiple -n's sounds like a nice RFE, though.

bob
(Assignee)

Comment 9

15 years ago
Created attachment 123345 [details] [diff] [review]
incorporate feedback from Nelson

- indent cert_and_key structure definition
- convert new functions to K&R style for consistency
- reorder parser options
Attachment #123200 - Attachment is obsolete: true
(Assignee)

Comment 10

15 years ago
Created attachment 123362 [details] [diff] [review]
get rid of -S since -o already does the job . document -o
Attachment #123345 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #123362 - Flags: review?(nelsonb)
(Assignee)

Comment 11

15 years ago
Created attachment 123365 [details] [diff] [review]
patch v5

- Make automatic cert selection really work
- Make "no cert" selection really mode through -n none
- remove info about fortezza in usage screen
Attachment #123362 - Attachment is obsolete: true
reassigning to Jullien, since he's doing all the work.

Julien, your patch v5 adds a line to the Usage message that explains the 
-S option, even though you've removed the -S option from the patch.
If you remove that line and put the command back on the line before it, then
you can check this patch in.

There is one other change I'd like to see made, but it could be made after
this is checked in.  The new GetClientAuthData function has two large 
sections, the second of which seems to duplicate a large part of the library
function NSS_GetClientAuthData.  I'd prefer to see this new function call
NSS_GetClientAuthData than duplicate it.  

Assignee: wtc → jpierre
Status: ASSIGNED → NEW
(Assignee)

Comment 13

15 years ago
I checked the patch into the NSS tip yesterday. Marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 14

15 years ago
Comment on attachment 123200 [details] [diff] [review]
update

My comments indicate I've already reviewed and approved the patch, cleaning up
the database.
Attachment #123200 - Flags: superreview?(rrelyea0264) → superreview+
(Assignee)

Updated

15 years ago
Attachment #123362 - Flags: review?(MisterSSL)
(Assignee)

Updated

15 years ago
Attachment #123200 - Flags: review?(MisterSSL)
(Assignee)

Updated

15 years ago
Attachment #122718 - Flags: review?(MisterSSL)
You need to log in before you can comment on or make changes to this bug.