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: deprecate default hash #61478

Closed
tiran opened this issue Feb 22, 2013 · 19 comments
Closed

HMAC: deprecate default hash #61478

tiran opened this issue Feb 22, 2013 · 19 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@tiran
Copy link
Member

tiran commented Feb 22, 2013

BPO 17276
Nosy @akuchling, @gpshead, @jcea, @pitrou, @vstinner, @tiran, @bitdancer
Files
  • 17276.patch
  • 17276-2.patch
  • 17276-3.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 = 'https://github.com/tiran'
    closed_at = <Date 2013-11-20.16:23:46.549>
    created_at = <Date 2013-02-22.12:16:42.941>
    labels = ['type-bug', 'library']
    title = 'HMAC: deprecate default hash'
    updated_at = <Date 2014-03-09.19:17:42.572>
    user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2014-03-09.19:17:42.572>
    actor = 'python-dev'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2013-11-20.16:23:46.549>
    closer = 'christian.heimes'
    components = ['Library (Lib)']
    creation = <Date 2013-02-22.12:16:42.941>
    creator = 'christian.heimes'
    dependencies = []
    files = ['31359', '31375', '32299']
    hgrepos = []
    issue_num = 17276
    keywords = ['patch']
    message_count = 19.0
    messages = ['182662', '182663', '182666', '182668', '182669', '195569', '195645', '196662', '200729', '200801', '200931', '200934', '200937', '200939', '203165', '203228', '203498', '212950', '212974']
    nosy_count = 8.0
    nosy_names = ['akuchling', 'gregory.p.smith', 'jcea', 'pitrou', 'vstinner', 'christian.heimes', 'r.david.murray', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue17276'
    versions = ['Python 3.4']

    @tiran
    Copy link
    Member Author

    tiran commented Feb 22, 2013

    As of now the hash algorithm for HMAC defaults to MD5. However MD5 is considered broken. HMAC-MD5 is still ok but shall not be used in new code. Applications should slowly migrate away from HMAC-MD5 and use a more modern algorithm like HMAC-SHA256.

    Therefore I propose that default digestmod should be deprecated in Python 3.4 and removed in 3.5. Starting with Python 3.5 developer are forced to choose a hash algorithm like SHA256. Our documentation shall suggest it, too.

    In addition I would like to enhance the meaning of the digestmod argument a bit. Right now it either must be a module or a callable. It should also support a name, e.g. hmac.new("secret", digestmod="sha256")

    @tiran tiran added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 22, 2013
    @jcea
    Copy link
    Member

    jcea commented Feb 22, 2013

    +1.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 22, 2013

    I don't know how you intend to make digestmod mandatory given the current function signature.

    Applications should slowly migrate away from HMAC-MD5 and use a more
    modern algorithm like HMAC-SHA256.

    Applications don't always choose their cipher. MD5 is needed for compatibility with existing protocols such as CRAM-MD5.

    @tiran
    Copy link
    Member Author

    tiran commented Feb 22, 2013

    I don't know how you intend to make digestmod mandatory given the current function signature.

    That's easy:

    if digestmod is None:
        raise TypeError("HMAC needs argument 'digestmod'")

    @tiran
    Copy link
    Member Author

    tiran commented Feb 22, 2013

    PS: I don't want to deprecate HMAC-MD5. I just want to deprecate that HMAC defaults to HMAC-MD5.

    @tiran
    Copy link
    Member Author

    tiran commented Aug 18, 2013

    Here is a patch that deprecates MD5 has implicit default hashing algorithm. It also implements digestmod string support.

    PEP-247 doesn't define the digestmod argument of keyed hashing algorithms. I'm going to define it in PEP-452.

    @tiran
    Copy link
    Member Author

    tiran commented Aug 19, 2013

    assertWarns() is much easier than the block I have copied and pasted. Thanks. :)

    @gpshead
    Copy link
    Member

    gpshead commented Aug 31, 2013

    comments added to the review.

    I don't think a DeprecationWarning should be raised as that'll infuriate users of python programs more than developers who can fix code.

    @tiran
    Copy link
    Member Author

    tiran commented Oct 21, 2013

    GPS, what do you suggest instead? Do you want me to remove the deprecation warning?

    @gpshead
    Copy link
    Member

    gpshead commented Oct 21, 2013

    yes just remove the DeprecationWarning. Document it as deprecated with a release now+0.2 as the earliest it will be removed. (if you want a warning at all, use PendingDeprecationWarning as that one is filtered out by default so it won't bother users of tools written in Python but only developers actively looking for issues)

    @tiran
    Copy link
    Member Author

    tiran commented Oct 22, 2013

    I've changed the deprecation warning to PendingDeprecationWarning. Please review my wording and grammar.

    @vstinner
    Copy link
    Member

    I would prefer to directly raise an exception in Python 3.4. Developers will not notice a warning, warning are hidden by default. How many developers run their tests using -Werror?

    Having to add a parameter to hmac() in applications to port them to Python 3.4 should not be so hard. And using MD5 is really a major security issue, don't you think so?

    @pitrou
    Copy link
    Member

    pitrou commented Oct 22, 2013

    Having to add a parameter to hmac() in applications to port them to
    Python 3.4 should not be so hard. And using MD5 is really a major
    security issue, don't you think so?

    Some uses of md5 don't have anything to do with security. I'm -1
    on removing the default value here.

    @tiran
    Copy link
    Member Author

    tiran commented Oct 22, 2013

    HMAC-MD5 is still fine for legacy support. I wouldn't use it in new program, though

    @tiran
    Copy link
    Member Author

    tiran commented Nov 17, 2013

    I'll commit the patch later.

    @tiran tiran self-assigned this Nov 17, 2013
    @vstinner
    Copy link
    Member

    Well, if deprecating is not an option, it's probably better to add a red warning explaining why the default choice may not fit all use cases.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 20, 2013

    New changeset 86107e7e6ee5 by Christian Heimes in branch 'default':
    Issue bpo-17276: MD5 as default digestmod for HMAC is deprecated. The HMAC
    http://hg.python.org/cpython/rev/86107e7e6ee5

    @tiran tiran closed this as completed Nov 20, 2013
    @bitdancer
    Copy link
    Member

    I don't understand why PendingDeprecationWarning was used here. DeprecationWarnings are silent by default. I'm also not clear on why this is being delayed until 3.6, instead of being changed in 3.5 after a deprecation, given that the default is considered to be a bit of a security issue.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 9, 2014

    New changeset c10ec51a2ce4 by R David Murray in branch 'default':
    whatsnew: hmac *digestmod* accepts strings, and default is deprecated. (bpo-17276)
    http://hg.python.org/cpython/rev/c10ec51a2ce4

    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants