-
Notifications
You must be signed in to change notification settings - Fork 121
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
Update ACVP SHAKE test implementations #1663
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1663 +/- ##
=======================================
Coverage 78.22% 78.22%
=======================================
Files 566 566
Lines 95193 95185 -8
Branches 13665 13663 -2
=======================================
- Hits 74465 74462 -3
+ Misses 20133 20129 -4
+ Partials 595 594 -1 ☔ View full report in Codecov by Sentry. |
@@ -268,18 +268,28 @@ static bool GetConfig(const Span<const uint8_t> args[], ReplyCallback write_repl | |||
}, | |||
{ | |||
"algorithm": "SHAKE-128", | |||
"revision": "2.0", | |||
"revision": "1.0", |
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.
Why is this going back to an older version? Is 1 the right thing to be testing?
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.
Revision 2.0 doesn't exist as of right now: https://pages.nist.gov/ACVP/draft-celi-acvp-sha3.html#name-sha3-and-shake-algorithm-ca
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.
Did it used to exist? I'm confused what happened here.
if (md[i-1].size() < 16) { | ||
memcpy(msg[i].data(), md[i-1].data(), md[i-1].size()); | ||
size_t pad_size = 16 - md[i-1].size(); | ||
memset(msg[i].data() + md[i-1].size(), 0, pad_size); |
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.
Isn't everything in msg already zero from line 1251?
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.
Yes I believe so. Thanks for pointing that out
45ba2f0
to
157258c
Compare
* Update ACVP tool to properly interact with SHAKE vectors * Update modulewrapper to run SHAKE tests correctly * Update modulewrapper to have updated registration json for SHAKE * Update ACVP tool tests
Issues:
Needed to move forward with ACVP testing in the future.
Description of changes:
Our SHAKE ACVP test implementations and registrations don't work with the current version of NIST's server, so this updates our logic to match what the server expects and fixes the following for SHAKE in our ACVP tool:
Call-outs:
N/A
Testing:
SHA1/2/3 and SHAKE now properly pass tests run against the NIST demo server.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.