9:19 a.m.
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.
11:10 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
2:05 p.m.
On 4/10/2014 9:19 AM, 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
Some more comments:
1. In KeyGenerateCLI there's a typo in the description for --usages
parameter: seperated.
2. In KeyGenerateCLI the description for --key-size parameter says "This
is required for AES and RC2" only but it looks like the parameter is now
required for RC4 too.
3. In KeyTemplate.java it's not necessary to call super().
4. In KeyTemplate.java the attribute name and variable name for "ID"
should be lower case "id". In general attribute/variable names are
internal, so they should follow Java convention (i.e. lower case "id").
Upper case attribute names are reserved for constants. Outputs that can
be seen by users should use the proper English capitalization (i.e.
upper case "ID").
5. Please run source formatter on SymKeyGenerationRequest.java.
6. There's an extra semicolon in KeyClient.java:
String[] list = usages.split(",");
String invalidUsages = null;
;
for (int i = 0; i < list.length; i++) {
...
7. The above code can be simplified into:
List<String> invalidUsages = new ArrayList<String>();
String (String usage : usages.split(",")) { // use for-each
if (!validUsages.contains(usage))
invalidUsages.add(usage);
}
In general it's better to use a collection to construct a list rather
than doing string concatenation. It's also possible to throw the
exception immediately when an invalid usage is found. It's not really
necessary to list all invalid usages.
8. I think KeyClient.generateSymmetricKey() should take an array or List
of string usages instead of a single string containing concatenated
usages. In general an API should take parameters in the parsed format
that can be used immediately without further processing. In this case
the KeyGenerateCLI and the DRMTest should construct a list of string
usages first, then pass it to generateSymmetricKey().
9. The KeyTemplateFindCLI is still hardcoding the list of templates. It
should get the list of files from the templates folder and generate the
template names from the file names.
How about renaming the /usr/share/pki/kra/templates folder into
/usr/share/pki/key/templates and renaming the XML files to match the
template name?
10. The KeyTemplateShowCLI is still partially hardcoding the templates.
I think the XML file path can be generated from the specified template
ID parameter. Then the class name can be obtained from the ClassName
element in the XML file. And the "message" can be obtained from another
attribute stored in the template (e.g. description).
11. In KeyTemplateShowCLI the code is checking for the length of the
filename parameter. As mentioned in the previous email this check is not
necessary and could be misleading.
if ((writeToFile != null) && (writeToFile.trim().length() != 0)) {
Also the above line contains some redundant parentheses.
12. In KeyTemplateShowCLI the printRequestTemplate() and
readFromTemplateFile() probably can be moved to ResourceMessage and be
called marshal() and unmarshal() as variations of the existing methods.
--
Endi S. Dewata
2:15 p.m.
One more thing:
13. It's not necessary to increment the release number during
development. You can change the spec, then optionally add a note in the
last changelog entry (instead of adding a new change log entry). We're
trying to avoid unnecessary inflating the release number. A new release
number is only needed to distinguish a release from the previous one,
which in case of 10.2 there is none.
--
Endi S. Dewata
12:43 p.m.
New subject: [PATCH] 90-2 Addressed fixes for comments on patch 90.
Please review patch 90-2 with fixes for all comments given by Ade and
Endi.
Patch application order - 87,89,90,90-2
-- Abhishek
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
1:35 p.m.
New subject: [PATCH] 90-2 Addressed fixes for comments on patch 90.
On 4/16/2014 12:43 PM, Abhishek Koneru wrote:
Please review patch 90-2 with fixes for all comments given by Ade
and
Endi.
Patch application order - 87,89,90,90-2
Some more comments:
1. The templates are still hardcoded in KeyTemplateFindCLI, i.e. we
can't add a new template without changing the code. See the following code:
switch (id) {
case "archiveKey":
data = ResourceMessage.unmarshall(KeyArchivalRequest.class,
templateDir + templateName);
template = new KeyTemplate(id, "Key archival request",
data.getAttribute("description"));
templates.add(template);
break;
case "retrieveKey":
data = ResourceMessage.unmarshall(KeyRecoveryRequest.class,
templateDir + templateName);
template = new KeyTemplate(id, "Key retrieval request",
data.getAttribute("description"));
templates.add(template);
break;
Notice that the above cases are identical except for the class name and
the template name. I think we can use ResourceMessage for the class name
since we're only using it to retrieve the attributes. For the template
name we can put that into the template as well, or we can just remove
the name since we already have ID and the description.
So the resulting code would be (no switch required):
data = ResourceMessage.unmarshall(ResourceMessage.class,
templateDir + templateName);
template = new KeyTemplate(id, data.getAttribute("description"));
templates.add(template);
2. Similar issue with KeyTemplateShowCLI.
3. There's an extra semicolon in KeyTemplateShowCLI.java:47.
4. In KeyTemplateShowCLI you could use try-with-resource so you don't
need to close the stream explicitly.
try (FileOutputStream fOS = new FileOutputStream(writeToFile)) {
...
} catch (IOException e) {
...
}
5. Please run source formatting on KeyClient and KeyGenerateCLI.
Once they are fixed, it's ACKed. Feel free to push. Thanks.
--
Endi S. Dewata
4:31 p.m.
New subject: [PATCH] 90-2 Addressed fixes for comments on patch 90.
Made the changes and pushed to master.
Also added the help option in all of the key CLIs
--Abhishek
----- Original Message -----
From: "Endi Sukma Dewata" <edewata(a)redhat.com>
To: "Abhishek Koneru" <akoneru(a)redhat.com>, "pki-devel"
<pki-devel(a)redhat.com>
Sent: Wednesday, April 16, 2014 2:35:55 PM
Subject: Re: [Pki-devel] [PATCH] 90-2 Addressed fixes for comments on patch 90.
On 4/16/2014 12:43 PM, Abhishek Koneru wrote:
Please review patch 90-2 with fixes for all comments given by Ade
and
Endi.
Patch application order - 87,89,90,90-2
Some more comments:
1. The templates are still hardcoded in KeyTemplateFindCLI, i.e. we
can't add a new template without changing the code. See the following code:
switch (id) {
case "archiveKey":
data = ResourceMessage.unmarshall(KeyArchivalRequest.class,
templateDir + templateName);
template = new KeyTemplate(id, "Key archival request",
data.getAttribute("description"));
templates.add(template);
break;
case "retrieveKey":
data = ResourceMessage.unmarshall(KeyRecoveryRequest.class,
templateDir + templateName);
template = new KeyTemplate(id, "Key retrieval request",
data.getAttribute("description"));
templates.add(template);
break;
Notice that the above cases are identical except for the class name and
the template name. I think we can use ResourceMessage for the class name
since we're only using it to retrieve the attributes. For the template
name we can put that into the template as well, or we can just remove
the name since we already have ID and the description.
So the resulting code would be (no switch required):
data = ResourceMessage.unmarshall(ResourceMessage.class,
templateDir + templateName);
template = new KeyTemplate(id, data.getAttribute("description"));
templates.add(template);
2. Similar issue with KeyTemplateShowCLI.
3. There's an extra semicolon in KeyTemplateShowCLI.java:47.
4. In KeyTemplateShowCLI you could use try-with-resource so you don't
need to close the stream explicitly.
try (FileOutputStream fOS = new FileOutputStream(writeToFile)) {
...
} catch (IOException e) {
...
}
5. Please run source formatting on KeyClient and KeyGenerateCLI.
Once they are fixed, it's ACKed. Feel free to push. Thanks.
--
Endi S. Dewata
3671
days inactive
3677
days old
6 comments
3 participants
participants (3)
-
Abhishek Koneru -
Ade Lee -
Endi Sukma Dewata