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

[John Magne]

Cfu gave verbal ack after addressing comments, which has been done.
Verbal ack from edewata.

Pushed to master.



----- Original Message -----
> From: "Christina Fu" <cfu at redhat.com>
> To: pki-devel at redhat.com
> Sent: Friday, April 11, 2014 11:22:57 AM
> Subject: 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:
> 
> 
> 
> Actually attach the patch this time...
> 
> ----- Forwarded Message -----
> From: "John Magne" <jmagne at redhat.com> To: "pki-devel" <pki-devel at redhat.com>
> Sent: Tuesday, April 8, 2014 2:30:05 PM
> Subject: [pki-devel][PATCH] 0008-Further-progress-Format-operation.patch
> 
> Patch accomplishes the following:
> 
> 1. Read applet into memory to prepare to write to token.
> 2. With tpsclient create secure channel by implementing Initialize Update and
> ExternalAuthenticate messages.
> 3. Support for MAC and encryption for messages going on after secure channel
> has been created.
> 4. Implemented method to remove an aid file or instance from the token.
> 5. Added some symkey methods to allow TPS to manipulate session keys.
> 
> Have not tried this with real token as of yet. The tpsclient does verify of
> the MAC coming from the server and decrypts encrypted messages. Decrypted
> messages have to be correct for the MAC verification to work.
> Next step will be to add the phone home servlet to the TPS and give it a try
> with a real token and esc.
> 
> 
> _______________________________________________
> Pki-devel mailing list Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
> 
> 
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel