-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(backend-shared, server-core, query-orchestrator): replace crypto md5 hashing with xxhash #10314
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
base: master
Are you sure you want to change the base?
Conversation
|
@ovr tagging as you requested the shared |
a0edab9 to
2b35b72
Compare
|
@KSDaemon curious why did you close the original PR? |
|
@h0tw1r3 Hey! Well... I stepped out of the Cube Core team, and I don't think anyone would take this initiative to finish... So all hope rests on you! :) |
It's ready to go! I don't want to be a bother, but I have no idea how to get anyone's attention for a review. Any help would be greatly appreciated. Very interested in using Cube, but the customers I work with have strict security guidelines (FIPS). If node can be run in FIPS mode, it opens up a new class of users to Cube. |
|
@h0tw1r3 I'll ping guys from the Core team. |
|
Hey @h0tw1r3 ! Thanks for contributing! I briefly checked PR and overall FIPS is on our plate to support however changing md5 to anything would be drastically breaking change so it's surely should be done under the flag. Second why is it xxhash and not sha? It isn't FIPS compliant as well. Thanks! |
@paveltiunov That's good to hear about FIPS support! I'm just saying it's impossible to enable node FIPS (ie. xxhash was selected because of the PR this was based on (fast, well supported), I just completed that work with a generic implementation, such that swapping the defaultHasher would be trivial. |
|
@paveltiunov implemented md5, sha256, sha512 hashing. defaults to md5, controllable through |
|
Hi @h0tw1r3 👋 May I kindly ask you to add |
|
@igorlukanin added to the configuration reference. |
|
final changes... in leu of feedback
|
774e809 to
fb4f292
Compare
Continuation of PR #9397
This PR just adds a few commits to create a single function as requested (#9397 (comment)) and test coverage.
In addition to the performance improvements with
xxhash, my motivation was to removemd5crypto as a whole because it's not supported when enabling FIPS mode.Original PR description below
Check List
Switch to using faster xxhash for cache keys and versions of data models instead of slow
crypto.createHash('md5').Use the fastest node xxhash implementation, written in ... of course rust :)
Here are some more info for those who is interested: