Use sha256 instead of md5. (#869) #870
Conversation
There was a problem hiding this comment.
@kresss would you like to take a look at doing the same for caching-transform? That module uses md5 to generate cache filenames so without updating you'll be forced to disable nyc cache. Not something that will hold up this review but probably worth while.
@bcoe regarding caching-transform if we're going to change the way filenames are generated I'd like to address caching-transform#4 in the same release. I'm thinking we can just use path.basename(metadata.filename) to generate a prefix when typeof metadata.filename === 'string'. I'm thinking that changing the way cache filenames are generated would be a breaking change, but maybe not?
|
@coreyfarrell I can take a look into it later tonight. Do you know if that project has code coverage and linting like nyc? As far as caching-transform#4 goes, I would probably ask from some help from someone with better domain knowledge. If I get into it tonight I will create an issue under caching-transform for moving from md5. Thank you all. |
|
Don't worry about caching-transform#4, I'll handle that. That module has linting and testing, the JavaScript code style is different but |
| const salt = JSON.stringify({ | ||
| istanbul: require('istanbul-lib-coverage/package.json').version, | ||
| nyc: CACHE_VERSION | ||
| }) | ||
|
|
||
| function Hash (code, filename) { | ||
| return md5hex([code, filename, salt]) + '_' + CACHE_VERSION |
There was a problem hiding this comment.
I've looked into this further, the Hash function is never actually run. The only reference to lib/hash.js is in index.js from NYC.prototype._createTransform. caching-transform does not use opts.hash so I think we should remove it. opts.salt is used so I think lib/hash.js should module.exports = {salt}, delete the Hash function completely.
There was a problem hiding this comment.
I see what happened, this is unused due to the update from caching-transform@1 to caching-transform@2. I'm working on a patch to address this part correctly, the Hash function isn't entirely unneeded, it needs to be replaced with other code.
| return md5hex( | ||
| process.hrtime().concat(process.pid).map(String) | ||
| ) | ||
| const hash = crypto.createHash('sha256') |
There was a problem hiding this comment.
This is valid and I don't have an issue with it, just have to ask what about using the uuid module instead? That might actually be more correct for this purpose but I'm not sure it's too important.
There was a problem hiding this comment.
@coreyfarrell @addaleax great point, in this context it seems like using uuid.v4() would be reasonable? this doesn't solve the issue with caching-transform performance, which I think is worth discussing further in that thread.
|
@kresss, perhaps you and @coreyfarrell can work together to see this over the finish line? sounds like there's a little bit of juggling required to retire the Hash function ... and I agree with @coreyfarrell that we should use a http://devtoolscommunity.herokuapp.com/ 👈 there's a slack where we can all chat. |
|
@coreyfarrell thanks for the update. Sorry for the silence. I went on holiday. The uuid solution sounds good. I will pull your branch and do some testing today on a FIPS system. If I have any issues I will report back on #883 Thank you. |
|
Since #883 we no longer directly use md5-hex from nyc, so I'm closing this PR as nothing needs to be done here. You should be able to use the next release of nyc with |
MD5 is not FIPS compliant and makes nyc inoperable on FIPS enabled systems.
The md5-hex library that was used for creating hashes uses the node crypto library unless it is run in a browser. I don't think that nyc has a browser use case so I removed the extra dependency of the md5-hex module and made calls directly to node's crypto library.
I arbitrarily picked sha256 as the new hashing algorithm. It is FIPS compliant and is expected to remain so for a while. sha1 could be used if performance is a large concern.