classification
Title: HMAC default to MD5 marked as to be removed in 3.6
Type: behavior Stage: patch review
Components: Versions: Python 3.8, Python 3.7
process
Status: open Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Leandro Lima, christian.heimes, gregory.p.smith, mbussonn, miss-islington, rhettinger
Priority: normal Keywords: patch

Created on 2018-05-22 18:46 by mbussonn, last changed 2019-11-27 03:56 by Leandro Lima.

Pull Requests
URL Status Linked Edit
PR 7062 merged mbussonn, 2018-05-22 18:48
PR 7063 merged mbussonn, 2018-05-22 20:29
PR 7065 merged miss-islington, 2018-05-22 22:56
PR 16805 merged gregory.p.smith, 2019-10-15 16:38
PR 16833 merged miss-islington, 2019-10-18 03:31
Messages (12)
msg317322 - (view) Author: Matthias Bussonnier (mbussonn) * Date: 2018-05-22 18:46
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
msg317342 - (view) Author: Matthias Bussonnier (mbussonn) * Date: 2018-05-22 20:30
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.
msg317348 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-05-22 22:55
New changeset 8bb0b5b03cffa2a2e74f248ef479a9e7fbe95cf4 by Gregory P. Smith (Matthias Bussonnier) in branch 'master':
bpo-33604: Remove Pending from hmac Deprecation warning. (GH-7062)
https://github.com/python/cpython/commit/8bb0b5b03cffa2a2e74f248ef479a9e7fbe95cf4
msg317350 - (view) Author: miss-islington (miss-islington) Date: 2018-05-22 23:40
New changeset 2751dccca4098b799ea575bb35cec9959c74684a by Miss Islington (bot) in branch '3.7':
bpo-33604: Remove Pending from hmac Deprecation warning. (GH-7062)
https://github.com/python/cpython/commit/2751dccca4098b799ea575bb35cec9959c74684a
msg324942 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-09-10 18:10
New changeset 51a4743d19abd016f0772a57fb31df7af9220e18 by Gregory P. Smith (Matthias Bussonnier) in branch 'master':
bpo-33604: Remove deprecated HMAC default value marked for removal in 3.8 (GH-7063)
https://github.com/python/cpython/commit/51a4743d19abd016f0772a57fb31df7af9220e18
msg354734 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2019-10-15 15:20
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.
msg354742 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2019-10-15 16:37
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.
msg354743 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-10-15 16:43
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.
msg354744 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-10-15 16:52
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
msg354859 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-10-18 03:30
New changeset f33c57d5c780da1500619f548585792bb5b750ee by Gregory P. Smith in branch 'master':
bpo-33604: Raise TypeError on missing hmac arg. (GH-16805)
https://github.com/python/cpython/commit/f33c57d5c780da1500619f548585792bb5b750ee
msg354860 - (view) Author: miss-islington (miss-islington) Date: 2019-10-18 03:48
New changeset c615db608dbb36a5d10188f7d265dcec7bfcc3cf by Miss Islington (bot) in branch '3.8':
bpo-33604: Raise TypeError on missing hmac arg. (GH-16805)
https://github.com/python/cpython/commit/c615db608dbb36a5d10188f7d265dcec7bfcc3cf
msg357552 - (view) Author: Leandro Lima (Leandro Lima) Date: 2019-11-27 03:56
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
History
Date User Action Args
2019-11-27 03:56:04Leandro Limasetnosy: + Leandro Lima
messages: + msg357552
2019-10-18 03:48:46miss-islingtonsetmessages: + msg354860
2019-10-18 03:31:13miss-islingtonsetpull_requests: + pull_request16380
2019-10-18 03:30:49gregory.p.smithsetmessages: + msg354859
2019-10-15 16:52:23gregory.p.smithsetmessages: + msg354744
2019-10-15 16:45:14gregory.p.smithsetassignee: gregory.p.smith
2019-10-15 16:43:50gregory.p.smithsetmessages: + msg354743
2019-10-15 16:38:55gregory.p.smithsetstage: commit review -> patch review
pull_requests: + pull_request16362
2019-10-15 16:37:24christian.heimessetmessages: + msg354742
2019-10-15 15:20:04rhettingersetstatus: closed -> open
nosy: + rhettinger
messages: + msg354734

2018-09-10 18:12:24gregory.p.smithsetstatus: open -> closed
type: behavior
stage: patch review -> commit review
resolution: fixed
versions: - Python 3.6
2018-09-10 18:10:04gregory.p.smithsetmessages: + msg324942
2018-05-22 23:40:54miss-islingtonsetnosy: + miss-islington
messages: + msg317350
2018-05-22 22:56:44miss-islingtonsetpull_requests: + pull_request6695
2018-05-22 22:55:41gregory.p.smithsetmessages: + msg317348
2018-05-22 20:38:27serhiy.storchakasetnosy: + gregory.p.smith, christian.heimes
2018-05-22 20:30:51mbussonnsetmessages: + msg317342
2018-05-22 20:29:18mbussonnsetpull_requests: + pull_request6693
2018-05-22 18:48:50mbussonnsetkeywords: + patch
stage: patch review
pull_requests: + pull_request6692
2018-05-22 18:46:00mbussonncreate