[Pki-devel] [PATCH] 862 TPS rewrite: provide connector service for JAVA-based TPS subsystem

[Endi Sukma Dewata]

Some comments:

1. As discussed over IRC the common code HttpConnection shouldn't 
contain a subsystem-specific code, which in this case is setting the 
content type to application/x-www-form-urlencoded for TPS.

2. Also discussed over IRC, there's an existing issue with 
HttpConnection constructors:
https://fedorahosted.org/pki/ticket/891

Suppose that issue is fixed later, it should be possible to merge the 
constructors. Then instead of calling different constructors for 
different timeout values like this:

     if (timeout == 0) {
         connector =
             new HttpConnector(
                 null, nickname, remauthority,
                 resendInterval, conf);
     } else {
         connector =
             new HttpConnector(
                 null, nickname, remauthority,
                 resendInterval, conf, timeout);
     }

it can be simplified like this:

     connector =
         new HttpConnector(
             null, nickname, remauthority,
             resendInterval, conf, timeout);

There's also a similar code in HttpConnFactory.

4. In HttpConnection the existing constructors were changed to pass "" 
as the default value of op. I think we should just pass a null value. 
This way the code can simply check the op like this:

     this(dest, factory, null);

     ...

     if (op == null)

If we really want to accept an empty string, then probably we should 
normalize the string by trimming the white spaces before checking for 
empty string.

5. The HttpConnection.send() checks if the parameter is null or empty 
string then returns a null value with a comment "throw later". This 
error will only be detected later if another code tries to use the null 
return value, but at that point we're looking at a secondary issue, 
which makes it harder to troubleshoot. I think in general it's better to 
throw an exception as soon as the error is detected.

6. The HttpConnection.send() checks for untrusted certificate by 
checking the exception message. This may work now but could become 
inaccurate later due to translation, text changes, or typo. It would be 
better if there's a specific exception type or error code that we can 
check for this specific case.

7. Similar to #5, HttpConnection.send() swallows NumberFormatException. 
I think we should not catch this to detect the error as early as 
possible and also NumberFormatException is already a RuntimeException so 
it doesn't need to be wrapped/declared.

8. Similar to #5, the HttpConnector.send() (not HttpConnection) would 
swallow the exception so some errors wouldn't be detected immediately. I 
think the code could be simplified as follows (the send() already throws 
EBaseException):

     try {
         curConn = mConnFactory.getConn(op);
         return curConn.send(msg);

     } finally {
         if (curConn != null) {
             mConnFactory.returnConn(curConn);
         }
     }

9. Similar to #5, the ConnectionManager.getXMLparser() would swallow the 
exceptions. I think it would be better to wrap the exception in a 
RuntimeException and rethrow it.

     try {
         ByteArrayInputStream bis =
             new ByteArrayInputStream(text.getBytes());
         return new XMLObject(bis);

     } catch (Exception e) {
         CMS.debug("TPSSubsystem: getXMLparser(): failed: "+ e.toString());
         throw new RuntimeException(e);
     }

We can change this code again in the future if necessary to use a more 
specific class or add the original exceptions into the throws list.

10. In ConnectionManager, the createConnector() will never return a 
null, so it's not necessary to check its return value in 
initConnectors(). It will simplify the code and some code analyzers like 
Coverity probably will report the part that handles the null case as 
dead code anyway.

11. Just some minor things:
- The debug log in getXMLparser() should say "ConnectionManager" instead 
of "TPSSubsystem".
- If we use camel-case consistently, the name should be getXMLParser() 
(with upper-case P).
- Some toString() invocations are redundant because string concatenation 
will call it automatically.

I'll leave it to others to review the functionality correctness.

-- 
Endi S. Dewata


On 3/3/2014 6:14 PM, Christina Fu wrote:
> The is a request for code review.
>
> Attached please find the code that implements the following trac ticket:
> https://fedorahosted.org/pki/ticket/862 TPS rewrite: provide connector
> service for JAVA-based TPS subsystem
>
> This patch makes available the connector that has been used between CA
> and KRA to TPS and other authorities (CA, TKS, KRA).
>
> A few things to note:
> 1.
> One key modification to the existing connector framework is the
> introduction of "muti-uri" implementation which will allow TPS to
> provide connectors according to an "op".   For example:
> tps.connector.ca1.uri.enrollment=/ca/ee/ca/profileSubmitSSLClient
> tps.connector.ca1.uri.renewal=/ca/ee/ca/profileSubmitSSLClient
> tps.connector.ca1.uri.revoke=/ca/ee/subsystem/ca/doRevoke
> tps.connector.ca1.uri.unrevoke=/ca/ee/subsystem/ca/doUnrevoke
> In the above configuration, "renewal" is an op, which will match to the
> servlet /ca/ee/ca/profileSubmitSSLClient in the uri.
>
> 2.
> The connection configuration has been modified to take after the same
> style as that of the CA/KRA.
>   - the change of target.Subsystem_Connections.pattern allows cli to
> work with the new connector parameters, however, the browser will show
> raw xml (it reports: "This XML file does not appear to have any style
> information associated with it. The document tree is shown below.")
>
> 3.
> Due to the unfilled parameters in the connector area, CS.cfg needs to be
> manually configured at this time.  Which means you need to add
> "pki_skip_configuration=True" to your tps.cfg for pkispawn, and manually
> configure it.
>
> 4.
> Due to some unfortunate constant eclipse crashing issue, I had to
> manually visit each modified/added file to check for warnings.  It is to
> my best knowledge that I did not add new warnings to the tree.
>
> 5.
> To know what NOT to expect in this ticket, please take a look of the
> following two tickets:
> https://fedorahosted.org/pki/ticket/888 - TPS rewrite: provide remote
> authority functions
> https://fedorahosted.org/pki/ticket/890 - TPS rewrite: connector
> configuration during installation
>
> thank you,
> Christina