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

Alter md5 call to make utils.py module FIPS compatible #8974

Closed
relativistic opened this issue Dec 31, 2024 · 4 comments · Fixed by #8979
Closed

Alter md5 call to make utils.py module FIPS compatible #8974

relativistic opened this issue Dec 31, 2024 · 4 comments · Fixed by #8979
Labels
good first issue Clearly described and easy to accomplish. Good for beginners to the project. help wanted

Comments

@relativistic
Copy link
Contributor

The utils.py module uses hashlib.md5. This fails on a FIPS system, where using the md5 function raises a ValueError.

It does not appear that the md5 function is used for security purposes in utils.py. I think the fix would be simply set usedforsecurity=False to allow the md5 function to work on a FIPS system.

So the new function would be:

@toolz.memoize
def color_of(x, palette=palette):
    h = md5(str(x).encode(), usedforsecurity=False)
    n = int(h.hexdigest()[:8], 16)
    return palette[n % len(palette)]
@relativistic relativistic changed the title Remove use of md5 for FIPS compatible utils.py module Alter md5 call to make utils.py module FIPS compatible Dec 31, 2024
@jacobtomlinson
Copy link
Member

That seems reasonable to me. Do you have any interest in making a PR to make this change @relativistic?

@jacobtomlinson jacobtomlinson added help wanted good first issue Clearly described and easy to accomplish. Good for beginners to the project. and removed needs triage labels Jan 6, 2025
@relativistic
Copy link
Contributor Author

I've not submitted a PR to github before, but I can give it a try @jacobtomlinson.

I'm not clear from looking at the contributions page how to best test this before submitting the PR. Should I just make the change, submit the PR, and then let the github-actions system run unit tests for me?

@jacobtomlinson
Copy link
Member

It's a tricky one to test because we have no way of testing on a FIPS system. We could mock things out, but that seems like overkill. I would just suggest making a PR and letting the CI run the tests and provided nothing majorly breaks I would be fine merging this in.

@relativistic
Copy link
Contributor Author

Yeah, I ran the local tests, and not everything passed, but that was even before I made any changes. So I'm not sure which tests to verify!

I'll try doing the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Clearly described and easy to accomplish. Good for beginners to the project. help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants