[Pki-devel] Review Request: CMC ECC support

[John Magne]

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