2:21 a.m.
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(a)redhat.com>
To: "pki-devel"<pki-devel(a)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(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