HMAC default to MD5 marked as to be removed in 3.6

[Carreau]

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]

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]

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]

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]

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

[gpshead]

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

[rhettinger]

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.

[tiran]

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]

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]

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]

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

[miss-islington]

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

[LeandroLima]

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]

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.