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

[Christina Fu]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140411/cca4082b/attachment.htm>