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