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

HMAC default to MD5 marked as to be removed in 3.6 #77785

Closed
Carreau mannequin opened this issue May 22, 2018 · 13 comments
Closed

HMAC default to MD5 marked as to be removed in 3.6 #77785

Carreau mannequin opened this issue May 22, 2018 · 13 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes type-bug An unexpected behavior, bug, or error

Comments

@Carreau
Copy link
Mannequin

Carreau mannequin commented May 22, 2018

BPO 33604
Nosy @rhettinger, @gpshead, @tiran, @Carreau, @miss-islington
PRs
  • bpo-33604: Bump removal notice from 3.6 to 3.8 #7062
  • bpo-33604: Remove deprecated HMAC default value marked for removal in 3.8 #7063
  • [3.7] bpo-33604: Remove Pending from hmac Deprecation warning. (GH-7062) #7065
  • bpo-33604: Raise TypeError on missing hmac arg. #16805
  • [3.8] bpo-33604: Raise TypeError on missing hmac arg. (GH-16805) #16833
  • 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 = 'https://github.com/gpshead'
    closed_at = <Date 2020-02-17.06:44:14.108>
    created_at = <Date 2018-05-22.18:46:00.080>
    labels = ['3.8', 'type-bug', '3.7']
    title = 'HMAC default to MD5 marked as to be removed in 3.6'
    updated_at = <Date 2020-02-17.06:44:14.107>
    user = 'https://github.com/Carreau'

    bugs.python.org fields:

    activity = <Date 2020-02-17.06:44:14.107>
    actor = 'gregory.p.smith'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2020-02-17.06:44:14.108>
    closer = 'gregory.p.smith'
    components = []
    creation = <Date 2018-05-22.18:46:00.080>
    creator = 'mbussonn'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33604
    keywords = ['patch']
    message_count = 13.0
    messages = ['317322', '317342', '317348', '317350', '324942', '354734', '354742', '354743', '354744', '354859', '354860', '357552', '362120']
    nosy_count = 6.0
    nosy_names = ['rhettinger', 'gregory.p.smith', 'christian.heimes', 'mbussonn', 'miss-islington', 'Leandro Lima']
    pr_nums = ['7062', '7063', '7065', '16805', '16833']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue33604'
    versions = ['Python 3.7', 'Python 3.8']

    @Carreau
    Copy link
    Mannequin Author

    Carreau mannequin commented May 22, 2018

    HMAC docs says digestmod=md5 default will be removed in 3.6, but was not.

    We should likely bumpt that to 3.8 in the documentation, and actually remove it in 3.8

    @Carreau Carreau mannequin added 3.7 (EOL) end of life 3.8 only security fixes labels May 22, 2018
    @Carreau
    Copy link
    Mannequin Author

    Carreau mannequin commented May 22, 2018

    I've sent two PRs, one that updates the deprecation from PendingDeprecationWarning to DeprecationWarning that likely should get applied to 3.6, and 3.7 ?

    And a PR to actually remove the default in 3.8.

    @gpshead
    Copy link
    Member

    gpshead commented May 22, 2018

    New changeset 8bb0b5b by Gregory P. Smith (Matthias Bussonnier) in branch 'master':
    bpo-33604: Remove Pending from hmac Deprecation warning. (GH-7062)
    8bb0b5b

    @miss-islington
    Copy link
    Contributor

    New changeset 2751dcc by Miss Islington (bot) in branch '3.7':
    bpo-33604: Remove Pending from hmac Deprecation warning. (GH-7062)
    2751dcc

    @gpshead
    Copy link
    Member

    gpshead commented Sep 10, 2018

    New changeset 51a4743 by Gregory P. Smith (Matthias Bussonnier) in branch 'master':
    bpo-33604: Remove deprecated HMAC default value marked for removal in 3.8 (GH-7063)
    51a4743

    @gpshead gpshead closed this as completed Sep 10, 2018
    @gpshead gpshead added the type-bug An unexpected behavior, bug, or error label Sep 10, 2018
    @rhettinger
    Copy link
    Contributor

    The docs still make it look like *digestmod* is an optional argument:
    https://docs.python.org/3/library/hmac.html#hmac.new

    The help output does as well:

        >>> help(hmac.new)
        Help on function new in module hmac:
        new(key, msg=None, digestmod=None)
            Create a new hashing object and return it.
            
            key: The starting key for the hash.
            msg: if available, will immediately be hashed into the object's starting
            state.
            
            You can now feed arbitrary strings into the object using its update()
            method, and can ask for the hash value at any time by calling its digest()
            method.

    Also, it is well outside the Python norms to have a required argument default to None and having that default value be invalid.

    Presumably, the type annotation for this would be, "digestmod: Optional[str]=None". That would further add to the confusion with a required Optional argument.

    Another thought: The usual exception for a missing argument is a TypeError, not a ValueError

    Lastly, I'm curious why another algorithm wasn't used (perhaps sha256) as a default rather than removing the default altogether. This doesn't seems like good API design.

    FWIW, this removal broke the third-party package, Bottle:

    Bottle v0.12.17 server starting up (using WSGIRefServer())...
    Listening on http://localhost:8081/
    Hit Ctrl-C to quit.
    
        127.0.0.1 - - [15/Oct/2019 07:53:10] "GET / HTTP/1.1" 200 1471
        Traceback (most recent call last):
          File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/bottle.py", line 862, in _handle
            return route.call(**args)
          File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/bottle.py", line 1742, in wrapper
            rv = callback(*a, **ka)
          File "webapp.py", line 32, in check_credentials
            response.set_cookie('token', token, max_age=3600, secret=secret)
          File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/bottle.py", line 1626, in set_cookie
            value = touni(cookie_encode((name, value), secret))
          File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/bottle.py", line 2600, in cookie_encode
            sig = base64.b64encode(hmac.new(tob(key), msg).digest())
          File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/hmac.py", line 146, in new
            return HMAC(key, msg, digestmod)
          File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/hmac.py", line 49, in __init__
            raise ValueError('`digestmod` is required.')
        ValueError: `digestmod` is required.

    @rhettinger rhettinger reopened this Oct 15, 2019
    @tiran
    Copy link
    Member

    tiran commented Oct 15, 2019

    The weird argument style of a required digestmod with None as default is an unfortunate outcome of the old API. The msg and digestmod argument can be passed in as keyword and as positional argument. I studied existing code and have considered to make digestmod a required keyword-only argument, but that would have broken too much code. The current style is backwards compatible with all code except for code that must be changed any way.

    Only code that depends on implicit default digestmod="md5" breaks. The code must adjusted for the deprecation no matter the argument style. The required change is fully backwards compatible with Python 2.7 to 3.7. Bottle is such a case that got broken by the deprecation.

    It does not make sense to default to another hashing algorithm:

    • This would also break software. Applications would suddenly get a different MAC for the same function call and arguments.
    • In cryptography the HMAC algorithm is an operation on a key, message, and PRF. Defaulting to MD5 didn't make sense in the first place.
    • Cryptographic primitives have a 'best before' date. SHA256 might become broken in a decade -- maybe 9 years and 364 days earlier, maybe 20 years later. I don't want to do another deprecation cycle.

    @gpshead
    Copy link
    Member

    gpshead commented Oct 15, 2019

    Thanks for the feedback. Better late than never. :)

    A default algorithm is a bad thing when it comes to authentication. Explicit is better than implicit. A default regularly becomes obsolete as math and cryptanalysis methods move forward and need to be changed every unpredictable N years. MD5 was _already_ a bad choice of default when hmac was added in 2.2.

    That said, we managed this deprecation and API evolution poorly.

    As it has shipped this way in 3.8, I'm first going to fix the documentation and the exception type (both suitable for 3.8). First PR sent.

    In 3.9 we could introduce a better named keyword only digest parameter, leaving digestmod supported as a legacy positional & alternate name for backwards incompatibility. (minor code gymnastics required to do that, but within reason)

    i wouldn't want to remove the digestmod positional/name support until after 3.8 is no longer relevant in the world.

    @gpshead gpshead self-assigned this Oct 15, 2019
    @gpshead
    Copy link
    Member

    gpshead commented Oct 15, 2019

    BTW, if you want the type annotation that'd be used for this, 3.8 effectively removes the Optional[] from the one listed in:

    https://github.com/python/typeshed/blob/master/stdlib/2and3/hmac.pyi#L16

    @gpshead
    Copy link
    Member

    gpshead commented Oct 18, 2019

    New changeset f33c57d by Gregory P. Smith in branch 'master':
    bpo-33604: Raise TypeError on missing hmac arg. (GH-16805)
    f33c57d

    @miss-islington
    Copy link
    Contributor

    New changeset c615db6 by Miss Islington (bot) in branch '3.8':
    bpo-33604: Raise TypeError on missing hmac arg. (GH-16805)
    c615db6

    @LeandroLima
    Copy link
    Mannequin

    LeandroLima mannequin commented Nov 27, 2019

    IMV, the adopted solution creates a problem of a mismatch between the signature and the function behavior.

    I was just hit by this, as I never cared to specify digestmod trusting that Python defaults are usually sound choices.

    I agree that changing the signature would break more code, but the choice made here violates the principle of least astonishment. An error that could be caught by a static analysis tool silently entered the code base to be provoked only when the function got called.

    The choice to break compatibility was already made. Introducing silent bugs is something that I think we should avoid.

    I've wrote an argument about this in a different message, and I'm not sure if I should repeat it here. Here's the link to it:
    https://bugs.python.org/msg357551

    @gpshead
    Copy link
    Member

    gpshead commented Feb 17, 2020

    Noted, but making it a keyword only argument would break a lot of existing code that has always just been passing three positional args. Needless pain.

    @gpshead gpshead closed this as completed Feb 17, 2020
    @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
    3.7 (EOL) end of life 3.8 only security fixes type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants