This is a much needed feature, I think.  Has it been decided that this will never be in Tomcat7?  Note that Bug 51966, marked for Tomcat 6, relates to storing passwords more securely using salt and password hashes.  I'd say that that feature should be available to Tomcat users without requiring them to write their own code, but it makes sense to use the pluggable interface proposed here to accomplish that.

I don't see a reason not to back-port it to Tomcat 7. Since it's a breaking API change, I'll be asking for RTC for a back-port. I suspect it will make it.

Note that Tomcat 6's implementation (identical in all important ways to that of Tomcat 7 and 8) is not /insecure/, just not terribly secure if being used without any additional controls.

As for providing salted passwords out of the box, I'd suggest that salting isn't enough and that iteration is also necessary, etc. and that at this point simply using PBKDF2 or some other password-munging scheme is more appropriate. I do note that PBKDF2 (mist-typed as PBKDF11 in the description) does not store the number of iterations in the generated password which means that you either need to adjust the data you actually store to include it, or you can never change the number of iterations.

I suspect we'll provide a PBKDF2 implementation out of the box, but nothing else to avoid any library dependencies. Using the PBKDF2 implementation as an example would make writing a bcrypt- or scrypt-based implementation fairly easy.

(In reply to Christopher Schultz from comment #2)
> I don't see a reason not to back-port it to Tomcat 7. Since it's a breaking
> API change, I'll be asking for RTC for a back-port. I suspect it will make
> it.

That would be very good, since many systems, for example those running Ubuntu 14.04 LTS for the next few years, will be running Tomcat 7 by default.  

> 
> Note that Tomcat 6's implementation (identical in all important ways to that
> of Tomcat 7 and 8) is not /insecure/, just not terribly secure if being used
> without any additional controls.

I understand that if the password table is never stolen this is not an issue, but I don't want to be irresponsible, and not use best practices.  The current implementation makes it difficult for admins to follow best practice.  

> 
> As for providing salted passwords out of the box, I'd suggest that salting
> isn't enough and that iteration is also necessary, etc. and that at this
> point simply using PBKDF2 or some other password-munging scheme is more
> appropriate. I do note that PBKDF2 (mist-typed as PBKDF11 in the
> description) does not store the number of iterations in the generated
> password which means that you either need to adjust the data you actually
> store to include it, or you can never change the number of iterations.

Agree that PBKDF2 is the way to go if one want to be conservative but follow best practice.  It is what I wish to use in my web applications.  Changing the number of iterations should be allowed.  I would prefer to have separate data columns for salt, password digest, and number of iterations, but wouldn't mind if they are all in one delimited field.  The beauty of the proposed pluggable interface is that it will be easy to do either, right?

> 
> I suspect we'll provide a PBKDF2 implementation out of the box, but nothing
> else to avoid any library dependencies. Using the PBKDF2 implementation as
> an example would make writing a bcrypt- or scrypt-based implementation
> fairly easy.

That would be splendid.  Noting that this bug is about the pluggable interface and not the PBKDF2 implementation, and that a PBKDF2 implementation would probably use the new pluggable interface (am I right to assume that?), should a new bug be created that depends on this one?

(In reply to Gabriel from comment #3)
> (In reply to Christopher Schultz from comment #2)
> > I don't see a reason not to back-port it to Tomcat 7. Since it's a breaking
> > API change, I'll be asking for RTC for a back-port. I suspect it will make
> > it.
> 
> That would be very good, since many systems, for example those running
> Ubuntu 14.04 LTS for the next few years, will be running Tomcat 7 by
> default.  

Just remember that switching from one password standard to another is ... hard. If you want to switch from using MD5 to SHA1 to SHA2 to RIPEMD, you basically have to write your own passwork-checker. Fortunately, this will be entirely possible using the tools we will provide.

> > Note that Tomcat 6's implementation (identical in all important ways to that
> > of Tomcat 7 and 8) is not /insecure/, just not terribly secure if being used
> > without any additional controls.
> 
> I understand that if the password table is never stolen this is not an
> issue, but I don't want to be irresponsible, and not use best practices. 
> The current implementation makes it difficult for admins to follow best
> practice.  

+1

> > As for providing salted passwords out of the box, I'd suggest that salting
> > isn't enough and that iteration is also necessary, etc. and that at this
> > point simply using PBKDF2 or some other password-munging scheme is more
> > appropriate. I do note that PBKDF2 (mist-typed as PBKDF11 in the
> > description) does not store the number of iterations in the generated
> > password which means that you either need to adjust the data you actually
> > store to include it, or you can never change the number of iterations.
> 
> Agree that PBKDF2 is the way to go if one want to be conservative but follow
> best practice.  It is what I wish to use in my web applications.  Changing
> the number of iterations should be allowed.  I would prefer to have separate
> data columns for salt, password digest, and number of iterations, but
> wouldn't mind if they are all in one delimited field.  The beauty of the
> proposed pluggable interface is that it will be easy to do either, right?

One delimited field is what pretty much everyone expects. Besides, the Tomcat interface is going to have to be simple so I suspect we'll just have a single "stored credential" byte array or string and present the user's (attempted) password in the same format. The password-munger can do whatever is necessary to compare the two.

I suppose it couldn't hurt to add "iterations" to the list of understood configuration attributes. Since we'll need it for PBKDF2, we may as well enable it for the digest-based algorithms, too.

> > I suspect we'll provide a PBKDF2 implementation out of the box, but nothing
> > else to avoid any library dependencies. Using the PBKDF2 implementation as
> > an example would make writing a bcrypt- or scrypt-based implementation
> > fairly easy.
> 
> That would be splendid.  Noting that this bug is about the pluggable
> interface and not the PBKDF2 implementation, and that a PBKDF2
> implementation would probably use the new pluggable interface (am I right to
> assume that?), should a new bug be created that depends on this one?

Let's just assume that PBKDF2 is my target implementation (as well as a backward-compatible plain-old-digest implementation of course) and go ahead and track suggestions for it, here. There's no need to file additional enhancement requests.

Created attachment 31615 [details]
Example of an interface and implementation of both MessageDigest and PBKDF2

Attaching a self-contained driver, interface, and implementation of CredentialMatcher for MessageDigest and PBKDF2.

This is the kind of thing I'd like to place into RealmBase (or, rather, factor-out of RealmBase).

RealmBase currently has some additional code to allow prefixes such as {MD5}, {SSHA}, etc. to alter the behavior of the existing message-digest-only code. None of that type of code has been included in this example as it can be trivially added once the interfaces have been established.

I was thinking that the <Realm> could have a sub-element <CredentialMatcher> (or similar... I'm not happy with the interface's name but can't think of a better one at the moment) that could identify the class to be used plus provide all of the configuration attributes like salt-length, iteration-count, algorithm name, and anything else an implementation may need. For backward-compatibility, we'd default to MessageDigestCredentialMatcher and route calls to RealmBase.setDigest() to MessageDigestCredentialMatcher.setAlgorithm().

Comments are welcome!

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

(Copied from bug #51966 comment #23):
> I'm currently working on taking Chris's proposal there and turning it into a patch that can be applied to 8.0.x.

If there's something specific you are looking for, I can update the patch.

(In reply to Christopher Schultz from comment #7)
> If there's something specific you are looking for, I can update the patch.

What I was looking for was an actual patch for 8.0.x. The current attachment is just a bunch of Java code. I must stress though it is very useful Java code, it just isn't in a form of a patch that can be directly applied to 8.0.x.

I'm already most of the way through converting it to a patch. Let me finish that off and then I'll attach it here for further comment.

(In reply to Mark Thomas from comment #8)
> (In reply to Christopher Schultz from comment #7)
> > If there's something specific you are looking for, I can update the patch.
> 
> What I was looking for was an actual patch for 8.0.x.

Aah, yes, of course. The idea was that I wanted some feedback on the design, particularly of the base class itself (which could be an interface, I suppose). I had intended to create a formal patch after some feedback, but nobody ever gave any :(

> I'm already most of the way through converting it to a patch. Let me finish
> that off and then I'll attach it here for further comment.

Sounds good.

We use our own Realm implementation here that has nothing to do with Tomcat, so I'll take a look at your forthcoming patch so see if it would be possible to integrate the two together. I'd anxious to get away from our own home-brewed solution that plugs-into securityfilter...

(In reply to Christopher Schultz from comment #9)
I had intended to create a formal patch after some feedback, but
> nobody ever gave any :(
And I intended to give you some feedback but I have not done it yet!  I'll try to have a look at Mark's patch when he attaches it.  

Any chance this will get back-ported to tomcat7?

(In reply to Gabriel from comment #10)
> Any chance this will get back-ported to tomcat7?

I think it depends upon user demand and the impedance caused by any major pre-existing changes between TC7 and TC8. I suspect there has been little divergence and so a Tomcat 7 backport seems reasonable.

OK. Here is my first pass at incorporating these ideas in Tomcat 8.

I spent a fair amount of time looking at how to keep existing configurations working while not making the API a complete mess.

I also tweaked the proposed API for the pluggable component based on how it was used by the Realms and trying to keep the number of conversions to/from String to a minimum.

Feedback welcome - probably best on the dev list.

http://people.apache.org/~markt/patches/2014-09-16-bug56403-tc8-v1.patch

The proposed patch went through a number of review cycles on the dev list and was applied to 8.0.x for 8.0.15 onwards.

Is there any decision on whether this will be backported to Tomcat 7?