Skip to content
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

Allow upgrading between different BCrypt encodings #7042

Conversation

@larsgrefer
Copy link
Contributor

commented Jun 26, 2019

This will allow the upgradeEncoding mechanism to upgrade from a lower-strength bcrypt to a higher-strength bcrypt encoding.

@rwinch
Copy link
Member

left a comment

Thanks for the PR @larsgrefer! I have provided comments inline

@larsgrefer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

I'm not sure what the default behavior for invalid inputs should be.

org.springframework.security.crypto.password.DelegatingPasswordEncoder#upgradeEncoding returns true for all invalid inputs (null, no parseable id, unknown id) while org.springframework.security.crypto.password.PasswordEncoder#upgradeEncoding always returns false.

The default of false makes sense, because it prevents useless re-encodings on every login.

How should org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder#upgradeEncoding behave for invalid inputs (null or non-bcrypt)? true, false or IllegalArgumentException ?

@rwinch

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Good question @larsgrefer

DelegatingPasswordEncoder returns true for null, no id, unknown id because we expect its configuration to change over time. All we care about is that the value is encoded with the configured PasswordEncoder. Note that these are actually valid as there is the potential to fallback if there is an unknown id, null id, etc.

BCryptPasswordEncoder should return false for a null input because it will blow up if it is encoded. For non-bcrypt it should fail with an Exception.

@larsgrefer larsgrefer force-pushed the larsgrefer:feature/upgradeBcryptEncoding branch from 07130ee to 1e05294 Jun 28, 2019

@rwinch

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Thanks for the updates @larsgrefer. It appears that the tests fail.

org.springframework.security.crypto.bcrypt.BCryptPasswordEncoderTests > upgradeFromNullOrEmpty FAILED
    org.junit.ComparisonFailure at BCryptPasswordEncoderTests.java:191

@larsgrefer larsgrefer force-pushed the larsgrefer:feature/upgradeBcryptEncoding branch from 1e05294 to f96bcd3 Jun 28, 2019

@larsgrefer larsgrefer force-pushed the larsgrefer:feature/upgradeBcryptEncoding branch from f96bcd3 to a875499 Jun 28, 2019

@larsgrefer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

@rwinch The tests are fixed now

@rwinch rwinch self-assigned this Jul 3, 2019

@rwinch rwinch added this to the 5.2.0.RC1 milestone Jul 3, 2019

@rwinch

rwinch approved these changes Jul 3, 2019

@rwinch rwinch closed this in d3d6a87 Jul 3, 2019

rwinch added a commit that referenced this pull request Jul 3, 2019

@rwinch

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Thanks for all your work on this PR @larsgrefer! This is now merged into master via d3d6a87 I added a little polish to the tests via 742df2c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.