Skip to content

Limit md5 import to allow for compatibility with fips environments #1288

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.

Sign up for GitHub

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

Closed
lratc wants to merge 3 commits into from
+10 −1
Closed

Limit md5 import to allow for compatibility with fips environments #1288

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ddd1621
Mark non-security md5 usage to allow for compatibility with fips
lratc Apr 23, 2026
002bf5c
Style fix in cassandra/metadata.py
lratc May 1, 2026
d77aae4
Only use md5 if it is available
lratc May 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cassandra/metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from collections import defaultdict
from collections.abc import Mapping
from functools import total_ordering
from hashlib import md5
import hashlib
Copy link
Copy Markdown
Contributor

@bschoening bschoening May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer we keep the narrow import scope on a security librarry.

Copy link
Copy Markdown
Contributor Author

@lratc lratc May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I have changed the approach to use a similar pattern to the one used for murmur3 (I inverted the if logic to avoid nesting the if statements).

import json
import logging
import re
Expand Down Expand Up @@ -1864,7 +1864,7 @@ class MD5Token(HashToken):
def hash_fn(cls, key):
if isinstance(key, str):
key = key.encode('UTF-8')
return abs(varint_unpack(md5(key).digest()))
return abs(varint_unpack(hashlib.md5(key,usedforsecurity=False).digest()))
Comment thread
lratc marked this conversation as resolved.
Outdated
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hashlib.md5(..., usedforsecurity=False) is not supported by all Python implementations (e.g., PyPy may raise TypeError: md5() takes no keyword arguments), which would break token hashing at runtime. Consider feature-detecting support once (e.g., try calling with usedforsecurity=False and fall back to a call without the kwarg) so the driver remains compatible while still fixing FIPS on CPython/OpenSSL builds that honor the flag.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@absurdfarce absurdfarce May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit concerned about this as well. The CPython issue in question (as well as the Python docs for the minimum support Python runtime) suggest that support for the usedforsecurity arg went in with Python 3.9. Assuming that's true we should be okay with all officially supported versions (since we only go back to 3.10)... but we would be introducing a breaking change for earlier Python versions. That's not critical since we don't claim support... but we also generally are pretty cautious about introducing changes known to break earlier versions.

Something like executing this functionality in a try block (and re-executing if we get the expected exception) might address this but... honestly, I'm a bit more worried about something else.

Those same 3.10 docs include the following:

md5() is normally available as well, though it may be missing or blocked if you are using a rare “FIPS compliant” build of Python.

That... isn't great. This potentially introduces a whole new class of errors; now we're not talking about the args md5() supports... instead we're talking about whether md5() exists at all.

I did some random Google searching to try to identify cases in which this might occur, especially since all of this functionality comes from OpenSSL (or an equivalent) in modern Python. Near as I can tell it requires some kind of custom build aimed at limiting exposed hash algorithms in order to meet the FIPS standard but nothing I found was super-clear on that point.

But since the whole point of this PR is better support for FIPS-compliant environments I'm starting to wonder if we shouldn't also handle the case in which md5() isn't present.

@bschoening any thoughts?

Copy link
Copy Markdown
Contributor

@bschoening bschoening May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@absurdfarce could move the import into Class MD5Token and only import if it's instantiated.

Copy link
Copy Markdown
Contributor Author

@lratc lratc May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You raise some very good points - on further inspection, the usedforsecurity=False isn't resolving the issue I was experiencing - not trying to import md5 at that point is what has resolved the issue. Moving the import into the class would be a much better solution.



class BytesToken(Token):
Expand Down