Bug 52500 - Improve client certificate authentication
Improve client certificate authentication
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Catalina
7.0.23
All All
: P2 enhancement (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2012-01-23 13:02 UTC by Michael
Modified: 2012-06-02 21:08 UTC (History)
0 users



Attachments
Java Code (19.02 KB, patch)
2012-01-23 13:20 UTC, Michael
Details | Diff
Examples of x509 authentication (142.55 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2012-01-23 13:21 UTC, Michael
Details
The document that describe client certificate and its configuration (149.91 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2012-01-24 15:48 UTC, Michael
Details
x509 java code (24.29 KB, patch)
2012-01-24 15:49 UTC, Michael
Details | Diff
x509 java code (22.70 KB, patch)
2012-01-30 20:28 UTC, Michael
Details | Diff
The document that describe client certificate and its configuration (20.85 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2012-01-30 20:29 UTC, Michael
Details
x509 java code (19.34 KB, patch)
2012-02-13 07:50 UTC, Michael
Details | Diff
The document that describe client certificate and its configuration (114.63 KB, application/rar)
2012-02-13 07:51 UTC, Michael
Details
x509 java code (15.61 KB, patch)
2012-02-18 08:38 UTC, Michael
Details | Diff
Realm configuration document (2.80 KB, application/octet-stream)
2012-02-18 08:39 UTC, Michael
Details
x509 java code (15.89 KB, patch)
2012-02-26 20:38 UTC, Michael
Details | Diff
Realm configuration (2.80 KB, patch)
2012-02-26 20:39 UTC, Michael
Details | Diff
Suggested patch for x509 configuration (4.71 KB, application/octet-stream)
2012-03-11 19:47 UTC, Michael
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael 2012-01-23 13:02:04 UTC

    
Comment 1 Michael 2012-01-23 13:20:15 UTC
Created attachment 28190 [details]
Java Code

Code that allow to improve current implementation of the client certificate authentication (SSLAuthenticator)
Comment 2 Michael 2012-01-23 13:21:33 UTC
Created attachment 28191 [details]
Examples of x509 authentication
Comment 3 Michael 2012-01-23 13:22:16 UTC
Hi!
I want to improve current implementation of the client certificate authentication (SSLAuthenticator)
Current implementation of SSLAuthenticator takes the user identifier from the whole SubjectDN.
During my work with our enterprise customers I have found the following:
1)	It is common case when the user identifier is stored in the Subject Alternative Name of the client certificate.
2)	When the user identifier is stored in the SubjectDN of the client certificate it can be stored in e-mail, CN or in the hold SubjectDN
See the attached x509Examples.docx
I recommend to improve the Tomcat client certificate authentication and allow to take user identifier from the Subject Alternative Name or from SubjectDN. 
The attached code allows to do it using the following configuration:
<Context>
	<Valve className="org.apache.catalina.authenticator.SSLAuthenticator" userIdentifierRetrieveField="SubjectAlternativeName"  userIdentifierRetrieveFieldPart="otherName" />  
</Context>

<Context>
	<Valve className="org.apache.catalina.authenticator.SSLAuthenticator" userIdentifierRetrieveField="SubjectDN"  userIdentifierRetrieveFieldPart="e" />  
</Context>


The attached code already works more than year at production in more than one customer. 
Open issues:
1)	The implementation of SubjectAlternativeNameRetriever depend on bouncy castel jar. If it is a problem possible to get all required source files into Tomcat source files. Alternatively, it is possible not to support Subject Alternative Name – but it is very common use case
2)	I am not sure if the configuration above is the best option – possible to discuss it
I will happy for your comments.
Best regards,
   Michael
Comment 4 Mark Thomas 2012-01-23 13:32:33 UTC
Patches should be provided in diff -u format against, in preference order:
- trunk
- 7.0.x/trunk
- 7.0.x/tags/<latest release>
- 7.0.x/tags/<other release>

The intended way to do this is to override the Realm implementation and provide an alternative implementation of getPrincipal(X509Certificate).

I'd be prepared to consider changes to RealmBase to provide options for extracting the user name from the certificate but I am -1 on doing this in the Authenticators.

An additional dependency on bouncy castle is not acceptable. On that topic, what is wrong with X509Certificate.getSubjectAlternativeNames() that has been present since Java 1.4?
Comment 5 Mark Thomas 2012-01-23 13:34:55 UTC
Moving to an enhancement.
Comment 6 Michael 2012-01-23 19:28:08 UTC
Dear Mark,
Thank you for the fastest comment!

>Patches should be provided in diff -u format against, in preference order:

I will try to do it when we will finalize patch.



>The intended way to do this is to override the Realm implementation and >provide an alternative implementation of getPrincipal(X509Certificate).

I have tried to explore the best way to provide the patch.
All realms in Tomcat extend RealmBase.
Do you suggest to create the new realm that will extend RealmBase (with the new implementation of getPrincipal) and all realms will extend the realm? 
Or do you want to override each realm?




>I'd be prepared to consider changes to RealmBase to provide options for
>extracting the user name from the certificate but I am -1 on doing this in >the Authenticators.

I just need Authenticator for the configuration and I need your help with the realm configuration.

Can you explain me how can I configure realms?

>An additional dependency on bouncy castle is not acceptable. On that topic,
>what is wrong with X509Certificate.getSubjectAlternativeNames() that has 
>been present since Java 1.4?

The SubjectAlternativeNameRetriever class uses X509Certificate.getSubjectAlternativeNames()
Unfortunately, generally the SubjectAlternativeName is stored in ASNDerEncodedByteArray
I use the bouncy castle classes to convert the value to string. Is bouncy castle open source?
Why it is not possible to copy 5 sources in the Tomcat baseline?
Alternatively, do you know in Apache foundation a library that provides the following services:
ASN1InputStream
ASN1Sequence
ASN1TaggedObject
DERObject
DERUTF8String

Thanks and best regards,
   Michael
Comment 8 Mark Thomas 2012-01-24 11:03:28 UTC
The suggestion is to make all Realm's more configurable be modifying RealmBase. One option would be:
- define an interface for transforming certs to user names
- allow users to write their own implementations of this interface
- add an attribute to RealmBase that allows custom implementations of this transform to be specified (by full class name)
- modify RealmBase to use this transformation
- provide a default transformation that is the same as the current code

See the Tomcat docs for how to configure Realms.

Bouncy castle is licensed under terms that would allow us to use it in Tomcat but I am -1 for adding a dependency for this feature. I would much rather add the extension point as outlined above and allow users to implement whatever they need using whatever libraries they choose.

At the moment, I am +0 for adding additional transformations such such as the one proposed here. My final view would depend on how much demand there was for the feature, how much new code was required and how complex it was.
Comment 9 Michael 2012-01-24 15:48:35 UTC
Created attachment 28199 [details]
The document that describe client certificate and its configuration
Comment 10 Michael 2012-01-24 15:49:44 UTC
Created attachment 28200 [details]
x509 java code

Diff from trunk\java
Comment 11 Michael 2012-01-24 15:51:11 UTC
Dear Mark,
Thank you for the comments!
- I have defined an interface for transforming certs to user names - UserIdentifierRetriever
- I have added an attribute to RealmBase that allows custom implementations of this transform to be specified (by full class name) - x509UserIdentifierRetrieverClassName
- I have modified RealmBase to use this transformation
- I have provided a default transformation that is the same as the current code - DefaultSubjectDnRetriever 

Any case, I strongly recommend to add additional transformations out of the box since it will allow to use Tomcat for the client certificate authentication.
It will allow easy configuration to do it. See and the examples below: 
<Realm className="…" x509UserIdentifierRetrieveField="SubjectAlternativeName"  x509UserIdentifierRetrieveFieldPart="otherName" />
<Realm className="…" x509UserIdentifierRetrieveField="SubjectDN"  x509UserIdentifierRetrieveFieldPart="e" />
I also can contribute the attached x509Configuration.docx for better explanation.
In addition, I strongly recommend to add out of the box SubjectAlternativeNameRetriever. It is used by big enterprise customers. For example, you can not take information from CAC (http://www.cac.mil/) without SubjectAlternativeNameRetriever 
As I stated before, I can copy 5 required classes from geronimo.apache.org to Tomcat baseline if you do not have objections.
Please find attached diff for asf/tomcat/trunk
My questions / Action items:
1)	I do not like the place that I use now to call to createUserIdentifierRetriever method
I want to call it when Realm is instantiated and all properties are set.
I need method like afterPropertiesSet of Spring :)
Do you have something like this for Realm?
2)	I need to add Javadocs for the new methods in Realm.
I will happy for your comments.
Best regards,
   Michael
Comment 12 Michael 2012-01-26 04:22:58 UTC
Dear Mark,
On the second mind: if you will agree for my suggestion to provide OOTB both SubjectAlternativeNameRetriever and SubjectDnRetriever you will not need to support users transforms by full class name.
The SubjectAlternativeNameRetriever and SubjectDnRetriever provide all required information from x509 certificate.
I do not believe it will be required to add transforms by users.
I will happy for your comments.
Best regards,
   Michael
Comment 13 Michael 2012-01-30 20:27:30 UTC
Dear Mark,
I still need your inputs on my suggestions.
1)	I do not understand if you want to use SubjectAlternativeNameRetriever OOTB
2)	I do not understand if you want support user transforms by full class name.
3)	I do not know the best way to call to createUserNameRetriever method

Please find attached patch with the following modifications:
1)	UserIdentifier was replaced by UserName in all places
2)	DefaultSubjectDnRetriever was removed since SubjectDnRetriever with the default constructor performs the same job.
Once again: 
1)	SubjectAlternativeNameRetriever and SubjectDnRetriever provide all required information from x509 certificate.
2)	Classes work already in production
3)	It works for the following browsers: IE, FF and Chrome

I will more than happy for your comments.
Best regards,
   Michael
Comment 14 Michael 2012-01-30 20:28:45 UTC
Created attachment 28237 [details]
x509 java code

Please find attached patch with the following modifications:
1)	UserIdentifier was replaced by UserName in all places
2)	DefaultSubjectDnRetriever was removed since SubjectDnRetriever with the default constructor performs the same job.
Comment 15 Michael 2012-01-30 20:29:50 UTC
Created attachment 28238 [details]
The document that describe client certificate and its configuration

UserIdentifier was replaced by UserName in all places
Comment 16 Christopher Schultz 2012-02-01 20:57:49 UTC
(In reply to comment #11)
> - I have provided a default transformation that is the same as the current code
> - DefaultSubjectDnRetriever 
> 
> Any case, I strongly recommend to add additional transformations out of the box
> since it will allow to use Tomcat for the client certificate authentication.
> It will allow easy configuration to do it. See and the examples below: 
> <Realm className="…" x509UserIdentifierRetrieveField="SubjectAlternativeName" 
> x509UserIdentifierRetrieveFieldPart="otherName" />

I think the idea was that you would be able to configure the realm like this:

<Realm className="..." x509UserIdentifierRetriever="....SubjectDnRetriever" />

(Where my example shown above is the default)

That way, the x509UserItentifierRetriever can support whatever requirements are necessary in the deployment environment, rather than having a large list of attributes for RealmBase to juggle-around.

> I also can contribute the attached x509Configuration.docx for better
> explanation.

In the future, please provide more democratic documentation. For instance, plain-text or OpenDocument format. Plain text is better because it does not require a viewer external to the we browser.

> In addition, I strongly recommend to add out of the box
> SubjectAlternativeNameRetriever.

Let's get the interface nailed-down first, then we can implement as many UserIdentifierRetrievers as are appropriate.
Comment 17 Michael 2012-02-02 18:31:36 UTC
Dear Christopher,
Thank you for your comments!
I need couple of clarifications
1)	I do not like the place that I use now to call to createUserIdentifierRetriever method
I want to call it when Realm is instantiated and all properties are set.
I need method like afterPropertiesSet of Spring or like javax.annotation.PostConstruct.
Do you have something like this for Realm?
2)
>In the future, please provide more democratic documentation. For instance,
>plain-text or OpenDocument format. Plain text is better because it does not
>require a viewer external to the we browser.

How can I provide a plain text together with pictures?
Do you know a tool for conversion from MS-Word documents to OpenDocument format?
 I will more than happy for your comments.
Best regards,
   Michael
Comment 18 Christopher Schultz 2012-02-03 14:48:40 UTC
(In reply to comment #17)
> 1)    I do not like the place that I use now to call to
> createUserIdentifierRetriever method
> I want to call it when Realm is instantiated and all properties are set.
> I need method like afterPropertiesSet of Spring or like
> javax.annotation.PostConstruct.
> Do you have something like this for Realm?

RealmBase (which you should be using) implements the Lifecycle interface which gets all kinds of notifications about lifecycle events. Read the javadoc for that interface to see what event is most appropriate in your estimation.

> 2)
> >In the future, please provide more democratic documentation. For instance,
> >plain-text or OpenDocument format. Plain text is better because it does not
> >require a viewer external to the we browser.
> 
> How can I provide a plain text together with pictures?

Multiple attachments?

> Do you know a tool for conversion from MS-Word documents to OpenDocument
> format?

OpenOffice.org, LibreOffice, or just about any software that can read Microsoft file formats and write-out OpenDocument. They are pretty much all completely free to download and use.
Comment 19 Michael 2012-02-13 07:50:05 UTC
Created attachment 28308 [details]
x509 java code
Comment 20 Michael 2012-02-13 07:51:03 UTC
Created attachment 28309 [details]
The document that describe client certificate and its configuration
Comment 21 Michael 2012-02-13 07:51:50 UTC
Dear Christopher,
Sorry for the delay!
Please find attached patch that provide OOTB UserNameRetriever that retrieve the user name from SubjectDN without any additional dependency.
I have added the UserNameRetrieverDecorator class that allows to load the user provided UserNameRetriever. In addition, I have added the UserNameRetrieverConfiguration interface that allow to configure the user provided UserNameRetriever
Please find the attached html file – I promise to convert it to the simple txt file when the patch fill be finalized.
 Waiting for your comments.

Best regards,
   Michael
Comment 22 Christopher Schultz 2012-02-13 20:51:40 UTC
> Please find attached patch that provide OOTB UserNameRetriever that retrieve
> the user name from SubjectDN without any additional dependency.

That looks great.

> I have added the UserNameRetrieverDecorator class that allows to load the user
> provided UserNameRetriever. In addition, I have added the
> UserNameRetrieverConfiguration interface that allow to configure the user
> provided UserNameRetriever

I'm not sure why either of these are necessary. I think that UserNameRetriever (maybe a better name would be X509UserNameRetriever now that I think about it) interface, the SubjectDNRetriever, and minimal changes to RealmBase.

Note that no changes are required to the Realm interface: the selection of a UserNameRetriever is an implementation detail that can safely be left in RealmBase.

> Please find the attached html file – I promise to convert it to the simple txt
> file when the patch fill be finalized.

The best thing to do is to have a decent patch against the Tomcat configuration documentation. Look at the file webapps/docs/config/realm.xml for the source to the current "Realm" configuration page: that's the proper place to document the new configuration attributes and describe how they can be used. Also, the javadocs should contain similar information (although obviously not XML-related because XML isn't part of the API itself). Basically, no documentation should be required that isn't part of your patch.

I'm happy to commit your patch with the above changes. If you'd like to take another crack at an updated patch, that's fine, too. If you do submit another one, please don't include "@author" tags in the source files: it's been our policy for some time not to include @author tags, though there certainly are many in the code that have been there for a long time and might not be purged just because nobody cares enough to do so. Don't worry, you'll get your name into the change log :)
Comment 23 Michael 2012-02-14 08:00:19 UTC
Dear Christopher,
Thank you for the fast reply!

>That looks great.
Thanks!



>I'm not sure why either of these are necessary. 
>I think that UserNameRetriever (maybe a better name 
>would be X509UserNameRetriever now that I think about it)
>interface, the SubjectDNRetriever, and minimal changes to RealmBase.


I am confused. According to my understanding, we want to provide the ability to use a user provided X509UserNameRetriever.

The purpose of UserNameRetrieverDecorator is to return the user name by the default X509UserNameRetriever if the X509UserNameRetriever provided by a user will return the empty user name.

I can move the UserNameRetrieverDecorator code to RealmBase, but I think it is clearer if it is in the different class.

Please tell me what you think about it.


Regarding UserNameRetrieverConfiguration – it allow easy configuration of a user provided X509UserNameRetriever.

I think it is very useful if you create your own X509UserNameRetriever.
Please tell me what you think about it.




>Note that no changes are required to the Realm interface: the selection of a
>UserNameRetriever is an implementation detail that can safely be left in
>RealmBase.

Ok, got it.

> If you do submit another one, please don't include 
>"@author" tags in the source files:

I will provide another patch upon your comments without the @author tag and with X509UserNameRetriever name.




>Look at the file webapps/docs/config/realm.xml for the source to
>the current "Realm" configuration page:
> that's the proper place to document the new configuration 
> attributes and describe how they can be used.

Ok, I will do it. I think to add the new configuration attributes into the Common Attributes section.

Please tell me what you think about it.




>Basically, no documentation should be required 
>that isn't part of your patch.


So, I will not provide the client certificate description. Correct?
Waiting for your comments.

Best regards,
   Michael
Comment 24 Christopher Schultz 2012-02-14 17:49:48 UTC
Michael,
> >I'm not sure why either of these are necessary. 
> >I think that UserNameRetriever (maybe a better name 
> >would be X509UserNameRetriever now that I think about it)
> >interface, the SubjectDNRetriever, and minimal changes to RealmBase.
> 
> I am confused. According to my understanding, we want to provide the ability to
> use a user provided X509UserNameRetriever.
> 
> The purpose of UserNameRetrieverDecorator is to return the user name by the
> default X509UserNameRetriever if the X509UserNameRetriever provided by a user
> will return the empty user name.

I think that any X509UserNameRetriever should be required to provide something that is non-null. There's no reason for "client code" not to extend SubjectDNRetriever and simply delegate to the superclass if the name isn't otherwise found. I don't think it's necessary to provide that fallback logic in Tomcat itself: it the user is going to provide an implementation of an X509UserNameRetriever, then it should be *the* implementation that is used, not one of several that Tomcat has to manage.

> Regarding UserNameRetrieverConfiguration – it allow easy configuration of a
> user provided X509UserNameRetriever.

I think all we need is a single method in the RealmBase class that can accept the name of the class. The default is SubjectDNRetriever, but the user can set it to whatever they want.

> I think it is very useful if you create your own X509UserNameRetriever.

I'm not sure what it adds.

> >Basically, no documentation should be required 
> >that isn't part of your patch.
> 
> So, I will not provide the client certificate description. Correct?
> Waiting for your comments.

You don't have to introduce the concept of client certificates, no.

I think it's obvious to anyone who is reading this type of documentation what is going on: you are making it possible to customize the information used from the client certificate to produce a username that Tomcat uses.
Comment 25 Michael 2012-02-18 08:38:14 UTC
Created attachment 28348 [details]
x509 java code

x509 java code
svn diff at trunk\java
Comment 26 Michael 2012-02-18 08:39:14 UTC
Created attachment 28349 [details]
Realm configuration document

Realm configuration document
svn diff at trunk\webapps
Comment 27 Michael 2012-02-18 08:39:43 UTC
Dear Christopher,
Thank you for your comments!
Please find attached patch.
1)	I have renamed my classes to X509UserNameRetriever and to X509SubjectDnRetriever
2)	The Realm interface is reverted
3)	All code is moved to the RealmBase class
I have updated the realm document – please find attachment.
Best regards,
   Michael
Comment 28 Michael 2012-02-24 19:00:50 UTC
Dear Christopher,
Did you see my patch?
Do you have any comments?
Best regards,
   Michael
Comment 29 Michael 2012-02-26 20:38:32 UTC
Created attachment 28386 [details]
x509 java code

I have changed x509UserNameRetrieveConfiguration to x509UserNameRetrieverConfiguration
diff at trunk\java
Comment 30 Michael 2012-02-26 20:39:48 UTC
Created attachment 28387 [details]
Realm configuration

I have changed x509UserNameRetrieveConfiguration to x509UserNameRetrieverConfiguration
diff at trunk\webapps
Comment 31 Michael 2012-02-26 20:40:43 UTC
Dear Christopher,
Sometimes it is better to wait :)
I have updated the patch.
It was typo in the name. I have changed x509UserNameRetrieveConfiguration to x509UserNameRetrieve<b>r</b>Configuration
Will happy for your comments.
Best regards,
   Michael
Comment 32 Michael 2012-02-29 20:05:29 UTC
Dear Christopher,
Did you see my patch?
Do you have any comments?
Best regards,
   Michael
Comment 33 Christopher Schultz 2012-03-08 17:52:52 UTC
Michael,

I've applied a simplified patch based upon yours. The differences are:

1. No 'X509UsernameRetrieverConfiguration' attribute. This can be handled with a custom class instead of the more complicated proposed configuration.

2. X509SubjectDNRetriever trivially returns cert.getSubjectDN().getName(), since the above change was made.

I will likely be adding a form of /your/ X509SubjectDNUsernameRetriever class that is intended to be subclassed to provide the name of the SubjectDN attribute you'd like to use.

Fixed in trunk in r1298476.
Fixed in 7.0.x in r1298479. Will be included in 7.0.27.
Proposed for 6.0.x.
Comment 34 Michael 2012-03-11 19:47:02 UTC
Created attachment 28450 [details]
Suggested patch for x509 configuration
Comment 35 Michael 2012-03-11 19:48:23 UTC
Dear Christopher,
Thank you for the patch!
Any case, I will happy if you will change the X509SubjectDnRetriever class to X509DefaultSubjectDnRetriever. In this case I will be able to use the X509SubjectDnRetriever name for my class. It will help me a lot.
Please do it for me :)
BTW, when 7.0.27 will be released?
>No 'X509UsernameRetrieverConfiguration' attribute. This can be handled with
>a custom class instead of the more complicated proposed configuration.

I am not sure that I agree with your suggestion :)
It is not possible to create a dedicated class for all possible values. The full X509SubjectDnRetriever implementation required the additional configuration.
I will ask to consider once again to add the x509UsernameRetrieverConfiguration as in the provided patch.
In this case X509UsernameRetriever provided by the classname (e.g. the full X509SubjectDnRetriever implementation) will be able to use the required configuration.
If you will not agree with the provided patch – I will need to change the implementation of the full X509SubjectDnRetriever implementation to read the configuration form the dedicated properties file.
It is not nice to have two configurations in the same Tomcat, but I will not have other option :(
I am waiting for your reply.
Best regards,
   Michael
Comment 36 Michael 2012-03-14 19:17:59 UTC
Dear Christopher,
I will really appreciate if you will rename the X509SubjectDnRetriever class to X509DefaultSubjectDnRetriever.
Please, please do it for me – it take about 3 min to do it but will help me a lot.
BTW, when 7.0.27 will be released?
Best regards,
   Michael
Comment 37 Christopher Schultz 2012-03-15 19:37:54 UTC
(In reply to comment #36)
> Dear Christopher,
> I will really appreciate if you will rename the X509SubjectDnRetriever class to
> X509DefaultSubjectDnRetriever.
> Please, please do it for me – it take about 3 min to do it but will help me a
> lot.

I'm -1 on re-naming that class, since it does just what it says: it returns the entire SubjectDN as the username: it's not a "default". I would think a better name for a class that takes part of the SubjectDN would be X509SubjectDNFieldRetriever.

> BTW, when 7.0.27 will be released?

When it's ready.

7.0.23 was released 2011-11-25
7.0.24 was not released because it was broken
7.0.25 was released 2012-01-21
7.0.26 was released 2012-02-22

I would expect that 7.0.27 will be released within the next 30 days: Mark has done a great job rolling regular releases.
Comment 38 Konstantin Kolinko 2012-06-02 21:08:46 UTC
Ported to 6.0 in r1345575 and will be in 6.0.36