Closed Bug 298955 Opened 15 years ago Closed 15 years ago

rsaperf won't authenticate to hardware tokens

Categories

(NSS :: Tools, defect, P2)

3.10
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julien.pierre, Assigned: julien.pierre)

Details

Attachments

(1 file)

When using rsaperf -n <token:nickname> , rsaperf uses CERT_FindCertByNickname,
which cannot authenticate to the token. The call simply needs to be replace with
PK11_FindCertFromNickname .
Attachment #187449 - Flags: superreview?(wtchang)
Attachment #187449 - Flags: review?(saul.edwards.bugs)
Targetting for 3.11 .
Priority: -- → P2
Target Milestone: --- → 3.11
Comment on attachment 187449 [details] [diff] [review]
use the proper API call

I don't know what's the differnece between these two
functions, so I'll deter the code review to Bob and
Nelson.

(Some JSS and PSM code calls both of them.)
Attachment #187449 - Flags: superreview?(wtchang)
Attachment #187449 - Flags: superreview?(rrelyea)
Attachment #187449 - Flags: review?(nelson)
Wan-Teh,

I think the differences become clear from the argument list. The CERT function
takes a trust domain and a token:nickname string . But since it doesn't have a
void* wincx, there is no way to pass authentication data.

The PK11 function on the other hand takes a token:nickname string and a void*
wincx - but no trust domain .
Julien, Will this patch cause users of rsaperf to experience authentication
requests in situations where it was not previously necessary?  
Bob, Nelson, Julien: if you know the difference between
CERT_FindCertByNickname and PK11_FindCertFromNickname,
please write a summary of the difference and how they
each should be used.  I'll convert the summary to an
NSS tech note or FAQ.  Thanks.
Nelson,

NSS will only request authentication to the tokens where needed, such as for a
search (traverse) . I believe PK11_FindCertFromNickname searches only the token
which matches the token:nickname string passed in . So, authentication should
only happen when necessary, in this case to find the cert. The patch fixes the
problem that a necessary authentication request wasn't happening with
CERT_FindCertByNickname. The API found the token unauthenticated, so it just
skipped it since it had no way to authenticate. Then it returned a NULL cert
because the token was not friendly.
Comment 4 above suggests to me that we have two functions, neither of which
is complete for all circumstances.  This line in comment 3
> (Some JSS and PSM code calls both of them.)
seems to prove that point.  

So, I suggest that we write a new public function that is complete, that is,
which has arguments to handle all the possible valid scenarios, and then
reimplement the other two functions as calls to that one new function.
IMO, that would be appropriate for 3.11.
Sigh,

CERT_FindCertByNickname calls PK11_FindCertFromNickname, but with a NULL wincx
(which means password functions that require a wincx value won't work).

CERT_FindCertByNickanme is a fairly old function (pre-pkcs11). It takes the old
'certdb' handle (which now points to a trust domain).

PK11_FindCertFromNickname originally went right to the token and ripped off the
cert from the token.

As part of the Stan work, both functions were changed.
PK11_FindCertFromNickname() is pretty much complete. It uses the default trust
domain (which is the only usable trust domain in the non-pure stan world),
appears to do all the proper work to make sure the cached version of the cert is
returned. It's confusing because it should be lower level function (one would
expect that they should call CERT_FindCertByNickname() as the higher level).

I agree we need a better high level call which could properly authenticate the
token if necessary....

NewAndImproved_FindCertByNickname(CERTHandle handle, const char *nickname, void
*pwarg)
{
    PK11SlotInfo *slot = PK11_GetSlotFromNickname(nickname, NULL);
    SECStatus rv = SECSuccess;

    if (slot == NULL) {
        return SECFAILURE;
    }

    rv = pk11_AuthenticateUnfriendly(slot, PR_TRUE, pwarg);
    if (rv != SECSuccess) {
        goto loser;
    }
    rv = CERT_FindCertByNickname(handle, nickname);

loser:
    PK11_FreeSlot(slot);
    return rv;
}

NOTE that PK11_GetSlotFromNickname() does not exist, but the code
to do so is replicated at least 3 times in pk11wrap, and probably elsewhere in
NSS and applications. The function would look something like this ..

PK11SlotInfo *
PK11_GetSlotFromNickname(const char *nickname, const char **local_nickname)
{
   char *tokenName;
   char *delimit;
   PK11SlotInfo *slot;

   if (local_nickname) {
       *local_nickname = nickname;
   }

  /* first find the slot associated with this nickname */
  if ((delimit = PORT_Strchr(nickname,':')) != NULL) {
      int len = delimit - nickname;
      tokenName = (char*)PORT_Alloc(len+1);
      PORT_Memcpy(tokenName,nickname,len);
      tokenName[len] = 0;

      slot = *slotptr = PK11_FindSlotByName(tokenName);
      PORT_Free(tokenName);
      /* if we couldn't find a slot, assume the nickname is an internal cert
       * with no proceding slot name */
      if (slot == NULL) {
              slot = *slotptr = PK11_GetInternalKeySlot();
      } else if (local_nickname) {
              *local_nickname = delimit+1;
      }
  } else {
      *slotptr = slot = PK11_GetInternalKeySlot();
  }

Anyway this sounds like a separate bug.

bob

Comment on attachment 187449 [details] [diff] [review]
use the proper API call

for rsaperf, this is an adequate fix. r=relyea
Attachment #187449 - Flags: superreview?(rrelyea) → superreview+
Bob,

Thanks for the review. I checked in this patch to the tip (for 3.11) .

Checking in rsaperf.c;
/cvsroot/mozilla/security/nss/cmd/rsaperf/rsaperf.c,v  <--  rsaperf.c
new revision: 1.17; previous revision: 1.16
done

I am also checking it in to NSS_PERFORMANCE_HACKS_BRANCH .

Checking in rsaperf.c;
/cvsroot/mozilla/security/nss/cmd/rsaperf/rsaperf.c,v  <--  rsaperf.c
new revision: 1.14.2.2; previous revision: 1.14.2.1
done

I agree the other issue you brought up with the current FindCert API
deficiencies should be a separate bug.

Marking this one fixed .
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #187449 - Flags: review?(saul.edwards.bugs)
Attachment #187449 - Flags: review?(nelson)
Bug 299291 has been created to address the multiple versions of
FindCert[By/From]Nickname.

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