Hi Jack,
Thank you for your review comments. The following patch should address
your comments:
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>
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:
> 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(a)redhat.com
>
https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel