[PATCH] 548 Updated CRMFPopClient parameter handling.
The CRMFPopClient has been modified to use Apache Commons CLI library to handle the parameters. The help message has been rewritten to make it more readable. The submitRequest() will now display the error reason. The options in ClientCertRequestCLI have been simplified. A new option was added to generate CRMF request without POP. https://fedorahosted.org/pki/ticket/1074 -- Endi S. Dewata
On 1/29/2015 12:18 PM, Endi Sukma Dewata wrote:
The CRMFPopClient has been modified to use Apache Commons CLI library to handle the parameters. The help message has been rewritten to make it more readable. The submitRequest() will now display the error reason.
The options in ClientCertRequestCLI have been simplified. A new option was added to generate CRMF request without POP.
New patch attached for additional cleanup. -- Endi S. Dewata
Minor comments, otherwise looks great: The following part of the usage string: System.out.println(" -q <POP option> POP option (default: POP_SUCCESS)"); + System.out.println(" - POP_NONE"); + System.out.println(" - POP_SUCCESS"); + System.out.println(" - POP_FAIL"); We originally added POP_NONE and POP_FAIL as a way to test Proof Of Possession. This was when this thing was mainly a test tool. I would imagine the major legit cases would be POP_SUCCESS and POP_NONE, with SUCCESS merely meaning that we include POP with the request. It might be nice to add a little more info to the string for POP_FAIL that this is to test a failure case, and POP_NONE is to not provide POP, while POP_SUCCESS means to provide POP information with the request. Here: if (sensitive != 0 && sensitive != 1 && sensitive != -1) { + printError("Illegal input parameters for -s: " + sensitive); + System.exit(1); + } + + if (extractable != 0 && extractable != 1 && extractable != -1) { + printError("Illegal input parameters for -e: " + extractable); + System.exit(1); + } There is checking for the validity of the sensitive and extractable params here.
From the usage string it looks like we only want this for the EC case, would it make sense to make sure ec is in effect here?
Also, when an error is detected in the params, should be re-print out the usage, or does the system handle this for us somehow? ----- Original Message ----- From: "Endi Sukma Dewata" <edewata@redhat.com> To: "pki-devel" <pki-devel@redhat.com> Sent: Wednesday, February 4, 2015 10:03:55 AM Subject: Re: [Pki-devel] [PATCH] 548 Updated CRMFPopClient parameter handling. On 1/29/2015 12:18 PM, Endi Sukma Dewata wrote:
The CRMFPopClient has been modified to use Apache Commons CLI library to handle the parameters. The help message has been rewritten to make it more readable. The submitRequest() will now display the error reason.
The options in ClientCertRequestCLI have been simplified. A new option was added to generate CRMF request without POP.
New patch attached for additional cleanup. -- Endi S. Dewata _______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
The patch has been updated. Please take a look, thanks. On 2/9/2015 1:45 PM, John Magne wrote:
System.out.println(" -q <POP option> POP option (default: POP_SUCCESS)"); + System.out.println(" - POP_NONE"); + System.out.println(" - POP_SUCCESS"); + System.out.println(" - POP_FAIL");
We originally added POP_NONE and POP_FAIL as a way to test Proof Of Possession. This was when this thing was mainly a test tool.
I would imagine the major legit cases would be POP_SUCCESS and POP_NONE, with SUCCESS merely meaning that we include POP with the request.
It might be nice to add a little more info to the string for POP_FAIL that this is to test a failure case, and POP_NONE is to not provide POP, while POP_SUCCESS means to provide POP information with the request.
Fixed. I added short descriptions on each option.
Here:
if (sensitive != 0 && sensitive != 1 && sensitive != -1) { + printError("Illegal input parameters for -s: " + sensitive); + System.exit(1); + } + + if (extractable != 0 && extractable != 1 && extractable != -1) { + printError("Illegal input parameters for -e: " + extractable); + System.exit(1); + }
There is checking for the validity of the sensitive and extractable params here. From the usage string it looks like we only want this for the EC case, would it make sense to make sure ec is in effect here?
Fixed. I moved/added parameter checking for each algorithm.
Also, when an error is detected in the params, should be re-print out the usage, or does the system handle this for us somehow?
It will print the following message ERROR: ... <error message> ... Try 'CRMFPopClient --help' for more information. I'd rather not display the full usage because it's quite long so people might get confused trying to find the error message. Later on we can try to improve it. -- Endi S. Dewata
----- Original Message ----- From: "Endi Sukma Dewata" <edewata@redhat.com> To: "pki-devel" <pki-devel@redhat.com> Sent: Wednesday, February 4, 2015 10:03:55 AM Subject: Re: [Pki-devel] [PATCH] 548 Updated CRMFPopClient parameter handling.
On 1/29/2015 12:18 PM, Endi Sukma Dewata wrote:
The CRMFPopClient has been modified to use Apache Commons CLI library to handle the parameters. The help message has been rewritten to make it more readable. The submitRequest() will now display the error reason.
The options in ClientCertRequestCLI have been simplified. A new option was added to generate CRMF request without POP.
New patch attached for additional cleanup.
Looks fine: ACK One quick minor question though... Unless I have the order wrong we have something like this: boolean sslECDH = Boolean.parseBoolean(cmd.getOptionValue("x", "false")); Then further down it appears that we do the tests to make sure the switches provided are algorithm appropriate.. if (algorithm.equals("rsa")) { ... ... ... + + if (cmd.hasOption("x")) { + printError("Illegal parameter for RSA: -x"); + System.exit(1); + } Does this ordering matter or are we using the statement above as a convenient way to initialize the variable "sslECDH". ----- Original Message ----- From: "Endi Sukma Dewata" <edewata@redhat.com> To: "John Magne" <jmagne@redhat.com> Cc: "pki-devel" <pki-devel@redhat.com> Sent: Monday, February 16, 2015 7:18:04 AM Subject: Re: [Pki-devel] [PATCH] 548 Updated CRMFPopClient parameter handling. The patch has been updated. Please take a look, thanks. On 2/9/2015 1:45 PM, John Magne wrote:
System.out.println(" -q <POP option> POP option (default: POP_SUCCESS)"); + System.out.println(" - POP_NONE"); + System.out.println(" - POP_SUCCESS"); + System.out.println(" - POP_FAIL");
We originally added POP_NONE and POP_FAIL as a way to test Proof Of Possession. This was when this thing was mainly a test tool.
I would imagine the major legit cases would be POP_SUCCESS and POP_NONE, with SUCCESS merely meaning that we include POP with the request.
It might be nice to add a little more info to the string for POP_FAIL that this is to test a failure case, and POP_NONE is to not provide POP, while POP_SUCCESS means to provide POP information with the request.
Fixed. I added short descriptions on each option.
Here:
if (sensitive != 0 && sensitive != 1 && sensitive != -1) { + printError("Illegal input parameters for -s: " + sensitive); + System.exit(1); + } + + if (extractable != 0 && extractable != 1 && extractable != -1) { + printError("Illegal input parameters for -e: " + extractable); + System.exit(1); + }
There is checking for the validity of the sensitive and extractable params here. From the usage string it looks like we only want this for the EC case, would it make sense to make sure ec is in effect here?
Fixed. I moved/added parameter checking for each algorithm.
Also, when an error is detected in the params, should be re-print out the usage, or does the system handle this for us somehow?
It will print the following message ERROR: ... <error message> ... Try 'CRMFPopClient --help' for more information. I'd rather not display the full usage because it's quite long so people might get confused trying to find the error message. Later on we can try to improve it. -- Endi S. Dewata
----- Original Message ----- From: "Endi Sukma Dewata" <edewata@redhat.com> To: "pki-devel" <pki-devel@redhat.com> Sent: Wednesday, February 4, 2015 10:03:55 AM Subject: Re: [Pki-devel] [PATCH] 548 Updated CRMFPopClient parameter handling.
On 1/29/2015 12:18 PM, Endi Sukma Dewata wrote:
The CRMFPopClient has been modified to use Apache Commons CLI library to handle the parameters. The help message has been rewritten to make it more readable. The submitRequest() will now display the error reason.
The options in ClientCertRequestCLI have been simplified. A new option was added to generate CRMF request without POP.
New patch attached for additional cleanup.
On 2/25/2015 7:32 AM, John Magne wrote:
Looks fine: ACK
Thanks. Pushed to master.
One quick minor question though...
Unless I have the order wrong we have something like this:
boolean sslECDH = Boolean.parseBoolean(cmd.getOptionValue("x", "false"));
Then further down it appears that we do the tests to make sure the switches provided are algorithm appropriate..
if (algorithm.equals("rsa")) { ... ... ... + + if (cmd.hasOption("x")) { + printError("Illegal parameter for RSA: -x"); + System.exit(1); + }
Does this ordering matter or are we using the statement above as a convenient way to initialize the variable "sslECDH".
The order does not really matter since it's only parsing the option value, not interpreting the value, and the current code is more concise. For boolean options like sslECDH, any non-boolean value will be parsed into "false". So if you specify: -a rsa -x <non-boolean value> it will generate an "Illegal parameter for RSA" message. For integer options it's a bit different, for example: int extractable = Integer.parseInt(cmd.getOptionValue("e", "-1")); if (algorithm.equals("rsa")) { if (cmd.hasOption("e")) { printError("Illegal parameter for RSA: -e"); System.exit(1); } } If you specify: -a rsa -e <non-integer value> it will generate a NumberFormatException instead of "Illegal parameter for RSA". But I think either way is fine. If we really want to see an "Illegal parameter" instead of NumberFormatException, the code has to be written as follows: int extractable = -1; if (algorithm.equals("rsa")) { if (cmd.hasOption("e")) { printError("Illegal parameter for RSA: -e"); System.exit(1); } } else if (algorithm.equals("ec")) { extractable = Integer.parseInt(cmd.getOptionValue("e", "-1")); } which is less concise. I think it's better to pay more attention to how the options are interpreted and make sure that it's done in the correct order, especially if it's resource intensive. For example loading a file after making sure the filename option is legal. -- Endi S. Dewata
participants (2)
-
Endi Sukma Dewata -
John Magne