[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]