Hidden cost of password upgrade

[jmisur]

Context

So DelegatingPasswordEncoder decided to upgrade password encoding. This is arguably a reasonable decision, however it's not required every time and it has a hidden cost.

For example, if you have a simple http basic in-memory authentication for use between internal services, where password is stored in a secure cloud storage attached to the service, you might not need such upgrade behavior.

DelegatingPasswordEncoder however criples the performance of the service by upgrading the password to bcrypt after first successful call (in-memory in this case).
See the attached performance analysis breakdown, it's cost is overwhelming for a simple CRUD service.

Possible solution
Keep NoOpPasswordEncoder (it's deprecated) and/or add some property to prevent an encoding upgrade.
Also documenting this would help people debugging performance issues.

[rwinch]

Thanks for the report.

This is arguably a reasonable decision, however it's not required every time and it has a hidden cost.

The password should only upgraded once since the underlying implementation is consulted if passwords should be upgraded or not. So it should not upgrade every time.

DelegatingPasswordEncoder however criples the performance of the service by upgrading the password to bcrypt after first successful call (in-memory in this case).
See the attached performance analysis breakdown, it's cost is overwhelming for a simple CRUD service.

I believe we did a good job on documenting the performance impact of validating properly encoded passwords and that long term credentials (i.e. passwords) should be exchanged for short term ones (i.e. session, OAuth token, etc), but we need to do better with the fact that passwords may be upgraded and how that works. I created gh-8505 to address this.

Keep NoOpPasswordEncoder (it's deprecated) and/or add some property to prevent an encoding upgrade.

It is only deprecated because it is considered insecure and there are no plans to remove it. This means you can use NoOpPasswordEncoder without worrying about it being removed. However, I must emphasize that it is NOT SECURE.

I created gh-8506 to update our Javadoc to make it clear that it is not going to be removed. Would you be interested in submitting a Pull Request for this?

With the above linked issues, I believe this addresses your concerns. Can you please confirm or let me know what is still missing?

[jmisur]

Thank you, a better documentation will help a lot. This can be closed.

[rwinch]

Thanks for the feedback @jmisur

Closing in favor of the issues that were created based on the feedback.

[ahahu]

The password should only upgraded once since the underlying implementation is consulted if passwords should be upgraded or not. So it should not upgrade every time.

Just experienced the very same issue and would like to note that the above comment seems a bit misleading: while the upgrade is performed only once, the resulting performance impact happens on every of the following authentications.
So in a very typical scenario with http basic in-memory authentication based on "{noop}" encoded passwords for internal service communication, this creates a really heavy performance penalty.

While I applaud the security thinking, I also consider this is a regression.

[rwinch]

@ahahu As mentioned previously, you can expose a NoOpPasswordEncoder Bean to go back to the previous behavior. The only implementation of UserDetailsPasswordService is InMemoryUserDetailsManager so the upgraded password is not persisted. This means that restarting your application will revert to using the previous encoding as well. In the end, this is a much better default as it helps ensure passwords are secure.