Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

chore(deps): update #369

Merged
merged 2 commits into from
Mar 17, 2020
Merged

chore(deps): update #369

merged 2 commits into from
Mar 17, 2020

Conversation

alexander-akait
Copy link
Member

BREAKING CHANGE: use md4 by default for hashing

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Use md4 by default for perf reasons

Breaking Changes

Yes

Additional Info

No

alexander-akait and others added 2 commits March 17, 2020 14:49
BREAKING CHANGE: use `md4` by default for hashing
@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #369 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #369   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           29        29           
  Branches        13        12    -1     
=========================================
  Hits            29        29           
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1fe27c...c69953c. Read the comment docs.

@alexander-akait alexander-akait merged commit ad39022 into master Mar 17, 2020
@alexander-akait alexander-akait deleted the chore-deps-update branch March 17, 2020 12:04
@jens-duttke
Copy link

May I ask why the default has been switched to MD4? Referring Wikipedia, the risk of a collision is much higher than for MD5 hashes.

I assume it can't be the speed on modern computers.

@alexander-akait
Copy link
Member Author

@jens-duttke for perf reasons, webpack also uses md4 for hashing

risk of a collision

No, not in out cases

@mrlubos
Copy link

mrlubos commented Mar 20, 2020

@jens-duttke for perf reasons, webpack also uses md4 for hashing

risk of a collision

No, not in out cases

Can you explain why please?

@krysopath
Copy link

krysopath commented Mar 27, 2020

when you retire an outdated hash algo due security concerns, then this security concern would be also the best reason to switch to sha256 algo or better immeditaly. In contrast, I cant see the reason for md4 at all.

btw, md5 is deprecated by experts since Dobbertin, 1996

And wrt md4, quote wikipedia:

MD5 is one in a series of message digest algorithms designed by Professor Ronald Rivest of MIT (Rivest, 1992). When analytic work indicated that MD5's predecessor MD4 was likely to be insecure, Rivest designed MD5 in 1991 as a secure replacement. (Hans Dobbertin did indeed later find weaknesses in MD4.)

Will CRC-1 be the next perf enhancement for webpack?

@odensc
Copy link

odensc commented Mar 29, 2020

I'd assume what @evilebottnawi meant is that since the hash is used for caching purposes (not cryptographic purposes) it doesn't matter if there's a high collision risk, because the consequences in the event of a collision are close to nil.

However a non-cryptographic hash function such as xxHash would be much better suited to this use-case than an obsolete cryptographic hashing function, and would most likely be even faster.

It would be nice to get some insight into the reasoning behind this change from the maintainer themselves.

@alexander-akait
Copy link
Member Author

alexander-akait commented Mar 31, 2020

I'd assume what @evilebottnawi meant is that since the hash is used for caching purposes (not cryptographic purposes)

Yes

However a non-cryptographic hash function such as xxHash would be much better suited to this use-case than an obsolete cryptographic hashing function, and would most likely be even faster.

yes, but we can't use it because it is require library compilation and can incompatibility with some platforms

Why md4?

  • not cryptographic purposes
  • integration xxHash can be problematic
  • md4 and md5 have collisions (therefore there is no point in arguing)
  • md4 is faster than md5

So using md4 is best solution right now

@tomaszs
Copy link

tomaszs commented Oct 14, 2020

This change is marked as breaking change. How to adjust file-loader to comply with this change?

@digilist
Copy link

@tomaszs I do not know the intention of the library authors for sure, but I guess they chose to bump the major version, because the build result will be different and so all caches will be invalidated. So I think there is no change necessary and you can simply update (this is what I did).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants