Skip to content

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

Merged
merged 6 commits into from
Nov 16, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions Doc/library/crypt.rst
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ The :mod:`crypt` module defines the following functions:
Accept ``crypt.METHOD_*`` values in addition to strings for *salt*.


.. function:: mksalt(method=None, *, log_rounds=12)
.. function:: mksalt(method=None, *, rounds=None)

Return a randomly generated salt of the specified method. If no
*method* is given, the strongest method available as returned by
Expand All @@ -125,14 +125,18 @@ The :mod:`crypt` module defines the following functions:
The return value is a string suitable for passing as the *salt* argument
to :func:`crypt`.

*log_rounds* specifies the binary logarithm of the number of rounds
for ``crypt.METHOD_BLOWFISH``, and is ignored otherwise. ``8`` specifies
``256`` rounds.
*rounds* specifies the number of rounds for ``METHOD_SHA256``,
``METHOD_SHA512`` and ``METHOD_BLOWFISH``, and is ignored otherwise.
For ``METHOD_SHA256`` and ``METHOD_SHA512`` it must be an integer between
``1000`` and ``999_999_999``, the default is ``5000``. For
``METHOD_BLOWFISH`` it must be a power of two between ``16`` (2\ :sup:`4`)
and ``2_147_483_648`` (2\ :sup:`32`), the default is ``4096``
Copy link
Member

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?

>>> 2**32
4294967296

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2**31

(2\ :sup:`12`).

.. versionadded:: 3.3

.. versionchanged:: 3.7
Added the *log_rounds* parameter.
Added the *rounds* parameter.


Examples
Expand Down
3 changes: 3 additions & 0 deletions Doc/whatsnew/3.7.rst
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,9 @@ crypt
Added support for the Blowfish method.
(Contributed by Serhiy Storchaka in :issue:`31664`.)

The :func:`~crypt.mksalt` function now allows to specify the number of rounds
for hashing. (Contributed by Serhiy Storchaka in :issue:`31702`.)

dis
---

Expand Down
25 changes: 19 additions & 6 deletions Lib/crypt.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ''
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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))
Expand All @@ -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)
Expand All @@ -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)
Expand Down
44 changes: 29 additions & 15 deletions Lib/test/test_crypt.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

@vstinner vstinner Oct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coding style: 1 << log_rounds (with spaces)

Copy link
Member Author

Choose a reason for hiding this comment

The 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 = in keyword argument passing. rounds=1 << log_rounds looks ugly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rounds=1 << log_rounds looks ugly.

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)

Copy link
Member

Choose a reason for hiding this comment

The 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__":
Expand Down
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.