Review Comments: PKCS10Client.java 1. Typo, I think: 50 + System.out.println(" -x <ture for SSL cert that does ECDH ECDSA; false otherwise; default false>\n"); 2. When parsing the command line args, it looks like we assume an even number or args, with the first arg being the switch and the second one being the value. When you check the switch, there is no code to handle the case when the switch is not in the list. Also, it appears that not every value is checked. For instance, if an ecc curve is specified, I don't see a check for null. 3. We have the following piece of code in two places near each other: if (dbdir == null) 141 dbdir = "."; 4. The following block: try { 170 + token.login(pass); 171 + System.out.println("PKCS10Client: token "+ tokenName + " logged in..."); 172 + } catch (Exception e) { 173 + System.out.println("PKCS10Client: login Exception: " + e.toString()); 174 + if (!token.isLoggedIn()) { 175 + System.out.println("PKCS10Client: token not logged in, calling initPassword..."); 176 + token.initPassword(pass, pass); 177 + } 178 + } What are we doing here? If the password is wrong, should we not bomb out? 5. Code: if (alg.equals("rsa")) { 182 + KeyPairGenerator kg = token.getKeyPairGenerator(KeyPairAlgorithm.RSA); 183 + kg.initialize(rsa_keylen); 184 + pair = kg.genKeyPair(); 185 + } else if (alg.equals("ec")) { 186 + // used with SSL server cert that doe What if the alg is some bogus value? The tool seems to happily skip over the entire block and keep going. Maybe some quick check? 6. certRequest = new CertificationRequest(certReqInfo, If certRequest is null, we just log the fact to the screen but keep going. 7. PublicKey pubk = pair.getPublic(); Here , I do not think we ever check to see if "pair" is generated without a null. Maybe that is taken care of with the exceptions. But, also "pubk" is used later without a check for null. 8. Same as #6, with the value of "certReq". CRMFPopClient.java 1. Check for the same argument parsing concerts as in the previous tool. 2. This check here: try { 599 + CryptoManager.initialize( DB_DIR ); 600 + } catch (Exception e) { 601 + // it is ok if it is already initialized 602 + System.out.println("CRMFPopClient: INITIALIZATION ERROR: " + e.toString()); 603 + //return; 604 + } Is it possible this would fail for some other reason besides that it was already initialized? 3. Same concern in the previous tool about what happens when token.login fails. CMCRequest.java 1. Same concern here about checking the password. HttpClient.java 1. This code: + X509Certificate cert = 1803 + cm.findCertByNickname(certname.toString()); 1804 if (cert == null) 1805 System.out.println("client cert is null"); 1806 else 1807 System.out.println("client cert is not null"); Do we care if there is no client cert? The code proceeds happily from this point on. ----- Original Message ----- From: "Christina Fu" <cfu@redhat.com> To: pki-devel@redhat.com Sent: Tuesday, 18 December, 2012 8:44:27 PM Subject: Re: [Pki-devel] Review Request: CMC ECC support Please find a newer patch which now also includes support for CMC revocation within the CMCRequest tool and op flags for EC key generation in CRMFPopClient and PKCS10Client. https://fedorahosted.org/pki/ticket/362 You will also find various Examples on how to test different scenarios in regards to CMC request issuance. thanks, Christina On 12/11/2012 04:51 PM, Christina Fu wrote:
The following code is ready for review for task https://fedorahosted.org/pki/ticket/362 Feature: CMC ECC which includes support for CMC CRMF, CMC PKCS10, CMC Revoke, as well as CMC Response checking support needed HTTPClient ECC support. It should inherently fix the following existing bugs: https://bugzilla.redhat.com/show_bug.cgi?id=805738 ECC support for CMC CRMF https://bugzilla.redhat.com/show_bug.cgi?id=837892 ECC support for CMC revoke
attachment: https://fedorahosted.org/pki/attachment/ticket/362/CMC-ECC-forReview1.diff
Code changes involve both server and several tools.
I will do some writeup and provide example on how to test in the task comment later.
thanks! Christina
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel