[Pki-devel] [PATCH] 547 Refactored CRMFPopClient.

[Endi Sukma Dewata]

Thanks for the review.

On 1/27/2015 8:45 PM, John Magne wrote:
> Looks nice. Thinks are much simpler.
>
>
> Quick Comments/Questions:
>
> 1. Looks like we've added a command line param for the transport cert.
> I notice there is still a check to see if the number of params is 4 or more.
> Is this still valid, or is the transport cert param optional?

I think it's no longer valid since the parameters are now specified 
using options instead of arguments. I'll remove it before push. Ideally 
the parameters should be handled using a getopt library.

> 2. Down in the code where we are generating keys either ec or rsa, we still
> check to see if the algorithm is valid. Was this check not already done at the top
> of the method?

Yes, in the CRMFPopClient's main program it does validate the algorithm 
already. However, since the CRMFPopClient methods now can be called by 
other code directly, and potentially be given an invalid algorithm, we 
should check the validity there too.

> 3. Inside "submitRequest" you have some System.out.println's lingering. Was the idea to
> get rid of all of those?

Some of the println()'s are displaying the server response (HTML page) 
and some others are displaying the request ID and status. I'll change it 
such that the server response is only displayed in verbose mode.

> 4. Does the usage string give an idea what params are optional and such? If not would this be
> helpful? Is it the "default" designation used for this purpose?

Currently it lists the ECC params under "Optionally...", but actually 
there are other params that should be optional too (e.g. the hostPort 
param). I think we should revise the usage string and also implement 
getopt. It can be done as a separate patch later.

The 'default' indicates the value of an optional parameter if it's not 
specified by the user. An optional parameter does not always need to 
have a 'default' value, and that means the value will be empty/null if 
not specified.

> Anyway, ACK on the functionality if its been of course tested to work.

Yes, I was able to submit CRMF request using CRMFPopClient and pki 
client-cert-request (see 
http://pki.fedoraproject.org/wiki/User_Certificate#Dogtag_10.2.2), and 
when it's approved it archives the key in KRA.

-- 
Endi S. Dewata