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@redhat.com> To: pki-devel@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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

Was told that there is a convention to follow -- reporting a push to the master when that happens... So here you go... pushed to master Christina On 03/17/2014 08:17 AM, Endi Sukma Dewata wrote:

On 3/14/2014 1:42 PM, Christina Fu wrote:

...

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

thanks, Christina

Just a few more comments:

1. The computeSessionKey() will throw an exception if it's missing the RESPONSE_STATUS. The other methods will just ignore that. I think they should be consistent.

2. Right now the code gets a string value from the response object, decodes the value into TPSBuffer, then puts it back to the same response object under the same name. Then the new Response classes would read from the same response object again. It works fine but code maintainer would have to be aware of the current type of the attribute because it could change.

// response contains String or TPSBuffer values Hashtable<String, Object> response = parseResponse(content);

value = (String) response.get(IRemoteRequest.TKS_RESPONSE_SessionKey); if (value == null) { CMS.debug(...); } else { CMS.debug(...); response.put(IRemoteRequest.TKS_RESPONSE_SessionKey, Util.specialDecode(value)); }

return new TKSComputeSessionKeyResponse(response);

It might be easier to simply wrap the original response object with the new Response class, then decode the value only when the attribute is used:

// response contains string values Hashtable<String, String> response = parseResponse(content); return new TKSComputeSessionKeyResponse(response);

public TPSBuffer getSessionKey() { String value = response.get(IRemoteRequest.TKS_RESPONSE_SessionKey); if (value == null) return null; // decode value only when needed return Util.specialDecode(value)); }

Or alternatively the new Response class can hold just the decoded values either in a separate hashtable or in fields:

Hashtable<String, String> response = parseResponse(content); TKSComputeSessionKeyResponse res = new TKSComputeSessionKeyResponse(); // don't use response

value = response.get(IRemoteRequest.TKS_RESPONSE_SessionKey); if (value == null) { CMS.debug(...); } else { CMS.debug(...); // store decoded value res.setSessionKey(Util.specialDecode(value)); }

return res;

Issue #1 I think should be fixed. ACK once that is addressed.

For #2, it's not a problem, but it would improve code clarity. I'll let you decide. Thanks.