Improved fixup for bcrypt >= 5.0.0, bcrypt version detection

[chapmajs]

This PR fixes the issue introduced with bcrypt >= 5.0.0 wherein testing for the blowfish wraparound bug breaks due to a change in bcrypt behavior. Prior to v5.0.0, input greater than 72 characters was either truncated, or (when the wraparound bug was actually present) triggered a fairly significant hash collision bug. Version 5.0.0 introduced throwing of a hard ValueError with input that exceeds 72 characters, breaking the tests in passlib.

This PR tests for the ValueError response, which indicates that the wraparound bug is not present, and truncates the test string down to 72 characters so that the test can proceed to verifying that the hashing algorithm actually works as expected.

Implementing this fix allows unpinning bcrypt so that v5.0.0 may be run. We are using this patch in production on Slackware Linux 15 AMD64 with bcrypt v5.0.0.

Also included in this PR is an improved version detection routine for the bcrypt backend, which will function with "new style" (post-November 2023) bcrypt, as well as older versions.

[notypecheck]

Do we have to do this instead of using packaging.metadata.version? 🤔

P.S. The updated hash check looks fine though

[chapmajs]

I only did the check that way as metadata.version() failed on a random old install I tested on. The above works in both cases.

[notypecheck]

On what version specifically does it fail? 🤔 From what I know metadata.version/distribution should get the info from .dist-info, maybe that specific version is missing some metadata or isn't packaged "correctly".

[chapmajs]

Old Slackware 14.2 i486 install, I'd have to start the machine up to check passlib/bcrypt versions. If it's guaranteed functionality/just some packaging aberration switch it back to packaging.metadata.version (or I can do it and amend the PR!)

[notypecheck]

Can you rollback changes related to retrieving bcrypt version? There are some problems left with linters/formatting, but I can fix that myself.

[chapmajs]

Done! Would you like it smashed down into a single commit?

[notypecheck]

Done! Would you like it smashed down into a single commit?

I can do it myself, and should be able to release a new patch a bit later today, if that's some kind of a blocker for you.

[chapmajs]

Thanks! It's not a hard block for me, we run our own Slackware package repository so we've been running the patches in this PR locally since it became an issue. I appreciate the quick turnaround though, hopefully this fork can become the official passlib and get some attention.

[dotlambda]

I'm trying this in Nixpkgs but still get

FAILED tests/test_handlers_bcrypt.py::bcrypt_bcrypt_test::test_70_hashes - ValueError: password cannot be longer than 72 bytes, truncate manually if n...
FAILED tests/test_handlers_bcrypt.py::bcrypt_bcrypt_test::test_77_fuzz_input - ValueError: password cannot be longer than 72 bytes, truncate manually if n...
FAILED tests/test_handlers_django.py::django_bcrypt_test::test_77_fuzz_input - ValueError: password cannot be longer than 72 bytes, truncate manually if n...
FAILED tests/test_handlers_bcrypt.py::bcrypt_bcrypt_test::test_secret_w_truncate_size - ValueError: password cannot be longer than 72 bytes, truncate manually if n...
FAILED tests/test_handlers_django.py::django_bcrypt_test::test_secret_w_truncate_size - ValueError: password cannot be longer than 72 bytes, truncate manually if n...

Any idea why this could be?

[mo7ty]

Hi @dotlambda

I'm trying this in Nixpkgs but still get

FAILED tests/test_handlers_bcrypt.py::bcrypt_bcrypt_test::test_70_hashes - ValueError: password cannot be longer than 72 bytes, truncate manually if n...
FAILED tests/test_handlers_bcrypt.py::bcrypt_bcrypt_test::test_77_fuzz_input - ValueError: password cannot be longer than 72 bytes, truncate manually if n...
FAILED tests/test_handlers_django.py::django_bcrypt_test::test_77_fuzz_input - ValueError: password cannot be longer than 72 bytes, truncate manually if n...
FAILED tests/test_handlers_bcrypt.py::bcrypt_bcrypt_test::test_secret_w_truncate_size - ValueError: password cannot be longer than 72 bytes, truncate manually if n...
FAILED tests/test_handlers_django.py::django_bcrypt_test::test_secret_w_truncate_size - ValueError: password cannot be longer than 72 bytes, truncate manually if n...

Any idea why this could be?

Some are fixed in #19:

• 050d68e
• f01cc58

Others aren't yet - refer to #19 (comment)

[dotlambda]

But the CI passed on this PR. That's why I expected there to be no failures anymore.

[chapmajs]

Looks like the tests are putting in a too-long secret and not handling the exception. (I now have them running locally so I could get more verbose output) This PR doesn't stop too-long secrets from being passed in, it just doesn't trip on them during the wraparound tests.

But the CI passed on this PR. That's why I expected there to be no failures anymore.

I was expecting CI to have run the tests too :P

[notypecheck]

Tests should run in CI, I think version older than 5.0.0 is pinned in the lockfile. Regardless of that I think passlib should raise it's own error when trying to pass passwords of length greater than 72 with bcrypt. There's already a mixin class that does that, and I'm currently planning to add that in next minor release 🤔

[chapmajs]

Oddly enough, it's supposed to be using the mixin already, but something's not right with it. I'm having a dig currently...

[notypecheck]

It's just not configured to raise an error 🤷

[chapmajs]

Selectively turning it on when the handler detects that the backend will raise on too-long passwords doesn't fix all of the failing test cases though.

[chapmajs]

Figured it out, new PR incoming.

[chapmajs]

OK, I think I now fully understand the intended functionality of the TruncateMixin, it kind of worked opposite of how I thought I understood it.