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.
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
Comment on attachment 122718 [details] [diff] [review] Add features Bob, Nelson, Could you please review this before I go on sabbatical ? Thanks.
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-
Created attachment 123200 [details] [diff] [review] update check key slot & cert slot
Attachment #122718 - Attachment is obsolete: true
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.
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
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
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
Created attachment 123362 [details] [diff] [review] get rid of -S since -o already does the job . document -o
Attachment #123345 - Attachment is obsolete: true
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
I checked the patch into the NSS tip yesterday. Marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
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+
You need to log in before you can comment on or make changes to this bug.