[PROPOSAL] Move all facade classes to a dedicated package

[Mark Thomas <ma...@apache.org>]

All,

The root cause of bug 40770[1] is that the default catalina.policy
file does not permit reflection on the Tomcat facade classes which are
exposed to applications as HttpServletRequest, HttpSession etc.

The necessary permissions to enable reflection can only be granted per
package. Granting these permissions on the packages where the Facade
classes currently reside would expose large sections of Tomcat internals.

I therefore propose moving the following classes to a new package,
o.a.c.facade
RequestFacade
ResponseFacade
ApplicationContextFacade
StandardWrapperFacade
StandardSessionFacade

This move will allow the permissions in catalina.policy to be set to
allow reflection on this package. The impact on spec compliant web
applications should be zero. Web applications that depend on Tomcat
internals may have issues.

My intention would be to make this change in 5.5.x and 6.0.x.

If the risk of breaking existing applications that depend on Tomcat
internals is considered too great than an alternative (based on how
log4j dealt with the Category to Logger move) is to create new classes
in the o.a.c.facade package that extend the existing ones. All the
internal Tomcat code can be changed to refer to the new classes but
any existing code that tries to use the old ones will still work.

Thoughts? Comments? Objections?

Mark

[1] http://issues.apache.org/bugzilla/show_bug.cgi?id=40770

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Henri Gomez <he...@gmail.com>]

While we're discuss about change in TC 5.5, what about using the TC 6
JMX modeler in TC 5.5 ?

the JMX modeler used in Tomcat (4.1/5/5.5) use too many memory when
the Tomcat is hosting many webapps each of with many Servlets/JSPs.

We switch from TC 3.3 to 5.5 and see a memory consumption multiplied
by a factor of 2.

Using iSeries Java analyse tools, we see that memory is used mainly
used in MBeans / modeler area. Costin, Remy and Yoav suggested me to
test TC 6.x and then the memory use fall back to a normal level (just
some Mb more than a TC 3.3).

Should we consider this memory use a bug or showstopper in general
case I'm not sure, but in our company case, we had to go back to TC
3.3 until the problem is fixed.

Code is available in Tomcat 6, to make Tomcat 5.5 less memory hungry
in such situations, question is now, could we use it ?

Regards and thanks for your time

>2006/11/15, Remy Maucherat <re...@apache.org>:
> Mark Thomas wrote:
> > Thoughts? Comments? Objections?
>
> On second thought, I'm not sure it's a good idea to do it without some
> verifications. For example, if reflection is allowed on the facades,
> then it will probably allow getting the reference to the real object,
> which should not be allowed.
>
> IMO, this reflection stuff is not legitimate use.
>
> Rémy
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Sandy McArthur <sa...@apache.org>]

On 11/15/06, Remy Maucherat <re...@apache.org> wrote:
> Yoav Shapira wrote:
> > On 11/15/06, Remy Maucherat <re...@apache.org> wrote:
> >> > Because these are internal implementations, if the user is whack
> >> > enough to try and access the real object behind the facade, I'd say
> >> > they're at their own risk.
> >>
> >> I disagree: if the security manager is there, then there should be no
> >> way to get the internal object.
> >
> > You know what, on second reading I tend to agree with you.  Users who
> > are security-conscious enough to enable their security manager are
> > going to care about this.  Can we do extra checking then, to prevent
> > access to the internal objects, without compromising performance?
> >
> > Won't the fact that the internal objects behind the facade live in
> > separate packages be sufficient for the security manager to
> > automatically deny access to them?
>
> The problem is that the users are trying to introspect a concrete
> implementation class, which I think isn't right since only the public
> API should be visible to the application. If they were introspecting the
> public interface and it was not working, then I would agree to fixing
> this. Their code can be modified to be smarter about what they're
> introspecting, I think.

My initial thought when reading the original proposal is that the
client code using reflection was broken. As I understand it, if they
call the methods from the javax.servlet interfaces then the reflection
will work fine despite what the actual implementation is.

-- 
Sandy McArthur

"He who dares not offend cannot be honest."
- Thomas Paine

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Remy Maucherat <re...@apache.org>]

Yoav Shapira wrote:
> Hi,
> 
> On 11/15/06, Remy Maucherat <re...@apache.org> wrote:
>> > Because these are internal implementations, if the user is whack
>> > enough to try and access the real object behind the facade, I'd say
>> > they're at their own risk.
>>
>> I disagree: if the security manager is there, then there should be no
>> way to get the internal object.
> 
> You know what, on second reading I tend to agree with you.  Users who
> are security-conscious enough to enable their security manager are
> going to care about this.  Can we do extra checking then, to prevent
> access to the internal objects, without compromising performance?
> 
> Won't the fact that the internal objects behind the facade live in
> separate packages be sufficient for the security manager to
> automatically deny access to them?

The problem is that the users are trying to introspect a concrete 
implementation class, which I think isn't right since only the public 
API should be visible to the application. If they were introspecting the 
public interface and it was not working, then I would agree to fixing 
this. Their code can be modified to be smarter about what they're 
introspecting, I think.

Introspection will allow direct access to the field (even if it's 
private/protected), which is the main issue. In that case, there's a 
permission check. So this would need some thought, and is a bit more 
complicated than simply "change the package" (overall, I still think 
it's not legitimate, though).

Rémy

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Yoav Shapira <yo...@apache.org>]

Hi,

On 11/15/06, Remy Maucherat <re...@apache.org> wrote:
> > Because these are internal implementations, if the user is whack
> > enough to try and access the real object behind the facade, I'd say
> > they're at their own risk.
>
> I disagree: if the security manager is there, then there should be no
> way to get the internal object.

You know what, on second reading I tend to agree with you.  Users who
are security-conscious enough to enable their security manager are
going to care about this.  Can we do extra checking then, to prevent
access to the internal objects, without compromising performance?

Won't the fact that the internal objects behind the facade live in
separate packages be sufficient for the security manager to
automatically deny access to them?

Yoav

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Mark Thomas <ma...@apache.org>]

Rainer Jung wrote:
> I added a comment concerning a little experiment I did to
> 
> https://issues.apache.org/jira/browse/VELTOOLS-66
> 
> Regards,
> 
> Rainer

Thanks for this. I was planning to do this tonight but you saved me
the effort. On this basis, I am going to close the Tomcat bug as
invalid - they should be using the interface.

Thanks again,

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Rainer Jung <ra...@kippdata.de>]

I added a comment concerning a little experiment I did to

https://issues.apache.org/jira/browse/VELTOOLS-66

Regards,

Rainer

Darryl Miles schrieb:
> Remy Maucherat wrote:
>> I disagree: if the security manager is there, then there should be no
>> way to get the internal object.
> 
> Yes.  Maybe the framework is broken for not trying to reflect on the
> available interfaces for the subject instance.  Maybe by default it
> should only fail terminally if it can't find anything it can grab hold
> of (concrete class, interface, etc...) and reflect on.
> 
> Darryl
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Darryl Miles <da...@netbauds.net>]

Remy Maucherat wrote:
> I disagree: if the security manager is there, then there should be no 
> way to get the internal object.

Yes.  Maybe the framework is broken for not trying to reflect on the 
available interfaces for the subject instance.  Maybe by default it 
should only fail terminally if it can't find anything it can grab hold 
of (concrete class, interface, etc...) and reflect on.

Darryl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Remy Maucherat <re...@apache.org>]

Yoav Shapira wrote:
> Hi,
> 
> On 11/15/06, Remy Maucherat <re...@apache.org> wrote:
>> Mark Thomas wrote:
>> > Thoughts? Comments? Objections?
>>
>> On second thought, I'm not sure it's a good idea to do it without some
>> verifications. For example, if reflection is allowed on the facades,
>> then it will probably allow getting the reference to the real object,
>> which should not be allowed.
> 
> Because these are internal implementations, if the user is whack
> enough to try and access the real object behind the facade, I'd say
> they're at their own risk.

I disagree: if the security manager is there, then there should be no 
way to get the internal object.

Rémy


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Yoav Shapira <yo...@apache.org>]

Hi,

On 11/15/06, Remy Maucherat <re...@apache.org> wrote:
> Mark Thomas wrote:
> > Thoughts? Comments? Objections?
>
> On second thought, I'm not sure it's a good idea to do it without some
> verifications. For example, if reflection is allowed on the facades,
> then it will probably allow getting the reference to the real object,
> which should not be allowed.

Because these are internal implementations, if the user is whack
enough to try and access the real object behind the facade, I'd say
they're at their own risk.

> IMO, this reflection stuff is not legitimate use.

Why?  It seems fairly reasonable to me for a view-level framework like
Velocity or WebWork or Struts Action, or...  A lot of them go this
OGNL-style stack traversal and reflection is a pretty generic way to
go about it.

I think putting the facades in their own package is kind of ugly, but
I'm not sure I see an easier solution for the security mode issue.

Yoav

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Remy Maucherat <re...@apache.org>]

Mark Thomas wrote:
> Thoughts? Comments? Objections?

On second thought, I'm not sure it's a good idea to do it without some 
verifications. For example, if reflection is allowed on the facades, 
then it will probably allow getting the reference to the real object, 
which should not be allowed.

IMO, this reflection stuff is not legitimate use.

Rémy

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: [PROPOSAL] Move all facade classes to a dedicated package

[Remy Maucherat <re...@apache.org>]

Mark Thomas wrote:
> This move will allow the permissions in catalina.policy to be set to
> allow reflection on this package. The impact on spec compliant web
> applications should be zero. Web applications that depend on Tomcat
> internals may have issues.
> 
> My intention would be to make this change in 5.5.x and 6.0.x.

I don't think it's a good idea to do it for Tomcat 5.5, then.

Rémy

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org