Re: [securityfilter-devel] Securityfilter + JBoss

[Sverker A. <sv...@ab...>]

I solved a problem I saw, I contributed the solution back to the project. It's your choice if you want to use it as it is, modify it, only parts of it or not at all.

I know of at least 5 different project which all use their own patched securityfilter to solve this same problem but none works very well. I found a perfectly working solution, I've published it so that if anybody want to use it then the patch can be downloaded from http://www.abrahamsson.com/securityfilter.patch

I also found a way to solve the original problem, without using security filter at all. I'll publish it at a suitable place when I have time for it.

See the rest of the comments below
/Sverker

> -----Original Message-----
> From: Christopher Schultz [mailto:ch...@ch...]
> Sent: den 27 april 2010 5:07
> To: Sverker Abrahamsson
> Cc: sec...@li...
> Subject: Re: [securityfilter-devel] Securityfilter + JBoss
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Sverker,
> 
> No need to reply to both me and the list: I am subscribed to it.
> 
> On 4/26/2010 6:53 PM, Sverker Abrahamsson wrote:
> > Correct, Request is the internal Catalina class for which they
> > provide a wrapper. The reason for this is that the Principal returned
> > by JBoss is a JBossGenericPrincipal so that is what is set as
> > principal in your wrapper, I believe it's the same for pure Tomcat as
> > well. To get the User Principal from that you need to call
> > principal.getUserPrincipal(). That is why I check if the principal is
> > an instance of GenericPrincipal and if so return the actual User
> > Principal.
> >
> > The reference to Tomcat Request here was just to show that internally
> Tomcat do the same
> 
> My question was: why should we change anything at all? What is the
> problem with the current implementation?

It doesn't work well with JBoss, that's problem enough for me.
 
> >>> If the Pincipal is an instance of
> >>> GenericPrincipal then it’s necessary to call getUserPrincipal on
> the
> >>> Principal to get the UserPrincipal. Also added a getPrincipal
> method
> >>> which is used internally where the full Principall is needed.
> >>
> >> None of this is necessary if you simply call
> Request.getUserPrincipal,
> >> which is why sf does that.
> >
> > Then you get a JbossGenericPrincipal instance as shown above and not
> my user principal instance.
> 
> If you are correctly using SecurityRequestWrapper, then you should be
> getting the principal created by your realm. Are you saying that, when
> using a custom realm, your Principal is being overwritten by the
> JBossGenericPrincipal?

No, it's not overwritten. My Principal is contained in the userPrincipal property of the GenericPrincipal instance returned from authentication.

> What is the difference between a "full principal" and whatever you
> think
> you're getting instead of that?
> 
> >> If you are trying to protect against users masking the UserPrincipal
> by
> >> re-wrapping the request, there are other techniques that will work
> and
> >> are more appropriate IMO.
> >
> > No, that's not the purpose at all especially since getPrincipal is
> > also public. That should maybe be protected though.
> 
> I still don't see the point of adding this method at all.

For the JBossRealm it's not necessary to use SecurityRequestWrapper as the necessary fields are set by security framework in the original request. However, that wouldn't work with other securityfilter realms as they depends on that securityfilter patch in their principal. The best way might be that the realm has a method like this:

public HttpServletRequest createRequestWrapper(HttpServletRequest request)
 
Then the realm is responsible for wrapping the request and can return an implementation which is suitable. However, that would be a bigger change of the code.

> >>> SecurityFilter
> >>> Call isUserInRole(String) on the wrapped request instead of
> directly
> >>> on the realm
> >>
> >> I think this probably makes sense. I'll have to look more closely at
> >> your patch and the code to figure out why we decided to call the
> realm
> >> directly: it's probably because the default isUserInRole method
> simply
> >> returns false.
> >
> > I changed it because in an earlier attempt to solve the problem it
> > was
> > needed to do so. In the final implementation it's not necessary as
> what
> > it does is to call the realm. A proper implementation would be that
> it
> > call isUserInRole on the underlying request, and that works with the
> > JBossRealm but I thought it might be problems with other Realm
> > implementations so I kept that requestwrapper calls hasUserRole (or
> > whatever the method name is) on the SecurityfilterRealm instance and
> in
> > JBossRealm I call the underlying request instance there.
> 
> Okay. Given that this is a security-related project, please keep your
> patches to a minimum. While I appreciate the patch, I'm not interested
> in getting the last 6 months of your tinkering-around with sf all at
> once. If you find a problem, demonstrate it, fix it, and submit the
> patch (including test cases).
>
> > That SecurityFilter call isUserInRole on request wrapper instead of
> > directly on realm makes it consistent with how an application calls
> it.
> 
> This seems like a reasonable change: the filter should call the
> request's isUserInRole method, which already delegates to the realm.
> I'll have to ask Max why he's done that.
> 
> >> Why did you remove the no-arg constructor for SimplePrincipal? The
> >> class is serializable and therefore must have a no-arg constructor.
> >
> > That is not correct
> 
> You're right: I was thinking that SimplePrincipal was being subclassed
> by other classes in sf. It's not: it's probably being subclassed by
> others, though, but you're right: the private constructor is not
> particularly useful.

It's not a problem if it's being subclassed. The only time a class, in this context, has to have an accessible no-arg constructor is if it is NOT serializable and a SUBCLASS wants to be serializable. Then all parent NON-serializable classes must have an accessible no-arg constructor.
 
> >>> In addition I removed all compiler warnings and wildcard imports in
> >>> all classes.
> >>>
> >>> It’s also needed to update catalina.jar to version 5.5.10 or newer
> >> and
> >>> add jboss-web-service.jar (I believe it was named jbossweb-
> >> service.jar
> >>> in 4.2.3 then jboss-web-service.jar from 5.0)
> >>>
> >>> The patch can be found at
> >>> http://www.abrahamsson.com/securityfilter.patch
> >>
> >> This patch cannot be accepted in its current form. In order to
> accept,
> >> you'll need to:
> >>
> >> 1. Remove all classes where no code was changed. I appreciate the
> re-
> >> formatting of the imports that your IDE performs, but we're not
> going
> >> to mass-patch files just for the sake of re-ordering import
> statements.
> >
> > The reason was not to re-format the imports but to remove all
> > wildcard imports, which are not good practice, and to remove unused.
> > It was then easiest to remove all imports where that was the case and
> > let Eclipse add them automatically.
> 
> I wouldn't say that using wildcard imports is "not good practice". The
> reality is that I'd rather not change classes that don't need changes.

The classical example of why wildcard imports are bad is this:

In jdk 1.1 it was perfectly ok to do like this:
import java.awt.*;
import java.util.*;

Compile the same code with jdk 1.2 will break, as it adds java.util.List that will then collide with java.awt.List. While that is a very old case, there is no guarantee that future jdk's will not add new classes that will break your code because of your wildcard imports.

> >> 3. Provide unit tests that demonstrate any problems with the current
> >> code and prove that your changes fix those problems.
> >
> > The problem can be trivially shown by
> > 1) SecurityFilter can't authenticate properly on JBoss container
> > 2) The principal that is returned by JBoss is an instance of
> > GenericPrincipal. It's needed that the request wrapper handles
> > extracting the user principal from it when calling getUserPrincipal()
> 
> sf tries to stay decoupled from other APIs. You'll notice that the only
> place GenericPrincipal is used is in the TrivialCatalinaRealm, which is
> separate from the main code (in examples). Likewise, the
> CatalinaRealmAdapter is in a separate set of packages from the main
> code. We're not going to re-write SecurityRealmWrapper to create a
> dependency on o.a.c.r.GenericPrincipal.
> 
> If you want to create a JBossRealmAdapter (or just use the
> CatalinaRealmAdapter directory), you're more than welcome to do so.

In the patch referenced to above it is in its own directory.
 
> > That is the actual code change, the rest is just removing compiler
> > warnings and wildcard imports which is good coding practice.
> 
> What if I told you that good coding practice was putting your curly
> braces in different places? Let's not treat opinions as facts.

Where you put your curly braces doesn't affect the compiled code. I've seen and solved bugs, real bugs, that would have been avoided if the programmer had taken warnings seriously. As you point out above, this is a security related project...
 
> >> 4. Remove the serialVersionId from SavedRequest (it's not
> necessary),
> >> which removes the whole class from the patch due to #1 above.
> Actually,
> >> remove all serialVersionIds from all files in the patch.
> >
> > Serializable classes without serialVersionUID gives a compiler
> warning.
> 
> It gives a warning from your IDE.

It gives a warning if you compile with jdk 1.5 or later
 
> > From javadoc: "However, it is strongly recommended that all
> serializable
> > classes explicitly declare serialVersionUID values"
> >
> > Hence it's good practice to have it there (although not strictly
> > needed)
> 
> What if we don't want different versions of our objects to be
> compatible
> when de-serialized? Setting the serialVersionID leave you one more
> thing
> to remember if/when you modify a class: change the serialVersionUID.
> 
> If you're looking to conduct a class on code cleanliness, you've come
> to
> the wrong place. If you're got something to offer (which it looks like
> you do), please do so. I think the best thing for you to do, here, is
> to
> use a custom realm and/or request wrapper that un-wraps the "user"
> Principal from your GenericPrincipal object. This way, the main sf code
> does not have to change.
> 
> - -chris
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (MingW32)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
> 
> iEYEARECAAYFAkvW/a0ACgkQ9CaO5/Lv0PCgNgCgw25wOrhEM2OCowxt9pRbddru
> HpwAn38cp0dHeXs/7jJxyOMKdkJES4xM
> =J7lE
> -----END PGP SIGNATURE-----