-
Notifications
You must be signed in to change notification settings - Fork 75
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
Use sha1, instead of md5 #191
Conversation
This allows prebuild-install to be used on fips systems
Just ran into the same issue this week - your fix looks good to me! |
I have had the same issue when working with sharp-js library. Is there a chnace that this PR would be approved? cc: @vweevers |
@lovell Any chance we could get this merged? I'm also running into the same issue. FIPS-compliant systems ban the use of MD5 altogether so any libraries which have |
For those using |
@@ -74,7 +74,7 @@ function trimSlashes (str) { | |||
} | |||
|
|||
function cachedPrebuild (url) { | |||
const digest = crypto.createHash('md5').update(url).digest('hex').slice(0, 6) | |||
const digest = crypto.createHash('sha512').update(url).digest('hex').slice(0, 6) |
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.
If the reason for not using an MD5 hash under the rules of FIPS is its predictability and therefore increased chance of collision then the subsequent use of slice(0, 6)
negates hash choice anyway. 😅
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.
Well, it doesn't much matter in the end. This code will not be running as part of the system, only during build.
At the same time, node unfortunately, does not have an equivalent of usedforsecurity
parameter from python's hashlib
I originally tagged this as |
Ready to merge? :) |
7.1.2 now available |
This allows prebuild-install to be used on fips systems