[Pki-devel] [PATCH] Ticket #888 - TPS rewrite: provide remote authority functions (patch 1: TKS functions)

[Christina Fu]

Thank you Endi and Jack for your comments.
Attached please find the patch that addressed your comments.

thanks,
Christina

On 03/13/2014 08:16 AM, Endi Sukma Dewata wrote:
> On 3/11/2014 9:13 PM, Christina Fu wrote:
>> This is a request for review for the attached patch.
>> The patch is the first part for https://fedorahosted.org/pki/ticket/888
>> - TPS rewrite: provide remote authority functions
>>
>> This patch provides functions for TKS:
>>    - computeSessionKey
>>    - createKeySetData
>>    - encryptData
>>    - computeRandomData
>>
>> Two things to note:
>> * Because of code not yet available, only computeRandomData() was
>> tested.  Other functions can be tested/adusted when the needed info are
>> available.
>> * Because we don't know where the tps profile name will be at this
>> point, computeSessionKey currently has a hard-coded entry in there. It
>> is noted in a TODO comment.  Again, this will be adjusted when it's more
>> clear where it comes from.
>>
>> thanks,
>> Christina
>
> Some comments:
>
> 1. In RemoteRequestHandler.parserResponse() now if the content is null 
> the method will return null too. I suppose content should never be 
> null, so this is an error case. As I mentioned in the previous review, 
> if an error is detected, it would be better to throw an exception 
> instead of returning null so that it can be traced to the actual error.
>
>   if (content == null) {
>       throw new EBaseException("RemoteRequestHandler: 
> parserResponse(): no response content.");
>   }
>
> On the other hand, if null content is considered a valid input 
> (meaning empty content), it would be better to return an empty 
> hashtable so the caller doesn't need to worry about null hashtable:
>
>   Hashtable<String, Object> vars = new Hashtable<String, Object>();
>   if (content == null) {
>       return vars;
>   }
>
> 2. In RemoteRequestHandler.parserResponse() it's not necessary to 
> create a new string nvs from string e:
>
>   for (String e : elements) {
>       String nvs = new String(e);
>       String[] nv = nvs.split(NAME_VALUE_EQUAL);
>       ...
>   }
>
> Java strings are immutable and the split() will create new strings, so 
> the original content won't be affected. The code can be simplified as 
> follows:
>
>   for (String nvs : elements) {
>       String[] nv = nvs.split(NAME_VALUE_EQUAL);
>       ...
>   }
>
> 3. In RemoteRequestHandler.parserResponse() the following null 
> checking is unnecessary because split() doesn't return null string 
> elements.
>
>   String[] nv = nvs.split(NAME_VALUE_EQUAL);
>   if (nv[0] != null && nv[1] != null) {
>       vars.put(nv[0], nv[1]);
>   }
>
> Instead, we should check whether the array contains 2 elements. What 
> should we do if it contains more, or less, than 2 elements?
>
>   if (nv.length == 2) {
>       vars.put(nv[0], nv[1]);
>   } else {
>       // throw error, skip, or ignore the extra elements?
>   }
>
> 4. Since both the name and the value are strings, 
> RemoteRequestHandler.parserResponse() can actually return a 
> Hashtable<String, String> so it's not necessary to downcast the values 
> into string again later.
>
> 5. Similar to #1, in TKSRemoteRequestHandler.computeSessionKey() if 
> the TKS response is null it's an error case, so it should throw an 
> exception instead of returning null again. Also note the extra 
> parentheses in the if-statement can be removed.
>
>   String content = resp.getContent();
>
>   if (content != null && !content.equals("")) {
>       ...
>
>   } else {
>       throw new EBaseException("TKSRemoteRequestHandler: 
> computeSessionKey(): no response content.");
>   }
>
> 6. Regardless of #1, the following null response checking in 
> TKSRemoteRequestHandler.computeSessionKey() is unnecessary because the 
> response will never be null since parseResponse() will never receive a 
> null content:
>
>   if (content != null && !content.equals("")) {
>
>        // content can't be null
>        Hashtable<String, Object> response =
>            parseResponse(content);
>
>        // response can't be null
>        if (response == null) {
>            CMS.debug("TKSRemoteRequestHandler: computeSessionKey(): 
> parseResponse returned null.");
>            return null;
>        }
>
> 7. Similar to #1, if missing status is considered an error then it 
> should throw an exception:
>
>   String value = response.get(TKS_RESPONSE_STATUS);
>   if (value == null) {
>       throw new EBaseException("TKSRemoteRequestHandler: 
> computeSessionKey(): Bad TKS Connection. Please make sure TKS is 
> accessible by TPS.");
>
>   } else {
>       CMS.debug("TKSRemoteRequestHandler: computeSessionKey(): got 
> status =" + value);
>   }
>
> 8. The computeSessionKey() now returns a hashtable containing 
> attributes, so someone calling this method will have to check the docs 
> to make sure he/she uses the right attributes and casts the values to 
> the right types. It would be better to define a class for the return 
> value:
>
>   CreateSessionKeyResponse response = tksReq.computeSessionKey(...);
>   String status = response.getStatus();
>   byte[] sessionKey = response.getSessionKey();
>
> 9. Comments #5-#8 also apply to createKeySetData(), 
> computeRandomData(), and encryptData().
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Ticket-888-part-1-TKS-TPS-rewrite-provide-remote-aut.patch
Type: text/x-patch
Size: 54056 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140314/761d61f9/attachment.bin>