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

[Christina Fu]

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:
> 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
Yes, that was the result of copying and pasting existing code.  Switched 
to using String.split()and Hashtable in the new patch.
>
>
> 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 ?

The NSS call checks to see if &der exists.  Also, in this case, der was 
actually declared as
  SECItem der.

> 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());

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.
>
> How are we freeing the eccParams? I see the commented out section. Does it happen later or automatically somewhere?
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.

> 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; }

I'm not sure I understand... I do not see such existing functions in my 
tree.  Did I miss something?

>
> 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.
Could you have uncommented it for debugging?  I can comment it out again 
(done so in this updated patch)
>
> ----- 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