CatalinaContainer majorly broken

[Adam Heath-2]

The problem I am attempting to describe below is a bit complex.  I
hope I can explain it well enough.

The catalina documentation has support for ssl accelerators.  An
accelerator sits in front of catalina, doing the
encryption/decryption.  However, for the application website to
continue working, the accelerator must somehow communicate to the
backend that the unencrypted backend communication pipe is actually
indeed secure.

The catalina docs say this:  Create a <Connector>, set the protocol to
"http", and the secure flag to "true".

So, in ofbiz-containers.xml, it is simple enought to do this.
However, due to several bad designs, this doesn't work at all.

First, in CatalinaContainer.createConnector, there is an
if/elseif/else block, that effectively removes the 'version' from the
protocol.  Also, if you specify 'http/1.1' as the protocol, as
outlined in the catalina docs, and set secure=true, that if block will
end up setting the protocol to https.  This means it's not possible to
have an unencrypted secure communication pipe.  Fixing this bug is
simple; disable the entire if block.

The second bug, is a side-effect of that if block.  It effectively
would remove the trailing '/$version' from the protocol value.
Passing that value directly into catalina makes catalina throw an
exception.  That bug is fixed by specifying 'ajp', 'http', or 'https'
directly in ofbiz-containers.xml.

The final bug was the really difficult one to track down.  The
free-form properties in ofbiz-containers.xml are processed by calling
catalina's Connector.setProperty(name, value).  However, that code in
catalina *only* configures the wrapped ProtocolHandler, and does *not*
configure *anything* in Connector itself.  This means, that when
setting secure=true, the ProtocolHandler is updated, but *not* the
Connector.

Later on, when catalina attempts to process the incoming request, it
handles the unecrypted http protocol correctly, but because the
Connector has secure=false(while the protocol itself thinks
secure=true), the Request/CoyoteAdapter/BaseRequest classes end up
returning false for ServletRequest.isSecure().  This means backend
webserver code can never work correctly in ssl-accelerator mode.

Technically, Connector.setProperty should *not* be public.  Ofbiz
should not really be calling that method.  Catalina itself uses an xml
Digester, which parses the server.xml, and calls bean methods on
Connector.  The correct fix  here is to move *all* catalina
configuration out of ofbiz-containers.xml, and use bog-standard
server.xml.

I have the first 2 bugs fixed in ofbiz(in a client site, not yet ready
to forward-port those changes to trunk).  The third bug I have hacked
around in catalina itself(Connector.setProperty checks for secure, and
updates it's local secure instance variable).

My question for the community, is whether we want to use server.xml
instead of ofbiz-containers.xml.  I am leaning towards yes.

[Adrian Crum-3]

I've always thought the ofbiz-containers.xml approach was too
simplistic, and a duplication of existing functionality. From my
perspective, it would be better if ofbiz-containers.xml simply specified
the container class, and it should have only one property - a pointer to
the folder where the container's configuration files are found.

-Adrian

On 8/16/2011 11:49 PM, Adam Heath wrote:

> The problem I am attempting to describe below is a bit complex.  I
> hope I can explain it well enough.
>
> The catalina documentation has support for ssl accelerators.  An
> accelerator sits in front of catalina, doing the
> encryption/decryption.  However, for the application website to
> continue working, the accelerator must somehow communicate to the
> backend that the unencrypted backend communication pipe is actually
> indeed secure.
>
> The catalina docs say this:  Create a<Connector>, set the protocol to
> "http", and the secure flag to "true".
>
> So, in ofbiz-containers.xml, it is simple enought to do this.
> However, due to several bad designs, this doesn't work at all.
>
> First, in CatalinaContainer.createConnector, there is an
> if/elseif/else block, that effectively removes the 'version' from the
> protocol.  Also, if you specify 'http/1.1' as the protocol, as
> outlined in the catalina docs, and set secure=true, that if block will
> end up setting the protocol to https.  This means it's not possible to
> have an unencrypted secure communication pipe.  Fixing this bug is
> simple; disable the entire if block.
>
> The second bug, is a side-effect of that if block.  It effectively
> would remove the trailing '/$version' from the protocol value.
> Passing that value directly into catalina makes catalina throw an
> exception.  That bug is fixed by specifying 'ajp', 'http', or 'https'
> directly in ofbiz-containers.xml.
>
> The final bug was the really difficult one to track down.  The
> free-form properties in ofbiz-containers.xml are processed by calling
> catalina's Connector.setProperty(name, value).  However, that code in
> catalina *only* configures the wrapped ProtocolHandler, and does *not*
> configure *anything* in Connector itself.  This means, that when
> setting secure=true, the ProtocolHandler is updated, but *not* the
> Connector.
>
> Later on, when catalina attempts to process the incoming request, it
> handles the unecrypted http protocol correctly, but because the
> Connector has secure=false(while the protocol itself thinks
> secure=true), the Request/CoyoteAdapter/BaseRequest classes end up
> returning false for ServletRequest.isSecure().  This means backend
> webserver code can never work correctly in ssl-accelerator mode.
>
> Technically, Connector.setProperty should *not* be public.  Ofbiz
> should not really be calling that method.  Catalina itself uses an xml
> Digester, which parses the server.xml, and calls bean methods on
> Connector.  The correct fix  here is to move *all* catalina
> configuration out of ofbiz-containers.xml, and use bog-standard
> server.xml.
>
> I have the first 2 bugs fixed in ofbiz(in a client site, not yet ready
> to forward-port those changes to trunk).  The third bug I have hacked
> around in catalina itself(Connector.setProperty checks for secure, and
> updates it's local secure instance variable).
>
> My question for the community, is whether we want to use server.xml
> instead of ofbiz-containers.xml.  I am leaning towards yes.
>

[Adam Heath-2]

On 08/16/2011 06:12 PM, Adrian Crum wrote:
> I've always thought the ofbiz-containers.xml approach was too
> simplistic, and a duplication of existing functionality. From my
> perspective, it would be better if ofbiz-containers.xml simply specified
> the container class, and it should have only one property - a pointer to
> the folder where the container's configuration files are found.

s/simplistic/complex/.

But yes, your suggestion gives me an idea of how to implement this
more correctly.

[Adrian Crum-3]

I meant simplistic in the idea that a simple list of properties will
satisfy the configuration needs of all containers.

-Adrian

On 8/17/2011 12:15 AM, Adam Heath wrote:

> On 08/16/2011 06:12 PM, Adrian Crum wrote:
>> I've always thought the ofbiz-containers.xml approach was too
>> simplistic, and a duplication of existing functionality. From my
>> perspective, it would be better if ofbiz-containers.xml simply specified
>> the container class, and it should have only one property - a pointer to
>> the folder where the container's configuration files are found.
> s/simplistic/complex/.
>
> But yes, your suggestion gives me an idea of how to implement this
> more correctly.