-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt(). #4110
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
Changes from 4 commits
4ebdd2a
1435af6
bb0d02c
8c385ca
957afaf
6c76715
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,18 +19,31 @@ def __repr__(self): | |
return '<crypt.METHOD_{}>'.format(self.name) | ||
|
||
|
||
def mksalt(method=None, *, log_rounds=12): | ||
def mksalt(method=None, *, rounds=None): | ||
"""Generate a salt for the specified method. | ||
|
||
If not specified, the strongest available method will be used. | ||
|
||
""" | ||
if method is None: | ||
method = methods[0] | ||
if not method.ident: | ||
if not method.ident: # traditional | ||
s = '' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest to raise a ValueError if rounds is set (is not None) but the method doesn't accept a number of rounds. |
||
elif method.ident[0] == '2': | ||
elif method.ident[0] == '2': # Blowfish variants | ||
if rounds is None: | ||
log_rounds = 12 | ||
else: | ||
log_rounds = int.bit_length(rounds-1) | ||
if rounds != 1 << log_rounds: | ||
raise ValueError('rounds must be a power of 2') | ||
if not 4 <= log_rounds <= 31: | ||
raise ValueError('rounds out of the range 2**4 to 2**31') | ||
s = f'${method.ident}${log_rounds:02d}$' | ||
elif method.ident in ('5', '6') and rounds is not None: # SHA-2 | ||
range(rounds) # raise a TypeError for non-integers | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a really weird way to check if rounds is an integer. Why not just testing isinstance(rounds, int) and raise a TypeError explicitly? The check should be done at the top of the function. |
||
if not 1000 <= rounds <= 999_999_999: | ||
raise ValueError('rounds out of the range 1000 to 999_999_999') | ||
s = f'${method.ident}$rounds={rounds}$' | ||
else: | ||
s = f'${method.ident}$' | ||
s += ''.join(_sr.choice(_saltchars) for char in range(method.salt_chars)) | ||
|
@@ -55,10 +68,10 @@ def crypt(word, salt=None): | |
# available salting/crypto methods | ||
methods = [] | ||
|
||
def _add_method(name, *args): | ||
def _add_method(name, *args, rounds=None): | ||
method = _Method(name, *args) | ||
globals()['METHOD_' + name] = method | ||
salt = mksalt(method, log_rounds=4) | ||
salt = mksalt(method, rounds=rounds) | ||
result = crypt('', salt) | ||
if result and len(result) == method.total_size: | ||
methods.append(method) | ||
|
@@ -74,7 +87,7 @@ def _add_method(name, *args): | |
# 'y' is the same as 'b', for compatibility | ||
# with openwall crypt_blowfish. | ||
for _v in 'b', 'y', 'a', '': | ||
if _add_method('BLOWFISH', '2' + _v, 22, 59 + len(_v)): | ||
if _add_method('BLOWFISH', '2' + _v, 22, 59 + len(_v), rounds=1<<4): | ||
break | ||
|
||
_add_method('MD5', '1', 8, 34) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,31 +39,45 @@ def test_methods(self): | |
else: | ||
self.assertEqual(crypt.methods[-1], crypt.METHOD_CRYPT) | ||
|
||
@unittest.skipUnless(crypt.METHOD_SHA256 in crypt.methods or | ||
crypt.METHOD_SHA512 in crypt.methods, | ||
'requires support of SHA-2') | ||
def test_sha2_rounds(self): | ||
for method in (crypt.METHOD_SHA256, crypt.METHOD_SHA512): | ||
for rounds in 1000, 10_000, 100_000: | ||
salt = crypt.mksalt(method, rounds=rounds) | ||
self.assertIn('$rounds=%d$' % rounds, salt) | ||
self.assertEqual(len(salt) - method.salt_chars, | ||
11 + len(str(rounds))) | ||
cr = crypt.crypt('mypassword', salt) | ||
self.assertTrue(cr) | ||
cr2 = crypt.crypt('mypassword', cr) | ||
self.assertEqual(cr2, cr) | ||
|
||
@unittest.skipUnless(crypt.METHOD_BLOWFISH in crypt.methods, | ||
'requires support of Blowfish') | ||
def test_log_rounds(self): | ||
self.assertEqual(len(crypt._saltchars), 64) | ||
def test_blowfish_rounds(self): | ||
for log_rounds in range(4, 11): | ||
salt = crypt.mksalt(crypt.METHOD_BLOWFISH, log_rounds=log_rounds) | ||
salt = crypt.mksalt(crypt.METHOD_BLOWFISH, rounds=1<<log_rounds) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. coding style: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure about this one. According the coding stile there shouldn't be spaces around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, coding style is not a matter of taste, but more on agreeing on one style and use it for the whole code base. The coding style is the PEP 8. For example, flake8 complains with "E227 missing whitespace around bitwise or shift operator". |
||
self.assertIn('$%02d$' % log_rounds, salt) | ||
self.assertIn(len(salt) - crypt.METHOD_BLOWFISH.salt_chars, {6, 7}) | ||
cr = crypt.crypt('mypassword', salt) | ||
self.assertTrue(cr) | ||
cr2 = crypt.crypt('mypassword', cr) | ||
self.assertEqual(cr2, cr) | ||
|
||
@unittest.skipUnless(crypt.METHOD_BLOWFISH in crypt.methods, | ||
'requires support of Blowfish') | ||
def test_invalid_log_rounds(self): | ||
for log_rounds in (1, -1, 999): | ||
salt = crypt.mksalt(crypt.METHOD_BLOWFISH, log_rounds=log_rounds) | ||
cr = crypt.crypt('mypassword', salt) | ||
if cr is not None: | ||
# On failure the openwall implementation returns a magic | ||
# string that is shorter than 13 characters and is guaranteed | ||
# to differ from a salt. | ||
self.assertNotEqual(cr, salt) | ||
self.assertLess(len(cr), 13) | ||
def test_invalid_rounds(self): | ||
for method in (crypt.METHOD_SHA256, crypt.METHOD_SHA512, | ||
crypt.METHOD_BLOWFISH): | ||
with self.assertRaises(TypeError): | ||
crypt.mksalt(method, rounds='4096') | ||
with self.assertRaises(TypeError): | ||
crypt.mksalt(method, rounds=4096.0) | ||
for rounds in (0, 1, -1, 1<<999): | ||
with self.assertRaises(ValueError): | ||
crypt.mksalt(method, rounds=rounds) | ||
with self.assertRaises(ValueError): | ||
crypt.mksalt(crypt.METHOD_BLOWFISH, rounds=1000) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest to add tests to ensure that passing rounds to a method which doesn't support rounds raises an exception. |
||
|
||
if __name__ == "__main__": | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
crypt.mksalt() now allows to specify the number of rounds for SHA-256 and | ||
SHA-512 hashing. |
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.
What is the upper limit?
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.
2**31