Skip to content

Password encoding upgrade is insufficiently configurable #8973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
andreild opened this issue Aug 19, 2020 · 2 comments
Closed

Password encoding upgrade is insufficiently configurable #8973

andreild opened this issue Aug 19, 2020 · 2 comments
Assignees
Labels
in: crypto An issue in spring-security-crypto status: declined A suggestion or change that we don't feel we should currently apply

Comments

@andreild
Copy link

andreild commented Aug 19, 2020

Summary

Hello,

I ran into #8498 this week. Specifically:

  • I upgraded from Spring Security 5.1.6 to 5.3.3.
  • in so doing our password hashing (performed on every application endpoint request) went from fast to slow.
  • because the strength-4 BCrypt hash was internally upgraded to a strength-10 one.
  • none of our existing tests caught this, because our service's functionality was unaffected, only its performance.
  • the impact was significant, as the endpoints we use this Basic Auth on have dozens - hundreds of requests / second.

So I would like to request that the password upgrade feature be made more configurable.

Actual Behavior

Upon encountering a BCrypt password hash of any strength smaller than 10, Spring Security will internally re-encode the plaintext matching this hash (once it is presented by a client) with a BCrypt encoder with strength 10. From that point on, all plaintext to hash comparisons will be performed at strength 10. This imposes a significant performance cost if done often.

This happens because:

  • org.springframework.security.crypto.factory.PasswordEncoderFactories#createDelegatingPasswordEncoder() creates a BCryptPasswordEncoder with strength 10 (not configurable).
  • org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder#upgradeEncoding returns true if the strength of the encoded password is smaller than the strength of that encoder instance, i.e. 10 (also not configurable).

I worked around this issue by defining my own DelegatingPasswordEncoder with its own custom-strength BCryptPasswordEncoder. That seems like a bit too much work for someone just wanting to use HTTP Basic Auth.

Expected Behavior

One or more of the following:

  • the password upgrade feature has an on/off flag.
  • the password encoder created by default (i.e. absent any user-defined beans) is configurable via properties.
  • perhaps a third option I'm not considering.

Configuration

See the attached project.

Version

Spring Security 5.3.4.RELEASE.

Sample

Attached.
demo.zip

Thank you.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 19, 2020
@mirsilstan
Copy link

We had the same issue on upgrading Spring Security.
In our case it was worse because previously we were using NoOpPasswordEncoder and this upgradeEncoding method switches directly to BCrypt.
The CPU usage of BCrypt is very high, but this is documented, what is kind of unexpected is the switch or upgrade.
Configuring the DaoAuthenticationProvider programmatically fixes the issue.

@jzheaux
Copy link
Contributor

jzheaux commented Feb 10, 2022

Sorry that you experienced trouble with the upgrade, @andreild, this could have been messaged better.

I'm going to close this as a duplicate of #5939. #5939 (comment) contains the recommended approach for overriding the defaults.

@jzheaux jzheaux closed this as completed Feb 10, 2022
@jzheaux jzheaux self-assigned this Feb 10, 2022
@jzheaux jzheaux added in: crypto An issue in spring-security-crypto status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: crypto An issue in spring-security-crypto status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants