fix(authentication): Only verify each hash once #36048
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Joas Schilling <[email protected]>
nickvergessen
requested review from
ChristophWurst,
juliusknorr and
marcelklehr
Member
|
Yep, I'm sorry. I messed up the |
We need to store the new authentication details when the hash did **not** verify the old password. Signed-off-by: Joas Schilling <[email protected]>
marcelklehr
reviewed
Jan 9, 2023
marcelklehr
reviewed
Jan 9, 2023
Signed-off-by: Joas Schilling <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…t be verified Signed-off-by: Joas Schilling <[email protected]>
nickvergessen
force-pushed
the
bugfix/36046/only-verify-the-same-hash-once
branch
from
0810d46 to
2fb4dac
Compare
marcelklehr
approved these changes
Jan 9, 2023
Member
|
Thanks! Looks much better now :) |
juliusknorr
approved these changes
Jan 10, 2023
ChristophWurst
added
4. to release
Member
|
Customer reports slowdowns on basic auth with this applied to stable25 |
Member
Author
|
See above your comment? |
Member
|
Yes, that is the patch that causes slowdowns |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #36046
Regression from #33898
While the given patch heavily improves the situation, it still performs kind of a DDoS attach on the database, as all entries get updated on each authentication, which also is on basic auth.
I might be wrong, but I think
$this->hasher->verifyis meant to be!$this->hasher->verify, so we update the password of all tokens that do NOT decrypt to the current password?Checklist