Repository navigation
Add QR-UOV signature schemes - #2582
0rlych1kk4 wants to merge 8 commits into
Conversation
|
coverage: 81.03% (-3.5%) from 84.482% — 0rlych1kk4:feat/qruov-integration into open-quantum-safe:main |
|
Hi @0rlych1kk4 ! first of all, let me thank you sincerely for the effort put into this, and for helping us with this endeavor. From a quick look, and based upon the number of new files present, is there a way to group common code in any way? +2k review files look spooky.... I have seen how the repo of the QR-UOV team works and it is true that a lot of files are exact, but they depend on compilation macros which represent different parametrization, so that effort may be be difficult. |
|
Thanks @RodriM11 . Yes, most of the file count comes from the importer materializing the same QR-UOV sources separately for each parameter set, PRG, and implementation. The upstream implementation selects the parameterization primarily through compile-time macros, so there should be room to reduce duplication. I’ll also work through the CI failures after that restructuring, since reducing the generated source layout may resolve some of them at the same time. |
|
You may want to take a look at the MQOM or SNOVA source code to recuce the number of files. |
|
another question @0rlych1kk4 ! Where are the integration files for liboqs allocated (i.e. the ones that the QR-UOV people would have to maintain somewhere)? |
664a4a9 to
078c4d3
Compare
|
Thanks @RodriM11 . I reworked the QR-UOV import to use shared common_dep source groups instead of materializing the full source tree for every parameter set and implementation. I revalidated the updated layout with a fresh build, all 30 test_sig runs, all 30 deterministic KATs, importer verification (395/395), and the datasheet/memory/SPDX checks. |
|
@RodriM11 The liboqs-specific integration files are maintained in the QR-UOV upstream repository under integration/liboqs/. I also reworked the import to use shared common_dep source groups, so the full QR-UOV source tree is no longer materialized for every parameter set and implementation. The 30 variants remain, while the common/ref/opt/AVX2 sources are shared centrally. |
Thank you a lot @0rlych1kk4 ! May I ask how have you worked around QR-UOV structure? Because they were dependent on the
I include the link to your copy of the QR-UOV repo with the inclusion files just to ease the review for other members. Thanks! |
|
Hi @0rlych1kk4 Great work here! I ran this on x86-64. Head 078c4d3, Windows 11 x86-64, gcc 16.1.0, cmake 4.4.3, OQS_USE_OPENSSL=OFF, minimal build restricted to the 30 QR-UOV schemes. Good news first. All 30 schemes pass the liboqs KAT check and test_sig on ref, opt and avx2, and all three produce identical digests. I also went past liboqs' own oracle, since kats.json here is self-generated: I reconstructed the NIST .rsp layout from kat_sig --all and hashed it against qruov/round3@3d84d0f:test/KAT_SHA256SUMS.txt. That is 100 vectors per scheme, 3000 published Round 3 vectors, all matching, on the avx2 build. AVX2 really executes, and I checked that under gdb... the ...avx2_crypto_sign* breakpoints are hit and the ref ones are not, and single-stepping retires real vpcmpeqd %ymm0,%ymm0,%ymm0 inside matmulfq_vnni_4x16. Now the part worth your attention, because I think it changes what your own 30/30 covered. Nothing except ref can be selected through the current option surface. .CMake/alg_support.cmake declares 30 OQS_ENABLE_SIG_qruov_*_avx2 options and zero _opt, while src/sig/qruov/CMakeLists.txt refers to _opt 360 times. So the 30 opt directories and qruov_qruov_opt/ ship and are never compiled. AVX2 is separately gated to Linux|Darwin, and on Apple Silicon the inner x86-64 guard is false. Putting those together, I believe your 30/30 was necessarily ref only, and opt and avx2 had not run anywhere before this. I only reached them by forcing the 30 cache variables on the cmake line, which is a configuration override, not a source change. One trap before you add the missing options. The wrapper tests #if _opt / #elif _avx2, so enabling both selects opt and drops AVX2. I built it that way to check: opt symbols 5, avx2 0, ref 0. Adding _opt on its own would therefore remove AVX2 from every default Linux build, and no test would notice, because the KATs pass either way. The ordering needs fixing in the same change. tests/test_kat_all.py fails 30/30 with KeyError: 'all'. All 30 QRUOV entries in tests/KATs/sig/kats.json have only single; all 77 existing sig entries have both. I have the 30 computed all values and can post them or open a PR against your branch, whichever you prefer. The importer points at a personal fork. copy_from_upstream.yml has git_url: https://github.com/0rlych1kk4/round3.git, pinned to a SHA that is the head of an unmerged PR in that fork. qruov/round3's default branch is main at 3d84d0f and contains neither that SHA nor the de98876 in the PR description. Every other sig family in ALGORITHMS.md points at the upstream project's own repository. I suspect this resolves itself once qruov/round3#1 lands, but as it stands liboqs would be importing from a contributor fork, so it is probably the one thing that has to change before merge. AES does not go through liboqs in an avx2 build. Measured by call counts over a full kat_sig run: OQS_AES128_CTR_inc_stream_iv 360 on ref and 360 on opt, 0 on avx2, with 720 calls into the vendored x86aesni stream function instead. The CMakeLists passes -DPRIM_AES_BACKEND_X86AESNI=1 alongside -DQRUOV_PRIM_OQS, and prim.c:212 tests the AES-NI macro first. SHAKE goes through liboqs in all three. Since avx2 is the default on Linux x86-64, that is the shipped default path, so it is worth saying explicitly rather than leaving "liboqs primitive integration for SHAKE and AES-CTR" to be read as covering all three. Smaller things: 72 -Wvla warnings from QR-UOV sources under -Wall -Wextra -Wpedantic -Wvla and zero without it; SPDX headers on 31 of 163 QR-UOV files against 100 percent for uov, mayo and snova; no entries in tests/constant_time/sig/{passes,issues}.json, and qruov_qruov_common/linsys.c branches on echelon->rank and on fastop_dotprod results, which look secret-derived in a signing path, so that is a question for the QR-UOV team rather than a claim from me; and ALGORITHMS.md advertises x86_64 (aes+avx2+bmi2) while a default Windows x86-64 build gets ref only. Two offers. The KAT validation and the portable non-AVX2 path were my half of the split, so the CI to cover them is mine to write: a matrix over ref, opt and avx2 on x86-64 Linux running the QR-UOV KATs, a guard that fails when a scheme ships an implementation directory no option can enable, and a kats.json completeness check. Every finding above would have been caught by one of those three. Say the word and I will open it against your branch. Boundaries, so none of this is read as more than it is: one machine, Windows x86-64, gcc only, no clang, no sanitizers and no valgrind, OQS_USE_OPENSSL=OFF, static minimal build. The AVX2 CPU-feature fallback was never exercised because this CPU has all three features. The gdb dispatch check was done on one scheme and the other 29 rest on identical generated wrappers plus KAT agreement. A Linux x86-64 run is still worth having, because there avx2 is reachable through your own options than my override. And I reviewed the integration, not the cryptography: nothing here speaks to QR-UOV's security or to AVX2 correctness outside the KAT set. |
@RodriM11 I did not remove the compile-time variant selection in qruov.h. The change is only in how the importer materializes the sources. The shared common/ref/opt/avx2 sources are declared as include_only common dependencies, so they are stored once in the generated tree but are still compiled separately for each scheme implementation target. Each target therefore receives its own QRUOV_PARAM_*, PRG_IS_AES, namespace prefix, and implementation-specific compile options. The liboqs integration files are here in my QR-UOV branch: integration/liboqs/. This contains the per-variant metadata, META_Common.yml, namespace.h, randombytes.c, and signature.c. |
Hi @paschal533 Thanks for doing the x86-64 sweep and for checking the implementation dispatch directly. This catches a few things my Apple Silicon validation could not cover. I’ll handle the integration-side fixes for:
I’ll also raise the linsys.c/constant-time question with the QR-UOV upstream maintainers rather than treating it as resolved locally. Thanks again, the independent 30/30 ref/opt/AVX2 KAT validation and the published Round 3 vector comparison are especially useful. |
|
@paschal533 I followed up on the missing opt option on Apple Silicon. I reverted the default-selection experiment and kept ref as the liboqs default. The final ref-default build also passes all 30 deterministic KATs, and copy_from_upstream.py verify reports 395/395 schemes matching. So I don't think we should expose opt through normal liboqs selection yet; its ARM64 behavior needs investigation first. AVX2 remains the x86-64 optimized path. |
078c4d3 to
4134ad9
Compare
|
Hi @0rlych1kk4. I opened the CI side against All 30 digests are re-derived on a I am on Windows, so the workflow has not run on Linux yet. Rest is in the PR description |
a4ad814 to
04686f2
Compare
|
CI update after rebasing onto the current main: 71 checks are now passing. The remaining failures are jammy-clang and noble-memopt, both of which reach the late stages of the test suite and hit the 60-minute GitHub Actions timeout. QR-UOV-specific functional tests pass before those timeouts. I don't see a QR-UOV functional failure in either remaining job. |
Yes, that’s my understanding. #2595 specifically targets the platform-test scheduling/runtime issue, including noble-memopt and jammy-clang. I’ll leave the QR-UOV implementation unchanged and rerun/rebase once that lands. |
|
Hi @0rlych1kk4 , #2595 has landed, so if you could rebase and check, that would be great |
04686f2 to
e5a1ba9
Compare
|
Hi @paschal533 DCO is failing on #2582 after your CI commits were merged. I checked the five commits and they have your author identity but no Signed-off-by: trailers. Could you please amend/rebase those commits with git commit --signoff and force-push your branch? I’d rather not add your DCO attestation on your behalf. |
|
Hi @0rlych1kk4 done, sorry about that. I rebased the five commits onto 35aefac with Since my pr is already merged, you'll need to move your branch onto the new commits. The merge commit goes too, which is fine because nothing else is on top of it |
Signed-off-by: 0rlych1kk4 <orlychikka@gmail.com>
Every QR-UOV entry in tests/KATs/sig/kats.json carried only `single`, while all 77 pre-existing signature entries carry both `single` and `all`. tests/test_kat_all.py reads `kats[name]['all']` with no guard, so it failed 30 times with KeyError rather than reporting a data problem. The digests were computed with the method test_kat_all.py itself uses, a sha256 over `kat_sig <alg> --all` with CRLF normalised, cross-checked for one scheme against a direct reimplementation of that test. Each of the 30 outputs also reconstructs to the corresponding published Round 3 PQCsignKAT_32.rsp hash in qruov/round3@3d84d0f, so they are not merely self-consistent with this tree. They were computed on an avx2 build. `single` already agrees across ref, opt and avx2 for all 30, and `--all` differs only in running 100 vectors where `single` runs one, so the digests should not depend on the implementation; the matrix added alongside this checks that on ref and opt rather than assuming it. test_kats_json.py then makes a missing digest fail as what it is. It reads the json alone, so it needs no build and runs on every platform, and it names the algorithms at fault instead of surfacing as a KeyError inside an unrelated test. Signed-off-by: paschal533 <okwuosahpaschal@gmail.com>
src/sig/qruov/CMakeLists.txt tests OQS_ENABLE_SIG_qruov_<scheme>_opt in 30 places and .CMake/alg_support.cmake declares no such option, so the condition is false in every configuration. The 30 opt directories, and the shared qruov_qruov_opt, ship in the tarball and are compiled nowhere. Nothing notices: the build succeeds and the KATs pass on whichever implementation was selected, because all three produce identical vectors. The check compares the options a CMakeLists tests against the options alg_support.cmake declares. Across the whole tree that is 636 declared and 477 referenced, and today exactly 30 are referenced but undeclared, all QR-UOV opt, with no other family affected. It reads CMake files only, so it needs no configure and no build. src/oqsconfig.h.cmake is checked the same way and independently, since it carries a #cmakedefine for all 90 QR-UOV implementations while only 60 can ever be defined. Three generated surfaces disagree because the templates filter implementations differently, so a defect that is invisible in one is visible in another. Withholding an implementation is legitimate, so KNOWN_UNSELECTABLE records it with a reason, and the comparison is exact in both directions: an unselectable implementation that is not recorded fails, and a recorded one that has become selectable also fails. A one-way allowlist would go stale without anyone noticing, which is the same failure mode being fixed here. QR-UOV opt is recorded rather than declared because making it selectable produced reproducible SIGBUS crashes in 8 of the 30 variants on ARM64. The entry also notes that the wrapper selects with `#if _opt / #elif _avx2`, so whenever opt does become selectable that ordering has to change in the same commit or enabling opt will silently drop AVX2. Note that a fix for QR-UOV belongs in scripts/copy_from_upstream/.CMake/alg_support.cmake/add_enable_by_alg_conditional.fragment, which emits a per-implementation option only when the implementation declares a supported platform, and QR-UOV opt declares none. Editing the generated file would be reverted by the upstreamcheck job. Signed-off-by: paschal533 <okwuosahpaschal@gmail.com>
A matrix over the three implementations on x86-64 Linux, each building the 30 schemes and running the QR-UOV KATs. Each job checks which implementation the configure step actually selected before it builds anything. That is the point rather than a detail: all three implementations produce identical KAT vectors, so a job that silently fell back to ref would pass while testing nothing, and that is how opt came to ship for 30 parameter sets without being compiled anywhere. A job that cannot select its target fails and says what it got instead. ref turns the avx2 options off, avx2 requires all 30 to be on, and opt forces the cache variables directly, since no option declares them. The opt job is therefore a configuration override, not a source change, and it exists so the sources in the tarball are still built and KAT-checked on x86-64 while the ARM64 behaviour is investigated. It says nothing about ARM64. The two source-tree checks are also wired into the stylecheck job in basic.yml, which needs no build, so a missing digest or an unreachable implementation fails in about a second rather than after a full matrix. Signed-off-by: paschal533 <okwuosahpaschal@gmail.com>
Two blockers found in review, both of which defeated the stated purpose. The avx2 job did not test avx2. OQS_DIST_BUILD defaults ON, and the generated wrapper then keeps a run-time CPU check that falls back to ref when the host lacks AVX2, BMI2 or AES. The job asserted a compile-time define, so on such a runner it would have gone green while executing ref: exactly the failure it was written to prevent. All three jobs now set OQS_DIST_BUILD=OFF with an explicit OQS_OPT_TARGET, matching what nistkat-x64 already does, so the define implies the code path, and the assertion step refuses to continue if OQS_DIST_BUILD is set at all. The assertion step could pass vacuously. Masking grep's exit status swallowed a real error as well as "no matches", leaving an empty string, and comparing an empty string to a number inside an `if` reads as false rather than tripping set -e. A missing header therefore passed every guard, and the KAT step would then skip all 60 tests and exit 0. It now requires a non-empty header and tolerates only grep's "no match" status. The job moves from a new workflow called by pr.yml into extended.yml, next to nistkat-x64, the other job that runs test_kat_all. Three full builds plus 30 KAT sweeps on every PR ran against the project's stated direction of moving heavy testing out of the PR path. The two source-tree checks stay in basic.yml, which is the cheap half. The KAT step is now run in parallel with a 360 minute timeout, which is what the comparable job uses; 60 minutes serial was optimistic. The allowlist of 30 literal scheme names is gone. Whether an implementation can have an option is decided by the generator from docs/algorithms metadata, so that is where the set is now derived from, leaving only a reason per family and implementation. It no longer needs hand-editing when a parameter set is added or renamed. Comment stripping now removes bracket forms first. A declaration inside a bracket comment was read as live, which would have let the coverage check be silenced, and a hash inside a bracket argument truncated a line that was live, which would have hidden a real finding. Both directions now have regression tests, so the docstring's claim is enforced rather than asserted. kats.json is also checked for whole missing entries against the public header, using the same exclusions test_kat_all.py skips on. Iterating the file only ever validated what was in it, so deleting an algorithm outright passed every check and then returned as the KeyError this module exists to pre-empt. Both new modules are added to the pytest ignore lists that already name test_code_conventions.py, so the source-tree checks run once in stylecheck rather than in every build job. Signed-off-by: paschal533 <okwuosahpaschal@gmail.com>
The constant-time job in extended.yml fails on all 30 QR-UOV parameter sets, because nothing records the operations valgrind flags in the ref implementation. PR CI does not run extended.yml, so this first shows up in a manual or weekly extended run. passes/qruov holds the branches that only touch discarded data: rejection sampling of PRG output (fastop.h) and the retry on an inconsistent linear system (linsys.c:140). issues/qruov holds the rest: the LU pivot search (linsys.c:38), the secret-indexed gf_inv table lookup (gf.h:53), and the canonicalisation of s (qruov_common.c:22, 31, 33). The last is believed harmless, since it branches on s, which is published, but it reveals whether s was negated, so it stays in issues until that is confirmed. Signed-off-by: paschal533 <okwuosahpaschal@gmail.com>
Signed-off-by: 0rlych1kk4 <orlychikka@gmail.com>
d98d59e to
347c321
Compare
|
Thanks @paschal533 I pulled in the re-signed commits, rebased the branch onto current main, and the PR is green now. Appreciate the quick fix. |
Signed-off-by: 0rlych1kk4 <orlychikka@gmail.com>
|
PR LGTM: one nit to take into account is that all the |
Thanks @RodriM11 for the review and approval. The portability comments are addressed in f7fb257, and all CI checks are passing. The PR is now waiting for the additional required maintainer approval. |
|
Hi @baentsch the latest portability review comments have been addressed, Rodrigo has approved the PR, and all CI checks are passing. When you have a chance, could you please take a final look? The PR currently needs one additional write-access approval to merge. |
baentsch
left a comment
baentsch
left a comment
There was a problem hiding this comment.
Thanks again @0rlych1kk4 for this work! You already did create at least one issue pertaining to this, but I think 2 or 3 more would be in order to make this truly long-term maintainable: A merge with the OQS common AES logic seems sensible and of course there should be a reminder to upstream your personal fork and then land this in an update in the future.
|
@0rlych1kk4 Thank you so much for bringing this integration across the finish line! I haven't had time to take a closer look at this pull request yet: I am currently occupied with reviewing another integration (SDitH) and with resolving two longstanding security advisories. Let me know if a third pair of eyes is necessary. Otherwise I am happy to merge this pull request. |
|
For me, the only one of the issues created by @0rlych1kk4 that we may want to debate including it in this PR is #2636 , i.e using |
fwiw, I wouldn't: a) This is not a first: We've got other algorithms already bringing their own baseline crypto too, and b) for the benefit of users, I'd rather make this algorithm available rather now than later/only after further integration efforts & reviews: The issue is created and anyone with interest for this can tackle this any time.
That is something we may want to discuss in tonight's TSC meeting. Again, personally, that'd be nice, but I wouldn't consider it essential: imo, priority should be on landing these algs, then fine-tuning. If I had to say anything, I'd press "Merge" now and move to the next alg (and/or suggest beginning work on optimizing/further integrating it). |
This branch has not been deployed
7 participants
Adds QR-UOV Round 3 signature schemes to liboqs.
Closes #2454.
This integrates all 30 QR-UOV parameter-set / PRG combinations, covering the
15 Round 3 parameter sets with both AES and SHAKE PRGs.
The integration includes:
OQS_randombytesintegrationThe QR-UOV source is currently pinned to:
33226e903ab5ab5dfe5a202102761034c7a553fdValidation
Tested from a clean liboqs build on macOS/Apple Silicon:
git diff --checkpassescopy_from_upstream.py verifyreports 395/395 schemes matching upstreamThe AVX2 implementation has also been validated independently on x86-64.
The portable
optimplementation is retained in the imported sources but isnot exposed through normal liboqs implementation selection at this point.
During Apple Silicon testing, enabling
optexposed reproducible SIGBUSfailures in a subset of parameter sets, so
refremains the default whilethat behaviour is investigated.
Version / API questions
No existing algorithm behaviour or KAT values are changed. This PR adds new
QR-UOV algorithms.
Yes. This PR adds the QR-UOV signature schemes to the list of algorithms
available through the liboqs signature API.
Generative AI disclosure
Generative AI was used as a development aid for documentation drafting. All
changes were reviewed by me, and the resulting implementation was built and
validated locally, including runtime signature tests and KAT verification.