Add build option for randomized ML-DSA signing#2298
Add build option for randomized ML-DSA signing#2298M-AlNoaimi wants to merge 4 commits intoopen-quantum-safe:mainfrom
Conversation
Signed-off-by: M-AlNoaimi <mo@malnoaimi.com>
baentsch
left a comment
There was a problem hiding this comment.
Thanks for the PR @M-AlNoaimi -- I'd be glad to get feedback to my single comments before approving this. And then I have 2 basic questions: 1) How and where does this change the actual logic in ML-DSA? 2) How shall users of the library recognize whether this config option has been set? Doing it this way seems like inviting a maintenance nightmare ("User: ML-DSA doesn't work! Maintainer: Which build flag did you set? User: How should I know? I only have a binary.")
|
Oops -- seeing just now that @ashman-p approved this PR. In that case I assume I am unaware of some things and am willing to retract my review: In order for me to do so, would you please address my questions, Norm such as to not have my possible lack of understanding cause undue work to the PR's author? |
These changes only introduces a build time option and a test, AFAIK the signing logic is only affected if the new option is enabled, in which case the ML-DSA impl will use randomized signing.
I guess right now the only way they'd be able to tell was at build time, but I don't think it should be much of an issue if the default behaviour is set to OFF, and the new option keeps it that way as suggested on liboqs-python #128 So I think if it's off by default, current usage would stay the same. |
|
Thanks for the PR!
I have the same question. From what I can tell, the OQS_ENABLE_SIG_ML_DSA_RANDOMIZED_SIGNING macro isn’t currently used in the ML-DSA implementation to control the switch between randomized and deterministic signing. As far as I understand, the ML-DSA implementation already defaults to randomized signing. Is there any evidence that this wasn't the case? and liboqs/src/sig/ml_dsa/pqcrystals-dilithium-standard_ml-dsa-65_ref/sign.c Lines 229 to 234 in 52169a1 |
…Python & C) Signed-off-by: M-AlNoaimi <mo@malnoaimi.com>
Correct. But this PR makes the option available (thanks for the latest commit making it actually available!), so this can be used to change the behaviour of the library and there should be a way to query this also in a binary. Otherwise this invites a maintenance nightmare. |
bhess
left a comment
There was a problem hiding this comment.
Thanks for adding the control logic in the last commit, @M-AlNoaimi. I left one inline comment.
Signed-off-by: M-AlNoaimi <mo@malnoaimi.com>
Signed-off-by: M-AlNoaimi <mo@malnoaimi.com>
| def is_use_option_enabled_by_name(name): | ||
| return name in available_use_options_by_name() | ||
|
|
||
| def is_ml_dsa_randomized_signing_enabled(): |
There was a problem hiding this comment.
This approach feels "brittle" (but I may be paranoid here): For example, would it be possible to build everything with "-DOQS_ENABLE_SIG_ML_DSA_RANDOMIZED_SIGNING" (passed to the compiler during a re-build)? If so, this function will fail to return the right value. And/or would it be sensible to have cmake variable and C define have different names instead? And then there's still the possibility that header file and binary under test don't match up, e.g., because of some user-specific LD_LIBRARY_PATH setup...
I still feel it would be good for users to have a way (e.g., static variable) to query which way (randomized or deterministic) the library under test/use has been built; this test function could use the same facility to pick which way to test (and test the user-level "build-type" facility at the same time).
xuganyu96
left a comment
There was a problem hiding this comment.
@M-AlNoaimi Thank you for the contribution!
There are a few things about this pull request's approach that I don't feel confident about. My primary concern is that this implementation does not allow deterministic and randomized signing routines to exist in the same build. I'd like to point to the implementations of deterministic encapsulation in OQS's ML-KEM, which expose encaps and encaps_derand at the same time. I also have reservations about creating CMake build flags that are specific to algorithm instead of having universal build flags that take algorithm-specific values, though this is admittedly a secondary issue.
I would like to propose an alternative approach in which liboqs implements crypto_sign_signature_derand using the internal sub-routine crypto_sign_signature_internal. This implementation can take the form of a patch on the upstream or in liboqs's internal code. This way randomized/deterministic signing can exist in the same build, and we get to skip over the awkward is_ml_dsa_randomized_signing_enabled implementation.
PS: FIPS 205 also specified a deterministic variant of SLH-DSA, so this approach can apply to SLH-DSA, too. As for other signature schemes that might not support deterministic signing, we can simply disable the relevant API's in the same way KEMs are handled.
|
@M-AlNoaimi fyi we discussed this PR in our status call and agreed to not land this as-is as it is using an algorithm code base that neither OQS nor the upstream supports any longer, namely "PQCrystals". That said, we would be very glad if you could raise a new PR adding this capability using the ML-DSA code base OQS does support, namely mldsa-native: Would you be willing to do this? Would you need any support beyond some of the still valid comments above? |
Hi @baentsch, thanks for the clarification and for raising this in the status call. I understand the concern with the current state, I agree this should be implemented against the mldsa-native implementation instead. The earlier comments were very helpful, i’ll take those into account when working on the new PR. I’ll start very soon, thanks again! |
This PR adds an option to enable randomized signing for ML-DSA, as requested in open-quantum-safe/liboqs-python#128.
I've added a
OQS_ENABLE_SIG_ML_DSA_RANDOMIZED_SIGNINGbuild flag (defaulting toOFF) and a conditional test to verify that signatures are non-deterministic when the flag is enabled.A small update to the CONFIGURE.md was done as well to note the flag.