[Pki-devel] [PATCH] 90 Fixes for comments on patches 87 and 89
Ade Lee
alee at redhat.com
Mon Apr 14 16:10:13 UTC 2014
Comments:
1. Patch 90 introduces a whitespace error.
2. In KeyClient.java, you can use StringUtils to make the following more
concise:
if (invalidUsages == null) {
invalidUsages = list[i];
} else {
invalidUsages = invalidUsages + ", " + list[i];
}
invalidUsages = StringUtils.join(list[i])
Other than that it looks good. So ACK from me. Please wait for Endi's ACK too.
I still would like to see some kind of CLI option to archive a symmetric key. Lets
add a ticket for that.
Ade
On Thu, 2014-04-10 at 10:19 -0400, Abhishek Koneru wrote:
> 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
More information about the Pki-devel
mailing list