[Pki-devel] [PATCH] 89 Added CLI commands for Key and Key request resources.

Endi Sukma Dewata edewata at redhat.com
Fri Apr 4 19:29:20 UTC 2014


On 4/2/2014 1:36 PM, Abhishek Koneru wrote:
> Please review the patch which adds new CLI commands for performing
> operations on Key and Key request resources.
>
>      key-archive, key-retrieve, key-recover, key-generate,
>      key-request-review, key-template-show, key-template-find
>
> Also attaching patch 87, which has to be applied before applying 89.
> Please review both the patches.
>
> --Abhishek

Some comments:

1. In KeyClient.java:197 the boolean expression contains redundant 
parentheses (which can make it harder to read). It can be simplified as 
follows:

   if (!status.equalsIgnoreCase(KeyResource.KEY_STATUS_ACTIVE)
       && !status.equalsIgnoreCase(KeyResource.KEY_STATUS_INACTIVE)) {

2. In KeyModifyCLI let's remove the square brackets and add spaces in 
the help message for the --status parameter to make it more readable.

   --status <status>   Status of the key.
                       Valid values: active, inactive.

Square brackets are used in command syntaxes to indicate optional 
parameters. Parameter description should contain explanation, not 
command syntaxes.

3. Same thing for KeyGenerateCLI. Here's my suggestion (also notice 
other differences and typo fixes):

   --key-algorithm <algorithm>   Algorithm to be used to create a key.
                                 Valid values: AES, DES, DES3, RC2, RC4,
                                 DESede.
   --key-size <size>             Size of the key to be generated.
                                 This is required for AES and RC2.
                                 Valid values for AES: 128, 192, 256.
                                 Valid values for RC2: 8-128.
   --usage <list of usages>      Comma separated list of usages.
                                 Valid values: wrap, unwrap, sign,
                                 verify, encrypt, decrypt.

Alternatively, move the list of valid key sizes into the manual page.

4. Same thing for KeyRequestReviewCLI:

   --action <action>    Action to be performed on the request.
                        Valid values: approve, reject, cancel.

5. In KeyModifyCLI I don't think we should compare the requested status 
and the new status after modification. If the operation fails for some 
reason the server should have thrown an exception, and the client would 
have reported the error without this additional check. So the code can 
be simplified like this:

   KeyInfo keyInfo = keyCLI.keyClient.getKeyInfo(keyId);
   KeyCLI.printKeyInfo(keyInfo);

6. In KeyRequestShowCLI, KeyShowCLI, KeyArchiveCLI, KeyRecoverCLI, and 
KeyRetrieveCLI let's use consistent capitalization for "ID":

   formatter.printHelp(getFullName() + " <Request ID>", options);
   formatter.printHelp(getFullName() + " <Key ID>", options);
   Option option = new Option(null, "clientKeyID", true, ...);
   Option option = new Option(null, "keyID", true, ...);

7. Please run source format on Template.java.

8. In Template.java it's not necessary to prefix the attributes with the 
class name. So the attributes can simply be called: id, name, description.

9. In KeyArchiveCLI, KeyRecoverCLI, and KeyRetrieveCLI I don't think we 
should check requestFile.trim().length() != 0. If people mistakenly put 
a blank filename (due to a scripting bug) we want the command to fail 
instead of doing something else (i.e. archiving a passphrase):

   pki key-archive --input "$filename"    --> blank $filename
   Error: Client Key Id is not specified. --> misleading error

10. This is an existing issue, the KeyArchivalRequest and 
KeyRecoveryRequest should have decoded the fields (e.g. 
PKIArchiveOptions, SymmetricAlgorithmParams, TransWrappedSessionKey) 
automatically, so it's not necessary to decode the fields explicitly:

   response = keyCLI.keyClient.archivePKIOptions(
       req.getClientKeyId(),
       req.getDataType(),
       req.getKeyAlgorithm(),
       req.getKeySize(),
       req.getPKIArchiveOptions());

11. Redundant parentheses in KeyCLI.java:75 can be removed:

   if (client.getConfig().getCertDatabase() != null
       && client.getConfig().getCertPassword() != null) {

12. In KeyGenerateCLI I think assigning the default key size is a 
responsibility of a CryptoProvider, not the CLI.

13. In KeyGenerateCLI.java:94 it's not necessary to wrap the list with 
another ArrayList. The list can be used directly as follows:

   usagesList = Arrays.asList(usages);

14. Typo in KeyRequestReviewCLI.java:43. It should be:

   System.err.println("Error: Invalid arguments provided.");

15. The following class names don't match the command names:

   KeyRequestTemplateFindCLI --> key-template-find
   KeyRequestTemplateShowCLI --> key-template-show

Either the class names or command names should be fixed to reflect the 
command hierarchy.

16. The KeyRequestTemplateShowCLI should not hard-code the list of 
template ID's in the command syntax. It should simply be:

   usage: key-template-show <Template ID> [OPTIONS]

The key-template-show should only accept whatever Template ID returned 
by key-template-find.

17. It would be better to store the templates as files, then both 
key-template-find and key-template-show can read from the same files. In 
the future the list of templates may come from the server.

18. For consistency it would be better to rename Template to KeyTemplate 
or KeyRequestTemplate.

-- 
Endi S. Dewata




More information about the Pki-devel mailing list