ThirVondukr
bcrypt improvements, test fixup
(this PR is on top of #22)
The work in this PR was triggered by @dotlambda in #21 experiencing test failure after #21 was closed.
In digging into the underlying issue, I found that having the TruncateMixin force activated as @mo7ty did in #19 was not accomplishing its intended goal: TruncateMixin allows for changing optional behavior, and shouldn't be used to solve this problem. I initially had its operation backwards in my mind and it took a bit of digging through other hashing algorithms to realize the mistake!
In the course of the work and in gaining a better understanding of the bcrypt handler vs. the bcrypt backend, I decided to introduce another fixup variable to the bcrypt handler, _backend_raises_on_truncation. It defaults to False, but can be set True if the Python bcrypt backend is being used and its version is v5.0.0 or higher. The detection was moved to the _BcryptBackend class as it should be entirely limited to the Python bcrypt backend only.
_backend_raises_on_truncation is now used to selectively skip the wraparound bug test. This is a minor improvement in efficiency over catching the ValueError exception. Presence of auto-truncation (with or without raising an error) should preclude the presence of the wraparound bug, but we continue to the belt-and-suspenders check that bcrypt still functions. I am hesitant to alter this functionality as it is security critical.
_backend_raises_on_truncation is also used in _BcryptBackend._calc_checksum() to auto-truncate the secret to 72 characters for the Python bcrypt backend. This solves the raising of ValueError during test case runs.
Fixing the errors reported by @dotlambda in #21 exposed an error in test_handlers_bcrypt.py where the fuzzer was calling bcrypt.hashpw() in the Python bcrypt library directly, and was tripping over the ValueError being raised. This was less obvious because that particular method actually catches ValueError and re-raises another ValueError, but did not report the original cause. The input to bcrypt.hashpw() is now truncated to 72 characters, and in the event of a ValueError, the text of the error is displayed in the re-raised ValueError.
All tests are now passing with bcrypt v5.0.0 on Slackware 15.
All tests are now passing with
bcryptv5.0.0 on Slackware 15.
I can confirm that the same is the case on NixOS.
Is there anything holding this PR up?
No, but I'm not quite sure if we need to bring in packaging as an extra dependency, and why simply checking if bcrypt raises ValueError wouldn't suffice here 🤔
I figured the version check was less brittle than depending on the ValueError, but I too am on the fence about bringing in packaging. I could split the version string and do the compare manually, but in researching "best practices" (e.g. what other projects are doing) for Python library version compares, it seems like packaging is often preferred -- which of course doesn't mean it's right, or that it's right for passlib!
Realistically, since this is a change that happens on a clean major release increment, the compare can be the first digit, which limits the potential for situations where a patch on a point-level version breaks the compare.
Thoughts?