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

Fix maximum password length enforcement #44

Merged
merged 1 commit into from
Feb 9, 2023
Merged

Fix maximum password length enforcement #44

merged 1 commit into from
Feb 9, 2023

Conversation

quinot
Copy link
Contributor

@quinot quinot commented Feb 6, 2023

NUL terminators play a role in keying only if the password is shorter than 72 bytes. For a password that is exactly 72 bytes, no cyclic repetition occurs in the key expansion phase, and no NUL is needed: the password can be used as-is; in other words, the NUL terminator should not be counted against the 72 bytes limit.

Adjust test cases accordingly.
Test vectors in testReferenceValuesWithoutNullTerminator have been tested against the Python bcrypt module.

NUL terminators play a role in keying only if the password is shorter
than 72 bytes. For a password that is exactly 72 bytes, no cyclic
repetition occurs in the key expansion phase, and no NUL is needed: the
password can be used as-is; in other words, the NUL terminator should
not be counted against the 72 bytes limit.

Adjust test cases accordingly.
Test vectors in testReferenceValuesWithoutNullTerminator
have been tested against the Python bcrypt module.
@patrickfav
Copy link
Owner

Thank you for your PR!

@quinot quinot mentioned this pull request Feb 10, 2023
@Andrew-Cottrell
Copy link

I suggest the public field BCrypt.Version.DEFAULT_MAX_PW_LENGTH_BYTE be retained but marked deprecated, unused, and replaced by BCrypt.Version.MAX_PW_LENGTH_BYTE. Then it might be deleted a few releases later.

My code currently uses BCrypt.Version.DEFAULT_MAX_PW_LENGTH_BYTE and while I am aware I need to modify the code when I upgrade the library, others might not be aware, and a deprecation message may help them identify the issue and also enable them to decouple the modifications and the upgrade.

@patrickfav
Copy link
Owner

@Andrew-Cottrell good point, I re-introduced it with 0.10.1, see #48

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