[#APF-666] Patch for rationalisation of password encryption - AppFuse JIRA

AppFuse

Patch for rationalisation of password encryption

Created: 28/Feb/07 03:20 AM Updated: 20/Nov/07 11:33 AM Resolved: 20/Nov/07 11:33 AM

Component/s:

Security, Service Layer, Web - General

Affects Version/s:

2.0-M3

Fix Version/s:

2.0.1

File Attachments:	1. APF-666.patch (47 kB) 2. appfuse-passwordEncoder-200702280344-patch.txt (44 kB) 3. appfuse-passwordEncoder-200711120319-patch.txt (79 kB)
Environment:	(all applicable AppFuse environments)

Description

« Hide

The attached patch improves password encoding capabilities so that the same password encoder object is used by both ACEGI at authentication time and AppFuse Actions/Controllers at user sign up / modification time. This will make it possible for AppFuse to use any other password encoding techniques featuring higher security than simple MessageDigests.

Also, the responsibility for performing password encoding is moved from the Actions/Controllers to the services layer (UserManager). UserManager gets the "daoAuthenticationProvider" dependency-injected, which lets UserManager perform the necessary password encoding operations because:

   * DaoAuthenticationProvider contains references to both PasswordEncoder and SaltSource.

   * If the user does not define a password encoder bean in security.xml, DaoAuthenticationProvider will use a PlaintextPasswordEncoder, this is, no encryption. And DaoAuthenticationProvider.getPasswordEncoder() will return to us that PlaintextPasswordEncoder object. This is exactly the behaviour we wanted (no "passwordEncoder" bean = no password encryption).

   * With this approach, if the user defines a SaltSource and passes it to the DaoAuthenticationProvider, we will also use it to parameterize password encryption at user sign up or modification time. If we didn't, ACEGI would never correctly match passwords at sign in time in this scenario.

A new method is declared in UserManager for saving users, called also "saveUser" (so that it benefits from the transaction declarations in applicationContext-service.xml), but it receives two parameters: "user", and a boolean called "passwordChanged". The old method (which only receives one parameter) is still there, but in theory it could be removed (unless you want to avoid problems with any users who could have already created code calling this method in their apps)...

This is what the patch does:

DATA:
    * Remove both ENCRYPT_PASSWORD and ENC_ALGORITHM from Constants.

SERVICE:
    * Remove the StringUtil class
    * Modify UserManager to add the saveUser(User, boolean) interface.
    * Modify UserManagerImpl to add the saveUser(User, boolean) implementation.
    * Add reference to daoAuthenticationProvider for userManager in applicationContext-service.xml
    * Add beans "daoAuthenticationProvider" and "userCache" in service/src/test/resources/applicationContext-resources.xml

WEB-COMMON
    * Modify StartupListener not to check "passwordEncoded", and not putting into config neither the ENCRYPT_PASSWORD nor the ENC_ALGORITHM values.
    * Add web/common/src/test/resources/applicationContext-resources.xml (for declaring daoAuthenticationProvider)
    * Modify StartupListenerTest to use the new applicationContext-resources.xml
    * Remove comment from security.xml telling that "passwordEncoder" has to be there for StartupListener to read it

WEB-APPFUSE-JSF
    * Modify SignupForm and UserForm in appfuse-jsf to remove the password encryption code and use the new saveUser method in UserManager.
    * Add web/jsf/src/test/resources/applicationContext-resources.xml (for declaring daoAuthenticationProvider)
    * Add a reference to web/jsf/src/test/resources/applicationContext-resources.xml from BasePageTestCase in appfuse-jsf

WEB-APPFUSE-SPRING
    * Modify SignupController and UserFormController in appfuse-spring to remove the password encryption code and use the new saveUser method in UserManager.
    * Add web/spring/src/test/resources/applicationContext-resources.xml (for declaring daoAuthenticationProvider)
    * Add a reference to web/spring/src/test/resources/applicationContext-resources.xml from BaseControllerTestCase in appfuse-spring

WEB-APPFUSE-STRUTS
    * Modify SignupAction and UserAction in appfuse-struts to remove the password encryption code and use the new saveUser method in UserManager.
    * Add web/struts/src/test/resources/applicationContext-resources.xml (for declaring daoAuthenticationProvider)
    * Add a reference to web/struts/src/test/resources/applicationContext-resources.xml from BaseActionTestCase in appfuse-struts

WEB-APPFUSE-TAPESTRY
    * Modify SignupForm and UserForm in appfuse-tapestry to remove the password encryption code and use the new saveUser method in UserManager.
    * Add web/tapestry/src/test/resources/applicationContext-resources.xml (for declaring daoAuthenticationProvider)
    * Add a reference to web/tapestry/src/test/resources/applicationContext-resources.xml from BasePageTestCase in appfuse-tapestry

Description

The attached patch improves password encoding capabilities so that the same password encoder object is used by both ACEGI at authentication time and AppFuse Actions/Controllers at user sign up / modification time. This will make it possible for AppFuse to use any other password encoding techniques featuring higher security than simple MessageDigests. Also, the responsibility for performing password encoding is moved from the Actions/Controllers to the services layer (UserManager). UserManager gets the "daoAuthenticationProvider" dependency-injected, which lets UserManager perform the necessary password encoding operations because: * DaoAuthenticationProvider contains references to both PasswordEncoder and SaltSource. * If the user does not define a password encoder bean in security.xml, DaoAuthenticationProvider will use a PlaintextPasswordEncoder, this is, no encryption. And DaoAuthenticationProvider.getPasswordEncoder() will return to us that PlaintextPasswordEncoder object. This is exactly the behaviour we wanted (no "passwordEncoder" bean = no password encryption). * With this approach, if the user defines a SaltSource and passes it to the DaoAuthenticationProvider, we will also use it to parameterize password encryption at user sign up or modification time. If we didn't, ACEGI would never correctly match passwords at sign in time in this scenario. A new method is declared in UserManager for saving users, called also "saveUser" (so that it benefits from the transaction declarations in applicationContext-service.xml), but it receives two parameters: "user", and a boolean called "passwordChanged". The old method (which only receives one parameter) is still there, but in theory it could be removed (unless you want to avoid problems with any users who could have already created code calling this method in their apps)... This is what the patch does: DATA: * Remove both ENCRYPT_PASSWORD and ENC_ALGORITHM from Constants. SERVICE: * Remove the StringUtil class * Modify UserManager to add the saveUser(User, boolean) interface. * Modify UserManagerImpl to add the saveUser(User, boolean) implementation. * Add reference to daoAuthenticationProvider for userManager in applicationContext-service.xml * Add beans "daoAuthenticationProvider" and "userCache" in service/src/test/resources/applicationContext-resources.xml WEB-COMMON * Modify StartupListener not to check "passwordEncoded", and not putting into config neither the ENCRYPT_PASSWORD nor the ENC_ALGORITHM values. * Add web/common/src/test/resources/applicationContext-resources.xml (for declaring daoAuthenticationProvider) * Modify StartupListenerTest to use the new applicationContext-resources.xml * Remove comment from security.xml telling that "passwordEncoder" has to be there for StartupListener to read it WEB-APPFUSE-JSF * Modify SignupForm and UserForm in appfuse-jsf to remove the password encryption code and use the new saveUser method in UserManager. * Add web/jsf/src/test/resources/applicationContext-resources.xml (for declaring daoAuthenticationProvider) * Add a reference to web/jsf/src/test/resources/applicationContext-resources.xml from BasePageTestCase in appfuse-jsf WEB-APPFUSE-SPRING * Modify SignupController and UserFormController in appfuse-spring to remove the password encryption code and use the new saveUser method in UserManager. * Add web/spring/src/test/resources/applicationContext-resources.xml (for declaring daoAuthenticationProvider) * Add a reference to web/spring/src/test/resources/applicationContext-resources.xml from BaseControllerTestCase in appfuse-spring WEB-APPFUSE-STRUTS * Modify SignupAction and UserAction in appfuse-struts to remove the password encryption code and use the new saveUser method in UserManager. * Add web/struts/src/test/resources/applicationContext-resources.xml (for declaring daoAuthenticationProvider) * Add a reference to web/struts/src/test/resources/applicationContext-resources.xml from BaseActionTestCase in appfuse-struts WEB-APPFUSE-TAPESTRY * Modify SignupForm and UserForm in appfuse-tapestry to remove the password encryption code and use the new saveUser method in UserManager. * Add web/tapestry/src/test/resources/applicationContext-resources.xml (for declaring daoAuthenticationProvider) * Add a reference to web/tapestry/src/test/resources/applicationContext-resources.xml from BasePageTestCase in appfuse-tapestry

Show »

Sort Order:

[ Permalink | « Hide ]

Matt Raible added a comment - 07/Nov/07 12:37 AM

I tried to apply this patch, but it's too out-of-date to work. I like the idea behind this, and I could modify the patch to work - but it'd be easier if you created a new patch. Also, I question the saveUser(User, boolean) method in UserManager. It seems like it'd be easier (and more robust) to fetch the user's password from the database, compare it with the one coming in, and use that logic to determine if the password has changed. if the password changed, encrypt it (providing that password encryption is turned on).

One problem I see with this vs. the existing implementation is that it looks difficult to turn off password encryption.

[ Permalink | « Hide ]

Daniel Fern?ndez Garrido added a comment - 08/Nov/07 02:41 PM

Ok Matt, I will have a look at it this week and tell you.

[ Permalink | « Hide ]

Daniel Fern?ndez Garrido added a comment - 11/Nov/07 08:49 PM

This is a new version of the patch, adapted to the current state of the code base. *I was not able to test it at all*, as some other unit tests in the service layer failed, and because of that I could not build the packages.

I have suppressed the "saveUser(User, boolean)" method in UserManager and UserManagerImpl, so that now the password for the user being saved is looked up in the database to check if it matches the one coming with the User object. Depending on the result of this matching, password encryption is re-done or not.

To support this new behaviour, I have added "getUserPassword" methods to UserDao interfaces and implementations for Hibernate, iBatis and JPA.

My previous solution (the "saveUser(User,boolean)" method) was based on the fact that "User" is a mapped class, and that, when we are updating a User object's password, this object can be still attached to a Hibernate session and thus all updates on it (like calling "setPassword(String)" for setting the new password) can potentially trigger UPDATE sentences to the database made by hibernate's dynamic proxy and, thus, password can be already set to the new unencrypted value at the database when we arrive at "saveUser(User)".

In practice these setter-related updates will only happen at session commit, unless a session.flush() call is made beforehand. But the risk is there :-)

Also, think that the query for getting the user password is done through hibernate and that, if query caché is enabled, hibernate could decide that the user which password we are asking for is already in the session (or in a second-level caché), and this way return to us the new, unencrypted, already-changed password. Another risk.

In brief, the only way to prevent all these risks would be not to include the password in the User object (which is always the safest option as it is not a bidirectional (read+write) attribute), as I agree that my previous "saveUser(User,boolean)" method would be tricky for the web service UserService interface (potential users of this web service would have to be educated on which "saveUser" method to use depending on password changes, which is, at least, difficult...)

Regards,
Daniel.

P.S.- I have also added a commented PlaintextPasswordEncoder entry in security.xml so that it is simpler for a user to switch password encryption on/off (if there ever were a reason in the universe for turning off password encryption ;-))

[ Show » ]

Daniel Fern?ndez Garrido added a comment - 11/Nov/07 08:49 PM This is a new version of the patch, adapted to the current state of the code base. *I was not able to test it at all*, as some other unit tests in the service layer failed, and because of that I could not build the packages. I have suppressed the "saveUser(User, boolean)" method in UserManager and UserManagerImpl, so that now the password for the user being saved is looked up in the database to check if it matches the one coming with the User object. Depending on the result of this matching, password encryption is re-done or not. To support this new behaviour, I have added "getUserPassword" methods to UserDao interfaces and implementations for Hibernate, iBatis and JPA. My previous solution (the "saveUser(User,boolean)" method) was based on the fact that "User" is a mapped class, and that, when we are updating a User object's password, this object can be still attached to a Hibernate session and thus all updates on it (like calling "setPassword(String)" for setting the new password) can potentially trigger UPDATE sentences to the database made by hibernate's dynamic proxy and, thus, password can be already set to the new unencrypted value at the database when we arrive at "saveUser(User)". In practice these setter-related updates will only happen at session commit, unless a session.flush() call is made beforehand. But the risk is there :-) Also, think that the query for getting the user password is done through hibernate and that, if query caché is enabled, hibernate could decide that the user which password we are asking for is already in the session (or in a second-level caché), and this way return to us the new, unencrypted, already-changed password. Another risk. In brief, the only way to prevent all these risks would be not to include the password in the User object (which is always the safest option as it is not a bidirectional (read+write) attribute), as I agree that my previous "saveUser(User,boolean)" method would be tricky for the web service UserService interface (potential users of this web service would have to be educated on which "saveUser" method to use depending on password changes, which is, at least, difficult...) Regards, Daniel. P.S.- I have also added a commented PlaintextPasswordEncoder entry in security.xml so that it is simpler for a user to switch password encryption on/off (if there ever were a reason in the universe for turning off password encryption ;-))

[ Permalink | « Hide ]

Matt Raible added a comment - 20/Nov/07 11:03 AM

Attached is the patch I'm going to commit. Differences include:

1. No applicationContext-test.xml in web/*/src/test/resources. This is because I put the daoAuthenticationProvider bean in applicationContext-service.xml.

2. No salt in UserManagerImpl.

3. No use of isPasswordValid in UserManagerImpl - it did not detect when passwords didn't match and would double-encrypt a password.