Closed Bug 617492 Opened 9 years ago Closed 9 years ago
_Key Gen With Template function to pk11wrap (for Firefox Sync)
None of the existing PK11_KeyGen functions take a template and we need to pass template arguments for J-PAKE.
Assignee: nobody → bsmith
blocking2.0: --- → ?
Priority: -- → P1
Target Milestone: --- → 3.12.10
Besides J-PAKE, I think this function will be useful for other mechanisms that pk11wrap doesn't know about. Also, I think the other PK11_[Token]KeyGen* mechanisms can be rewritten to be based on this function to eliminate some duplicate code in the future.
Attachment #496092 - Attachment description: Patch to add PK11_KeyGenWithTemplate → Patch to add PK11_KeyGenWithTemplate (Trunk)
> Also, I think the other PK11_[Token]KeyGen* mechanisms can be rewritten > to be based on this function to eliminate some duplicate code in the future. PK11_[Token]KeyGen family already goes through a single function. I'd like to see how that single function can be merged with the 'KeyGenWithTemplate' code. bob
Here is my first approximation of that. I will run it through the tests when I get back to the office. Notice in particular the handling of symKey->type and symKey->size. This confusing logic regarding these two properties of the key is why I didn't attempt to modify pk11_TokenKeyGenWithFlagsAndKeyType in the initial patch. Also, the current version of pk11_TokenKeyGenWithFlagsAndKeyType does PK11_DoesMechanism(slot,type) but I think this should really be PK11_DoesMechanism(slot,mechanism.mechanism) since the point is to find a slot for the mechanism we actually will ask for.
Attachment #496206 - Flags: feedback?(rrelyea)
The properties are to support how applications typically want to deal with keys. Different algorithms have different characteristics. Some algorithms have fixed size keys, others have a choice of key sizes. Sometimes applications request a keys size, sometimes they just want the 'standard' key size. This is where all this code is hashed out (was well as dealing with the fact the differrent PKCS #11 mechanisms require a keylength, while others don't. bob
Bob, do you want to go with the original patch, the newest patch that modifies pk11_TokenKeyGenWithFlagsAndKeyType, or something else? If you can review at least the interface maybe I can ask Kai to review the implementation if you're too busy.
We need these patches reviewed and checked in immediately, even if that means getting on the phone and walking through the review and making changes in real time. We *must* get beta 8 out the door as soon as possible. We're taking too long.
Comment on attachment 496206 [details] [diff] [review] Patch demonstrating pk11_TokenKeyGenWithFlagsAndKeyType r+ with some small requests. in pk11_TokenKeyGenWithFlagsAndKeyType: rename mechanismType to keyGenType in PK11_KeyGenWithTemplate() rename type to keyGenType (General comments: The application typically sends mechanism types down like 'CKM_AES_CBC' which get turned into the appropriate keygen mechanism type (CMK_AES_GENERATE_KEY. The former is kept in the key structure to identify what type of key was generated. keySize is saved to faciliate the PK11_GetKeySize() call which tells how big the key is. If the key is sensitive, we can't tell by pulling it out of the token, so we work hard to keep any information we have about the key size.) Your code that sets keysize and type after is correct. bob
Attachment #496206 - Flags: feedback?(rrelyea) → feedback+
Comment on attachment 496206 [details] [diff] [review] Patch demonstrating pk11_TokenKeyGenWithFlagsAndKeyType r+ for this patch. This will be the one to land. (per my phone call with brian). One minor change, the PK11_KeyGenWithTemplate will take 2 Mechanism parameters, type and keyGenType. The symkey will be set type, and the GetBestSlot will use type.
Attachment #496206 - Flags: review+
One this patch lands, Brian will tag the release NSS_3_12_9_BETA2_CANDIDATE. Mozilla can use that for ff 4.0 beta8. Brian should have the juice now to do this. One we get release eng builds, we will release NSS_3_12_9_BETA2. This may be a while since key release eng machines are in the process of moving (which is why we were trying to get the code in last weekend;). At that point we can start QA for NSS_3_12_9_RTM. There should only critical bug fixes between NSS_3_12_9_BETA2_CANDIDATE and NSS_3_12_9_RTM. bob
Comment on attachment 496206 [details] [diff] [review] Patch demonstrating pk11_TokenKeyGenWithFlagsAndKeyType In lib/pk11wrap/pk11pub.h: > PK11SymKey *PK11_KeyGen(PK11SlotInfo *slot,CK_MECHANISM_TYPE type, > SECItem *param, int keySize,void *wincx); >+/* Generates a key using the exact template supplied by the caller. The other >+ * PK11_[Token]KeyGen mechanisms should be used instead of this one whenever >+ * they work because they include/exclude the CKA_VALUE_LEN template value >+ * based on the mechanism type as required by many tokens. >+ */ >+PK11SymKey *PK11_KeyGenWithTemplate(PK11SlotInfo *slot, CK_MECHANISM_TYPE type, >+ SECItem *param, CK_ATTRIBUTE * attrs, >+ unsigned attrsCount, void *wincx); > PK11SymKey *PK11_TokenKeyGen(PK11SlotInfo *slot, CK_MECHANISM_TYPE type, > SECItem *param, int keySize, SECItem *keyid, > PRBool isToken, void *wincx); > PK11SymKey *PK11_TokenKeyGenWithFlags(PK11SlotInfo *slot, > CK_MECHANISM_TYPE type, SECItem *param, > int keySize, SECItem *keyid, CK_FLAGS opFlags, > PK11AttrFlags attrFlags, void *wincx); Please list PK11_KeyGenWithTemplate after PK11_TokenKeyGen and PK11_TokenKeyGenWithFlags. Please use 'unsigned int' instead of 'unsigned' as the type of attrsCount.
This version of the patch incorporates the changes that address the reviews from Robert and Wan-Teh. Thank you for reviewing them. Tonight I was busy working on bug 601645 and I did not get this landed. I am not going to land it now because I will not be awake to watch the tinderboxes. Maybe Kai can land it overnight since it is daytime in Europe now. Otherwise, I will land this in the morning Pacific time.
The latest patch adds "PK11_FindCertsFromEmailAddress" as an exported function from 3.12.10, but that function is already exported in version 3.12.9 I don't see that function mentioned in this bug, I conclude it's a merging mistake?
this is v4 = (v3 - 3.12.10 export section) I'll land this patch to trunk and branch.
trunk commit: Checking in lib/nss/nss.def; /cvsroot/mozilla/security/nss/lib/nss/nss.def,v <-- nss.def new revision: 1.212; previous revision: 1.211 done Checking in lib/pk11wrap/pk11pub.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v <-- pk11pub.h new revision: 1.35; previous revision: 1.34 done Checking in lib/pk11wrap/pk11skey.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v <-- pk11skey.c new revision: 1.121; previous revision: 1.120 done 3.12 branch checkin: Checking in lib/nss/nss.def; /cvsroot/mozilla/security/nss/lib/nss/nss.def,v <-- nss.def new revision: 220.127.116.11; previous revision: 18.104.22.168 done Checking in lib/pk11wrap/pk11pub.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v <-- pk11pub.h new revision: 22.214.171.124; previous revision: 126.96.36.199 done Checking in lib/pk11wrap/pk11skey.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v <-- pk11skey.c new revision: 188.8.131.52; previous revision: 184.108.40.206 done
Leaving bug open until people confirm the patch I've landed for the urgent beta was correct.
Confirmed! Thanks, Kai!
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.