-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-129327: revise hashlib documentation to account for FIPS removing sha1 #129334
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 changes: 3 additions & 0 deletions
3
Misc/NEWS.d/next/Documentation/2025-01-27-01-21-55.gh-issue-129327.sv2NB1.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Clarify that hashlib's SHA1 is no longer a FIPS secure algorithm. Clarify that | ||
| hashlib has a mixture of cryptographically secure and non cryptographically | ||
| secure hash algorithms. Patch by Eli Schwartz. | ||
|
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doc changes don't need NEWS entries |
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, "non-cryptographically-secure" may be too verbose. Let just say "as well as SHA1 (formerly...) and RSA's MD5 (..))". If we want to be precise we should rather add a note concerning the weaknesses of sha1/md5 as cryptographically secure hash functions but I don't know if we wouldn't reinvent the wheel instead in this PR.
WDYT @gpshead? do you think it's fine not to talk too much about security/applications in the first paragraph of hashlib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just go with "legacy".
"""... as well as the legacy algorithms SHA1 (formerly part of ...) and the MD5 algorithm (defined in ...)"""
perhaps. This elides attributing MD5 to RSA as well as it isn't important where MD5 came from, that's what the linked-to RFC is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that but I would have used the term "legacy" when talking about something that is only kept for backwards compatibility, and not something that I expect to see in production. Maybe it's more a language issue on my side though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm however happy if we can remove the mention to RSA though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
legacy means a lot of things. It is simpler and more pedantically accurate than saying not-cryptographically-secure.
I'd edit this PR myself with those words but it appears the author unchecked the allow-edits checkbox on it (Github isn't showing me an edit option)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eli-schwartz do you plan to update this?