Re: [Pki-devel] Review Request: CMC ECC support

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(a)redhat.com&gt; To: pki-devel(a)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: _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel