You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by Polar Humenn <ph...@iona.com> on 2007/03/19 17:28:52 UTC
CXF-438 Patch; HTTP(S) Trust Decision
Greetings,
I have submitted a patch for JIRA-CXF-438. This patch handles the issue of
making a Trust Decision in the rt-transports-http module. It is attached
to the JIRA
issue. I hope someone will review. Although I have applications that
test this, I will
organize them into a system test shortly.
Cheers,
-Polar
Re: CXF-438 Patch; HTTP(S) Trust Decision
Posted by Polar Humenn <ph...@iona.com>.
Dan,
I'm just trying to be thorough, from low level to high level. The trust
decider is issued the conduit name, then upwards, URL Connection(info),
endpoint (for addressing information), binding, service. I actually got
the BindingInfo, and ServiceInfo off the Exchange object.
I'm trying to keep the Trust Decider (architecturally) clean from being
able to influence any CXF internals, since it is a piece of user code.
The EndpointInfo object is even too general and dangerous, and it should
be that only parts of the EndpointInfo need be exposed (no "set"
methods). So, maybe replace EndpointInfo with:
QName endpointName <-- EndpointInfo.getName()
String endpointAddress <- EndpointInfo.getAddress()
However, now I see that the internal architecture is such that even the
BindingInfo and ServiceInfo has a bunch of set methods that the
TrustDecider can go off and violate all kinds of things if it wants to.
Oh well.
So, what does the Trust Decider really need?
It definitely needs the URLConnection Info. And it needs some "other"
things for figuring out whether it should be trusted for the Message in
question.
It maybe that this architecture is so general in that maybe we should
just have
void establishTrust(
String conduitName,
URLConnectionInfo connInfo,
Message message
) ....
and leave it at that, with a stipulation that the implementation of
establishTrust must play *nice* and not change anything on the message.
Cheers,
-Polar
Dan Diephouse wrote:
> Only had a moment to look at this, butt: Do we need to pass in the
> BindingInfo/ServiceInfo as well to the trust decider? Those are easily
> accessible via endpointInfo.getService() and endpointInfo.getBinding();
>
> - Dan
>
> On 3/19/07, Polar Humenn <ph...@iona.com> wrote:
>>
>> Greetings,
>>
>> I have submitted a patch for JIRA-CXF-438. This patch handles the
>> issue of
>> making a Trust Decision in the rt-transports-http module. It is attached
>> to the JIRA
>> issue. I hope someone will review. Although I have applications that
>> test this, I will
>> organize them into a system test shortly.
>>
>> Cheers,
>> -Polar
>>
>
>
>
Re: CXF-438 Patch; HTTP(S) Trust Decision
Posted by Dan Diephouse <da...@envoisolutions.com>.
Only had a moment to look at this, butt: Do we need to pass in the
BindingInfo/ServiceInfo as well to the trust decider? Those are easily
accessible via endpointInfo.getService() and endpointInfo.getBinding();
- Dan
On 3/19/07, Polar Humenn <ph...@iona.com> wrote:
>
> Greetings,
>
> I have submitted a patch for JIRA-CXF-438. This patch handles the issue of
> making a Trust Decision in the rt-transports-http module. It is attached
> to the JIRA
> issue. I hope someone will review. Although I have applications that
> test this, I will
> organize them into a system test shortly.
>
> Cheers,
> -Polar
>
--
Dan Diephouse
Envoi Solutions
http://envoisolutions.com | http://netzooid.com/blog
RE: CXF-438 Patch; HTTP(S) Trust Decision
Posted by "Glynn, Eoghan" <eo...@iona.com>.
Today is a public holiday in Ireland, but I can take a look at this
patch tomorrow.
/Eoghan
> -----Original Message-----
> From: Polar Humenn [mailto:phumenn@iona.com]
> Sent: 19 March 2007 16:29
> To: cxf-dev@incubator.apache.org
> Subject: CXF-438 Patch; HTTP(S) Trust Decision
>
> Greetings,
>
> I have submitted a patch for JIRA-CXF-438. This patch handles
> the issue of making a Trust Decision in the
> rt-transports-http module. It is attached to the JIRA issue.
> I hope someone will review. Although I have applications that
> test this, I will organize them into a system test shortly.
>
> Cheers,
> -Polar
>
Re: CXF-438 Patch; HTTP(S) Trust Decision
Posted by Fred Dushin <fr...@dushin.net>.
On Mar 21, 2007, at 4:09 AM, Glynn, Eoghan wrote:
> So its not a case of extra code inserted to get the coverage to 100%
> (which is a bit of an unrealistic goal anyway, if we had 80%
> everywhere
> we'd be doing very well and I'd be delighted). Instead it's a trivial
> bit of code required to allow the HTTPConduit be tested in isolation.
Understood -- I appreciate the difficulty.
>
> BTW speaking of testing, there doesn't seem to be any tests at all in
> Polar's TrustDecider patch. A unit test (possibly extending the
> existing
> HTTPConduitTest) is probably the least required.
Yup -- I agree. This needs a test -- Polar should provide one with
his patch.
RE: CXF-438 Patch; HTTP(S) Trust Decision
Posted by "Glynn, Eoghan" <eo...@iona.com>.
Hi Fred,
In this case, the "production code used for testing" is simply a ctor
parameter that allows us to specify a mocked up URLConnectionFactory to
be used in the unit test, instead of a real URLConnectionFactory that
would obviously go away and create real HttpURLConnections, which we
clearly do not want to do in a unit test.
In order for this ctor parameter to take effect, we also need to
suppress the creation of the real URLConnectionFactory, which is
normally done *post*-construction after the Conduit's config has been
retrieved. The reason for this is that the presence of a SSLClientPolicy
in config determines whether a Http*s*URLConnectionFactory is needed.
Hence the "if statement" Polar refers to.
So its not a case of extra code inserted to get the coverage to 100%
(which is a bit of an unrealistic goal anyway, if we had 80% everywhere
we'd be doing very well and I'd be delighted). Instead it's a trivial
bit of code required to allow the HTTPConduit be tested in isolation.
BTW speaking of testing, there doesn't seem to be any tests at all in
Polar's TrustDecider patch. A unit test (possibly extending the existing
HTTPConduitTest) is probably the least required.
Cheers,
Eoghan
> -----Original Message-----
> From: Fred Dushin [mailto:fred@dushin.net]
> Sent: 21 March 2007 00:34
> To: cxf-dev@incubator.apache.org
> Subject: Re: CXF-438 Patch; HTTP(S) Trust Decision
>
> I think it's fairly a defensible claim that having logic in
> production code that is only used for testing purposes is bad
> practice.
>
> If you need to hit 100% cvg and can't do it in a unit test,
> then consider using a system test, instead.
>
> -Fred
>
> On Mar 20, 2007, at 2:23 PM, Polar Humenn wrote:
>
> > Well, I don't understand why we have *if* statements in the
> code just
> > to support the testing of the code. But maybe I can consider it
> > "instrumentation". However, in any case, this explicit use of it
> > should be documented up the wazzzo to prevent the "Huh??? what is
> > *this* used for?" that wastes time in discovery.
>
>
Re: CXF-438 Patch; HTTP(S) Trust Decision
Posted by Fred Dushin <fr...@dushin.net>.
I think it's fairly a defensible claim that having logic in
production code that is only used for testing purposes is bad practice.
If you need to hit 100% cvg and can't do it in a unit test, then
consider using a system test, instead.
-Fred
On Mar 20, 2007, at 2:23 PM, Polar Humenn wrote:
> Well, I don't understand why we have *if* statements in the code
> just to support the testing of the code. But maybe I can consider
> it "instrumentation". However, in any case, this explicit use of it
> should be documented up the wazzzo to prevent the "Huh??? what is
> *this* used for?" that wastes time in discovery.
RE: CXF-438 Patch; HTTP(S) Trust Decision
Posted by "Glynn, Eoghan" <eo...@iona.com>.
> -----Original Message-----
> From: Polar Humenn [mailto:phumenn@iona.com]
> Sent: 20 March 2007 18:23
> To: cxf-dev@incubator.apache.org
> Subject: Re: CXF-438 Patch; HTTP(S) Trust Decision
>
> > 1. "//TODO: Revaluate whether we should really be
> enginnering for the
> > tests to succeed."
> >
> > The alternateConnectionFactory is not "engineering for the tests to
> > succeed", its engineering so that the code is testable in isolation.
> > This is basic unit testing methodology. Not some trickery to ensure
> > that tests succeed.
> >
> >
>
> Well, I don't understand why we have *if* statements in the
> code just to support the testing of the code.
The if statement is needed because the mocked up URLConnectionFactory is
specified as a ctor parameter, whereas the retrieval of the real
URLConnectionFactory (which we need to suppress in the test scenario)
must be deferred until after the config has been retrieved
post-construction. At one point, both of these tasks were accomplished
in the ctor. It was changed to the current mechanism to accommodate an
evolution of the config infrastructure. No rocket science involved :)
I don't have time for a big long discussion on testing methodology. But
here's a thought ... some unit tests must be better than none, right?
> But maybe I can
> consider it "instrumentation". However, in any case, this
> explicit use of it should be documented up the wazzzo to
> prevent the "Huh??? what is *this* used for?" that wastes
> time in discovery.
Hmmm, here's a few thoughts on documenting "up the wazzzo", presumably
via in-code commentary.
Code is validated by tests. Comments are NOT.
When code is broken by a change somewhere, the tests fail and force us
fix the breakage.
However we get no warning on "comment rot", i.e where comments fall out
of date with respect to evolving code.
Similarly if the commentary is ambiguous, open to misinterpretation or
just plain wrong in the first place. On the other hand there can be no
ambiguity in the code.
I don't have time to get into a huge discussion about this, but briefly
consider the following ideas:
- Comment-rot is much worse than no comments at all.
- Unless the code is *really, really* complex, the intent is often
clearer to a competent programmer from the code itself, as opposed to
someone else's commentary.
Note that I'm NOT of the school that would ban comments altogether from
the code. Comments are a Good Thing *if* used judiciously. As a rule of
thumb, a warning light should go off if there are more lines of
commentary than executable code.
Now in the case in point, this discussion originated from a question you
asked last week ... why do we have a unused ctor param in HTTPConduit?
But this param was not in fact unused, as could easily have been seen
via the Eclipse 'open call hierarchy' for that ctor. Now I'd argue that
the overhead of viewing the call hierarchy and reading a comment is
about the same, but the former approach has the advantage that its
guaranteed to be a correct reflection of reality, whereas a comment may
be incorrect/ambiguous/out-of-date.
And lets not get in a rant about whether we should force anyone reading
the code to use Eclipse. I was a emacs-Luddite for long enough myself :)
So I don't care what IDE folks use. Even if their idea of an IDE is vi,
there they could still get the call hierarchy with a bit of:
find trunk -name "*.java" | xargs grep "new HTTPConduit(" ...
The point being that the code itself should be the definitive source of
all knowledge.
Just some ideas to consider, and possibly ignore if you disagree. Either
way, lets not get into another time-burning discussion on this.
> > 3. " protoId.equals(HTTPS_PROTOCOL_ID)"
> >
> > Please move the HTTPS check out of HTTPConduit, to the
> > HttpsURLConnectionFactory.
> >
> >
> Aside from the fact that check won't work if the URL is
> indeed "https" at that point.
I don't get your point.
If the SSLClientPolicy is configured, then the connection factory would
be a Http*s*URLConnection. You already check in the
Http*s*URLConnection.createConnection() whether the scheme is "https":
public URLConnection createConnection(Proxy proxy, URL url)
throws IOException {
+
+ if (!url.getProtocol().equals(HTTPS_URL_PROTOCOL_ID)) {
+ throw new IOException("Illegal Protocol "
+ + url.getProtocol()
+ + " for HTTPS URLConnection Factory.");
+ }
You also check this in the implementation of URLConnectionFactory
responsible for creating plain-text HttpURLConnections:
+ public URLConnection createConnection(Proxy proxy, URL url)
+ throws IOException {
+
+ if (!url.getProtocol().equals(HTTP_URL_PROTOCOL_ID)) {
+ throw new IOException("Illegal Protocol "
+ + url.getProtocol()
+ + " for HTTP URLConnection Factory.");
+ }
So why do you need to also do this check upfront in the
HTTPConduit.send() just before the URLConnection.createConnection() is
called? The effect is to needlessly introduce HTTPS logic into the HTTP
code for *no* benefit (as this check is done anyway in the relevant
URLConnectionFactory).
Also, is it deliberate that if the SSLClientPolicy is configured, but a
"http://" URL is provided, this would no longer work? I think this would
work currently, and support the "semi-secure" client idea, i.e. use the
configured trust/keystores/ciphers etc. if the server is secure,
otherwise use plain-text HTTP. BTW I'm not sure that makes any sense,
but I know some other products do support this style of usage.
> Furthermore, this URLConnectionFactory should really be a
> HttpURLConnectionFactory emitting a HttpURLConnection or
> HttpsURLConnection.
Do you mean it should be called "HttpURLConnectionFactory"?
But its an inner class of HTTPTransportFactory that implements the
URLConnectionInterface for plain-text HttpURLConnections. Inner classes
are *anonymous* in Java. The following code:
new URLConnectionFactory() {
};
does NOT create a class called URLConnectionFactory. It creates an
*anonymous* class that implements the URLConnectionFactory interface.
And we already have a *separate* Http*s*URLConnectionFactory for
creating Http*s*URLConnection instances. This separation was the
motivation for introducing the URLConnectionFactory interface in the
first place.
> Although, I can see otherwise that if the application can
> change out the ENDPOINT_ADDRESS with something like
> "ftp://span.com/file1" then we have a problem.
>
> I don't know where to cut the cheese on this one? Is it an
> HTTP Conduit, or not?
See the explanation as to the generic URLConnection usage that I posted
to the list in response to you question last week.
> > 4. "// TODO: Question, why are we not getting a HttpURLConnection?"
> >
> > I've already answered this question on this list.
> >
>
> I really believe this has to do with the "testing"
> URLConnection factory. If it doesn't please give me a
> suitable reason why.
The intent is NOT to test the URLConnectionFactory! The idea is to
remove the real URLConnectionFactory from the mix, so that the rest of
the HTTPConduit code can be tested in isolation. This is called the mock
object pattern. It's basic tenet of unit testing.
> > 6. Does the configurer.configureBean() call for the
> > MessageTrustDecider really need to be done within the HTTPConduit
> > code? Could this instead just be pulled in as a result of the
> > configurer.configureBean() call on the HTTPConduit itself
> done within the HTTTransportFactory?
> >
> >
> It's "declarative" right? It's either there to get picked up or not.
> This kind of stuff is all over the place. What does it
> matter?
It could matter for testing. How do you intend to unit test this logic?
It would probably be easier to simply mock up the return from a call to
something like:
EndpointInfo.getTraversedExtensor(new MessageTrustDeciderPolicy(),
MessageTrustDeciderPolicy.class);
Rather than mocking up a Bus instance, Configurer instance, and register
the latter as an extension of the former.
> Do you want the TrustDecider in the HTTPConduit
> Constructor then? I don't care.
Why not just pick it up in the same way as the other configuration
policies are picked up?
On a completely separate point, shouldn't MessageTrustDeciderPolicy (or
whatever you call it) be specified in the security schema, and just
JAXB-generated like the AuthorizationPolicy etc.?
> > 8. MessageTrustDecider should probably be an interface, as it adds
> > little value as class, but prevents instances from extending some
> > other class (e.g. "GUIMessageTrustDecider extends Widget implements
> > MessageTrustDecider" would be impossible).
> >
> >
>
> Well, I had thought about it, originally I did have it as an
> interface, but then I thought about it some more and decided
> it an abstract for the better. Since it is a "Configurable"
> "Spring" "Bean" I decided that since its class has to be
> instantiated by Spring anyway it was only going to hold as
> well just carry ONLY the things that we are supposed to get
> for it. I think being an abstract class with a default
> logical name implementation is a perfect way to go.
Remember that Java is restricted to single inheritance of
implementation. So each class can only extend *one* other, but can
implement *any* number of interfaces. So a common rule of thumb is not
to waste the one shot at implementation inheritance just in order to
pick up something trivial.
/Eoghan
Re: CXF-438 Patch; HTTP(S) Trust Decision
Posted by Polar Humenn <ph...@iona.com>.
Glynn, Eoghan wrote:
> Hi Polar,
>
> A few quick points on your patch ...
>
> HTTPConduit ...
>
> 1. "//TODO: Revaluate whether we should really be enginnering for the
> tests to succeed."
>
> The alternateConnectionFactory is not "engineering for the tests to
> succeed", its engineering so that the code is testable in isolation.
> This is basic unit testing methodology. Not some trickery to ensure that
> tests succeed.
>
>
Well, I don't understand why we have *if* statements in the code just to
support the testing of the code. But maybe I can consider it
"instrumentation". However, in any case, this explicit use of it should
be documented up the wazzzo to prevent the "Huh??? what is *this* used
for?" that wastes time in discovery.
> 2. "// TODO: Invesitgate whether the name is meaning full outside of
> configuration"
>
> I don't think personal reminders like this should be committed to the
> code. Unless you intend someone else to do the investigation, in which
> case you should open a JIRA instead.
>
Okay. Fair enough.
> 3. " protoId.equals(HTTPS_PROTOCOL_ID)"
>
> Please move the HTTPS check out of HTTPConduit, to the
> HttpsURLConnectionFactory.
>
>
Aside from the fact that check won't work if the URL is indeed "https"
at that point.
Furthermore, this URLConnectionFactory should really be a
HttpURLConnectionFactory emitting a HttpURLConnection or HttpsURLConnection.
Although, I can see otherwise that if the application can change out the
ENDPOINT_ADDRESS with something like "ftp://span.com/file1" then we have
a problem.
I don't know where to cut the cheese on this one? Is it an HTTP Conduit,
or not?
> 4. "// TODO: Question, why are we not getting a HttpURLConnection?"
>
> I've already answered this question on this list.
>
I really believe this has to do with the "testing" URLConnection
factory. If it doesn't please give me a suitable reason why.
If we are indeed not dealing with HTTP(S) here, then I believe we really
need to change the name of the Conduit. URLConduit?
> 5. " // TODO: Is this cast because of the "test" URL connection
> factory?"
>
> No the cast has nothing to do with testing.
>
> And please, enough of the TODOs already :) Seriously though, 25-ish
> TODOs in the patch disrupts the readability of the code.
>
>
whatever. Okay.
> 6. Does the configurer.configureBean() call for the MessageTrustDecider
> really need to be done within the HTTPConduit code? Could this instead
> just be pulled in as a result of the configurer.configureBean() call on
> the HTTPConduit itself done within the HTTTransportFactory?
>
>
It's "declarative" right? It's either there to get picked up or not.
This kind of stuff is all over the place. What does it matter? Do you
want the TrustDecider in the HTTPConduit Constructor then? I don't care.
I just did it in the same place the HTTPConduit "configures" itself,
figuring that it was a logical place to go. Sorry.
> MessageTrustDecider ...
>
> 7. Tabs all over the place. This will break PMD/codestyle in the build.
>
>
Sorry, that's an Eclipse problem that I thought I had fixed, apparently
not. My bad.
> 8. MessageTrustDecider should probably be an interface, as it adds
> little value as class, but prevents instances from extending some other
> class (e.g. "GUIMessageTrustDecider extends Widget implements
> MessageTrustDecider" would be impossible).
>
>
Well, I had thought about it, originally I did have it as an interface,
but then I thought about it some more and decided it an abstract for the
better. Since it is a "Configurable" "Spring" "Bean" I decided that
since its class has to be instantiated by Spring anyway it was only
going to hold as well just carry ONLY the things that we are supposed to
get for it. I think being an abstract class with a default logical name
implementation is a perfect way to go.
> 9. I agree with Dan that explicit establishTrust() params for the
> endpointInfo, serviceInfo & serviceInfo is overkill. All these and more
> can be accessed from the Exchange. Why not just pass this instead?
>
>
Yeah, I agree, understanding more of the message oriented nature instead
of the RPC oriented architecture moving there really isn't any
justification to go that far. I'll just give them the Message, which has
got everything it may care to use.
Cheers,
-Polar
> /Eoghan
>
>
>> -----Original Message-----
>> From: Polar Humenn [mailto:phumenn@iona.com]
>> Sent: 19 March 2007 16:29
>> To: cxf-dev@incubator.apache.org
>> Subject: CXF-438 Patch; HTTP(S) Trust Decision
>>
>> Greetings,
>>
>> I have submitted a patch for JIRA-CXF-438. This patch handles
>> the issue of making a Trust Decision in the
>> rt-transports-http module. It is attached to the JIRA issue.
>> I hope someone will review. Although I have applications that
>> test this, I will organize them into a system test shortly.
>>
>> Cheers,
>> -Polar
>>
>>
RE: CXF-438 Patch; HTTP(S) Trust Decision
Posted by "Glynn, Eoghan" <eo...@iona.com>.
Hi Polar,
A few quick points on your patch ...
HTTPConduit ...
1. "//TODO: Revaluate whether we should really be enginnering for the
tests to succeed."
The alternateConnectionFactory is not "engineering for the tests to
succeed", its engineering so that the code is testable in isolation.
This is basic unit testing methodology. Not some trickery to ensure that
tests succeed.
2. "// TODO: Invesitgate whether the name is meaning full outside of
configuration"
I don't think personal reminders like this should be committed to the
code. Unless you intend someone else to do the investigation, in which
case you should open a JIRA instead.
3. " protoId.equals(HTTPS_PROTOCOL_ID)"
Please move the HTTPS check out of HTTPConduit, to the
HttpsURLConnectionFactory.
4. "// TODO: Question, why are we not getting a HttpURLConnection?"
I've already answered this question on this list.
5. " // TODO: Is this cast because of the "test" URL connection
factory?"
No the cast has nothing to do with testing.
And please, enough of the TODOs already :) Seriously though, 25-ish
TODOs in the patch disrupts the readability of the code.
6. Does the configurer.configureBean() call for the MessageTrustDecider
really need to be done within the HTTPConduit code? Could this instead
just be pulled in as a result of the configurer.configureBean() call on
the HTTPConduit itself done within the HTTTransportFactory?
MessageTrustDecider ...
7. Tabs all over the place. This will break PMD/codestyle in the build.
8. MessageTrustDecider should probably be an interface, as it adds
little value as class, but prevents instances from extending some other
class (e.g. "GUIMessageTrustDecider extends Widget implements
MessageTrustDecider" would be impossible).
9. I agree with Dan that explicit establishTrust() params for the
endpointInfo, serviceInfo & serviceInfo is overkill. All these and more
can be accessed from the Exchange. Why not just pass this instead?
/Eoghan
> -----Original Message-----
> From: Polar Humenn [mailto:phumenn@iona.com]
> Sent: 19 March 2007 16:29
> To: cxf-dev@incubator.apache.org
> Subject: CXF-438 Patch; HTTP(S) Trust Decision
>
> Greetings,
>
> I have submitted a patch for JIRA-CXF-438. This patch handles
> the issue of making a Trust Decision in the
> rt-transports-http module. It is attached to the JIRA issue.
> I hope someone will review. Although I have applications that
> test this, I will organize them into a system test shortly.
>
> Cheers,
> -Polar
>