6:14 p.m.
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
6:29 p.m.
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
6:12 p.m.
Hi,
First of all, thank you Endi for your through review.
Per irc discussions, I have filed the following tickets:
* https://fedorahosted.org/pki/ticket/892 Need better error handling for
HttpConnection send()
- this ticket will address the comments irt pre-existing code
* https://fedorahosted.org/pki/ticket/893 subsystems (ca, kra, tks, tps)
mixed-up their subsystem certs when sharing same Tomcat instance
- this ticket will provide investigation for an issue I found irt
shared tomcat instance causing mixed-up subsystem certs among subsystems
Attached please find the patch that addressed the remaining comments.
thanks,
Christina
On 03/04/2014 04:29 PM, Endi Sukma Dewata wrote:
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.
11:28 a.m.
I decided to change the default for connector enable's to false instead
of true so that the server can start even if no connectors are enabled.
Attached please find the small patch.
thanks,
Christina
On 03/05/2014 04:12 PM, Christina Fu wrote:
Hi,
First of all, thank you Endi for your through review.
Per irc discussions, I have filed the following tickets:
* https://fedorahosted.org/pki/ticket/892 Need better error handling
for HttpConnection send()
- this ticket will address the comments irt pre-existing code
* https://fedorahosted.org/pki/ticket/893 subsystems (ca, kra, tks,
tps) mixed-up their subsystem certs when sharing same Tomcat instance
- this ticket will provide investigation for an issue I found irt
shared tomcat instance causing mixed-up subsystem certs among subsystems
Attached please find the patch that addressed the remaining comments.
thanks,
Christina
On 03/04/2014 04:29 PM, Endi Sukma Dewata wrote:
> 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.
>
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
7:20 p.m.
ACK:
I looked over the original and all the subsequent patches.
Functionality looks good. Just a couple of quesitons.
1. Should we have some nice defines in the java code to use that represent the various
known "ops".
2. How about some simple sample code somewhere in there that demonstrates the use of this
feature.
Maybe just make a call to the TKS to get random data, and show the code to handle the
response?
----- Original Message -----
From: "Endi Sukma Dewata" <edewata(a)redhat.com>
To: "Christina Fu" <cfu(a)redhat.com>, pki-devel(a)redhat.com
Sent: Thursday, March 6, 2014 12:39:48 PM
Subject: Re: [Pki-devel] [PATCH] 862 TPS rewrite: provide connector service for JAVA-based
TPS subsystem
On 3/6/2014 11:28 AM, Christina Fu wrote:
I decided to change the default for connector enable's to false
instead
of true so that the server can start even if no connectors are enabled.
Attached please find the small patch.
thanks,
Christina
ACK.
--
Endi S. Dewata
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
7:38 p.m.
Hi jack,
Thanks for the review!
Please see response below:
On 03/06/2014 05:20 PM, John Magne wrote:
ACK:
I looked over the original and all the subsequent patches.
Functionality looks good. Just a couple of quesitons.
1. Should we have some nice defines in the java code to use that represent the various
known "ops".
Actually, these "ops" are admin-defined in the
CS.cfg as explained in
the comments of ConnectionManager.java (about line 63 and on) .
I'd like to think of the callers being some kind of plugins that can be
written so I think it might be better not to hard-code them.
2. How about some simple sample code somewhere in there that demonstrates the use of this
feature.
Maybe just make a call to the TKS to get random data, and show the code to handle the
response?
Very valid request. In fact, I have a patch coming soon (If I can be
allowed to check in this patch ;-) which will provide all the TKS remote
requests sending and response parsing. It is for ticket #888, which I
plan to submit in three batches, one for each remote subsystem, starting
with TKS. When it comes, not only will you be able to see examples, you
can just call these remote request handlers and get response all sorted
out and parsed for you.
----- Original Message -----
From: "Endi Sukma Dewata" <edewata(a)redhat.com>
To: "Christina Fu" <cfu(a)redhat.com>, pki-devel(a)redhat.com
Sent: Thursday, March 6, 2014 12:39:48 PM
Subject: Re: [Pki-devel] [PATCH] 862 TPS rewrite: provide connector service for
JAVA-based TPS subsystem
On 3/6/2014 11:28 AM, Christina Fu wrote:
> I decided to change the default for connector enable's to false instead
> of true so that the server can start even if no connectors are enabled.
>
> Attached please find the small patch.
>
> thanks,
> Christina
ACK.
3:44 p.m.
Was told that there is a convention to follow -- reporting a push to the
master when that happens...
So here you go... (well, from two weeks ago)
pushed to master,
Christina
On 03/06/2014 05:38 PM, Christina Fu wrote:
Hi jack,
Thanks for the review!
Please see response below:
On 03/06/2014 05:20 PM, John Magne wrote:
> ACK:
>
> I looked over the original and all the subsequent patches.
> Functionality looks good. Just a couple of quesitons.
>
> 1. Should we have some nice defines in the java code to use that
> represent the various known "ops".
Actually, these "ops" are admin-defined in the CS.cfg as explained in
the comments of ConnectionManager.java (about line 63 and on) .
I'd like to think of the callers being some kind of plugins that can
be written so I think it might be better not to hard-code them.
>
> 2. How about some simple sample code somewhere in there that
> demonstrates the use of this feature.
> Maybe just make a call to the TKS to get random data, and show the
> code to handle the response?
Very valid request. In fact, I have a patch coming soon (If I can be
allowed to check in this patch ;-) which will provide all the TKS
remote requests sending and response parsing. It is for ticket #888,
which I plan to submit in three batches, one for each remote
subsystem, starting with TKS. When it comes, not only will you be
able to see examples, you can just call these remote request handlers
and get response all sorted out and parsed for you.
>
> ----- Original Message -----
> From: "Endi Sukma Dewata" <edewata(a)redhat.com>
> To: "Christina Fu" <cfu(a)redhat.com>, pki-devel(a)redhat.com
> Sent: Thursday, March 6, 2014 12:39:48 PM
> Subject: Re: [Pki-devel] [PATCH] 862 TPS rewrite: provide connector
> service for JAVA-based TPS subsystem
>
> On 3/6/2014 11:28 AM, Christina Fu wrote:
>> I decided to change the default for connector enable's to false instead
>> of true so that the server can start even if no connectors are enabled.
>>
>> Attached please find the small patch.
>>
>> thanks,
>> Christina
> ACK.
>
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
2:36 p.m.
On 3/5/2014 6:12 PM, Christina Fu wrote:
Hi,
First of all, thank you Endi for your through review.
Per irc discussions, I have filed the following tickets:
* https://fedorahosted.org/pki/ticket/892 Need better error handling for
HttpConnection send()
- this ticket will address the comments irt pre-existing code
* https://fedorahosted.org/pki/ticket/893 subsystems (ca, kra, tks, tps)
mixed-up their subsystem certs when sharing same Tomcat instance
- this ticket will provide investigation for an issue I found irt
shared tomcat instance causing mixed-up subsystem certs among subsystems
Attached please find the patch that addressed the remaining comments.
thanks,
Christina
Some additional comments:
12. As discussed over IRC, the empty string checking in
HttpConnection.java:126 is unnecessary:
if ((contentType != null) && (!contentType.equals(""))) {
If we don't want an empty string to be a possible value, we probably
should not accept it in the first place by adding this code in
RemoteAuthority.java:59:
if (contentType.equals(""))
throw new IllegalArgumentException(
"Content type cannot be empty");
13. The code in HttpConnection.java:125-129 that sets the content type
currently will only be executed if HttpConnection if op is not null. Is
having a content type dependent on op being not null? The code probably
can be changed like this:
if (op == null)
mHttpreq.setURI(mDest.getURI());
else
mHttpreq.setURI(mDest.getURI(op));
// set content type regardless of op value
String contentType = dest.getContentType();
if (contentType != null) {
CMS.debug("HttpConnection: setting Content-Type");
mHttpreq.setHeader("Content-Type", contentType );
}
14. The above code that sets the content type currently only exists in
the HttpConnection constructor that takes a timeout parameter. The other
constructor that doesn't have a timeout parameter doesn't have this code
(see line 81). This will lead to further divergence of the constructors.
See https://fedorahosted.org/pki/ticket/891. I think until #891 is
addressed we should make any changes consistent in all constructors.
15. In ConnectionManager.java:42, the HTTP_CONTENT_TYPE_APP_URLENCODED
probably should be final & static since it's a constant. Or you can use
this constant:
http://docs.oracle.com/javaee/7/api/javax/ws/rs/core/MediaType.html#APPLI...
16. Since ConnectionManager will work in TPS only, the following comment
probably can be removed.
/* start is for resender only. no Resender for TPS
conn.start();
*/
That's it. ACK after addressing these issues.
--
Endi S. Dewata
3699
days inactive
3715
days old
8 comments
3 participants
participants (3)
-
Christina Fu -
Endi Sukma Dewata -
John Magne