bpo-31664: Add support of Blowfish in crypt.#3854
bpo-31664: Add support of Blowfish in crypt.#3854serhiy-storchaka merged 7 commits intopython:masterfrom
Conversation
| .. data:: METHOD_BLF | ||
|
|
||
| Another Modular Crypt Format method with 22 character salt and 31 | ||
| character hash. |
There was a problem hiding this comment.
Please add the "Blowfish" name, which is better known than "BLF".
|
|
||
| A Modular Crypt Format method with 16 character salt and 86 character | ||
| hash. This is the strongest method. | ||
| hash based on the SSH-512 hash function. This is the strongest method. |
There was a problem hiding this comment.
Oh, I often do this mistake. :)
| The return value is a string suitable for passing as the *salt* argument | ||
| to :func:`crypt`. | ||
|
|
||
| *log_round* specifies the binary logarithm of the number of rounds |
There was a problem hiding this comment.
You should mention log_rounds is ignored otherwise.
There was a problem hiding this comment.
I'm going to rename this parameter to rounds, because SHA-* methods allow to specify the number of rounds.
| .. versionadded:: 3.3 | ||
|
|
||
| .. versionchanged:: 3.7 | ||
| Added the *log_round* parameter. |
| if not method.ident: | ||
| s = '' | ||
| elif method.ident[0] == '2': | ||
| s = f'${method.ident}${log_rounds:02d}$' |
There was a problem hiding this comment.
What if log_rounds is more than 99?
There was a problem hiding this comment.
log_rounds should be between 4 and 31. crypt() will return None if it is invalid or out of range. Should we silently bound log_rounds into the range 4 to 31, or raise an error, or keep all as is and let crypt() detect an error?
There was a problem hiding this comment.
I prefer to have a thin wrapper to the C crypt() function. Maybe add unit tests for invalid round values? Negative, 0, 999, etc.?
| _add_method('SHA512', '6', 16, 106) | ||
| _add_method('SHA256', '5', 16, 63) | ||
|
|
||
| for _v in 'b', 'y', 'a', '': |
There was a problem hiding this comment.
Can you please add a comment explaining this and the following line?
|
|
||
| A Modular Crypt Format method with 16 character salt and 86 character | ||
| hash. This is the strongest method. | ||
| hash based on the SHA-512 hash function. This is the strongest method. |
There was a problem hiding this comment.
I don't know what is stronger, SHA or Blowfish. But since SHA methods were added in glibc when the Blowfish method already was provided by the third-party library, I suppose that at least SHA-512 is not weaker (and likely is stronger) than Blowfish.
There was a problem hiding this comment.
You're right. Reference for the SHA2 methods is at https://www.akkadia.org/drepper/SHA-crypt.txt
| character hash. | ||
| character hash based on the SHA-256 hash function. | ||
|
|
||
| .. data:: METHOD_BLF |
There was a problem hiding this comment.
FreeBSD provides an API for getting a list of supported methods. Their names are: "md5", "blf", "nth", "sha256", "sha512" and "des".
There was a problem hiding this comment.
Do we care about the FreeBSD API? We don't expose it...
There was a problem hiding this comment.
I don't have any arguments for other names.
There was a problem hiding this comment.
Well, my point is that METHOD_BLOWFISH is obvious while METHOD_BLF is obscure (except perhaps for someone familiar with FreeBSD APIs :-)).
There was a problem hiding this comment.
Okay, I'll change the name. In any time we can add an alias if it is needed.
There was a problem hiding this comment.
I prefer METHOD_BLOWFISH. I agree with @pitrou, Python is not FreeBSD.
|
This is really stupid. Can we at least give a This is not an enhancement of Python, this is a security risk for its users. |
|
Ignoring your uncalled for rudeness... As a programming language we err on the side of being useful for practical purposes, not to pass judgement on if the data users need to process and/or systems they are writing software to interact with are legacy or not. At most, I suggest a PR noting APIs that should not be used in new system designs in the documentation (hyperlinking to supporting information for people to read). |
|
Hold on, how is a In what world is informing users that using deprecated algorithms leads to insecure code? |
|
Also, what problem is this solving by being in the standard library instead of letting the user use any of the well reviewed and well used libraries that implement these insecure algorithms? Putting this in the standard library will lead to users using them when they shouldn't. |
|
DeprecationWarning is only for things that are going to be removed. Also, warnings are seen by users of programs more often than by developers. Any time a warning is raised, it spams the wrong people. Use warnings sparingly, only after great thought. Also I strongly suspect you haven't bothered to read the diff of the commit for this issue. Look at the NEWS entry if nothing else. locking the conversation as I feel like you are being rude. |
|
Thanks @gpshead, I'm supporting your action. |
https://bugs.python.org/issue31664