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

Fix to hashing compatability issues [2697] with some environments (FIPS) #2817

Merged
merged 6 commits into from
Apr 16, 2024

Conversation

caplinje-NOAA
Copy link
Contributor

@caplinje-NOAA caplinje-NOAA commented Mar 26, 2024

This PR resolves 2697, and is mostly a find and replace of hashlib.md5 and hashlib.sha1 with hashlib.sha256. I did not replace one instance of sha1 in testing. sha256 is slightly slower than md5, but testing did not show a measurable change in performance for creating callback ids. Further, though they are generally extremely unlikely, sha256 has a reduced risk of collisions.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • swapped hashing algorithms md5 and sha1 to sha256
  • I have run the tests locally and one test did indeed fail (test_inin027_multi_page_without_pages_folder). Apparently this is not an issue as the tests ran fine for others locally and in the CI.
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR
    • tested performance of create_callback_id for md5, sha256, blake2b, and sha512. There was no measurable impact to performance, even when creating ids for thousands of components, despite md5 being known to be somewhat faster. This means hashing ids is a negligible portion of the total runtime associated with creating callbacks.
    • tested that switching to sha256 solves the compatibility issue in my deployment environment, by simply overloading the method in the current dash release.
    • Both additional tests exist here: dash_md5_patch_testing

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@emilykl
Copy link
Contributor

emilykl commented Mar 26, 2024

All looks good to me. Thanks for your work on this, @caplinje-NOAA .

The failing test you mentioned is working in the CI, and also on my local machine, so I don't see an issue there.

@emilykl emilykl marked this pull request as ready for review March 29, 2024 21:00
Co-authored-by: Philippe Duval <t4rk@outlook.com>
Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

💃

Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

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

LGTM

@T4rk1n T4rk1n merged commit d098e76 into plotly:dev Apr 16, 2024
3 checks passed
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.

[BUG] implementation of hashlib in _utils.py fails in FIPS environment
3 participants