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

[John Magne]

ACK:

This is for the new classes that give clients easy access to the data and the status
of requests sent to the TKS. This will help me nicely when negotiating the secure channel
protocol.

When I start using it and determine a new need, I'll just add it in a separate ticket at the time.

----- Original Message -----
From: "Christina Fu" <cfu at redhat.com>
To: pki-devel at redhat.com
Sent: Friday, March 14, 2014 11:42:43 AM
Subject: Re: [Pki-devel] [PATCH] Ticket #888 - TPS rewrite: provide remote authority functions (patch 1: TKS functions)

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().
>


_______________________________________________
Pki-devel mailing list
Pki-devel at redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel