noobaa-core icon indicating copy to clipboard operation
noobaa-core copied to clipboard

fix native MD5 calculation: Bump isa-l_crypto to 4f72976

Open tangledbytes opened this issue 2 years ago • 3 comments

Signed-off-by: Utkarsh Srivastava [email protected]

Explain the changes

This PR bumps isa-l_crypto to 4f72976 (master). We rely on this package for MD5 calculation (and other use cases) in our crypto package monkey patching. v2.24.0 has the issue where the calculation of MD5, SHA1, etc does not produce the correct result when the given data is not aligned with their respective block sizes. This was fixed in https://github.com/intel/isa-l_crypto/pull/97

For example, node -p 'new (require("./src/util/nb_native")().MD5_MB)().update(Buffer.from(new Array(65).fill("0").join(""))).digest().toString()' would produce the EXACT same MD5 as node -p 'new (require("./src/util/nb_native")().MD5_MB)().update(Buffer.from(new Array(64).fill("0").join(""))).digest().toString()' would. This is because the library had an issue where it misses the data which are not multiple of MD_BLOCK_SIZE.

Incorrect calculation of MD5 in FIPS environment lead to failed deployments of NooBaa in FIPS environment.

Issues: Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2129126

Testing Instructions:

  1. node -p 'new (require("./src/util/nb_native")().MD5_MB)().update(Buffer.from(new Array(65).fill("0").join(""))).digest().toString()' should produce f8c702aaa8c658413a4efb3a614d7707 instead of 10eab6008d5642cf42abd2aa41f847cb.
  2. Deploy NooBaa in FIPS environment, the deployment should go through.
  • [ ] Doc added/updated
  • [ ] Tests added

tangledbytes avatar Oct 06 '22 03:10 tangledbytes

@liranmauda We need to see we don't break, p&z and FIPS

nimrod-becker avatar Oct 06 '22 06:10 nimrod-becker

@utkarsh-pro can you update the version info in https://github.com/noobaa/noobaa-core/blob/master/src/native/third_party/versions.md too?

guymguym avatar Oct 06 '22 06:10 guymguym

@liranmauda We need to see we don't break, p&z and FIPS

True, We need to ask for a build on P&Z. As for FIPS, I don't see a reason why it will break, But we definitely need QE to test it on FIPS.

@b-ranto @deepshikhaaa Can we run a build on this commit hash?

liranmauda avatar Oct 06 '22 10:10 liranmauda

@liranmauda @nimrod-becker this PR comes to fix FIPS if I'm not mistaken

dannyzaken avatar Oct 18 '22 09:10 dannyzaken

@dannyzaken, yes that's right this PR was intended to fix our crypto monkey patching in FIPS environment.

I mentioned the lesser relevant story in the PR description and forgot to mention the clear intention 🤦 .

Edit: Fixed the PR description.

tangledbytes avatar Oct 18 '22 09:10 tangledbytes