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

Add a work around bcrypt maximum password length #45

Merged
merged 2 commits into from
Feb 4, 2017
Merged

Add a work around bcrypt maximum password length #45

merged 2 commits into from
Feb 4, 2017

Conversation

bovarysme
Copy link
Contributor

@bovarysme bovarysme commented Feb 4, 2017

Hello,

The bcrypt algorithm has a maximum password length of 72 bytes, and ignores any bytes beyond that.
This pull request adds a workaround via the BCRYPT_HANDLE_LONG_PASSWORDS configuration value. When set to True, a given password is hashed using sha256 before being fed to bcrypt.
Note: this option should not be set to True on projects that are already using Flask-Bcrypt, or password checking will break (as we are hashing the sha256 of the password instead of the password itself).

I would like to discuss about a few points:

  • An argument could be added to the helpers to enable this option on the fly. But I believe this adds too much complexity, as the option would have to be enabled on both hash generation and hash checking.
  • By sub classing BasicTestCase, tests are now running twice: once with the option set to False, and once with the option set to True. I'm not sure if this is really necessary.

By default, the bcrypt algorithm has a maximum password length of 72 bytes
and ignores any bytes beyond that. A common workaround is to hash the
given password using a cryptographic hash (such as `sha256`), take its
hexdigest to prevent NULL byte problems, and hash the result with bcrypt.
If the `BCRYPT_HANDLE_LONG_PASSWORDS` configuration value is set to `True`,
the workaround described above will be enabled.

WARNING: enabling this option on a project that is already using Flask-Bcrypt
will break password checking.
@maxcountryman
Copy link
Owner

Thank you, this is fantastic!

I agree with your first point I think. Although I'm open to adding the option if people find it useful.

As to your second point it's been so long since those tests were written I can't honestly recall how they work. However running them twice is probably fine.

@maxcountryman maxcountryman merged commit 01aca6a into maxcountryman:master Feb 4, 2017
@bovarysme
Copy link
Contributor Author

Thank you for the merge!

Another thing to consider is that this work around can provide less entropy in some cases: http://stackoverflow.com/a/16597402
But it can be an acceptable trade-off for people who want to add support for longer passwords.

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.

2 participants