Incorrect URL decoding in MulticastConnectionFactory$URI::parseParameters

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Incorrect URL decoding in MulticastConnectionFactory$URI::parseParameters

zbedell
Greetings all,

Pretty sure I found a bug in the way org.apache.openejb.client.MulticastConnectionFactory decodes URL parameters.  The final result of the issue is that if you use HTTP basic authentication when calling ServerServlet and openejb.ejbd.authenticate-with-request=true, you can't login with a password that contains an ampersand character.

The flow looks something like:

1) Create a new IntitialContext with PROVIDER_URL set to failover:sticky+random:https://myserver/ejb/invoke?basic.username=xyz&basic.password=pass%26word
  a) /ejb/invoke is where I have org.apache.openejb.server.httpd.ServerServlet mapped
  b) web.xml on that mapping requires BASIC auth.
  c) key part of URL is the literal password "pass&word" URL encoded with ampersand -> %26

2) TomEE internals eventually end up at HttpConnectionFactory$HttpConnection's constructor where line 76 calls:
        params = MulticastConnectionFactory.URIs.parseParamters(uri);
        By this time, various unwrapping has paired the URL down to:
        https://myserver/ejb/invoke?basic.username=xyz&basic.password=pass%26word

3) MulticastConnectionFactory...parseParameters, IE line 136:
        return uri.getQuery() == null ? new HashMap<String, String>(0) : parseQuery(stripPrefix(uri.getQuery(), "?"));

That calls URI.getQuery() which decodes the URI's query string, then passes that into parseQuery() which splits up the query parameters delimited by ampersands.  The call to URI.getQuery() is the problem.  For the above URI, the result is:
        basic.username=xyz&basic.password=pass&word
        The ampersand in the query parameter basic.password is decoded and then indistinguishable from a query parameter separator.  When passed into parseQuery, the resulting value for basic.password is just "pass".

Since MulticastConnectionFactory$URIs::parseQuery already calls URLDecoder.decode() on both name and value pairs, the call in parseParameters should be to URI::getRawQuery() instead of getQuery().  I think there's also a possible double decoding issue here which could corrupt certain values by decoding the value a second time.

For the time being, I think I can work around this by passing the authorization query parameter with the user:pass already base64 encoded.  Pretty sure this should be a complete & safe fix:


diff --git a/server/openejb-client/src/main/java/org/apache/openejb/client/MulticastConnectionFactory.java b/server/openejb-client/src/main/java/org/apache/openejb/client/MulticastConnectionFactory.java
index 22f2f86a6a..eedb54840e 100644
--- a/server/openejb-client/src/main/java/org/apache/openejb/client/MulticastConnectionFactory.java
+++ b/server/openejb-client/src/main/java/org/apache/openejb/client/MulticastConnectionFactory.java
@@ -133,7 +133,7 @@ public class MulticastConnectionFactory implements ConnectionFactory {
         }

         public static Map<String, String> parseParamters(final URI uri) throws URISyntaxException {
-            return uri.getQuery() == null ? new HashMap<String, String>(0) : parseQuery(stripPrefix(uri.getQuery(), "?"));
+            return uri.getQuery() == null ? new HashMap<String, String>(0) : parseQuery(stripPrefix(uri.getRawQuery(), "?"));
         }

         public static String stripPrefix(final String value, final String prefix) {



Best regards,
Zac Bedell


signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect URL decoding in MulticastConnectionFactory$URI::parseParameters

Romain Manni-Bucau
Hi Zachary


We have some coverage for it in
https://github.com/apache/tomee/blob/master/server/openejb-client/src/test/java/org/apache/openejb/client/HttpConnectionTest.java

Multicast not being used that often with http (more likely with ejbd) we
didnt test it, do you want to try to add a test and fix?

Le 20 sept. 2017 18:28, "Zachary Bedell" <[hidden email]> a écrit :

Greetings all,

Pretty sure I found a bug in the way
org.apache.openejb.client.MulticastConnectionFactory
decodes URL parameters.  The final result of the issue is that if you use
HTTP basic authentication when calling ServerServlet and
openejb.ejbd.authenticate-with-request=true, you can't login with a
password that contains an ampersand character.

The flow looks something like:

1) Create a new IntitialContext with PROVIDER_URL set to
failover:sticky+random:https://myserver/ejb/invoke?basic.
username=xyz&basic.password=pass%26word
  a) /ejb/invoke is where I have org.apache.openejb.server.httpd.ServerServlet
mapped
  b) web.xml on that mapping requires BASIC auth.
  c) key part of URL is the literal password "pass&word" URL encoded with
ampersand -> %26

2) TomEE internals eventually end up at HttpConnectionFactory$HttpConnection's
constructor where line 76 calls:
        params = MulticastConnectionFactory.URIs.parseParamters(uri);
        By this time, various unwrapping has paired the URL down to:
        https://myserver/ejb/invoke?basic.username=xyz&basic.
password=pass%26word

3) MulticastConnectionFactory...parseParameters, IE line 136:
        return uri.getQuery() == null ? new HashMap<String, String>(0) :
parseQuery(stripPrefix(uri.getQuery(), "?"));

That calls URI.getQuery() which decodes the URI's query string, then passes
that into parseQuery() which splits up the query parameters delimited by
ampersands.  The call to URI.getQuery() is the problem.  For the above URI,
the result is:
        basic.username=xyz&basic.password=pass&word
        The ampersand in the query parameter basic.password is decoded and
then indistinguishable from a query parameter separator.  When passed into
parseQuery, the resulting value for basic.password is just "pass".

Since MulticastConnectionFactory$URIs::parseQuery already calls
URLDecoder.decode() on both name and value pairs, the call in
parseParameters should be to URI::getRawQuery() instead of getQuery().  I
think there's also a possible double decoding issue here which could
corrupt certain values by decoding the value a second time.

For the time being, I think I can work around this by passing the
authorization query parameter with the user:pass already base64 encoded.
Pretty sure this should be a complete & safe fix:


diff --git a/server/openejb-client/src/main/java/org/apache/openejb/client/
MulticastConnectionFactory.java b/server/openejb-client/src/
main/java/org/apache/openejb/client/MulticastConnectionFactory.java
index 22f2f86a6a..eedb54840e 100644
--- a/server/openejb-client/src/main/java/org/apache/openejb/client/
MulticastConnectionFactory.java
+++ b/server/openejb-client/src/main/java/org/apache/openejb/client/
MulticastConnectionFactory.java
@@ -133,7 +133,7 @@ public class MulticastConnectionFactory implements
ConnectionFactory {
         }

         public static Map<String, String> parseParamters(final URI uri)
throws URISyntaxException {
-            return uri.getQuery() == null ? new HashMap<String, String>(0)
: parseQuery(stripPrefix(uri.getQuery(), "?"));
+            return uri.getQuery() == null ? new HashMap<String, String>(0)
: parseQuery(stripPrefix(uri.getRawQuery(), "?"));
         }

         public static String stripPrefix(final String value, final String
prefix) {



Best regards,
Zac Bedell
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect URL decoding in MulticastConnectionFactory$URI::parseParameters

zbedell
I'll give it a shot.  Running into some trouble getting stock tests to pass on Mac OS and separate problems behind a corporate proxy.  Spinning a Linux VM on the open WiFi to try.

FWIW, we're not using multicast for this.  I just looks like HttpConnectionFactory calls the URI utility methods physically located in the static URI class on MulticastConnectionFactory.  Otherwise this is https with sticky failover.

-Zac

> On Sep 20, 2017, at 12:50, Romain Manni-Bucau <[hidden email]> wrote:
>
> Hi Zachary
>
>
> We have some coverage for it in
> https://github.com/apache/tomee/blob/master/server/openejb-client/src/test/java/org/apache/openejb/client/HttpConnectionTest.java
>
> Multicast not being used that often with http (more likely with ejbd) we
> didnt test it, do you want to try to add a test and fix?
>
> Le 20 sept. 2017 18:28, "Zachary Bedell" <[hidden email]> a écrit :
>
> Greetings all,
>
> Pretty sure I found a bug in the way
> org.apache.openejb.client.MulticastConnectionFactory
> decodes URL parameters.  The final result of the issue is that if you use
> HTTP basic authentication when calling ServerServlet and
> openejb.ejbd.authenticate-with-request=true, you can't login with a
> password that contains an ampersand character.
>
> The flow looks something like:
>
> 1) Create a new IntitialContext with PROVIDER_URL set to
> failover:sticky+random:https://myserver/ejb/invoke?basic.
> username=xyz&basic.password=pass%26word
>  a) /ejb/invoke is where I have org.apache.openejb.server.httpd.ServerServlet
> mapped
>  b) web.xml on that mapping requires BASIC auth.
>  c) key part of URL is the literal password "pass&word" URL encoded with
> ampersand -> %26
>
> 2) TomEE internals eventually end up at HttpConnectionFactory$HttpConnection's
> constructor where line 76 calls:
>        params = MulticastConnectionFactory.URIs.parseParamters(uri);
>        By this time, various unwrapping has paired the URL down to:
>        https://myserver/ejb/invoke?basic.username=xyz&basic.
> password=pass%26word
>
> 3) MulticastConnectionFactory...parseParameters, IE line 136:
>        return uri.getQuery() == null ? new HashMap<String, String>(0) :
> parseQuery(stripPrefix(uri.getQuery(), "?"));
>
> That calls URI.getQuery() which decodes the URI's query string, then passes
> that into parseQuery() which splits up the query parameters delimited by
> ampersands.  The call to URI.getQuery() is the problem.  For the above URI,
> the result is:
>        basic.username=xyz&basic.password=pass&word
>        The ampersand in the query parameter basic.password is decoded and
> then indistinguishable from a query parameter separator.  When passed into
> parseQuery, the resulting value for basic.password is just "pass".
>
> Since MulticastConnectionFactory$URIs::parseQuery already calls
> URLDecoder.decode() on both name and value pairs, the call in
> parseParameters should be to URI::getRawQuery() instead of getQuery().  I
> think there's also a possible double decoding issue here which could
> corrupt certain values by decoding the value a second time.
>
> For the time being, I think I can work around this by passing the
> authorization query parameter with the user:pass already base64 encoded.
> Pretty sure this should be a complete & safe fix:
>
>
> diff --git a/server/openejb-client/src/main/java/org/apache/openejb/client/
> MulticastConnectionFactory.java b/server/openejb-client/src/
> main/java/org/apache/openejb/client/MulticastConnectionFactory.java
> index 22f2f86a6a..eedb54840e 100644
> --- a/server/openejb-client/src/main/java/org/apache/openejb/client/
> MulticastConnectionFactory.java
> +++ b/server/openejb-client/src/main/java/org/apache/openejb/client/
> MulticastConnectionFactory.java
> @@ -133,7 +133,7 @@ public class MulticastConnectionFactory implements
> ConnectionFactory {
>         }
>
>         public static Map<String, String> parseParamters(final URI uri)
> throws URISyntaxException {
> -            return uri.getQuery() == null ? new HashMap<String, String>(0)
> : parseQuery(stripPrefix(uri.getQuery(), "?"));
> +            return uri.getQuery() == null ? new HashMap<String, String>(0)
> : parseQuery(stripPrefix(uri.getRawQuery(), "?"));
>         }
>
>         public static String stripPrefix(final String value, final String
> prefix) {
>
>
>
> Best regards,
> Zac Bedell


signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect URL decoding in MulticastConnectionFactory$URI::parseParameters

Romain Manni-Bucau
then
https://github.com/apache/tomee/blob/master/server/openejb-client/src/test/java/org/apache/openejb/client/HttpConnectionTest.java#L171
was supposed to cover it but happy to get it enhanced ;)


Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://blog-rmannibucau.rhcloud.com> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
<https://javaeefactory-rmannibucau.rhcloud.com>

2017-09-20 21:17 GMT+02:00 Zachary Bedell <[hidden email]>:

> I'll give it a shot.  Running into some trouble getting stock tests to
> pass on Mac OS and separate problems behind a corporate proxy.  Spinning a
> Linux VM on the open WiFi to try.
>
> FWIW, we're not using multicast for this.  I just looks like
> HttpConnectionFactory calls the URI utility methods physically located in
> the static URI class on MulticastConnectionFactory.  Otherwise this is
> https with sticky failover.
>
> -Zac
>
> > On Sep 20, 2017, at 12:50, Romain Manni-Bucau <[hidden email]>
> wrote:
> >
> > Hi Zachary
> >
> >
> > We have some coverage for it in
> > https://github.com/apache/tomee/blob/master/server/
> openejb-client/src/test/java/org/apache/openejb/client/
> HttpConnectionTest.java
> >
> > Multicast not being used that often with http (more likely with ejbd) we
> > didnt test it, do you want to try to add a test and fix?
> >
> > Le 20 sept. 2017 18:28, "Zachary Bedell" <[hidden email]> a écrit
> :
> >
> > Greetings all,
> >
> > Pretty sure I found a bug in the way
> > org.apache.openejb.client.MulticastConnectionFactory
> > decodes URL parameters.  The final result of the issue is that if you use
> > HTTP basic authentication when calling ServerServlet and
> > openejb.ejbd.authenticate-with-request=true, you can't login with a
> > password that contains an ampersand character.
> >
> > The flow looks something like:
> >
> > 1) Create a new IntitialContext with PROVIDER_URL set to
> > failover:sticky+random:https://myserver/ejb/invoke?basic.
> > username=xyz&basic.password=pass%26word
> >  a) /ejb/invoke is where I have org.apache.openejb.server.
> httpd.ServerServlet
> > mapped
> >  b) web.xml on that mapping requires BASIC auth.
> >  c) key part of URL is the literal password "pass&word" URL encoded with
> > ampersand -> %26
> >
> > 2) TomEE internals eventually end up at HttpConnectionFactory$
> HttpConnection's
> > constructor where line 76 calls:
> >        params = MulticastConnectionFactory.URIs.parseParamters(uri);
> >        By this time, various unwrapping has paired the URL down to:
> >        https://myserver/ejb/invoke?basic.username=xyz&basic.
> > password=pass%26word
> >
> > 3) MulticastConnectionFactory...parseParameters, IE line 136:
> >        return uri.getQuery() == null ? new HashMap<String, String>(0) :
> > parseQuery(stripPrefix(uri.getQuery(), "?"));
> >
> > That calls URI.getQuery() which decodes the URI's query string, then
> passes
> > that into parseQuery() which splits up the query parameters delimited by
> > ampersands.  The call to URI.getQuery() is the problem.  For the above
> URI,
> > the result is:
> >        basic.username=xyz&basic.password=pass&word
> >        The ampersand in the query parameter basic.password is decoded and
> > then indistinguishable from a query parameter separator.  When passed
> into
> > parseQuery, the resulting value for basic.password is just "pass".
> >
> > Since MulticastConnectionFactory$URIs::parseQuery already calls
> > URLDecoder.decode() on both name and value pairs, the call in
> > parseParameters should be to URI::getRawQuery() instead of getQuery().  I
> > think there's also a possible double decoding issue here which could
> > corrupt certain values by decoding the value a second time.
> >
> > For the time being, I think I can work around this by passing the
> > authorization query parameter with the user:pass already base64 encoded.
> > Pretty sure this should be a complete & safe fix:
> >
> >
> > diff --git a/server/openejb-client/src/main/java/org/apache/openejb/
> client/
> > MulticastConnectionFactory.java b/server/openejb-client/src/
> > main/java/org/apache/openejb/client/MulticastConnectionFactory.java
> > index 22f2f86a6a..eedb54840e 100644
> > --- a/server/openejb-client/src/main/java/org/apache/openejb/client/
> > MulticastConnectionFactory.java
> > +++ b/server/openejb-client/src/main/java/org/apache/openejb/client/
> > MulticastConnectionFactory.java
> > @@ -133,7 +133,7 @@ public class MulticastConnectionFactory implements
> > ConnectionFactory {
> >         }
> >
> >         public static Map<String, String> parseParamters(final URI uri)
> > throws URISyntaxException {
> > -            return uri.getQuery() == null ? new HashMap<String,
> String>(0)
> > : parseQuery(stripPrefix(uri.getQuery(), "?"));
> > +            return uri.getQuery() == null ? new HashMap<String,
> String>(0)
> > : parseQuery(stripPrefix(uri.getRawQuery(), "?"));
> >         }
> >
> >         public static String stripPrefix(final String value, final String
> > prefix) {
> >
> >
> >
> > Best regards,
> > Zac Bedell
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect URL decoding in MulticastConnectionFactory$URI::parseParameters

zbedell
Sorry for the delay in submitting this.  I've not had much luck building & passing the unit test suite for the full TomEE build.  Currently failing with "javax.ws.rs.NotSupportedException: HTTP 415 Unsupported Media Type" in OpenEJB :: Examples :: REST XML JSON.  Is there any documentation in terms of what's expected for the build environment for TomEE?  Any Docker or other devops-ish canned configurations?  I'm trying on a fresh Ubuntu 17.04 VM with JDK 8u144 and the master branch cloned.

In any case, I got far enough for the "OpenEJB :: Server :: Client" tests to run and fail with my added unit test.  Included patch fixes the problem and passes the new unit test.  No additional failures with my patch beyond the Unsupported Media Type which failed for me with a fresh clone before changing anything.

Sent via PR #104 on Github.

Best regards,
Zac Bedell

> On Sep 20, 2017, at 15:19, Romain Manni-Bucau <[hidden email]> wrote:
>
> then
> https://github.com/apache/tomee/blob/master/server/openejb-client/src/test/java/org/apache/openejb/client/HttpConnectionTest.java#L171
> was supposed to cover it but happy to get it enhanced ;)
>
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> <https://blog-rmannibucau.rhcloud.com> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> <https://javaeefactory-rmannibucau.rhcloud.com>
>
> 2017-09-20 21:17 GMT+02:00 Zachary Bedell <[hidden email]>:
>
>> I'll give it a shot.  Running into some trouble getting stock tests to
>> pass on Mac OS and separate problems behind a corporate proxy.  Spinning a
>> Linux VM on the open WiFi to try.
>>
>> FWIW, we're not using multicast for this.  I just looks like
>> HttpConnectionFactory calls the URI utility methods physically located in
>> the static URI class on MulticastConnectionFactory.  Otherwise this is
>> https with sticky failover.
>>
>> -Zac
>>
>>> On Sep 20, 2017, at 12:50, Romain Manni-Bucau <[hidden email]>
>> wrote:
>>>
>>> Hi Zachary
>>>
>>>
>>> We have some coverage for it in
>>> https://github.com/apache/tomee/blob/master/server/
>> openejb-client/src/test/java/org/apache/openejb/client/
>> HttpConnectionTest.java
>>>
>>> Multicast not being used that often with http (more likely with ejbd) we
>>> didnt test it, do you want to try to add a test and fix?
>>>
>>> Le 20 sept. 2017 18:28, "Zachary Bedell" <[hidden email]> a écrit
>> :
>>>
>>> Greetings all,
>>>
>>> Pretty sure I found a bug in the way
>>> org.apache.openejb.client.MulticastConnectionFactory
>>> decodes URL parameters.  The final result of the issue is that if you use
>>> HTTP basic authentication when calling ServerServlet and
>>> openejb.ejbd.authenticate-with-request=true, you can't login with a
>>> password that contains an ampersand character.
>>>
>>> The flow looks something like:
>>>
>>> 1) Create a new IntitialContext with PROVIDER_URL set to
>>> failover:sticky+random:https://myserver/ejb/invoke?basic.
>>> username=xyz&basic.password=pass%26word
>>> a) /ejb/invoke is where I have org.apache.openejb.server.
>> httpd.ServerServlet
>>> mapped
>>> b) web.xml on that mapping requires BASIC auth.
>>> c) key part of URL is the literal password "pass&word" URL encoded with
>>> ampersand -> %26
>>>
>>> 2) TomEE internals eventually end up at HttpConnectionFactory$
>> HttpConnection's
>>> constructor where line 76 calls:
>>>       params = MulticastConnectionFactory.URIs.parseParamters(uri);
>>>       By this time, various unwrapping has paired the URL down to:
>>>       https://myserver/ejb/invoke?basic.username=xyz&basic.
>>> password=pass%26word
>>>
>>> 3) MulticastConnectionFactory...parseParameters, IE line 136:
>>>       return uri.getQuery() == null ? new HashMap<String, String>(0) :
>>> parseQuery(stripPrefix(uri.getQuery(), "?"));
>>>
>>> That calls URI.getQuery() which decodes the URI's query string, then
>> passes
>>> that into parseQuery() which splits up the query parameters delimited by
>>> ampersands.  The call to URI.getQuery() is the problem.  For the above
>> URI,
>>> the result is:
>>>       basic.username=xyz&basic.password=pass&word
>>>       The ampersand in the query parameter basic.password is decoded and
>>> then indistinguishable from a query parameter separator.  When passed
>> into
>>> parseQuery, the resulting value for basic.password is just "pass".
>>>
>>> Since MulticastConnectionFactory$URIs::parseQuery already calls
>>> URLDecoder.decode() on both name and value pairs, the call in
>>> parseParameters should be to URI::getRawQuery() instead of getQuery().  I
>>> think there's also a possible double decoding issue here which could
>>> corrupt certain values by decoding the value a second time.
>>>
>>> For the time being, I think I can work around this by passing the
>>> authorization query parameter with the user:pass already base64 encoded.
>>> Pretty sure this should be a complete & safe fix:
>>>
>>>
>>> diff --git a/server/openejb-client/src/main/java/org/apache/openejb/
>> client/
>>> MulticastConnectionFactory.java b/server/openejb-client/src/
>>> main/java/org/apache/openejb/client/MulticastConnectionFactory.java
>>> index 22f2f86a6a..eedb54840e 100644
>>> --- a/server/openejb-client/src/main/java/org/apache/openejb/client/
>>> MulticastConnectionFactory.java
>>> +++ b/server/openejb-client/src/main/java/org/apache/openejb/client/
>>> MulticastConnectionFactory.java
>>> @@ -133,7 +133,7 @@ public class MulticastConnectionFactory implements
>>> ConnectionFactory {
>>>        }
>>>
>>>        public static Map<String, String> parseParamters(final URI uri)
>>> throws URISyntaxException {
>>> -            return uri.getQuery() == null ? new HashMap<String,
>> String>(0)
>>> : parseQuery(stripPrefix(uri.getQuery(), "?"));
>>> +            return uri.getQuery() == null ? new HashMap<String,
>> String>(0)
>>> : parseQuery(stripPrefix(uri.getRawQuery(), "?"));
>>>        }
>>>
>>>        public static String stripPrefix(final String value, final String
>>> prefix) {
>>>
>>>
>>>
>>> Best regards,
>>> Zac Bedell
>>
>>


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect URL decoding in MulticastConnectionFactory$URI::parseParameters

Romain Manni-Bucau
Hi Zachary,

environment is just a java one, nothing special (except on Mac where the
network config can need some tuning), the "NotSupportedException: HTTP 415
Unsupported Media Type" is weird, can be related last changes on master
with dependencies maybe, our CI is available at
https://ci.apache.org/builders/tomee-trunk-ubuntu

The PR misses a test for the code change (multicast) AFAIK otherwise it
looks a good start.



Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau>

2017-10-02 19:15 GMT+02:00 Zachary Bedell <[hidden email]>:

> Sorry for the delay in submitting this.  I've not had much luck building &
> passing the unit test suite for the full TomEE build.  Currently failing
> with "javax.ws.rs.NotSupportedException: HTTP 415 Unsupported Media Type"
> in OpenEJB :: Examples :: REST XML JSON.  Is there any documentation in
> terms of what's expected for the build environment for TomEE?  Any Docker
> or other devops-ish canned configurations?  I'm trying on a fresh Ubuntu
> 17.04 VM with JDK 8u144 and the master branch cloned.
>
> In any case, I got far enough for the "OpenEJB :: Server :: Client" tests
> to run and fail with my added unit test.  Included patch fixes the problem
> and passes the new unit test.  No additional failures with my patch beyond
> the Unsupported Media Type which failed for me with a fresh clone before
> changing anything.
>
> Sent via PR #104 on Github.
>
> Best regards,
> Zac Bedell
>
> > On Sep 20, 2017, at 15:19, Romain Manni-Bucau <[hidden email]>
> wrote:
> >
> > then
> > https://github.com/apache/tomee/blob/master/server/
> openejb-client/src/test/java/org/apache/openejb/client/
> HttpConnectionTest.java#L171
> > was supposed to cover it but happy to get it enhanced ;)
> >
> >
> > Romain Manni-Bucau
> > @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> > <https://blog-rmannibucau.rhcloud.com> | Old Blog
> > <http://rmannibucau.wordpress.com> | Github <https://github.com/
> rmannibucau> |
> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
> > <https://javaeefactory-rmannibucau.rhcloud.com>
> >
> > 2017-09-20 21:17 GMT+02:00 Zachary Bedell <[hidden email]>:
> >
> >> I'll give it a shot.  Running into some trouble getting stock tests to
> >> pass on Mac OS and separate problems behind a corporate proxy.
> Spinning a
> >> Linux VM on the open WiFi to try.
> >>
> >> FWIW, we're not using multicast for this.  I just looks like
> >> HttpConnectionFactory calls the URI utility methods physically located
> in
> >> the static URI class on MulticastConnectionFactory.  Otherwise this is
> >> https with sticky failover.
> >>
> >> -Zac
> >>
> >>> On Sep 20, 2017, at 12:50, Romain Manni-Bucau <[hidden email]>
> >> wrote:
> >>>
> >>> Hi Zachary
> >>>
> >>>
> >>> We have some coverage for it in
> >>> https://github.com/apache/tomee/blob/master/server/
> >> openejb-client/src/test/java/org/apache/openejb/client/
> >> HttpConnectionTest.java
> >>>
> >>> Multicast not being used that often with http (more likely with ejbd)
> we
> >>> didnt test it, do you want to try to add a test and fix?
> >>>
> >>> Le 20 sept. 2017 18:28, "Zachary Bedell" <[hidden email]> a
> écrit
> >> :
> >>>
> >>> Greetings all,
> >>>
> >>> Pretty sure I found a bug in the way
> >>> org.apache.openejb.client.MulticastConnectionFactory
> >>> decodes URL parameters.  The final result of the issue is that if you
> use
> >>> HTTP basic authentication when calling ServerServlet and
> >>> openejb.ejbd.authenticate-with-request=true, you can't login with a
> >>> password that contains an ampersand character.
> >>>
> >>> The flow looks something like:
> >>>
> >>> 1) Create a new IntitialContext with PROVIDER_URL set to
> >>> failover:sticky+random:https://myserver/ejb/invoke?basic.
> >>> username=xyz&basic.password=pass%26word
> >>> a) /ejb/invoke is where I have org.apache.openejb.server.
> >> httpd.ServerServlet
> >>> mapped
> >>> b) web.xml on that mapping requires BASIC auth.
> >>> c) key part of URL is the literal password "pass&word" URL encoded with
> >>> ampersand -> %26
> >>>
> >>> 2) TomEE internals eventually end up at HttpConnectionFactory$
> >> HttpConnection's
> >>> constructor where line 76 calls:
> >>>       params = MulticastConnectionFactory.URIs.parseParamters(uri);
> >>>       By this time, various unwrapping has paired the URL down to:
> >>>       https://myserver/ejb/invoke?basic.username=xyz&basic.
> >>> password=pass%26word
> >>>
> >>> 3) MulticastConnectionFactory...parseParameters, IE line 136:
> >>>       return uri.getQuery() == null ? new HashMap<String, String>(0) :
> >>> parseQuery(stripPrefix(uri.getQuery(), "?"));
> >>>
> >>> That calls URI.getQuery() which decodes the URI's query string, then
> >> passes
> >>> that into parseQuery() which splits up the query parameters delimited
> by
> >>> ampersands.  The call to URI.getQuery() is the problem.  For the above
> >> URI,
> >>> the result is:
> >>>       basic.username=xyz&basic.password=pass&word
> >>>       The ampersand in the query parameter basic.password is decoded
> and
> >>> then indistinguishable from a query parameter separator.  When passed
> >> into
> >>> parseQuery, the resulting value for basic.password is just "pass".
> >>>
> >>> Since MulticastConnectionFactory$URIs::parseQuery already calls
> >>> URLDecoder.decode() on both name and value pairs, the call in
> >>> parseParameters should be to URI::getRawQuery() instead of
> getQuery().  I
> >>> think there's also a possible double decoding issue here which could
> >>> corrupt certain values by decoding the value a second time.
> >>>
> >>> For the time being, I think I can work around this by passing the
> >>> authorization query parameter with the user:pass already base64
> encoded.
> >>> Pretty sure this should be a complete & safe fix:
> >>>
> >>>
> >>> diff --git a/server/openejb-client/src/main/java/org/apache/openejb/
> >> client/
> >>> MulticastConnectionFactory.java b/server/openejb-client/src/
> >>> main/java/org/apache/openejb/client/MulticastConnectionFactory.java
> >>> index 22f2f86a6a..eedb54840e 100644
> >>> --- a/server/openejb-client/src/main/java/org/apache/openejb/client/
> >>> MulticastConnectionFactory.java
> >>> +++ b/server/openejb-client/src/main/java/org/apache/openejb/client/
> >>> MulticastConnectionFactory.java
> >>> @@ -133,7 +133,7 @@ public class MulticastConnectionFactory implements
> >>> ConnectionFactory {
> >>>        }
> >>>
> >>>        public static Map<String, String> parseParamters(final URI uri)
> >>> throws URISyntaxException {
> >>> -            return uri.getQuery() == null ? new HashMap<String,
> >> String>(0)
> >>> : parseQuery(stripPrefix(uri.getQuery(), "?"));
> >>> +            return uri.getQuery() == null ? new HashMap<String,
> >> String>(0)
> >>> : parseQuery(stripPrefix(uri.getRawQuery(), "?"));
> >>>        }
> >>>
> >>>        public static String stripPrefix(final String value, final
> String
> >>> prefix) {
> >>>
> >>>
> >>>
> >>> Best regards,
> >>> Zac Bedell
> >>
> >>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect URL decoding in MulticastConnectionFactory$URI::parseParameters

zbedell
In reply to this post by zbedell
Not sure what you mean by misses a test?  The code I modified is in the class MulticastConnectionFactory.java, but the current tests that exercise that code are in HttpConnectionTest.java.  Seems like a bit of a weird cross-over, but a pre-existing condition.  MulticastConnectionFactory contains a static subclass URIs with methods that are called by the HTTP related connection code.

I could move the test somewhere else, but there was already a test in HttpConnectionTest (httpBasicSpecificConfig) that tested the exact code I had to modify.  The existing test didn't include an ampersand in the test data.  I added a new test that does include an ampersand which fails without the modification to MulticastConnectionFactory.  Both the existing & new tests pass with the change to MulticastConnectionFactory.

It does seem a little odd that the Http connection code ends up in a class with "multicast" in the name, but that's a much deeper change than I feel comfortable attempting at this point.

-Zac


> On Oct 2, 2017, at 13:15, Zachary Bedell <[hidden email]> wrote:
>
> Sorry for the delay in submitting this.  I've not had much luck building & passing the unit test suite for the full TomEE build.  Currently failing with "javax.ws.rs.NotSupportedException: HTTP 415 Unsupported Media Type" in OpenEJB :: Examples :: REST XML JSON.  Is there any documentation in terms of what's expected for the build environment for TomEE?  Any Docker or other devops-ish canned configurations?  I'm trying on a fresh Ubuntu 17.04 VM with JDK 8u144 and the master branch cloned.
>
> In any case, I got far enough for the "OpenEJB :: Server :: Client" tests to run and fail with my added unit test.  Included patch fixes the problem and passes the new unit test.  No additional failures with my patch beyond the Unsupported Media Type which failed for me with a fresh clone before changing anything.
>
> Sent via PR #104 on Github.
>
> Best regards,
> Zac Bedell
>
>> On Sep 20, 2017, at 15:19, Romain Manni-Bucau <[hidden email]> wrote:
>>
>> then
>> https://github.com/apache/tomee/blob/master/server/openejb-client/src/test/java/org/apache/openejb/client/HttpConnectionTest.java#L171
>> was supposed to cover it but happy to get it enhanced ;)
>>
>>
>> Romain Manni-Bucau
>> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
>> <https://blog-rmannibucau.rhcloud.com> | Old Blog
>> <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
>> <https://javaeefactory-rmannibucau.rhcloud.com>
>>
>> 2017-09-20 21:17 GMT+02:00 Zachary Bedell <[hidden email]>:
>>
>>> I'll give it a shot.  Running into some trouble getting stock tests to
>>> pass on Mac OS and separate problems behind a corporate proxy.  Spinning a
>>> Linux VM on the open WiFi to try.
>>>
>>> FWIW, we're not using multicast for this.  I just looks like
>>> HttpConnectionFactory calls the URI utility methods physically located in
>>> the static URI class on MulticastConnectionFactory.  Otherwise this is
>>> https with sticky failover.
>>>
>>> -Zac
>>>
>>>> On Sep 20, 2017, at 12:50, Romain Manni-Bucau <[hidden email]>
>>> wrote:
>>>>
>>>> Hi Zachary
>>>>
>>>>
>>>> We have some coverage for it in
>>>> https://github.com/apache/tomee/blob/master/server/
>>> openejb-client/src/test/java/org/apache/openejb/client/
>>> HttpConnectionTest.java
>>>>
>>>> Multicast not being used that often with http (more likely with ejbd) we
>>>> didnt test it, do you want to try to add a test and fix?
>>>>
>>>> Le 20 sept. 2017 18:28, "Zachary Bedell" <[hidden email]> a écrit
>>> :
>>>>
>>>> Greetings all,
>>>>
>>>> Pretty sure I found a bug in the way
>>>> org.apache.openejb.client.MulticastConnectionFactory
>>>> decodes URL parameters.  The final result of the issue is that if you use
>>>> HTTP basic authentication when calling ServerServlet and
>>>> openejb.ejbd.authenticate-with-request=true, you can't login with a
>>>> password that contains an ampersand character.
>>>>
>>>> The flow looks something like:
>>>>
>>>> 1) Create a new IntitialContext with PROVIDER_URL set to
>>>> failover:sticky+random:https://myserver/ejb/invoke?basic.
>>>> username=xyz&basic.password=pass%26word
>>>> a) /ejb/invoke is where I have org.apache.openejb.server.
>>> httpd.ServerServlet
>>>> mapped
>>>> b) web.xml on that mapping requires BASIC auth.
>>>> c) key part of URL is the literal password "pass&word" URL encoded with
>>>> ampersand -> %26
>>>>
>>>> 2) TomEE internals eventually end up at HttpConnectionFactory$
>>> HttpConnection's
>>>> constructor where line 76 calls:
>>>>      params = MulticastConnectionFactory.URIs.parseParamters(uri);
>>>>      By this time, various unwrapping has paired the URL down to:
>>>>      https://myserver/ejb/invoke?basic.username=xyz&basic.
>>>> password=pass%26word
>>>>
>>>> 3) MulticastConnectionFactory...parseParameters, IE line 136:
>>>>      return uri.getQuery() == null ? new HashMap<String, String>(0) :
>>>> parseQuery(stripPrefix(uri.getQuery(), "?"));
>>>>
>>>> That calls URI.getQuery() which decodes the URI's query string, then
>>> passes
>>>> that into parseQuery() which splits up the query parameters delimited by
>>>> ampersands.  The call to URI.getQuery() is the problem.  For the above
>>> URI,
>>>> the result is:
>>>>      basic.username=xyz&basic.password=pass&word
>>>>      The ampersand in the query parameter basic.password is decoded and
>>>> then indistinguishable from a query parameter separator.  When passed
>>> into
>>>> parseQuery, the resulting value for basic.password is just "pass".
>>>>
>>>> Since MulticastConnectionFactory$URIs::parseQuery already calls
>>>> URLDecoder.decode() on both name and value pairs, the call in
>>>> parseParameters should be to URI::getRawQuery() instead of getQuery().  I
>>>> think there's also a possible double decoding issue here which could
>>>> corrupt certain values by decoding the value a second time.
>>>>
>>>> For the time being, I think I can work around this by passing the
>>>> authorization query parameter with the user:pass already base64 encoded.
>>>> Pretty sure this should be a complete & safe fix:
>>>>
>>>>
>>>> diff --git a/server/openejb-client/src/main/java/org/apache/openejb/
>>> client/
>>>> MulticastConnectionFactory.java b/server/openejb-client/src/
>>>> main/java/org/apache/openejb/client/MulticastConnectionFactory.java
>>>> index 22f2f86a6a..eedb54840e 100644
>>>> --- a/server/openejb-client/src/main/java/org/apache/openejb/client/
>>>> MulticastConnectionFactory.java
>>>> +++ b/server/openejb-client/src/main/java/org/apache/openejb/client/
>>>> MulticastConnectionFactory.java
>>>> @@ -133,7 +133,7 @@ public class MulticastConnectionFactory implements
>>>> ConnectionFactory {
>>>>       }
>>>>
>>>>       public static Map<String, String> parseParamters(final URI uri)
>>>> throws URISyntaxException {
>>>> -            return uri.getQuery() == null ? new HashMap<String,
>>> String>(0)
>>>> : parseQuery(stripPrefix(uri.getQuery(), "?"));
>>>> +            return uri.getQuery() == null ? new HashMap<String,
>>> String>(0)
>>>> : parseQuery(stripPrefix(uri.getRawQuery(), "?"));
>>>>       }
>>>>
>>>>       public static String stripPrefix(final String value, final String
>>>> prefix) {
>>>>
>>>>
>>>>
>>>> Best regards,
>>>> Zac Bedell
>>>
>>>
>


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect URL decoding in MulticastConnectionFactory$URI::parseParameters

Romain Manni-Bucau
Le 2 oct. 2017 20:54, "Zachary Bedell" <[hidden email]> a écrit :

Not sure what you mean by misses a test?  The code I modified is in the
class MulticastConnectionFactory.java, but the current tests that exercise
that code are in HttpConnectionTest.java.  Seems like a bit of a weird
cross-over, but a pre-existing condition.  MulticastConnectionFactory
contains a static subclass URIs with methods that are called by the HTTP
related connection code.

I could move the test somewhere else, but there was already a test in
HttpConnectionTest (httpBasicSpecificConfig) that tested the exact code I
had to modify.  The existing test didn't include an ampersand in the test
data.  I added a new test that does include an ampersand which fails
without the modification to MulticastConnectionFactory.  Both the existing
& new tests pass with the change to MulticastConnectionFactory.

It does seem a little odd that the Http connection code ends up in a class
with "multicast" in the name, but that's a much deeper change than I feel
comfortable attempting at this point.



Yes, this is fine for now. What i had in mind was other possible multicast
usages like ejbd protocol which can be impacted by this change. Not sure we
already cover with a unit test but should be here to avoid a regression at
least.


-Zac


> On Oct 2, 2017, at 13:15, Zachary Bedell <[hidden email]> wrote:
>
> Sorry for the delay in submitting this.  I've not had much luck building
& passing the unit test suite for the full TomEE build.  Currently failing
with "javax.ws.rs.NotSupportedException: HTTP 415 Unsupported Media Type"
in OpenEJB :: Examples :: REST XML JSON.  Is there any documentation in
terms of what's expected for the build environment for TomEE?  Any Docker
or other devops-ish canned configurations?  I'm trying on a fresh Ubuntu
17.04 VM with JDK 8u144 and the master branch cloned.
>
> In any case, I got far enough for the "OpenEJB :: Server :: Client" tests
to run and fail with my added unit test.  Included patch fixes the problem
and passes the new unit test.  No additional failures with my patch beyond
the Unsupported Media Type which failed for me with a fresh clone before
changing anything.
>
> Sent via PR #104 on Github.
>
> Best regards,
> Zac Bedell
>
>> On Sep 20, 2017, at 15:19, Romain Manni-Bucau <[hidden email]>
wrote:
>>
>> then
>> https://github.com/apache/tomee/blob/master/server/
openejb-client/src/test/java/org/apache/openejb/client/
HttpConnectionTest.java#L171
>> was supposed to cover it but happy to get it enhanced ;)
>>
>>
>> Romain Manni-Bucau
>> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
>> <https://blog-rmannibucau.rhcloud.com> | Old Blog
>> <http://rmannibucau.wordpress.com> | Github <https://github.com/
rmannibucau> |
>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory
>> <https://javaeefactory-rmannibucau.rhcloud.com>
>>
>> 2017-09-20 21:17 GMT+02:00 Zachary Bedell <[hidden email]>:
>>
>>> I'll give it a shot.  Running into some trouble getting stock tests to
>>> pass on Mac OS and separate problems behind a corporate proxy.
Spinning a
>>> Linux VM on the open WiFi to try.
>>>
>>> FWIW, we're not using multicast for this.  I just looks like
>>> HttpConnectionFactory calls the URI utility methods physically located
in

>>> the static URI class on MulticastConnectionFactory.  Otherwise this is
>>> https with sticky failover.
>>>
>>> -Zac
>>>
>>>> On Sep 20, 2017, at 12:50, Romain Manni-Bucau <[hidden email]>
>>> wrote:
>>>>
>>>> Hi Zachary
>>>>
>>>>
>>>> We have some coverage for it in
>>>> https://github.com/apache/tomee/blob/master/server/
>>> openejb-client/src/test/java/org/apache/openejb/client/
>>> HttpConnectionTest.java
>>>>
>>>> Multicast not being used that often with http (more likely with ejbd)
we

>>>> didnt test it, do you want to try to add a test and fix?
>>>>
>>>> Le 20 sept. 2017 18:28, "Zachary Bedell" <[hidden email]> a écrit
>>> :
>>>>
>>>> Greetings all,
>>>>
>>>> Pretty sure I found a bug in the way
>>>> org.apache.openejb.client.MulticastConnectionFactory
>>>> decodes URL parameters.  The final result of the issue is that if you
use

>>>> HTTP basic authentication when calling ServerServlet and
>>>> openejb.ejbd.authenticate-with-request=true, you can't login with a
>>>> password that contains an ampersand character.
>>>>
>>>> The flow looks something like:
>>>>
>>>> 1) Create a new IntitialContext with PROVIDER_URL set to
>>>> failover:sticky+random:https://myserver/ejb/invoke?basic.
>>>> username=xyz&basic.password=pass%26word
>>>> a) /ejb/invoke is where I have org.apache.openejb.server.
>>> httpd.ServerServlet
>>>> mapped
>>>> b) web.xml on that mapping requires BASIC auth.
>>>> c) key part of URL is the literal password "pass&word" URL encoded with
>>>> ampersand -> %26
>>>>
>>>> 2) TomEE internals eventually end up at HttpConnectionFactory$
>>> HttpConnection's
>>>> constructor where line 76 calls:
>>>>      params = MulticastConnectionFactory.URIs.parseParamters(uri);
>>>>      By this time, various unwrapping has paired the URL down to:
>>>>      https://myserver/ejb/invoke?basic.username=xyz&basic.
>>>> password=pass%26word
>>>>
>>>> 3) MulticastConnectionFactory...parseParameters, IE line 136:
>>>>      return uri.getQuery() == null ? new HashMap<String, String>(0) :
>>>> parseQuery(stripPrefix(uri.getQuery(), "?"));
>>>>
>>>> That calls URI.getQuery() which decodes the URI's query string, then
>>> passes
>>>> that into parseQuery() which splits up the query parameters delimited
by

>>>> ampersands.  The call to URI.getQuery() is the problem.  For the above
>>> URI,
>>>> the result is:
>>>>      basic.username=xyz&basic.password=pass&word
>>>>      The ampersand in the query parameter basic.password is decoded and
>>>> then indistinguishable from a query parameter separator.  When passed
>>> into
>>>> parseQuery, the resulting value for basic.password is just "pass".
>>>>
>>>> Since MulticastConnectionFactory$URIs::parseQuery already calls
>>>> URLDecoder.decode() on both name and value pairs, the call in
>>>> parseParameters should be to URI::getRawQuery() instead of
getQuery().  I
>>>> think there's also a possible double decoding issue here which could
>>>> corrupt certain values by decoding the value a second time.
>>>>
>>>> For the time being, I think I can work around this by passing the
>>>> authorization query parameter with the user:pass already base64
encoded.

>>>> Pretty sure this should be a complete & safe fix:
>>>>
>>>>
>>>> diff --git a/server/openejb-client/src/main/java/org/apache/openejb/
>>> client/
>>>> MulticastConnectionFactory.java b/server/openejb-client/src/
>>>> main/java/org/apache/openejb/client/MulticastConnectionFactory.java
>>>> index 22f2f86a6a..eedb54840e 100644
>>>> --- a/server/openejb-client/src/main/java/org/apache/openejb/client/
>>>> MulticastConnectionFactory.java
>>>> +++ b/server/openejb-client/src/main/java/org/apache/openejb/client/
>>>> MulticastConnectionFactory.java
>>>> @@ -133,7 +133,7 @@ public class MulticastConnectionFactory implements
>>>> ConnectionFactory {
>>>>       }
>>>>
>>>>       public static Map<String, String> parseParamters(final URI uri)
>>>> throws URISyntaxException {
>>>> -            return uri.getQuery() == null ? new HashMap<String,
>>> String>(0)
>>>> : parseQuery(stripPrefix(uri.getQuery(), "?"));
>>>> +            return uri.getQuery() == null ? new HashMap<String,
>>> String>(0)
>>>> : parseQuery(stripPrefix(uri.getRawQuery(), "?"));
>>>>       }
>>>>
>>>>       public static String stripPrefix(final String value, final String
>>>> prefix) {
>>>>
>>>>
>>>>
>>>> Best regards,
>>>> Zac Bedell
>>>
>>>
>