Closed Bug 617492 Opened 9 years ago Closed 9 years ago

Add PK11_KeyGenWithTemplate function to pk11wrap (for Firefox Sync)

Categories

(NSS :: Libraries, defect, P1, blocker)

3.12.10
defect

Tracking

(blocking2.0 beta8+)

RESOLVED FIXED
3.12.9
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file, 4 obsolete files)

None of the existing PK11_KeyGen functions take a template and we need to pass template arguments for J-PAKE.
Assignee: nobody → bsmith
Blocks: 599324
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 - Flags: review?(rrelyea)
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.
Target Milestone: 3.12.10 → 3.12.9
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.
Attachment #496092 - Attachment is obsolete: true
Attachment #496093 - Attachment is obsolete: true
Attachment #496206 - Attachment is obsolete: true
Attachment #496092 - Flags: review?(rrelyea)
Attachment #496093 - Flags: review?(rrelyea)
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: 1.206.2.5; previous revision: 1.206.2.4
done
Checking in lib/pk11wrap/pk11pub.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v  <--  pk11pub.h
new revision: 1.32.2.3; previous revision: 1.32.2.2
done
Checking in lib/pk11wrap/pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.119.2.2; previous revision: 1.119.2.1
done
Attachment #496446 - Attachment is obsolete: true
Attachment #496464 - Attachment description: Patch v4 → Patch v4 [checked in]
Attachment #496464 - Attachment is obsolete: true
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
blocking2.0: ? → beta8+
Attachment #496464 - Attachment is obsolete: false
You need to log in before you can comment on or make changes to this bug.