[Pki-devel] Review Request: Token Management System ECC infrastructure

[John Magne]

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 at redhat.com>
To: "pki-devel" <pki-devel at redhat.com>
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 at redhat.com)
* TMS ECC enrollment with server-side key generation and key archival 
support (provided by myself - cfu at 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 at redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel