-
Notifications
You must be signed in to change notification settings - Fork 484
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 GitHub Actions workflows for stateful signatures #1692
Conversation
@baentsch @dstebila For your consideration: at a glance it looks to me like a lot of our CircleCI runs (most of the Linux and MacOS configs) could be ported over to GitHub Actions, and we might get M1 Mac testing "for free" in the process. Is there a reason to prefer keeping them on CircleCI? Since we have to revise the workflows anyway to enable stateful sigs, this seems like an opportunity to kill two birds with one stone: move as many as possible to GH and enable stateful sigs en route. |
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.
Thanks! LGTM. What about adding one test config with only LMS, one only XMSS, one both disabled, too? I'd not suggest adding this to the matrix but maybe just to our primary build platform, a linux under x64
to catch "interdependency" errors in that alg category?
Not really: I have a long-running goal of moving off CCI but there's (been?) a catch. |
Hi @SWilson4 , I can help fix the XMSS errors. I've been away for a while, can you let me know what is the changes that lead to XMSS kat errors? |
It looks to me like it's just a matter of the remaining / total signatures not printing correctly when keygen / sign are disabled: Line 327 in 0a44a25
|
Thanks @SWilson4 , I will handle it from here. Can I make a commit to this PR? (I assume yes, just ask to be sure) |
That would be great -- Goal would be to "turn CI green": Thanks in advance! |
Yes please! |
Go for it! No reason to stick with CircleCI if we can get what we need from GitHub Actions. |
scripts/build-android.sh
Outdated
@@ -107,7 +109,10 @@ cmake .. -DOQS_USE_OPENSSL=OFF \ | |||
-DBUILD_SHARED_LIBS=ON \ | |||
-DCMAKE_TOOLCHAIN_FILE="$NDK"/build/cmake/android.toolchain.cmake \ | |||
-DANDROID_ABI="$ABI" \ | |||
-DANDROID_NATIVE_API_LEVEL="$MINSDKVERSION" | |||
-DANDROID_NATIVE_API_LEVEL="$MINSDKVERSION" \ | |||
-DOQS_ENABLE_SIG_STFL_LMS=ON \ |
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.
This activates XMSS/LMS by default for android builds. Sensible for CI, not so for "unsuspecting" users of the script: Suggest to add at least an output warning message that this is an dangerous and not-recommended setting.
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.
Good catch. I've reworked the script so that XMSS/LMS are disabled by default.
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.
Thanks for adding tests also with algs=ON and keygen/sig=ON|OFF! Apart from one nit LGTM.
Co-authored-by: Duc Nguyen <ductri.nguyen@sandboxquantum.com>
Co-authored-by: Duc Nguyen <ductri.nguyen@sandboxquantum.com>
Co-authored-by: Duc Nguyen <ductri.nguyen@sandboxquantum.com>
Co-authored-by: Duc Nguyen <ductri.nguyen@sandboxquantum.com>
Co-authored-by: Duc Nguyen <ductri.nguyen@sandboxquantum.com>
Co-authored-by: Duc Nguyen <ductri.nguyen@sandboxquantum.com>
commit 001e96a Update GitHub Actions workflows for stateful signatures (#1692) commit 8524a16 Post-rebase cleanup commit 5da49e3 Satisfy astyle commit a535114 Fix macOS build error: lld -> llu commit 71ee535 Bring EVP_DigestUpdate calls in line with main commit 154d8e4 Fix test program linkage for cross-compiling commit e92aab3 Stateful sigs: Rename keygen / sign option, add more tests, fix memory errors (#1755) commit b075878 Clean up OQS_SIG_STFL_SECRET_KEY_free commit db000c2 Remove unused sig member commit 9b60f60 Naming convention for serialize / deserialize functions commit 7dd4ea0 Test stateful sigs on arm64, s390x, and powerpc (#1772) commit 31bdf13 Clean up unresolved comments on stateful-sigs PR (#1793) commit 8e75f98 Update config variable name commit ca27922 Strengthen warning in CONFIGURE.md Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>
Integrate stateful signatures into our CI workflows.
I took the approach of simply switching on LMS and XMSS for all of our GitHub Actions builds that support all algorithms and adding an additional run in select places to test the configuration in which keygen and sign are disabled.
It looks like the logic around enabling / disabling keygen/sign needs to be tweaked in the XMSS KAT testing function.
We still need to test these on MacOS, which will require updating CircleCI, but I thought it best to put this draft up here as is for now to unblock anyone who wants to work on the test failures.