Limit md5 import to allow for compatibility with fips environments#1288
Limit md5 import to allow for compatibility with fips environments#1288lratc wants to merge 3 commits intoapache:trunkfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Cassandra driver’s MD5-based token hashing to explicitly mark MD5 usage as non-security-related, improving compatibility with FIPS-140 environments where MD5 may be disallowed for security purposes.
Changes:
- Replace
from hashlib import md5withimport hashlib. - Update
MD5Token.hash_fnto callhashlib.md5(..., usedforsecurity=False).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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())) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@absurdfarce could move the import into Class MD5Token and only import if it's instantiated.
There was a problem hiding this comment.
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.
| from collections.abc import Mapping | ||
| from functools import total_ordering | ||
| from hashlib import md5 | ||
| import hashlib |
There was a problem hiding this comment.
would prefer we keep the narrow import scope on a security librarry.
There was a problem hiding this comment.
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).
| 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())) |
There was a problem hiding this comment.
@absurdfarce could move the import into Class MD5Token and only import if it's instantiated.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@lratc @absurdfarce what about this approach: I don't think catching the exception is necessarily helpful. Note, this suggest reworking import murmur3 along the same lines. |
In a FIPS-140 environment, the cassandra-python-driver may error as md5 is not compiled into the hashlib module.
ModuleNotFoundError: No module named 'md5'As md5 is not available, the MD5Token class may not be used but the overall module is still usable.