Re: [Pki-devel] [pki-devel][PATCH] 0015-First-cut-of-enrollment-feature.patch

This is the bulk of the work that writes objects to the token with a lot of goodies. The following are my my review comments for 0015-First-cut-of-end-to-end-enrollment-feature.patch * CreatePinAPDU.java - It appears that the old TPS has SetP1(p1). Is there a reason why you removed that here? * SecureChannel.java -appendKeyCapabilities: the getting keyCapabilities part from the config is very repetitive. You might want to write a convenience routine to iterate through all capabilities. - for most perms, you did: + TPSBuffer perms = new TPSBuffer(); + + perms.add((byte) 0xff); + perms.add((byte) 0xff); + perms.add((byte) 0x40); + perms.add((byte) 0x00); + perms.add((byte) 0x40); + perms.add((byte) 0x00); but for "createCertificate" you did: + byte[] perms = { (byte) 0xff, (byte) 0xff, 0x40, 0x00, 0x40, 0x00 }; + + TPSBuffer permissions = new TPSBuffer(perms); both achieve the same thing, but you probably want to be consistent. The 2nd way seems cleaner - writeObject() throws TPSStatus.STATUS_ERROR_MAC_ENROLL_PDU in case of response checkResult failure. Is it always the same error at each call? How about createObject()? * PKCS11Obj - addObjectSpec() - I think it'd be more useful if you put objectId in the the debug message when one duplicated object is removed. - getRawHeaderData() - typo "returing" *TPSProcessor - mapPattern() - some debug messages call themselves buildTokenLabel instead... - mapPattern() - I am not sure it is what you wanted. You had + if ((nextPos - firstPos) <= 1) { + return "token"; + } that will literally return "token" if the pattern turns out to be a string of non-tokens (not pattern). What you want to do is to return the exact same pattern if it contains no '$'. * makeKeyIDFromPublicKeyInfo() - Most our existing code call MessageDigest like this: MessageDigest md = MessageDigest.getInstance("SHA1"); I'm curious if it makes any difference if you specify the provider or not? I'm also not so sure "mozilla" is the appropriate name for the digesgt (I know we call our JSS provider "Mozilla-JSS", but a message digest is just a message digest) * about authentication: per our discussion, with what we have now, the CS.cfg configuration needs to be manually changed and server restarted between swapping the clients between ESC and tpsclient. I will just add my changes after your patch is checked in. Christina On 07/18/2014 11:26 AM, John Magne wrote: