-
Notifications
You must be signed in to change notification settings - Fork 4
bcrypt improvements, test fixup #23
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
base: main
Are you sure you want to change the base?
Conversation
…on check in its specific backend, truncating before hashing if the backend will raise an error
…ing secret in fuzzer to avoid raising an error, adding more verbosity when an error is raise
I can confirm that the same is the case on NixOS. |
chapmajs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change creates a hard dependency on packaging if optional dependency bcrypt is used.
|
Is there anything holding this PR up? |
No, but I'm not quite sure if we need to bring in |
|
I figured the version check was less brittle than depending on the 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? |
chapmajs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removes dependency on packaging, checking the version manually with split() and an int cast is definitely the better solution here.
(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
TruncateMixinforce activated as @mo7ty did in #19 was not accomplishing its intended goal:TruncateMixinallows 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
bcrypthandler vs. thebcryptbackend, I decided to introduce another fixup variable to thebcrypthandler,_backend_raises_on_truncation. It defaults toFalse, but can be setTrueif the Pythonbcryptbackend is being used and its version is v5.0.0 or higher. The detection was moved to the_BcryptBackendclass as it should be entirely limited to the Pythonbcryptbackend only._backend_raises_on_truncationis now used to selectively skip the wraparound bug test. This is a minor improvement in efficiency over catching theValueErrorexception. 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 thatbcryptstill functions. I am hesitant to alter this functionality as it is security critical._backend_raises_on_truncationis also used in_BcryptBackend._calc_checksum()to auto-truncate the secret to 72 characters for the Pythonbcryptbackend. This solves the raising ofValueErrorduring test case runs.Fixing the errors reported by @dotlambda in #21 exposed an error in
test_handlers_bcrypt.pywhere the fuzzer was callingbcrypt.hashpw()in the Pythonbcryptlibrary directly, and was tripping over theValueErrorbeing raised. This was less obvious because that particular method actually catchesValueErrorand re-raises anotherValueError, but did not report the original cause. The input tobcrypt.hashpw()is now truncated to 72 characters, and in the event of aValueError, the text of the error is displayed in the re-raisedValueError.All tests are now passing with
bcryptv5.0.0 on Slackware 15.