OpenSAML user discussion

[Brent Putman <>]

Hayden Bakkum [DATACOM] wrote:

Hi,

 

The following tests are failing for me when building the OpenSAML-J (v2.3.1) project:

 

Failed tests:

 (a) testMessageEndpointInvalidHost(org.opensaml.saml2.binding.decoding.HTTPRedirectDeflateDecoderTest)

 (b) testMessageEndpointInvalidHost(org.opensaml.saml1.binding.decoding.HTTPSOAP11DecoderTest)

 (c) testMessageEndpointInvalidHost(org.opensaml.saml2.binding.decoding.HTTPPostDecoderTest)

Interesting, they don't fail for me, with either Java 5 or 6 (Sun's JDK).   Perhaps your environment is different in some way.

 

I’ve isolated the cause of the failures down to the use of the URL.equals(URL) in the BaseSAMLMessageDecoder.compareEndpointURIs(String,String) method. Was there a reason for the use of the java.net.URL class here? This class is known to have a somewhat dodgy implementation in which it performs a blocking DNS lookup of the host strings during a call to URL.equals(URL) and then compares the IP addresses for equality. However, if no internet connection is available it may return a different result...

Well the intended reason was to implement the equality check so as to preserve the equivalence semantics of URL's, e.g. host name portion is case-insensitive, presence/absence of default port for scheme is insignificant. 

Technically the protocol Destination is a URI, but since virtually all cases here in SAML (for these decoders) are URL's, the impl there was optimized for URL's.  The Javadocs indicate that the compareEndpointURIs method was intended to be overridden as necessary for other URI types and so on.

I wasn't aware of the DNS lookup and IP comparison behavior there in URL equals().  I confirmed that it works as you say.  I agree that that is undesirable behavior here, at the very least for performance reasons, probably also for DNS compromise reasons.  So we should probably change the implementation of that method so that it just does an intelligent comparison of the string representation, or something similar.

I went ahead and created an issue for this change:

https://bugs.internet2.edu/jira/browse/JOST-100

 

For example, if I do the following:

 

1)      With networking enabled, run the build, unit tests fail (see above).

2)      Disable networking, run the build, no tests fail.

 

From unit test (a) above this translates to the following behaviour:

 

[networking enabled] https://bogusidp.example.com/idp/sso == https://idp.example.com/idp/sso -> test fails

[networking disabled] https://bogusidp.example.com/idp/sso != https://idp.example.com/idp/sso -> test passes

Yeah, that's odd, don't know why you are seeing the failures.  If anything, I would have expected the opposite result.  Although perhaps it's a side-effect of those being dummy hostnames.

 

I think it would be better here to use the URI class for comparing URL’s (however you would probably  need to remove default ports from the URL strings first as http://fqdn/blah != http://fqdn:80/blah for java.net.URI). Thoughts?

Right, I looked at using the URI equals() equivalence at first, but it doesn't respect the default port case as you mention for URL types, as well as the insignificant case of the host part of URL's. 

So I choose URL for those reasons, since that's the majority case for SAML, and I though it was important to preserve those URL equivalence semantics.  I'm pretty sure at least one person disagrees with me, and he's probably going to tell me so.  ;-)


Thanks,
Brent