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

perf: use sha1 instead of sha256 for hashing #13421

Merged
merged 3 commits into from
Oct 15, 2022

Conversation

mitchhentgesspotify
Copy link
Contributor

@mitchhentgesspotify mitchhentgesspotify commented Oct 10, 2022

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.

@mitchhentgesspotify
Copy link
Contributor Author

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.
It wasn't significantly impactful on performance for me, but it seemed like in general it should be a better fit.

@SimenB
Copy link
Member

SimenB commented Oct 10, 2022

From jshttp/etag#17 (comment), is sha1 gonna be denied soon?

@mitchhentgesspotify
Copy link
Contributor Author

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?

@SimenB
Copy link
Member

SimenB commented Oct 11, 2022

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)

@SimenB
Copy link
Member

SimenB commented Oct 11, 2022

CI is failing, tho 🙂

@mitchhentgesspotify mitchhentgesspotify force-pushed the sha1-instead-of-sha256 branch 4 times, most recently from 3be5c47 to 6f68fb1 Compare October 14, 2022 15:44
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>
Signed-off-by: Mitchell Hentges <mhentges@spotify.com>
@mitchhentgesspotify
Copy link
Contributor Author

The only failure here seems to be the same (packages/jest-diff/src/__tests__/joinAlignedDiffs.test.ts) as this landed PR, so I think I'm green.

@mitchhentgesspotify mitchhentgesspotify marked this pull request as ready for review October 14, 2022 17:32
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

👍

@SimenB SimenB changed the title Use sha1 instead of sha256 for hashing perf: use sha1 instead of sha256 for hashing Oct 15, 2022
@SimenB SimenB merged commit 1f6c4e9 into jestjs:main Oct 15, 2022
@mitchhentgesspotify mitchhentgesspotify deleted the sha1-instead-of-sha256 branch October 16, 2022 11:59
@SimenB
Copy link
Member

SimenB commented Oct 18, 2022

tujoworker added a commit to dnbexperience/eufemia that referenced this pull request Oct 21, 2022
- Here is the [PR](jestjs/jest#13421)
- Update snapshots regarding the [snapshotFormat change](https://jestjs.io/blog/2022/08/25/jest-29)
tujoworker added a commit to dnbexperience/eufemia that referenced this pull request Oct 21, 2022
* 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
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants