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 1 commit
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 ``999999999``. For ``METHOD_BLOWFISH`` it must be a power of
two between ``16`` and ``2147483648`` (2\ :sup:`32`), the default is
``4096``. If it isn't a power of two, it will be rounded up to the next
Copy link
Member

Choose a reason for hiding this comment

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

Can we use "_" in integer literals to make them more readable?

999999999: 9_999_99_999
2147483648: 2_147_483_648

power of two.
Copy link
Member

Choose a reason for hiding this comment

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

For blowfish, you may suggest to use "1 << log_rounds" syntax with an example:

For example, ``rounds=1 << 10`` for 2\ :sup:`10` (1024) rounds.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO 2 ** 10 would be clearer than 1 << 10.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since there are different ways of writing a power of two (you can also use binary, octal or hexadecimal literals) I prefer to not suggest a one particular syntax.


.. 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
14 changes: 10 additions & 4 deletions Lib/crypt.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ 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.
Expand All @@ -30,7 +30,13 @@ def mksalt(method=None, *, log_rounds=12):
if not method.ident:
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':
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this test. Which methods are supposed to be matched? Would you mind to add just one example in a comment? Same question for "elif method.ident in ('5', '6')" below.

if rounds is None:
log_rounds = 12
else:
log_rounds = (rounds-1).bit_length()
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the discussion in bpo-31702, ValueError should be raised in case rounds isn't a power of two.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. You must raise a VAlueError if it's not exactly a power of 2.

if (1 << log_rounds) != rounds: raise ValueError("rounds must be a power of 2, got %s" % rounds)

s = f'${method.ident}${log_rounds:02d}$'
elif method.ident in ('5', '6') and rounds is not None:
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 +61,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 +80,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=16):
Copy link
Member

Choose a reason for hiding this comment

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

You might write 1 << 4 instead of 16.

break

_add_method('MD5', '1', 8, 34)
Expand Down
26 changes: 20 additions & 6 deletions Lib/test/test_crypt.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,26 @@ 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, 10000, 100000:
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind to write: "1_000, 10_000, 100_000"? It would be easier to read :-)

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)
Expand All @@ -54,9 +68,9 @@ def test_log_rounds(self):

@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)
def test_invalid_rounds(self):
for log_rounds in (0, 1, 1<<999):
Copy link
Member

Choose a reason for hiding this comment

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

Rename also log_rounds variable, no? below, you use "rounds"?

salt = crypt.mksalt(crypt.METHOD_BLOWFISH, rounds=rounds)
self.assertIsNone(crypt.crypt('mypassword', salt))

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.


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.