Just a few Comments/Questions ... GenerateKeyPairServlet.java Apparently StringTokenizer is discouraged in new code in favor of String.split. Could the following code be made a bit more efficient using some built in container? : // is the specified curve supported? 61 + boolean isSupportedCurve = false; 62 + for (int i=0; i<supportedECCurves.length; i++) { 63 + if (rKeycurve.equals(supportedECCurves[i])) { 64 + isSupportedCurve = true; 65 + } 66 + } RA_Enroll_Processor.cpp In this code: rv = ATOB_ConvertAsciiToItem (&der, pKey_ascii); 293 if (rv != SECSuccess){ 294 - RA::Debug(LL_PER_CONNECTION,FN, 295 - "failed to convert b64 private key to binary"); 296 - SECITEM_FreeItem(&der, PR_FALSE); 297 - status = STATUS_ERROR_MAC_ENROLL_PDU; 298 - PR_snprintf(audit_msg, 512, "ServerSideKeyGen: failed to convert b64 private key to binary"); 299 - goto loser; 300 - }else { If the call to ATOB_ConvertAsciiToItem fails, will there be a "der" variable to free with SECITEM_FreeItem ? Also, I'm curious why the change of the original code? Looks like we've added an extra step of processing as here: + Buffer *decodePubKey = Util::URLDecode(pKey); 287 + char *pKey_ascii = 288 + BTOA_DataToAscii(decodePubKey->getBuf(), decodePubKey->getLen()); How are we freeing the eccParams? I see the commented out section. Does it happen later or automatically somewhere? Buffer.cpp/Buffer.h Looks like we've added functions to the buffer that are already present. The length is gotten with the method "size()". TPS_PUBLIC unsigned int size() const { return len; } The raw buffer is obtainable with the operator "()". TPS_PUBLIC operator BYTE*() { return buf; } SecureChannel.cpp Looks like debug buffer calls like the following were previously commented out: RA::DebugBuffer("Secure_Channel::ComputeAPDUMac", "Data To MAC'ed", 1387 + &data); Was this done for a reason? I ask because we are actually now issuing these debug commands. ----- Original Message ----- From: "Christina Fu" <cfu(a)redhat.com&gt; To: "pki-devel" <pki-devel(a)redhat.com&gt; Sent: Tuesday, September 11, 2012 6:19:16 PM Subject: [Pki-devel] Review Request: Token Management System ECC infrastructure https://fedorahosted.org/pki/attachment/ticket/304/TPS-ECC.patch2 This patch provides TMS ECC infrastructure as described in task #304: https://fedorahosted.org/pki/ticket/304 I have merged/sanitized the code from two sources: * Token ECC enrollment with client-side key generation support (provided by jmagne(a)redhat.com) * TMS ECC enrollment with server-side key generation and key archival support (provided by myself - cfu(a)redhat.com) The following tests have been conducted: * ECC enrollment via tpsclient * RSA enrollment via tpsclient * RSA server-side key generation via tpsclient * ECC server-side key generation via tpsclient * ECC enrollment via smart card token (Safenet 330j) * RSA enrollment via smart card token (Safenet sc650) note 1: For ECC enrollments, you will need a newer java applet, which is not yet ready for checkin. note 2: server-side key generation is currently not yet supported by the smart card token because of the lack of the key injection code, which will be covered by task #235 (https://fedorahosted.org/pki/ticket/235) thanks, Christina _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

Conditional ACK: Just a couple of things. Look in Buffer.h for the existing buffer functions in favor of your new ones? And make the new HashTable private to the class. I wouldn't imagine anyone on the outside would want it. ----- Original Message ----- From: "Christina Fu" <cfu(a)redhat.com&gt; To: "John Magne" <jmagne(a)redhat.com&gt; Cc: "pki-devel" <pki-devel(a)redhat.com&gt; Sent: Friday, September 14, 2012 12:21:17 AM Subject: Re: [Pki-devel] Review Request: Token Management System ECC infrastructure https://fedorahosted.org/pki/attachment/ticket/304/TPS-ECC.forReview2 Thanks for the comments. A new patch has been uploaded. Please see in-line response for some of your other comments/questions. On 09/12/2012 05:41 PM, John Magne wrote: Yes, that was the result of copying and pasting existing code. Switched to using String.split()and Hashtable in the new patch. The NSS call checks to see if &der exists. Also, in this case, der was actually declared as SECItem der. I actually found this bug. We missed doing URL encoding/decoding before, and that happened to work. However, in case of ECC, I ran into issues. Doing URL Decoding and encoding is the right thing. The commented out section should be removed (I removed it in the updated patch) as it causes confusion. The eccParams was assigned to point to part of the pk_p: eccParams = &pk_p->u.ec.DEREncodedParams; where pk_p was gotten from SECKEY_ExtractPublicKey if it's server side key generation. The code in the patch states that in case of serverKeygen, SECKEY_DestroyPublicKey to free pk_p, where the eccParams would be freed automatically; while in the case of client side key generation, free() is used on pk_p, as the memory was allocated with an arena. I'm not sure I understand... I do not see such existing functions in my tree. Did I miss something? Could you have uncommented it for debugging? I can comment it out again (done so in this updated patch)