Created attachment 1499 [details]
JNDIRealm patch

I think/hope the only contentious issue in the patch is:

  return (new GenericPrincipal(this, username, ****null**** , roles))

Javadoc for GenericPrincipal describes the password string as 'Credentials used
to authenticate this user'.  I set it to null rather than trying finding to it
from the realm because this is not necessarily what the user may have provided
for authentication, e.g the user didn't provide a password in the CLIENT-CERT
case.  This probably doesn't make much difference from trying to get it from the
realm but I think it preserves the semantics better.  Have I misunderstood?

*** Bug 12335 has been marked as a duplicate of this bug. ***

CLIENT-CERT authentication is done via SSLAuthenticator class which
executes RealmBase.authenticate(X509Certifcate[] certs) method.
This method uses getPrincipal(String username) method to return principal for 
given username. If this returs null SSLAutheticator denies to authenticate user.
For SSLAuthenticator it is only important to check if user exists in realm and
find roles becues AUTHENTICATION is done SSLAuthenticator (checking validity
od certificate)
This is my implementation for JDBCRealm.getPrincipal which works :
(If you want to consult this patch please mail me)

    /**
     * Return the Principal associated with the given user name.
     * This method is used in RealmBase.authenticate(X509Certificate[] creds)
     * which is then used in SSLAuthenticator to authenticate
     * with client with CLIENT-CERT method
     * Absence of this method (returning null) makes CLEINT-CERT authorization 
     * impossible. 
     *
     * @author Marek Mosiewicz <marek.mosiewicz@jotel.com.pl>
     */
    protected Principal getPrincipal(String username) {
        Connection dbConnection = null;

        try {
            // Ensure that we have an open database connection
            dbConnection = open();
	      
		String dbCredentials = null;	      
		PreparedStatement stmt = credentials(dbConnection, username);
		ResultSet rs = stmt.executeQuery();
	      while (rs.next()) {
            	dbCredentials = rs.getString(1).trim();
	      }
	      rs.close();
	      if (dbCredentials == null) {
            	return (null);
        	}
	      // Accumulate the user's roles
      	ArrayList list = new ArrayList();
	      stmt = roles(dbConnection, username);
	      rs = stmt.executeQuery();
	      while (rs.next()) {
      	    list.add(rs.getString(1).trim());
	      }
	      rs.close();
      	dbConnection.commit();

            // Release the database connection we just used
            release(dbConnection);

        	// Create and return a suitable Principal for this user
        	return (new GenericPrincipal(this, username, null, list));
        } catch (SQLException e) {
            // Log the problem for posterity
            log(sm.getString("jdbcRealm.exception"), e);

            // Close the connection so that it gets reopened next time
            if (dbConnection != null)
                close(dbConnection);

            // Return "null" principal
            return (null);
        }
    }

@marek:
I am not happy with this. I think a correct implementation should not use the 
Cert-Subject for the username.

I have implemented my own JNDIRealm which tries to lookup a user with the 
certificate and uses the name found for the principal-object.

So it makes no difference which certificate the user uses, or if you use BASIC 
Authentication with my JNDIRealm, for the application it is almost always the 
same user.

The dark side of this solution is, that it depends on how the LDAP-Server saves 
certificates. My solution currently works with Windows Active Directory, 
however, it should be easy to adopt it.

I have tried to discuss this on tomcat-dev (search "CLIENT-CERT and JNDI"), but 
no one has answered yet.

I am looking forward to share my thoughts.

Created attachment 6666 [details]
Discussion base for a common solution on how to authenticate clients certificates

Realm Configuration for Attachment#2 [details]

			<Realm 
className="com.ops.webcontrol.tomcat.JNDIRealmCertAD" 
				debug="99"
				connectionURL="ldap://server:389"
				userBase="CN=Users,dc=company,dc=hq"
				certSearch="(altSecurityIdentities={0})"
				certUserName="sAMAccountName"
				userSearch="(sAMAccountName={0})"
				userRoleName="member"
				roleBase="CN=Users,dc=company,dc=hq"
				roleName="cn"
				roleSearch="(member={0})"
			connectionName="CN=tomcat,CN=Users,DC=company,DC=hq"
				connectionPassword="*******"
				roleSubtree="true"
				userSubtree="true" />

For me it seems that this moule has no maitainer right now, so it is leaved "as 
is" and no is interested in this. Does aonybody knows who should we contact to 
put his changes to CVS.
Acctually in "contribution" part of Jakarta it is said that you can make patch 
but no way who should I contact - leave this patch on bugzilla maybe someone 
will pick it up.

to @Mario Ivankovits:
I think tha JDBCRealm should store DN on database (it can map DN via view if
someone needs this). Looking into LDAP is good for JNDI realm.

Marek Mosiewicz
http://www.jotel.com.pl/

I have overseen, that you talk about JDBCRealm, this bug depends on JNDIRealm.
For sure, you have to store the DN in the database, but the resulting principal 
should contain the username as username and not the DN.

A user might have multiple certificates, but it is always the same user.
Or a user revoke his certificate an creates a new one, mabe this result in a 
new DN (other provider, new e-mail address, ...)

If we do not solve this problem in the realm's we move such logik to the 
application. The results is a tomcate-user to application-user mapping, which 
(i think) should not be needet.

Created attachment 6735 [details]
2 JNDIRealms: one for LDAP userCertificate Attribute and another for Windows ActiveDirectory

Configuration Example für Attachment#3 [details]: LDAP userCertificate

<Realm className="com.ops.webcontrol.tomcat.JNDIRealmCertOpenExchange" 
				debug="99"
				connectionURL="ldap://smtp:389"
				userBase="dc=company,dc=co,dc=at"
				certSearch="(userCertificate={0})"
				certUserName="uid"
				userSearch="(uid={0})"
				roleBase="dc=company,dc=co,dc=at"
				roleName="cn"
				roleSearch="(memberUid={1})"
				connectionName="uid=cyrus,dc=ops,dc=co,dc=at"
				connectionPassword="******"
				roleSubtree="true"
				userSubtree="true" />


I think this Realms should now find there way into the tomcat distribution.

Note for LDAP userCertificate:

maybe you have to edit your slapd.conf to add
<code>index userCertificate eq</code>
and modify the <code>core.schema</code> to allow userCertificate equality match
by adding <code>EQUALITY octetStringMatch</code> to the attributetypedefinition.

You are right about multiple certificates (I use only one so it does not 
matter) I can change this code, but no one seems to pick up this code :-)

*** Bug 21115 has been marked as a duplicate of this bug. ***

Created attachment 14901 [details]
Updated version for the two Realms

Updated just to put my latest version here

I do want to add support for certificate authentication to the JNDIRealm and
your patch has given me food for thought. I am minded, however, to use your
patch as a basis for an implementation of getPrincipal() rather than over-riding
authenticate(X509Certificate).

In terms of suporting muliple LDAP servers my intention is to provide something
that works for OpenLDAP and can be over-ridden as required for other directories.

I have started to look at your patch and have the following comments. Where I
have questions, any further information you can provide will help me understand
the rationale for the approach you took.

1. I moved the classes into the o.a.c.Realm package.
2. Please keep to the coding standards of the original when copying source. It
makes it much easier to find where you have made any subtle changes.
3. CertUser looks to be unnecessary - why not use User from JNDIRealm?
4. Your changes to authenticate(String, String) appear to be unrelated to adding
support for certificates. Please keep patches for different issues separate so
they can be considered separately. Feel free to file a new bug for this one.
5. You appear to have reverted the patches for bugs 23190, 16541 and 26487. What
is the reason for this?
6. The patch for bug 22236 has also been reverted. Is this intentional?
7. If there a reason that getCertUserByPattern() isn't supported?
8. A change commiited at the same time as bug 22236 to
addAttributeValues(String, Attributes, ArrayList) that modified the return value
from null to values in a couple of places has also been reverted. Why?

> I am minded, however, to use your
> patch as a basis for an implementation of getPrincipal() rather than over-riding
> authenticate(X509Certificate).
Sorry, I dont know what you mean.
I use "authenticate(X509Certificate)", whats bad with it? Thats the place where
authentication should occur, no?
But I dont know the latest codebase, so it will be that some stuff has been changed.


> In terms of suporting muliple LDAP servers my intention is to provide something
> that works for OpenLDAP and can be over-ridden as required for other directories.
Something which my patch tries to address to.
There are implementations for ActiveDirectory and OpenExchange (I guess this is
OpenLDAP)

> 1. I moved the classes into the o.a.c.Realm package.
> 2. Please keep to the coding standards of the original when copying source. It
> makes it much easier to find where you have made any subtle changes.
Yes. Sorry for this.

> 3. CertUser looks to be unnecessary - why not use User from JNDIRealm?
I need CertUser to be able to hold both, the username and the dn of the ldap entry.
Internally it works with the "dn" but as username it will use what ever the user
configured to use.
I dont wanted to pass the rather large dn (and meaningless from the point of the
application) back to the application.

> 4. Your changes to authenticate(String, String) appear to be unrelated to adding
> support for certificates. Please keep patches for different issues separate so
> they can be considered separately. Feel free to file a new bug for this one.
As you might have seen I started coding mid 2003, so I cant remember what I
changed here, though, the best would be to make it possible to extend JNDIRealm
and change only what needed to handle the certificate stuff.
For some reason I cant rembemer this was not possible.

> 5. You appear to have reverted the patches for bugs 23190, 16541 and 26487. What
> is the reason for this?
> 6. The patch for bug 22236 has also been reverted. Is this intentional?
As I said, I started mid 2003, the last addition in 2005 is based on this rather
old version - none of those bugs were there when I started.

> 7. If there a reason that getCertUserByPattern() isn't supported?
I cant remember.

> 8. A change commiited at the same time as bug 22236 to
> addAttributeValues(String, Attributes, ArrayList) that modified the return value
> from null to values in a couple of places has also been reverted. Why?
See 5 & 6.


All in all I waited all the time to find a tomcat committer which will start
looking at it and point me to the right direction.
My "patch" was meant to be a discussion base and hopefully not that bad so we
can have a cleaned wersion sometimes in the codebase.

Now, it looks like there is one :-)

I can update the patch to the tomcat 5.5.x codebase if wanted.
E.g. starting to refactor JNDIRealm so that in JNDIRealmCertBase only the
certificate relevant stuff is included.
That way I wont mask the other patches.
It just I am so out of time, so I'll do this only when I know that you pick it up.

Ciao,
Mario

I'm happy to look at adding CLIENT-CERT support to the JNDI realm based on your
proposal. If I put together a patch are you willing to test it? If yes, I can
use 5.5.x or 4.1.x. Which would you prefer?

Cool, Thanks!

For sure I will test it.

I prefer 5.5.x

I have committed a couple of changes that should make this a lot easier.

Given the wide variety of options for mapping from certificates to users in the
directory, I have left the default as certificate DN == user DN.

To change this you will need to extend JNDIRealm and override
getPrincipal(X509Certificate). In my testing I used the following:

protected Principal getPrincipal(X509Certificate usercert) {
    StringTokenizer dnTokens =
            new StringTokenizer(usercert.getSubjectDN().getName(),",");
        
    while (dnTokens.hasMoreTokens()) {
        String token = dnTokens.nextToken(); 
        if (token.substring(0, 3).equalsIgnoreCase("cn=")) {
            return getPrincipal(token.substring(3));
        }
    }
        
    return null;
}

Since the user certificate is public, it isn't much use (on its own) as a
credential for authentication. It is the combination of the user's possession of
the private key associated with the certifcate, the subject of the certificate
and the chain of certificates back to a CA you trust that matters. Therefore, I
have not incorporated a direct comparission of the certificate presented by the
user with what may be held in the directory.

Let me know how you get on. Barring any major problems in your testing, I'll
port the changes back to 4.1.x and close this bug.

I have ported the changes to TC4. This puts everything in place to enable users
to get CLIENT-CERT working. Basic testing confirms it works as expected.

If you find anything wrong in your testing, please re-open.
If have have ideas for further enhancements, pleaes open a new bugzilla item.