Re: (ITS#7359) [PATCH] MozNSS: prefer unlocked slot when getting private key

[hyc@symas.com]

jvcelak@redhat.com wrote:
> Full_Name: Jan Vcelak
> Version: git master
> OS: Linux
> URL: ftp://ftp.openldap.org/incoming/jvcelak-20120820-nss-prefer-unlocked-slot-private-key.patch
> Submission from: (NULL) (94.113.220.43)
> 
> 
> With last MozNSS patches for OpenLDAP, the library explicitly opens the
> certificate database when retrieving the certificates, even if the database is
> already opened. (Requried for safe certificate lookup from a nickname.) This
> might also require a re-authentication to a slot, which holds the private key.
> 
> Some application might expect that the slot with private key is already unlocked
> before passing the control to libldap. This got broken with the recent changes.

The quality of this code seems to be getting progressively worse. It seems
I've been accepting the last several patches without giving feedback though.

1) the recent patches do not adhere to the existing whitespace conventions.
Please fix this.

2) the code in this patch is unnecessarily clumsy:
+static SECKEYPrivateKey *
+tlsm_find_unlocked_key(tlsm_ctx *ctx, void *pin_arg)
+{
+   SECKEYPrivateKey *result = NULL;
+
+   PK11SlotList *slots = PK11_GetAllSlotsForCert(ctx->tc_certificate, NULL);
+   if (!slots) {
+       PRErrorCode errcode = PR_GetError();
+       Debug(LDAP_DEBUG_ANY,
+               "TLS: cannot get all slots for certificate '%s' (error %d: %s)",
+               tlsm_ctx_subject_name(ctx), errcode,
+               PR_ErrorToString(errcode, PR_LANGUAGE_I_DEFAULT));
+       return result;
+   }
+
+   PK11SlotListElement *le;
+   for (le = slots->head; le && !result; le = le->next) {
+       PK11SlotInfo *slot = le->slot;
+       if (!PK11_IsLoggedIn(slot, NULL))
+           continue;
+
+       result = PK11_FindKeyByDERCert(slot, ctx->tc_certificate, pin_arg);
+   }
+
+   PK11_FreeSlotList(slots);
+   return result;
+}

This should just be:

+   for (le = slots->head; le; le = le->next) {
+       PK11SlotInfo *slot = le->slot;
+       if (PK11_IsLoggedIn(slot, NULL)) {
+           result = PK11_FindKeyByDERCert(slot, ctx->tc_certificate,
pin_arg);
+           break;
+       }
+   }

> I'm attaching a patch which fixes it. If the certificate (and corresponding key)
> is held in multiple slots, libldap will take the key from an already
> authenticated slot.
> 
> 
> The attached file is derived from OpenLDAP Software. All of the modifications to
> OpenLDAP Software represented in the following patch(es) were developed by Red
> Hat. Red Hat has not assigned rights and/or interest in this work to any party.
> I, Jan Vcelak am authorized by Red Hat, my employer, to release this work under
> the following terms. 
> 
> Red Hat hereby place the following modifications to OpenLDAP Software (and only
> these modifications) into the public domain. Hence, these modifications may be
> freely used and/or redistributed for any purpose with or without attribution
> and/or other notice. 
> 
> 


-- 
  -- Howard Chu
  CTO, Symas Corp.           http://www.symas.com
  Director, Highland Sun     http://highlandsun.com/hyc/
  Chief Architect, OpenLDAP  http://www.openldap.org/project/