<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Please see my review comments below.<br>
<br>
Christina<br>
================<br>
<br>
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:<br>
a. It would be easier for people (like myself) to match the tps
tomcat code with the tps c++ code<br>
b. Now you have two "encryptData" functions in two separate files
-- one in util.java, and one in APDU.java. Could get confusing.<br>
<br>
2. The specialURLEncode() function you added in Util.java looks
almost identical to SpecialEncode() in TKSKnownSessionKey.java.<br>
How about filing a separate ticket for making
TKSKnownSessionKey.java call your version instead and remove the one
in TKSKnownSessionKey.java?<br>
<br>
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. <br>
<br>
4. for JNI function GetSymKeyByName().<br>
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.<br>
b. you don't need to do if (tokenName) again after you already
checked it right before:<br>
if(tokenName == NULL || sharedSecretKey == NULL ||
sessionKeyBA == NULL) {<br>
goto loser;<br>
}<br>
c. Also, same as above. if you file a ticket from #3 (moving
appropriate things into JSS), this should be considered.<br>
<br>
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.<br>
Same comment applies to wherever else that's appropriate.<br>
<br>
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?<br>
<br>
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?<br>
<br>
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.<br>
e.g. DeleteFileAPDU<br>
<br>
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.<br>
<br>
10. in getChannelBlockSize, would it possible to throw
EBaseException when you already have a default value? Same comment
with other similar getXXX() functions.<br>
<br>
<br>
<div class="moz-cite-prefix">On 04/08/2014 02:33 PM, John Magne
wrote:<br>
</div>
<blockquote
cite="mid:1014377896.1926714.1396992801502.JavaMail.zimbra@redhat.com"
type="cite">
<pre wrap="">Actually attach the patch this time...
----- Forwarded Message -----
From: "John Magne" <a class="moz-txt-link-rfc2396E" href="mailto:jmagne@redhat.com"><jmagne@redhat.com></a>
To: "pki-devel" <a class="moz-txt-link-rfc2396E" href="mailto:pki-devel@redhat.com"><pki-devel@redhat.com></a>
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.
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
Pki-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Pki-devel@redhat.com">Pki-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/pki-devel">https://www.redhat.com/mailman/listinfo/pki-devel</a></pre>
</blockquote>
<br>
</body>
</html>