-
Notifications
You must be signed in to change notification settings - Fork 476
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
Test against all 100 KAT values #1560
Conversation
While working on this, I noticed that our implementation of Falcon-1024 differs from the upstream on a single KAT, number 82. I put together a demonstration here: https://github.com/SWilson4/falcon-diff This seems to be explained by the signature buffer size used for the KATs: our implementation uses the signature size of 1280 bytes while the upstream code passes a larger buffer of 1330 bytes (even though the signature size upstream is still defined to be 1280 bytes). I'm not sure yet why this is done, but redefining the signature size to be 1330 in liboqs does make all 100 KATs pass. |
This also speaks for your option 3 in #1561 (comment). Yes, @dstebila this may be a deviation from the "sig size" number given in the Falcon spec, but a) that number seems to be an "arbitrary choice" and b) aren't KATs there to "tie down" any spec ambiguities? |
5e35f96
to
8883e71
Compare
Now that Falcon updates are (hopefully) coming soon, I think it's time to get this merged. I would rather it not be held up by the (apparently buggy) CircleCI ARM environment, so for now the full KATs only run on GitHub Actions once a week, using the same configuration as the constant-time tests. They will fail for Falcon-1024 until the new code for that algorithm lands. |
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.
Conceptually LGTM. I'd approve if there'd be a good rationale for the API (or a change to "externalize" the state :)
The environment says Thanks for your diligent exclusion of this long-running test from the standard CI suite. I just wonder whether there'd be a more future-proof way to do this, e.g., some sort of "weekly_only" test list that can be summarily excluded from all PR and push Ci tests (?) Or should we create a separate issue for this? Something separating the .py files into "WEEKLY_TESTS", "PR_ONLY_TESTS" and "ALWAYS_TESTS"...
92a49d1
to
c0e4c2b
Compare
I'm going to mark this PR as draft while I tinker with the internal API configuration. Will reopen for review when I'm ready for feedback. |
Once (if) #1667 lands, I'll rebase this branch and reopen this PR. |
This has been rebased to use the new internal API and is once again ready for review. Pardon the long and messy commit history. |
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.
Pardon the long and messy commit history.
No reason to apologize. Sorry in turn for the questions triggering some of the work. Didn't read single commits but only reviewed the final files changes and those basically LGTM (2 separate nits for which there's probably easy answers). The merge indeed should be a thorough "Squash" with presumably most commit messages trimmed. I also presume you tested the weekly job using act
or so?!
Add functionality to test against all 100 NIST KAT values.
kat_kem
andkat_sig
programs to generate all 100 KAT valuestest_nistkat.py
to run all 100 testsFixes #1418