10:16 a.m.
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