bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt(). #4110

serhiy-storchaka · 2017-10-24T18:25:12Z

The log_rounds parameter for Blowfish is replaced with the rounds parameter.

https://bugs.python.org/issue31702

The log_rounds parameter for Blowfish is replaced with the rounds parameter.

taleinat · 2017-10-25T14:10:14Z

Lib/crypt.py

+        if rounds is None:
+            log_rounds = 12
+        else:
+            log_rounds = (rounds-1).bit_length()


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

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)

vstinner

I'm not sure between "1 << log_rounds" and "2 ** log_round", I have no preference. It's up to you ;-)

vstinner · 2017-10-26T09:33:44Z

Lib/test/test_crypt.py

-        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):


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

vstinner · 2017-10-26T09:34:56Z

Lib/test/test_crypt.py

+                        '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:


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

vstinner · 2017-10-26T09:36:00Z

Doc/library/crypt.rst

+   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


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

999999999: 9_999_99_999
2147483648: 2_147_483_648

vstinner · 2017-10-26T09:37:45Z

Doc/library/crypt.rst

+   ``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
+   power of two.


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.

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

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.

vstinner · 2017-10-26T09:38:57Z

Lib/crypt.py

+        if rounds is None:
+            log_rounds = 12
+        else:
+            log_rounds = (rounds-1).bit_length()


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)

vstinner · 2017-10-26T09:40:04Z

Lib/crypt.py

@@ -30,7 +30,13 @@ def mksalt(method=None, *, log_rounds=12):
    if not method.ident:
        s = ''
    elif method.ident[0] == '2':


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.

vstinner · 2017-10-26T09:40:50Z

Lib/crypt.py

@@ -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):


You might write 1 << 4 instead of 16.

vstinner · 2017-10-26T09:41:42Z

Lib/test/test_crypt.py

        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)


coding style: 1 << log_rounds (with spaces)

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.

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".

bedevere-bot · 2017-10-26T09:42:18Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka · 2017-10-31T12:01:10Z

I have made the requested changes; please review again.

bedevere-bot · 2017-10-31T12:01:12Z

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.

taleinat

LGTM

vstinner · 2017-10-31T12:39:43Z

Doc/library/crypt.rst

+   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``


What is the upper limit?

>>> 2**32 4294967296

vstinner · 2017-10-31T12:41:37Z

Lib/crypt.py

        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


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.

vstinner · 2017-10-31T12:42:19Z

Lib/crypt.py

    """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 = ''


I suggest to raise a ValueError if rounds is set (is not None) but the method doesn't accept a number of rounds.

vstinner · 2017-10-31T12:44:41Z

Lib/test/test_crypt.py

        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)


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".

vstinner · 2017-10-31T12:45:18Z

Lib/test/test_crypt.py

+                with self.assertRaises(ValueError):
+                    crypt.mksalt(method, rounds=rounds)
+        with self.assertRaises(ValueError):
+            crypt.mksalt(crypt.METHOD_BLOWFISH, rounds=1000)



I suggest to add tests to ensure that passing rounds to a method which doesn't support rounds raises an exception.

bedevere-bot · 2017-10-31T12:45:25Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

serhiy-storchaka · 2017-11-16T07:47:05Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-16T07:47:07Z

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.

vstinner

LGTM!

vstinner · 2017-11-16T11:02:38Z

Thanks, I prefer the new code to handle invalid values and the new tests.

* 'master' of https://github.com/python/cpython: (550 commits) bpo-31867: Remove duplicates in default mimetypes. (python#4388) tokenizer: Remove unused tabs options (python#4422) bpo-31691: Specify where to find build instructions for the Windows installer (python#4426) Fix typo in atexit documentation. (pythonGH-4419) bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt(). (python#4110) bpo-32043: New "developer mode": "-X dev" option (python#4413) bpo-30349: Raise FutureWarning for nested sets and set operations (python#1553) bpo-32037: Use the INT opcode for 32-bit integers in protocol 0 pickles. (python#4407) bpo-30143: 2to3 now generates a code that uses abstract collection classes (python#1262) bpo-32030: Enhance Py_Main() (python#4412) bpo-32030: Split Py_Main() into subfunctions (python#4399) bpo-32034: Make IncompleteReadError & LimitOverrunError pickleable python#4409 bpo-32025: Add time.thread_time() (python#4410) bpo-32018: Fix inspect.signature repr to follow PEP 8 (python#4408) bpo-30399: Get rid of trailing comma in the repr of BaseException. (python#1650) bpo-30950: Convert round() to Argument Clinic. (python#2740) bpo-32011: Revert "Issue python#15480: Remove the deprecated and unused TYPE_INT64 code from marshal." (python#4381) bpo-32023: Disallow genexprs without parenthesis in class definitions. (python#4400) bpo-31949: Fixed several issues in printing tracebacks (PyTraceBack_Print()). (python#4289) bpo-32032: Test both implementations of module-level pickle API. (python#4401) ...

bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt().

4ebdd2a

The log_rounds parameter for Blowfish is replaced with the rounds parameter.

serhiy-storchaka added the type-feature A feature request or enhancement label Oct 24, 2017

the-knights-who-say-ni added the CLA signed label Oct 24, 2017

bedevere-bot added the awaiting merge label Oct 24, 2017

taleinat reviewed Oct 25, 2017

View reviewed changes

Merge branch 'master' into crypt-sha2-rounds

1435af6

vstinner requested changes Oct 26, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Oct 26, 2017

serhiy-storchaka added 2 commits October 26, 2017 15:07

Merge branch 'master' into crypt-sha2-rounds

bb0d02c

Address review comments.

8c385ca

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 31, 2017

taleinat reviewed Oct 31, 2017

View reviewed changes

vstinner requested changes Oct 31, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Oct 31, 2017

serhiy-storchaka added 2 commits November 9, 2017 18:05

Merge branch 'master' into crypt-sha2-rounds

957afaf

Address review comments.

6c76715

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 16, 2017

vstinner approved these changes Nov 16, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 16, 2017

serhiy-storchaka merged commit cede8c9 into python:master Nov 16, 2017

bedevere-bot removed the awaiting merge label Nov 16, 2017

serhiy-storchaka deleted the crypt-sha2-rounds branch November 16, 2017 11:22

JelleZijlstra mentioned this pull request Mar 18, 2018

Implement 3.7 features python/typeshed#1965

Closed

41 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt(). #4110

bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt(). #4110

serhiy-storchaka commented Oct 24, 2017 •

edited by bedevere-bot

Loading

taleinat Oct 25, 2017

vstinner Oct 26, 2017

vstinner left a comment

vstinner Oct 26, 2017

vstinner Oct 26, 2017

vstinner Oct 26, 2017

vstinner Oct 26, 2017

taleinat Oct 26, 2017

serhiy-storchaka Oct 26, 2017

vstinner Oct 26, 2017

vstinner Oct 26, 2017

vstinner Oct 26, 2017

vstinner Oct 26, 2017 •

edited by taleinat

Loading

serhiy-storchaka Oct 26, 2017

vstinner Oct 31, 2017

bedevere-bot commented Oct 26, 2017

serhiy-storchaka commented Oct 31, 2017

bedevere-bot commented Oct 31, 2017

taleinat left a comment

vstinner Oct 31, 2017

serhiy-storchaka Nov 9, 2017

vstinner Oct 31, 2017

vstinner Oct 31, 2017

vstinner Oct 31, 2017

vstinner Oct 31, 2017

bedevere-bot commented Oct 31, 2017

serhiy-storchaka commented Nov 16, 2017

bedevere-bot commented Nov 16, 2017

vstinner left a comment

vstinner commented Nov 16, 2017

bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt(). #4110

bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt(). #4110

Conversation

serhiy-storchaka commented Oct 24, 2017 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner Oct 26, 2017 • edited by taleinat Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 26, 2017

serhiy-storchaka commented Oct 31, 2017

bedevere-bot commented Oct 31, 2017

taleinat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 31, 2017

serhiy-storchaka commented Nov 16, 2017

bedevere-bot commented Nov 16, 2017

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Nov 16, 2017

serhiy-storchaka commented Oct 24, 2017 •

edited by bedevere-bot

Loading

vstinner Oct 26, 2017 •

edited by taleinat

Loading