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

[Endi Sukma Dewata]

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

-- 
Endi S. Dewata