Skip to content

Conversation

@CarlSchwan
Copy link
Member

using password_hash is expensive and should be used for hashing
passwords when saving them in the database. Here we just want to see if
the bind was already done with the given password.

Time spent on testing.nextcloud.com for password hashing 160ms:

image

@CarlSchwan CarlSchwan requested review from blizzz and come-nc May 2, 2022 19:50
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe name it sum instead of hash and use a fast checksum method like sha instead?
I dislike storing the clear password in a random variable, it might cause problems down the road.

@come-nc
Copy link
Contributor

come-nc commented May 3, 2022

Maybe name it sum instead of hash and use a fast checksum method like sha instead? I dislike storing the clear password in a random variable, it might cause problems down the road.

This sum could be of dn+prefix+pwd to test if the bind was with the same parameters in one comparison.

@CarlSchwan CarlSchwan force-pushed the performance/remove-bind-hash branch 2 times, most recently from a521e0b to e3e3e6e Compare May 3, 2022 08:14
@CarlSchwan
Copy link
Member Author

Maybe name it sum instead of hash and use a fast checksum method like sha instead? I dislike storing the clear password in a random variable, it might cause problems down the road.

I switched to an md5 hash instead. It's still quite useless in my opinion since the password is already accessible in clear from the app config database, so adding a hash to compare two strings is not required even if the string is confidential.

At least with md5, the hashing will be quite a bit faster.

@come-nc
Copy link
Contributor

come-nc commented May 3, 2022

I switched to an md5 hash instead. It's still quite useless in my opinion since the password is already accessible in clear from the app config database, so adding a hash to compare two strings is not required even if the string is confidential.

Yeah I know we store the data but the idea is to not have it in too many variables to not encourage its use everywhere, and be able to track each use by looking at configuration reads.

@CarlSchwan CarlSchwan force-pushed the performance/remove-bind-hash branch from e3e3e6e to 0648ecc Compare May 5, 2022 14:03
Using password_hash is expensive and should be used for hashing
passwords when saving them in the database. Here we just want to see if
the bind was already done with the given password, so use a fast hashing
algorythm.

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan CarlSchwan force-pushed the performance/remove-bind-hash branch from 0648ecc to 95b5187 Compare May 5, 2022 14:29
@CarlSchwan CarlSchwan requested a review from PVince81 May 5, 2022 16:05
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@PVince81 PVince81 merged commit 99151a5 into master May 5, 2022
@PVince81 PVince81 deleted the performance/remove-bind-hash branch May 5, 2022 16:09
@CarlSchwan
Copy link
Member Author

/backport to stable23

@CarlSchwan
Copy link
Member Author

/backport to stable22

@CarlSchwan
Copy link
Member Author

/backport to stable24

@blizzz
Copy link
Member

blizzz commented May 5, 2022

sha256 over md5 to be more robust against collisions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants