Bumping this up to major because of the salt issue - salts should be supported sooner rather than later

I am working on a patch for this and have a couple of questions.

First, the API for adding salt support. Here is one way to do it:

Add
protected byte[] getSaltForUser(String username)
and have the default implementation query the database using
protected String userSaltQuery = DEFAULT_USER_SALT_QUERY;
Exposing getSaltForUser enables users to override it with whatever salt-generation scheme they choose.

Sound OK?

Second, I noticed that there are no tests for JdbcRealm. In developing a test class, we need to decide how to create or simulate the realm database. Here are some options:

0) I see EasyMock is already used elsewhere, but that might be a little ugly / hard to follow.

1) derby

2) hsqldb

3) grab DBCP's Tester* classes

4) something else?

I would personally favor 1) or 2) but will use EasyMock (or whatever else) if we don't want to add any more test dependencies.

Hi Phil,

Thanks for offering to help out on this.

My belief is that most people will store the salt alongside the password as a separate column, or even embedded in the password hash column itself. For example, I've been working a little bit on supporting a unix crypt(3) format for Shiro's user passwords in INI. In *nix crypt, all information necessary to perform a password hash comparison is encoded in a single field via the Modular Crypt Format [1].

Granted, this isn't as secure as splitting this information up in multiple places (having it all in one place doesn't protect against brute force attacks), but I believe it is well within the 80% of the 80/20 rule for most end users.

So my expectation is that if either a separate column or embedded, that this information would be retrieved as part of the single authentication query. This would imply that the JdbcRealm should be refactored to allow for either possibility. This can typically be done w/ one query, e.g:

1. If storing a crypt-style password hash, the existing query will work as-is. The logic just needs to be changed to reconstruct the SaltedAuthenticationInfo object based on the MCF tokens.

2. If the salt is a separate column, the query will need to be something like "select password, password_salt from users where username = ?".

As for testing, we can use whatever dependencies we want (and as much of them as we want) as long as they're ASF 2.0 compatible. Since test libraries don't impact end user applications, we don't have to be as frugal with them. Feel free to use whatever mechanism you prefer!

Does this help?

[1] http://packages.python.org/passlib/modular_crypt_format.html

Also, DBUnit sounds like a good option for testing this - just a thought.

Thanks, Wes!

Definitely better ideas above. I will start with an initial patch that does not quite do 1, but defines the option. I assume we will grab or create some MCF handler machinery somewhere that can be used here. In the mean time, I will get 2. working and also expose getSaltForUser as in my original suggestion to provide a hook for whatever users want to do.

Here is a first cut at a patch to add salt support. It seems to me there are four possibilities for how to get the salt: 0) no salt 1) crypt-style 2) salt in a separate database column and 4) salt supplied by an external function. I tried to make room for all 4, but did not implement 1). If we like the setup, the enum should either be separated or replaced by constants. I also dropped (for now) the extension point, buildAuthenticationInfo, which we can add back later once we agree on the API for salting. I used hsqldb for the unit tests because that dependency already exists elsewhere. The javadoc still needs to be improved throughout. I will do this once we agree on the API.

Applied and committed the patch. Quite good unit tests Phil - nicely done! And thanks again for the patch - it is greatly appreciated!

I have a few comments so far:

1) I think we should add back in the buildAuthenticationInfo method (the original) and add a new one that takes in the salt. doGetAuthenticationInfo would call the new method and it in turn can just call the older one for backwards-compatibility.

2) I see that the salt returned from the JDBC query is a String. I think that this string would probably represent a Base64 or Hex-encoded String though and need to be decoded first before acquiring the bytes. (the current call to ByteSource.Util.bytes(salt) returns the Strings direct bytes without decoding first). A theme in Shiro's INI configuration is that if the value should be decoded, you can assume that a prefix of '0x' (zero x) indicates a hex value and anything else is assumed to be Base64.

3) I'm wondering if we should forego an Enum to represent how we acquire the salt and instead provide a JdbcAccountResolver that takes in a Connection and the AuthenticationToken as arguments (or maybe just a ResultSet) and returns a Map of data that we can check (i.e. map.get("username"), map.get("password"), map.get("salt")).

This way we can provide the 3 default implementations that replace the SaltStyle.NO_SALT, CRYPT, and COLUMN cases. EXTERNAL would just be a custom implementation of the JdbcAccountResolver interface. I'm keen on allowing people to plugin in simple implementations instead of having to override methods where possible. Also, using the username as a salt is not recommended for security reasons, which is why I think allowing end-users to plug in a resolver of their own is better than defaulting to the username.

Thoughts?

Les

Phil et al., any thoughts based on my last comment? I'm trying to clear out issues for the 1.2 release.

Sorry to take so long to get back to this.

+1 for 1)
+0 for 2), assuming I understand it correctly. The key here is to document carefully what Shiro requires in the salt data returned from the database or external source.
I don't think I understand 3), which looks more complicated to me than having the salt method configurable in the INI, which is what I was trying to set up. That's what I meant by externalizing the enum, so its values could be referenced in the INI and the setters would be invoked by the framework (so users would not have to subclass or override anything). The exception would be EXTERNAL, where you really need the user to provide an impl. I think I am most likely missing the point here.

Agreed on badness of using the username as salt in the default implementation of getSaltForUser, which is only called if salt style is EXTERNAL. Other options here would be a) throw UOE in the default impl, since EXTERNAL should only be used when this method is overridden or b) forget EXTERNAL altogether, but expose getSaltForUser and move the code that gets the salt using the other options into this method. Then users can override if they wish. IIUC, your suggestion enables b).

This was merged in 36a848ba30990dacdc13efcd5ed75b393dfa0fd5 (in 1.2.0)