Add a KDF password manager. #14
Conversation
Fixes #9 Also refactor some of the internals of password.py to remove accumulated duplication in match() methods and encoding.
src/zope/password/password.py
Outdated
| return ( | ||
| encoded_password.startswith(b'{SHA}') or | ||
| encoded_password.startswith(self._prefix) or | ||
| encoded_password.startswith(b'{SHA1}')) |
There was a problem hiding this comment.
You could shorten this to a single .startswith((self._prefix, b'{SHA1}')) call.
There was a problem hiding this comment.
Thanks. I always forget that. Possibly because if you leave off the extra set of parentheses, bad things happen.
src/zope/password/password.py
Outdated
| """ | ||
| BCRYPT KDF password manager. | ||
|
|
||
| This manager converts a plain text password into a byte array . |
There was a problem hiding this comment.
Strange space in front of the ..
src/zope/password/password.py
Outdated
| #: The number of rounds of hashing that should be applied. | ||
| #: The higher the number, the slower it is. It should be at least | ||
| #: 50. | ||
| rounds = 1<<10 |
There was a problem hiding this comment.
I'm wondering if rounds = 2**10 wouldn't be a bit more Pythonic.
There was a problem hiding this comment.
Or even just the integer 1024.
I just did what z3c.bcrypt did, although I reduced the rounds a bit. The original rounds was taking something like four seconds on my newish-computer, which seems excessive.
src/zope/password/password.py
Outdated
| # XXX: Do we need to use a timing-safe comparison function here? | ||
| # z3c.bcrypt does, and so does bcrypt.hashpw. But there is no such | ||
| # public function | ||
| return hashed_password == encoded_password |
There was a problem hiding this comment.
hmac.compare_digest() is such a function, but only available in Python starting with 3.3.
There was a problem hiding this comment.
Thanks, I'll add that to the comment.
There was a problem hiding this comment.
Actually, I'll just use it when available. Thanks.
There was a problem hiding this comment.
Interestingly, hmac.compare_digest was actually backported to 2.7.7 in 2014. Do we need to care about supporting older point releases in a new version? I can add appropriate requires-python metadata to make sure a new release is not installed by older versions.
There was a problem hiding this comment.
Do we need to care about supporting older point releases in a new version?
Given that this is a security feature for a security-critical package, and that people who care about security are probably running 2.7.9+ anyway, I feel like it is going to be OK to require 2.7.7+ (a three-year-old release) in a new version. Especially since it adds new security-related features. And especially especially since the requires-python metadata generally means it won't get installed by accident.
I added this to the PR.
| if not self.match(hashed_password): | ||
| return False | ||
| rounds, salt, key = hashed_password[len(self._prefix):].split(b'$') | ||
| rounds = int(rounds, 16) |
There was a problem hiding this comment.
I'm worrying that it's easy to make this function crash if you provide corrupt hashes (e.g. wrong number of '$' signs or non-hex digits in the rounds portion).
I am not a crypto person and I don't know whether that's fine (hashes are probably not attacker-supplied?) or not.
There was a problem hiding this comment.
We want to avoid false positives here, so a crash is better than that, I think. But the hashed password would not be supplied by the attacher, the clear_password is. The hashed password is stored by the system and previously generated by this object, if it's being used according to the contract of IMatchingPasswordManager.
- whitespace - startswith can take a tuple - Use hmac.compare_digest for timing-safe comparisons. Require Python 2.7.7 or above. Add appropriate require_python metadata for this.
|
Thanks! |
Fixes #9
Also refactor some of the internals of password.py to remove accumulated duplication in match() methods and encoding.