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

distutils.command.upload md5_digest #84875

Closed
tiran opened this issue May 20, 2020 · 8 comments
Closed

distutils.command.upload md5_digest #84875

tiran opened this issue May 20, 2020 · 8 comments
Labels
3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@tiran
Copy link
Member

tiran commented May 20, 2020

BPO 40698
Nosy @gpshead, @tiran, @merwok, @dstufft, @stratakis, @miss-islington
PRs
  • bpo-40698: Improve distutils upload hash digests #20260
  • [3.9] bpo-40698: Improve distutils upload hash digests (GH-20260) #20261
  • 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 2020-05-20.14:59:09.077>
    created_at = <Date 2020-05-20.11:32:43.388>
    labels = ['type-bug', 'library', '3.9', '3.10']
    title = 'distutils.command.upload md5_digest'
    updated_at = <Date 2020-05-20.14:59:09.076>
    user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2020-05-20.14:59:09.076>
    actor = 'christian.heimes'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-05-20.14:59:09.077>
    closer = 'christian.heimes'
    components = ['Library (Lib)']
    creation = <Date 2020-05-20.11:32:43.388>
    creator = 'christian.heimes'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 40698
    keywords = ['patch']
    message_count = 8.0
    messages = ['369442', '369446', '369447', '369448', '369452', '369457', '369458', '369459']
    nosy_count = 6.0
    nosy_names = ['gregory.p.smith', 'christian.heimes', 'eric.araujo', 'dstufft', 'cstratak', 'miss-islington']
    pr_nums = ['20260', '20261']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue40698'
    versions = ['Python 3.9', 'Python 3.10']

    @tiran
    Copy link
    Member Author

    tiran commented May 20, 2020

    The distutils upload command creates a MD5 digest of the file content. This is not compatible with systems with systems that run under a strict security policy that blocks MD5.

    Possible fixes are:

    • declare that the MD5 digest is not used for security. Security is provided by TLS/SSL and HTTPS. The digest is just a simple checksum to detect file corruption during upload.
    • Remove MD5 digest completely
    • Don't create a MD5 digest if hashlib.md5(content) fails
    • Skip the test case if MD5 is not available

    Does PyPI support other digests, e.g. SHA2-256 digest?

    @tiran tiran added 3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels May 20, 2020
    @tiran
    Copy link
    Member Author

    tiran commented May 20, 2020

    Charis pointed me to pypi/warehouse#681 / pypi/warehouse#891

    @dstufft
    Copy link
    Member

    dstufft commented May 20, 2020

    Does PyPI support other digests, e.g. SHA2-256 digest?

    There is a simple and a complicated answer to this.

    The simple answer is yes, PyPI supports uploads with any combination of MD5, SHA256, and blake2_256 (blake2b with a 256 digest, no personalization or key). It will also compute all 3 on an upload on it's own and verify that they match any provided hashes and to fill in any missing hashes.

    The more complicated answer is the upload API is an old API from long before we started documenting and standardizing them, so when you start talking about non PyPI implementations of that API, what they support is kind of a big who knows.

    More to the problem at hand:

    We don't rely on this hash for security (We couldn't, it comes in the exact same payload as the artifact itself from the exact same source, someone who can modify the artifact en route can modify the hash too). So the inclusion of MD5 is not a concern.

    Removing it *might* break non-PyPI servers that attempted to implement this API and assumed it was a mandatory field (though I do not have any a priori knowledge of this being the case).

    Adding additional hashes *might* break non-PyPI servers that assumed what distutils used to send was all it would ever send (this is unlikely though, most web tools ignore unknown form fields).

    I looked into what twine is doing here, and it appears it is sending md5, sha256, and blake2_256 hashes all along with every request. However if FIPS mode has disabled MD5 it just skips generating and sending MD5 (but still sends the other two) and it appears it's done this for 2+ years.

    It's probably safe to just mimc what twine is doing here, sending all 3 hashes, skip MD5 if it's unavailable.

    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented May 20, 2020

    There is also pypi/warehouse#888

    So I would assume it's safe it change the digest to sha256.

    @tiran
    Copy link
    Member Author

    tiran commented May 20, 2020

    Thanks for your elaborate explanation, Donald!

    I have implemented your proposal in PR 20260.

    @miss-islington
    Copy link
    Contributor

    New changeset e572c7f by Christian Heimes in branch 'master':
    bpo-40698: Improve distutils upload hash digests (GH-20260)
    e572c7f

    @miss-islington
    Copy link
    Contributor

    New changeset f541a37 by Miss Islington (bot) in branch '3.9':
    bpo-40698: Improve distutils upload hash digests (GH-20260)
    f541a37

    @tiran
    Copy link
    Member Author

    tiran commented May 20, 2020

    Thanks Charis and Donald!

    @tiran tiran closed this as completed May 20, 2020
    @tiran tiran closed this as completed May 20, 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.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants