[Pki-devel] [PATCH] 90-2 Addressed fixes for comments on patch 90.

Please review the patch with fixes for comments given by Ade and Endi. All the solutions were discussed on IRC. All comments are addressed except comment 10 in Endi's comments and one comment from Ade to add a cli option to archive symmetric key from its binary string. Also attaching patches 87, 89. Please apply them in this order 87, 89, 90 --Abhishek Ade's Comments: KeyModifyCLI.java: 1) super("mod", "Get key request", keyCLI); "Get key request" is not right .. This appears to be a problem in multiple CLIs. 2) help does not look right -- do options follow the <keyId> ? 3) What happens if you choose a non-existent id? This is for all the CLIs. ** All the CLIs return the 404 exception thrown from the server for a given ID. 4. No validation of status input. Template.java 1) Add line after field declarations and before constructor. KeyArchiveCLI 1) Archive a secret at the DRM --> in the DRM. 2) There appears to be no option to archive a symmetric key? Given that its one of our primary use cases, we should have an option for it. ** Not done in this patch. KeyGenerate.java 1) In help, do [OPTIONS] follow the args? 2. There is no validation of usages() - and the list really ought to be generated from the SymKeyGenerationRequest definition. 3. "Required for all algorithms AES and RC2." doesn't sound right. "Required for AES and RC2 algorithms". RC4 I think requires a key size and uses a default in case one is not provided. 4. If I recall correctly, there is a JSS function that checks whether an key_size is valid. We should probably do some validation here. KeyRecoverCLI 1. This should probably be "Create a key recovery request" --> rather than "Recover key" and at the end, this would be "Key Recovery Request Info". and so maybe this should be "key-request-recovery" ? KeyRequestTemplateFind: "Template file for submitting a key archival request"); "Template for submitting a key recovery request."); "Template for submitting a symmetric key generation request."); TemplateShowCLI - 1. No need to put list of templates in help -- thats what template-find is for. KeyRetrievalCLI 1. There should be an option to store the raw output to a file. Binary data doesn't print well. 2. If a wrapping key was not initially provided, then the encrypted data makes no sense. Similarly if a wrapping key was not provided, then the enencrypted data makes no sense. Endi's 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. _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel