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

refactor: add 'usedforsecurity=False' arg to hashlib.md5 usage #1190

Merged
merged 3 commits into from
Sep 15, 2022

Conversation

cquick01
Copy link
Contributor

This argument was introduced in Python 3.9, but using hashlib.new() prevents <3.9 versions from throwing an exception.

Tested running locally against 3.7, 3.9, and 3.10.

Closes #1187

@erezsh
Copy link
Member

erezsh commented Sep 15, 2022

I didn't realize we call md5 from 3 different places. Would you mind refactoring it into a single function, like load_grammar.md5_digest(), and call that instead?

@erezsh
Copy link
Member

erezsh commented Sep 15, 2022

Thanks! Also, mypy fails with

lark/load_grammar.py:1419: error: Unexpected keyword argument "usedforsecurity" for "new"  

Can you check if there's a way to fix this? If not, adding an "ignore" comment is good enough for me.

@erezsh erezsh requested a review from MegaIng September 15, 2022 19:24
@cquick01
Copy link
Contributor Author

Latest commit adds an explicit check for Python 3.9+ to pass the usedforsecurity argument. It's slightly messier now so if you'd prefer just changing to # type: ignore we can change back to that.

Latest commit:

if sys.version_info >= (3, 9):
    return hashlib.md5(s.encode('utf8'), usedforsecurity=False).hexdigest()
else:
    return hashlib.md5(s.encode('utf8')).hexdigest()

Or slightly cleaner version with less repeated code

hashlib.new("md5", s.encode("utf8"), usedforsecurity=False).hexdigest()  # type: ignore

@erezsh
Copy link
Member

erezsh commented Sep 15, 2022

Looks good to me. I'll wait a bit to see if @MegaIng has anything to add.

Copy link
Member

@MegaIng MegaIng left a comment

Choose a reason for hiding this comment

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

I don't really have anything to add. IMO a version check is cleaner than a type ignore.

@erezsh erezsh merged commit f775df3 into lark-parser:master Sep 15, 2022
@erezsh
Copy link
Member

erezsh commented Sep 15, 2022

@cquick01 Thank you for your contribution!

@jjtroberts
Copy link

Thank you all for working on this fix. When might the next release be cut?

@erezsh
Copy link
Member

erezsh commented Oct 5, 2022

It's probably about time for a new release! I'll try to do it early next week.

@erezsh
Copy link
Member

erezsh commented Oct 11, 2022

@jjtroberts Released 1.1.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add usedforsecurity=False argument to hashlib.md5 instances to support running on FIPS-enabled systems
4 participants