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

Replace bcrypt hashing with blake3 for improved performance #1030

Closed
wants to merge 6 commits into from

Conversation

mrpriv4te
Copy link

Hi nielsfaber! I made this change because the bcrypt hash function was too slow, and with several users and codes on my installation, I observed delays of 5-10s to verify the pincode/password.

@nielsfaber
Copy link
Owner

Hi @mrpriv4te, thanks for the PR.
I am not an expert on hashing methods so if you found a better alternative it's good to me.
I must say I'm surprised with the high delay you are experiencing, I believe Alarmo uses a similar implementation as HA.

One thing holding me back from merging this:
Alarmo stores the hashed passwords, so users have now bcrypt-ed hashes in their configuration.
I assume these are not compatible with your modified code, so users will need to reset their password before it would work.
I think we should make a backwards-compatible implementation, for example by detecting the pattern of the stored hash and using bcrypt if so.
What are your thoughts about this?

@mrpriv4te
Copy link
Author

After further testing, I realized that the time saving was not significant, so I opted for sha256, which is faster.

I must say I'm surprised with the high delay you are experiencing, I believe Alarmo uses a similar implementation as HA.

If you want to reproduce the latency, add several users (12 in my case) to Alarmo and try arming or disarming with the code of the last user created.

I think we should make a backwards-compatible implementation, for example by detecting the pattern of the stored hash and using bcrypt if so.
What are your thoughts about this?

I totally agree with you, and now it's done.

@marazmarci
Copy link

@mrpriv4te please don't just use plain SHA256 without salting because a password will be much easier to reverse from known hashes or by using a rainbow table.

The original bcrypt.hashpw method used a randomly generated salt and ran 2^12=4096 rounds of hashing, which is pretty standard and secure. We should come up with a better solution that doesn't compromise security.

@mrpriv4te
Copy link
Author

mrpriv4te commented Sep 20, 2024

@mrpriv4te please don't just use plain SHA256 without salting because a password will be much easier to reverse from known hashes or by using a rainbow table.

The original bcrypt.hashpw method used a randomly generated salt and ran 2^12=4096 rounds of hashing, which is pretty standard and secure. We should come up with a better solution that doesn't compromise security.

Hi @marazmarci, thanks for your feedback !

I've now added salting to the password hash, and I've replaced sha256 with blake3, which is not vulnerable to length extension attack.

There's still one problem, blake3's python module needs rust/cargo and gcc to build, but I don't know how an HA integration can install these dependencies.

@mrpriv4te mrpriv4te changed the title Replace bcrypt hashing with argon2 for improved performance Replace bcrypt hashing with blake3 for improved performance Sep 20, 2024
Remove unused const
@nielsfaber
Copy link
Owner

@mrpriv4te Following the discussion I'm not confident we should change the current password encryption.
I don't have the knowledge to evaluate the impact of your proposed change so I rather keep the same method as used by HA.

I also think there is not much benefit to this change for other users.
You said you created 12 different pin codes, which I don't expect many other users will have.
In addition the computation time can also be related to the hardware you are using for running HA.

If you still want to see this PR merged, I would like this change to be reviewed by multiple people who have experience with encryption methods.

@mrpriv4te
Copy link
Author

Hi @nielsfaber, I totally understand, I'll use my fork on my side.

@mrpriv4te mrpriv4te closed this Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants