[Pki-devel] Review Request: CMC ECC support

Wed Jan 9 23:51:04 UTC 2013

Checked over code again to verify the concerns were taken care of. 

The were:

ACK

----- Original Message -----
From: "Christina Fu" <cfu at redhat.com>
To: "John Magne" <jmagne at redhat.com>
Cc: pki-devel at redhat.com
Sent: Wednesday, 9 January, 2013 10:36:38 AM
Subject: Re: [Pki-devel] Review Request: CMC ECC support

Hi Jack,
Thank you for your review comments.  The following patch should address 
your comments:
https://fedorahosted.org/pki/attachment/ticket/362/CMC-ECC-forReview2.diff

thanks,
Christina

On 12/21/2012 02:46 PM, John Magne wrote:
> 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 at redhat.com>
> To: pki-devel at 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/pki-devel
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel