absurdfarce commented on code in PR #1288:
URL:
https://github.com/apache/cassandra-python-driver/pull/1288#discussion_r3171484136
##########
cassandra/metadata.py:
##########
@@ -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()))
Review Comment:
I was a bit concerned about this as well. The [CPython
issue](https://bugs.python.org/issue9216) in question (as well as the [Python
docs for the minimum support Python
runtime](https://docs.python.org/3.10/library/hashlib.html#hash-algorithms))
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
