Re: [Pki-devel] Fwd: [pki-devel][PATCH] 0008-Further-progress-Format-operation.patch

Please see my review comments below. Christina ================ 1. In APDU.java I'd prefer you kept the old name "secureMessage" instead of "encryptData" for the function that calls Util.encryptData() for two reasons: a. It would be easier for people (like myself) to match the tps tomcat code with the tps c++ code b. Now you have two "encryptData" functions in two separate files -- one in util.java, and one in APDU.java. Could get confusing. 2. The specialURLEncode() function you added in Util.java looks almost identical to SpecialEncode() in TKSKnownSessionKey.java. How about filing a separate ticket for making TKSKnownSessionKey.java call your version instead and remove the one in TKSKnownSessionKey.java? 3. for JNI function UnwrapSessionKeyWithSharedSecret, just wondering, in JSS, there is a native function called nativeWrapSymWithSym, which is mapped from pkcs11/PK11KeyWrapper.java:wrap(). have you looked into if that would have been utilized? And if not, would your method be something to be made generic and placed into jss? I am not sure if we already have a ticket for this, but perhaps file one to put all the functions from symkey that fit to be generic JSS functions to be moved into JSS instead. 4. for JNI function GetSymKeyByName(). a. I think it's customary to initialize slot to be PK11_GetInternalKeySlot(). So in the case of tokenName not supplied by the caller, the function will at least try looking into internal token by default. And if you must require tokenName, you should have checked it at the very beginning along with other required param such as keyName. b. you don't need to do if (tokenName) again after you already checked it right before: if(tokenName == NULL || sharedSecretKey == NULL || sessionKeyBA == NULL) { goto loser; } c. Also, same as above. if you file a ticket from #3 (moving appropriate things into JSS), this should be considered. 5. In ReturnSymKey(), you added some printf for debugging I presume. since this is JNI C code, one should be careful on not making attempt to print something that could turn out to be null. I suggest you add check to null before trying to print. Same comment applies to wherever else that's appropriate. 6. CreateDesKey24Bit is moved from RA::CreateDesKey24Byte()? I believe the DES key is 24 "bytes" not "bits". You probably want to name it back to CreateDesKey24Byte? 7. In SecureChannel.java, I'm not sure I understand the value of the constructor that does not take any parameters if you bomb them out if any of the class variables are not set. If one calls new SecureChannel() without any parameters how would those variables be set if it just bombs you out right away? 8. For those APDU classes under common/src/org/dogtagpki/tps/apdu/, I'd prefer you kept the APDU in the names of those APDU classes/files.. It would have made it much easier for the readers. e.g. DeleteFileAPDU 9. in format(), the tksConnId is just temporarily hard coded to tks1 right? You might wan to add a TODO comment there to retrieve from profile in config. 10. in getChannelBlockSize, would it possible to throw EBaseException when you already have a default value? Same comment with other similar getXXX() functions. On 04/08/2014 02:33 PM, John Magne wrote: