Skip to content
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

Add hashlib.scrypt #72115

Closed
tiran opened this issue Sep 1, 2016 · 18 comments
Closed

Add hashlib.scrypt #72115

tiran opened this issue Sep 1, 2016 · 18 comments
Labels
type-feature A feature request or enhancement

Comments

@tiran
Copy link
Member

tiran commented Sep 1, 2016

BPO 27928
Nosy @gpshead, @tiran, @benjaminp, @alex, @zhangyangyu
Files
  • Add-hashlib.scrypt-4.patch
  • hashlib.scrypt.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-10-12.09:36:12.974>
    created_at = <Date 2016-09-01.12:29:19.758>
    labels = ['type-feature']
    title = 'Add hashlib.scrypt'
    updated_at = <Date 2021-10-12.09:36:12.973>
    user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2021-10-12.09:36:12.973>
    actor = 'christian.heimes'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-10-12.09:36:12.974>
    closer = 'christian.heimes'
    components = []
    creation = <Date 2016-09-01.12:29:19.758>
    creator = 'christian.heimes'
    dependencies = []
    files = ['44361', '44380']
    hgrepos = []
    issue_num = 27928
    keywords = ['patch']
    message_count = 18.0
    messages = ['274118', '274175', '274180', '274181', '274217', '274225', '274253', '274258', '274271', '274281', '274358', '274589', '274788', '274820', '274824', '274830', '274833', '274838']
    nosy_count = 6.0
    nosy_names = ['gregory.p.smith', 'christian.heimes', 'benjamin.peterson', 'alex', 'python-dev', 'xiang.zhang']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27928'
    versions = ['Python 2.7', 'Python 3.6']

    @tiran
    Copy link
    Member Author

    tiran commented Sep 1, 2016

    OpenSSL 1.1 has EVP_PBE_scrypt(). hashlib.scrypt() is a low-hanging fruit for Python 3.6. I have a working patch with some tests. I need to write more tests and documentation:

    https://github.com/tiran/cpython/commits/feature/openssl110_scrypt

    @tiran tiran added the type-feature A feature request or enhancement label Sep 1, 2016
    @gpshead
    Copy link
    Member

    gpshead commented Sep 1, 2016

    Rather than PyArg_ParseTupleAndKeywords can you have it use argument clinic?

    Also, how about making all arguments other than password be keyword only so
    that code calling the function is more clear. Otherwise it's a bit of
    positional argument soup with a lot of integers and potential to invert
    password and salt without realizing it.

    @tiran
    Copy link
    Member Author

    tiran commented Sep 1, 2016

    Argument is easy.

    Your second request is a very good idea but also harder to implement. Neither PyArg_Parse nor clinic have a way to declare arguments that required and keyword only but have no default value. I have a workaround but it ain't beautiful.

    @gpshead
    Copy link
    Member

    gpshead commented Sep 1, 2016

    If clinic doesn't support required keyword only args then don't worry about it for now. :)

    @tiran
    Copy link
    Member Author

    tiran commented Sep 2, 2016

    Here is a new patch with argument clinic, more tests and required keyword arguments.

    @zhangyangyu
    Copy link
    Member

    It looks that new patch when used like this hashlib.scrypt(b'password') will generate a "an integer is required" exception message which is misleading. I don't test it since I don't get openssl 1.1.

    And the phrase "interpreted as buffers of bytes" in the doc may better be "bytes-like objects".

    @tiran
    Copy link
    Member Author

    tiran commented Sep 2, 2016

    You are right. Let's try this again. How do you like:

    >>> hashlib.scrypt(b'', n=2, r=2, p=3)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: salt is required
    >>> hashlib.scrypt(b'', salt=b'')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: n is required and must be an unsigned int
    >>> hashlib.scrypt(b'', n=None, r=2, p=3)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: scrypt() argument 3 must be int, not None

    @zhangyangyu
    Copy link
    Member

    It looks good. But Christian, may I ask how do you generate the argument clinic? It looks from me that the declaration cannot give you such a format "y*|$y*O!O!O!ll:scrypt". I rerun clinic.py and the .c.h file is altered. Maybe it's better to abandon AC for right now?

    @tiran
    Copy link
    Member Author

    tiran commented Sep 2, 2016

    It's not a limitation of the argument clinic. PyArg_Parse*() does not support required, keyword-only arguments without a default value. I'm using None as default value, require PyLong_Type and added some extra checks.

    @alex
    Copy link
    Member

    alex commented Sep 2, 2016

    Bug in the error message "n must be a multiple of 2." it should say "n must be a power of 2."

    @tiran
    Copy link
    Member Author

    tiran commented Sep 4, 2016

    Thanks Alex, multiple is the wrong term. The argument 'n' must be 2^m for m > 1.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 6, 2016

    New changeset d926fa1a833c by Christian Heimes in branch 'default':
    Issue bpo-27928: Add scrypt (password-based key derivation function) to hashlib module (requires OpenSSL 1.1.0).
    https://hg.python.org/cpython/rev/d926fa1a833c

    @tiran
    Copy link
    Member Author

    tiran commented Sep 7, 2016

    Benjamin, what's your take on Alex's suggestion?

    <Crys> gutworth: Alex_Gaynor has asked me if hashlib.scrypt() can go into 2.7, too. It's a password-based KDF like hashlib.pbkdf2() but more secure than PBKDF2. It requires OpenSSL 1.1.0.
    <Alex_Gaynor> gutworth: I think it'd be good if this were approved, for the same reasons as PEP-466
    <Crys> contrary to PKBDF2 it doesn't make sense to have a pure-Python implementation. scrypt uses ChaCha20 cipher. I don't want to add a cipher to CPython core (possible legal issue) and it's not available in OpenSSL < 1.1.0.

    @gpshead
    Copy link
    Member

    gpshead commented Sep 7, 2016

    No, scrypt is a simple new feature. An extension module on PyPI is the appropriate place for that for 2.6 through 3.5. Wholly unrelated to PEP-466.

    @alex
    Copy link
    Member

    alex commented Sep 7, 2016

    PEP-466 includes hashlib.pbkdf2_hmac(). Any reasoning that includes that surely is applicable to scrypt as well.

    @benjaminp
    Copy link
    Contributor

    Why are we adding scrypt and not argon2 anyway?

    On Wed, Sep 7, 2016, at 03:25, Christian Heimes wrote:

    Christian Heimes added the comment:

    Benjamin, what's your take on Alex's suggestion?

    <Crys> gutworth: Alex_Gaynor has asked me if hashlib.scrypt() can go into
    2.7, too. It's a password-based KDF like hashlib.pbkdf2() but more secure
    than PBKDF2. It requires OpenSSL 1.1.0.
    <Alex_Gaynor> gutworth: I think it'd be good if this were approved, for
    the same reasons as PEP-466
    <Crys> contrary to PKBDF2 it doesn't make sense to have a pure-Python
    implementation. scrypt uses ChaCha20 cipher. I don't want to add a cipher
    to CPython core (possible legal issue) and it's not available in OpenSSL
    < 1.1.0.

    ----------
    nosy: +benjamin.peterson
    versions: +Python 2.7


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue27928\>


    @alex
    Copy link
    Member

    alex commented Sep 7, 2016

    OpenSSL supports scrypt

    On Sep 7, 2016 12:28 PM, "Benjamin Peterson" <report@bugs.python.org> wrote:

    Benjamin Peterson added the comment:

    Why are we adding scrypt and not argon2 anyway?

    On Wed, Sep 7, 2016, at 03:25, Christian Heimes wrote:
    >
    > Christian Heimes added the comment:
    >
    > Benjamin, what's your take on Alex's suggestion?
    >
    > <Crys> gutworth: Alex_Gaynor has asked me if hashlib.scrypt() can go into
    > 2.7, too. It's a password-based KDF like hashlib.pbkdf2() but more secure
    > than PBKDF2. It requires OpenSSL 1.1.0.
    > <Alex_Gaynor> gutworth: I think it'd be good if this were approved, for
    > the same reasons as PEP-466
    > <Crys> contrary to PKBDF2 it doesn't make sense to have a pure-Python
    > implementation. scrypt uses ChaCha20 cipher. I don't want to add a cipher
    > to CPython core (possible legal issue) and it's not available in OpenSSL
    > < 1.1.0.
    >
    > ----------
    > nosy: +benjamin.peterson
    > versions: +Python 2.7
    >
    > _______________________________________
    > Python tracker <report@bugs.python.org>
    > <http://bugs.python.org/issue27928\>
    > _______________________________________

    ----------


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue27928\>


    @benjaminp
    Copy link
    Contributor

    PEP-466 is explicitly not blanket approval for backporting All The
    Things to 2.7. The only justification for pbkdf2 in PEP-466 is to "lower
    the barriers to secure password storage and checking in Python 2 server
    applications". While scrypt is probably a bit better, applications using
    pkbdf2 are still in a much better situation than ones using, e.g., a
    naïve salted hash.

    There is a self-contained, easily-installable scrypt module on PyPI.

    @tiran tiran closed this as completed Oct 12, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants