Bug 28631 - JAASRealm fix to permit user-specified user/group Principals
Summary: JAASRealm fix to permit user-specified user/group Principals
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 5
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 5.0.19
Hardware: All All
: P3 normal with 1 vote (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-04-27 16:16 UTC by Andrew Jaquith
Modified: 2004-11-16 19:05 UTC (History)
0 users



Attachments
JAASRealm.patch (17.09 KB, patch)
2004-04-27 16:17 UTC, Andrew Jaquith
Details | Diff
JAASCallbackHandler.patch (4.77 KB, patch)
2004-04-27 16:17 UTC, Andrew Jaquith
Details | Diff
LocalStrings.patch (2.80 KB, patch)
2004-04-27 16:18 UTC, Andrew Jaquith
Details | Diff
Small Ant project that builds a test case WAR, dummy JAAS LoginModule JAR and associated context descriptors. (23.58 KB, application/octet-stream)
2004-09-18 16:29 UTC, Andrew Jaquith
Details
JAASRealm.java (whole file, not a diff) (21.89 KB, text/plain)
2004-09-18 16:31 UTC, Andrew Jaquith
Details
JAASCallbackHandler.java (whole file; not a diff) (5.12 KB, text/plain)
2004-09-18 16:32 UTC, Andrew Jaquith
Details
LocalStrings.properties (whole file; not a diff) (4.46 KB, text/plain)
2004-09-18 16:32 UTC, Andrew Jaquith
Details
JAASRealm.java (patch cf. tomcat 5.028 release) (17.58 KB, patch)
2004-09-18 16:36 UTC, Andrew Jaquith
Details | Diff
JAASCallbackHandler (patch cf. tomcat 5.028 release) (4.32 KB, patch)
2004-09-18 16:37 UTC, Andrew Jaquith
Details | Diff
LocalStrings.properties (patch cf. tomcat 5.028 release) (2.78 KB, patch)
2004-09-18 16:37 UTC, Andrew Jaquith
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Jaquith 2004-04-27 16:16:08 UTC
This bug report was previously posted to tomcat-dev as "[PATCH] JAASRealm fixes" on 24-Apr-2004.
----
These patches enable JAASRealm to work as advertised.
In particular, JAASRealm can use user-specified Principal classes
for both user Principals and group Principals, just like
the documentation says it is supposed to be able to do. In addition,
I have added support for digested passwords similar to the way the
other Realm implementations work.

I have patched three files: JAASRealm.java, JAASCallbackHandler.java,
and LocalStrings.properties. I have unquestionably fouled up
the @version Javadoc tags... one assumes the responsible parties
(surely not me!) can fix these prior to committing.
Comment 1 Andrew Jaquith 2004-04-27 16:17:27 UTC
Created attachment 11354 [details]
JAASRealm.patch
Comment 2 Andrew Jaquith 2004-04-27 16:17:57 UTC
Created attachment 11355 [details]
JAASCallbackHandler.patch
Comment 3 Andrew Jaquith 2004-04-27 16:18:27 UTC
Created attachment 11356 [details]
LocalStrings.patch
Comment 4 Andrew Jaquith 2004-04-27 22:39:43 UTC
I should make clear that the patch files I submitted affect 5.0.19, but were diffed against those 
from CVS HEAD, on the morning of 4/14/2004. Sorry if that was not readily apparent.
Comment 5 Yoav Shapira 2004-06-01 16:47:57 UTC
Please submit test cases for this patch if possible.
Comment 6 Yoav Shapira 2004-07-28 15:07:35 UTC
No test cases?  This item may be closed if none are submitted.
Comment 7 Andrew Jaquith 2004-07-29 02:40:24 UTC
Two issues preventing me from writing test cases:

1) It isn't clear how to do that.

There isn't much documentation on how testing works; I was a but put off by that initially. After some 
investigation, I found jakarta-tomcat-catalina/tester/src/bin/tester.xml, which indeed appears to be an 
Ant file that performs out-of-container HTTP testing. So, sure, I can see how, if the container were 
configured to use my JAASRealm patch, I could re-test the authentication tests in jakarta-tomcat-
catalina/tester/src/tester/org/apache/tester/Authentication0*.java.

Sounds simple, but it all turns on the phrase "if the container were configured to use my JAASRealm 
patch." All of the tests use, by default, uses UserDatabaseRealm. How could I make Tomcat use 
JAASRealm instead? It appears that I would need to either:
- Create a custom context descriptor for the "tester" webapp that uses JAASRealm
- Edit the existing conf/server.xml file
... as well as cause the test script to bounce the server and re-load it with the new context descriptor. 
Aha. To do this, now I need to hack the tester.xml Ant script.

I have *never* written a Tomcat test case before, so this is just my best guess on what it would take. 
Am I wrong?

2) I'm not sure what it would contain. 

Remember that JAASRealm needs an implementation of a JAAS LoginModule to function. There isn't one 
supplied with Tomcat, so this means I'd need to:
- Create a dummy LoginModule that authenticates certain hard-coded users but not others (e.g., "Fred/
bedrock" but not "Barney/feet")
- Create a "sample" LoginModule that authenticates against an existing user registry, like tomcat-
users.xml. But that seems like real work to me; I might as well port MemoryRealm to JAAS while I'm at 
it. (Which is essentially what I'd be doing). 

Frankly, this seems like a lot of bother for a patch that I *know* works; is has been in production on my 
personal wiki for about three months. I can do it if you want to, as long as you accept that this means 
that there will be quite a bit of changes to certain build files, and other things.

What is the best path forward? The key issue here is lack of flexibility in the test scripts, which don't 
permit changing Realms for exercising authentication tests.
Comment 8 Yoav Shapira 2004-07-29 13:43:55 UTC
Typical test cases submitted for Tomcat are WAR files.  They contain the code 
to be tested along with the META-INF/context.xml file which is the Context 
element with the proper declarations, e.g. for a Realm.  Don't bother using the 
Tester approach, that's wasted effort on build files.

For this case a dummy LoginModule with hard-coded user names and passwords is 
fine.  Also for this case, since the Realm has to be in a common classloader 
repository, a single WAR won't be possible, so maybe a separate JAR.

Basically, we ask for whatever you would use to unit-test your own patch.  I 
understand it works for you, and that's great, and if you don't unit-test 
that's also OK (I sometimes don't do it either, we're not perfect ;)).  But we 
do prefer to have unit tests before committing into the core Tomcat code.

I also wanted to assess your interest in this: I'm glad you responded, and 
quickly.  If I ask for tests, give it a couple of months with no response, then 
ask again and get no response within a month or so, I sometimes close the 
bugzilla issue for lack of interest.  Because you responded we can keep this 
open and resolve it.
Comment 9 Andrew Jaquith 2004-09-10 05:44:34 UTC
After a bit (!) of a hiatus, patches against the most recent release (5.0.28), and matching test cases, are 
progressing. I expect to have both submitted early next week.
Comment 10 Andrew Jaquith 2004-09-18 16:29:55 UTC
Created attachment 12761 [details]
Small Ant project that builds a test case WAR, dummy JAAS LoginModule JAR and associated context descriptors.
Comment 11 Andrew Jaquith 2004-09-18 16:31:33 UTC
Created attachment 12762 [details]
JAASRealm.java (whole file, not a diff)
Comment 12 Andrew Jaquith 2004-09-18 16:32:08 UTC
Created attachment 12763 [details]
JAASCallbackHandler.java (whole file; not a diff)
Comment 13 Andrew Jaquith 2004-09-18 16:32:41 UTC
Created attachment 12764 [details]
LocalStrings.properties (whole file; not a diff)
Comment 14 Andrew Jaquith 2004-09-18 16:36:41 UTC
Created attachment 12765 [details]
JAASRealm.java (patch cf. tomcat 5.028 release)
Comment 15 Andrew Jaquith 2004-09-18 16:37:18 UTC
Created attachment 12766 [details]
JAASCallbackHandler (patch cf. tomcat 5.028 release)
Comment 16 Andrew Jaquith 2004-09-18 16:37:47 UTC
Created attachment 12767 [details]
LocalStrings.properties (patch cf. tomcat 5.028 release)
Comment 17 Andrew Jaquith 2004-09-18 16:41:26 UTC
I've uploaded a small Ant project contains a series of test cases for the JAASRealm patches. The patches 
were made against the source tree for Tomcat 5.0.28.

First, a short explanation of what the patch itself is all about. As several observers have
noted, the current JAASRealm implementation doesn't actually deliver on all of the functionality
it promises, particularly with respect to its ability to return Principals of user-specified
types. In addition, the current implementation is limited by its inability to hash
passwords. The patch fixes both of these issues.

Files that have been modified (relative to jakarta-tomcat-catalina/catalina/src/share):
org/apache/catalina/realm/JAASRealm.java
org/apache/catalina/realm/LocalStrings.properties
org/apache/catalina/realm/JAASCallbackHandler.java

See the other attachments to this bug for the patch files, whole files, and test case tar file.
Comment 18 Yoav Shapira 2004-09-21 23:31:52 UTC
Patch applied on both TOMCAT_5_0 and HEAD (Tomcat 5.5) branches.  Thanks for 
contributing.
Comment 19 Andrew Jaquith 2004-09-30 21:54:47 UTC
It looks like the most recent commit of JAASRealm has a slight problem in it. 

The whole-file version I submitted has these lines, starting at line 536:

            if (userPrincipal == null && userClasses.contains(principalClass)) {
                userPrincipal = principal;
                if( log.isDebugEnabled() ) {
                    log.debug(sm.getString("jaasRealm.userPrincipalSuccess", principal.getName()));
                }
            }
            if (roleClasses.contains(principalClass)) {
                roles.add(principal);
                if( log.isDebugEnabled() ) {
                    log.debug(sm.getString("jaasRealm.rolePrincipalAdd", principal.getName()));
                }
            }

The current file in HEAD looks like this (also starting at 536):

            if (userPrincipal == null && userClasses.contains(principalClass)) {
                userPrincipal = principal;
                if( log.isDebugEnabled() ) {
                    log.debug(sm.getString("jaasRealm.userPrincipalSuccess", principal.getName()));
                }
            }

            if (roleClasses.contains(principalClass)) {
                roles.add(principal.getName());
            }

            if (roleClasses.contains(principalClass)) {
                roles.add(principal);
                if( log.isDebugEnabled() ) {
                    log.debug(sm.getString("jaasRealm.rolePrincipalAdd", principal.getName()));
                }
            }

The middle chunk of code (3 lines) shouldn't be there. It is adding a String to what is supposed to be a 
collection of Principal objects. This is causing downstream calls to hasRole(Principal, String) to choke 
and die with a ClassCastException.

The patch file I submitted also has this middle chunk marked for removal (-), so I think this was just a 
mistake in the commit process.
Comment 20 Yoav Shapira 2004-10-01 12:31:20 UTC
My mistake -- good catch.  Thank you for pointing it out.  I've fixed it on 
both HEAD and TOMCAT_5_0.