-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
perf: use sha1 instead of sha256 for hashing #13421
perf: use sha1 instead of sha256 for hashing #13421
Conversation
I'm not very attached or convinced by this PR, but I don't also see how it can necessarily be harmful? I trust your judgement on the merge-ability of this. |
a496155
to
9007c31
Compare
From jshttp/etag#17 (comment), is sha1 gonna be denied soon? |
Hmm, that's fair, though I'm not aware of the approximate time frame before FIPS indeed excludes sha1. Of course, for our purposes, we aren't interested in the security properties of our hashing algorithm - though FIPS environments don't know that. Perhaps the alternate move is to expose this as a configuration option? Allow choosing between md5 and sha256? |
I'm fine with landing this. If it breaks FIPS, then we'll change in the future. Up until we can actually run tests in FIPS systems, regressions are bound to sneak in. But in this case it should be supported just fine (in som potential future it'll be unsupported, but no reason to deal with that now) |
CI is failing, tho 🙂 |
3be5c47
to
6f68fb1
Compare
This should be more performant while still being FIPS compliant (see jestjs#12722). sha1 isn't as secure as sha256, but since the usage context is just "has this file changed? 🤔", this should be an acceptable degredation. pu Signed-off-by: Mitchell Hentges <mhentges@spotify.com>
6f68fb1
to
c4d07a9
Compare
Signed-off-by: Mitchell Hentges <mhentges@spotify.com>
The only failure here seems to be the same ( |
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.
👍
- Here is the [PR](jestjs/jest#13421) - Update snapshots regarding the [snapshotFormat change](https://jestjs.io/blog/2022/08/25/jest-29)
* chore(Jest): upgrade jest form v28 to v29 for faster executions - Here is the [PR](jestjs/jest#13421) - Update snapshots regarding the [snapshotFormat change](https://jestjs.io/blog/2022/08/25/jest-29) * Update snapshots
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This should be more performant while still being FIPS compliant (see #12722).
sha1 isn't as secure as sha256, but since the usage context is just "has this file changed? 🤔", this should be an acceptable degredation.