<html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> First of all, this is pretty good stuff to get it working. Looks pretty good for first cut. I'm giving you conditional ack pending addressing key issues below. Christina ========= APDU.java *I don't see getCalculateSingleDesBeforeMac() or setCalculateSingleDesBeforeMac() being used. calculateSingleDesBeforeMac seems to be hard-coded in every APDU, and I am not seeing it used anywhere. * it seems the only difference between the existing secureMessage() and your secureMessageSCP02() is very miner: if (rem == 0) { + + padNeeded = 8; instead of padNeeded = 0; Couldn't this be handled by modifying the existing function? DeleteFileGP211APDU.java * seems to be identical with DeleteFileAPDU.java, with the exception of the unused variable mentioned above: + calculateSingleDesBeforeMac = true; but maybe it's not a bad thing to have separate APDU files. ExternalAuthenticateAPDUGP211.java * did you intentionally leave out CMAC_RMAC in byteToSecurityLevel()? * Please provide comment to new functions describing at least what the params are for Util.java * computeEncEcbDes() You should add a // TODO comment there just to be sure we don't ship it with secret keys printed to the debug log. + TPSBuffer desDebug = new TPSBuffer(des.getEncoded()); + + CMS.debug("des key debug bytes: " + desDebug.toHexString()); + TokenServlet.java * processComputeSessionKeySCP02() is a copy of processComputeSessionKey() with mods: I am not in favor of making a copy of an existing function and make changes to it. It is very difficult to look through it to visually find diffs. Also, if we ever need to fix anything it needs to go to multiple places. This seems to be the practice in this patch. * I am not in favor of reformatting the entire existing file when you modify it. It makes it really difficult to sift through for the real changes. If you really wish to reformat it, try to make two separate patches just to save your reviewer's time. (I did what I could visually hunting for diffs due to this, it is possible I missed review something..) DeriveDESKeyFrom3DesKey * the caller to DeriveDESKeyFrom3DesKey (Util.java:computeMACdes3des()) seems to hard code CKM_DES_CBC, where do you expect one to change the alg? or is this for future? SymKey.java * why does this added line do? + transportKey = ReturnSymKey( slot, GetSharedSecretKeyName(NULL)); * why can't you just use CreateUnWrappedSymKeyOnToken() and you have to write UnwrapWrappedSymKeyOnToken()? They look awfully similar. <div class="moz-cite-prefix">On 02/23/2015 10:36 PM, John Magne wrote: </div> <blockquote cite="mid:1842762544.22651306.1424759810285.JavaMail.zimbra@redhat.com" type="cite"> <pre wrap="">Subject: [PATCH] Ticket: TPS Rewrite: Implement Secure Channel Protocol 02 (#883). First cut of gp211 and secure channel protocol 02 for tokens. Allow token operations using a GP211 token over secure channel protocol 02. This patch supports the following: 1. Token operations with a GP211 card and SCP02 protocol, implementation 15. 2. Token still supports GP201 cards with SCP01. 3. SCP02 tested with SC650 gp211/scp02 card. 4. Formatting ,e nrollment, and pin reset tested to work. 5. Symmetric key changeover upgrade and downgrade tested to work. 6. APDU security methods of CMAC and Encryption supported, as well as the combination of the two. Things still to do: 1. Right now the SCP02 support has been tested with the current gp201 applet and enrollment and formatting works just fine. We need to modify and compile the applet against the GP211 spec and retest to see if any further changes are needed. 2. The nistSP800 key derivation stuff is not completed for the SCP02 protocol. Some of the routines are self contained vs similar SCP01 ones. We have another ticket to complete the nistSP800 support from end to end. This work will be done for that ticket. 3. One of the new scp02 deriviation functions can make use of a new NSS derive mechanism. As of now this work is done by simple encryption, this can be done later. 4. The security APDU level of "RMAC" is not supported because the card does not support it. It could have been done to the spec, but it having the card to test is more convenient and there were more crucial issues to this point. 5. Right now the security apdu level is set hard coded to the max supported level for scp01 and 02. Need to make that more configurable, although there is no reason to downgrade the apdu security. </pre> <fieldset class="mimeAttachmentHeader"></fieldset> <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> </body> </html>