Add API support for External-MU Signature[ML-DSA]#2366
Add API support for External-MU Signature[ML-DSA]#2366abhi-dev-engg wants to merge 34 commits intoopen-quantum-safe:mainfrom
Conversation
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
|
Hi @bhess , I was trying to implement the external mu OQS APIs based on approach 1 suggested here by you. However, in my (limited) understanding it seems difficult adding a "new variant" without having support from the upstream repo(see one error below). Tried adding the new variants "ml_dsa_xx_extmu" in 'copy_from_upstream.yml' under ml-dsa umbrella leading to 'FileNotFoundError: [Errno 2] No such file or directory: 'repos/mldsa-native/integration/liboqs/ML-DSA-44-EXTMU_META.yml'. Additionally, the extmu and the existing variants shall be using the same set of source files, which could lead to linking errors due to "same definition" if both the variant are enabled. Please let me know your opinion or please guide me incase I am in the wrong direction. Thanks. |
|
Hi @abhi-dev-engg, thanks for working on enabling externalMu. In my view, the preferred approach is to add the external-mu API definition directly to the upstream YAML file (for example, in ML-DSA-44_META.yml, signature_signature_extmu/signature_verify_extmu). By doing this, we can then adapt the copy-from-upstream script to automatically generate the code for the -externalmu variant. This keeps the integration easier to maintain if upstream APIs change. Let me know if that makes sense to you! |
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
|
Hi @bhess , thank you for your reply. I agree with your approach. would try to do the same and yes it requires "some" changes in copy_from_upstream.py for this to work. Let me spend some time over this and get back. |
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Hi @bhess , the first version is ready now. Pls have a look at your convenience. |
xuganyu96
left a comment
There was a problem hiding this comment.
Thank you for the contribution.
In addition to the individual code change comments I've made, I have a bigger concern with the strategy used to implement the external-mu API. From what I have read, your strategy for implementing external mu is to treat it as a separate implementation of ML-DSA whose sign and verify behave differently from the non-external-mu implementations of ML-DSA. This feels inconsistent with how liboqs intends "different implementations of the same scheme" to behave. The expectation is that the same function across different implementations of the same scheme (e.g. x86 impl of ML-DSA's sign versus aarch64's impl, versus the portable impl) should have identical external behavior, such as "passing all of the same KAT". This is not true for the "sign" and "verify" function under the external-mu implementations.
Because external-mu sign/verify have different external behaviors from non-external-mu sign-verify, I feel strongly that they should be separate functions instead of separate implementations. I am thinking of something like this:
// list of OQS API
OQS_SIG *OQS_SIG_ml_dsa_44_new(void);
OQS_API OQS_STATUS OQS_SIG_ml_dsa_44_keypair(uint8_t *public_key, uint8_t *secret_key);
OQS_API OQS_STATUS OQS_SIG_ml_dsa_44_sign(uint8_t *signature, size_t *signature_len, const uint8_t *message, size_t message_len, const uint8_t *secret_key);
OQS_API OQS_STATUS OQS_SIG_ml_dsa_44_verify(const uint8_t *message, size_t message_len, const uint8_t *signature, size_t signature_len, const uint8_t *public_key);
OQS_API OQS_STATUS OQS_SIG_ml_dsa_44_sign_with_ctx_str(uint8_t *signature, size_t *signature_len, const uint8_t *message, size_t message_len, const uint8_t *ctx, size_t ctxlen, const uint8_t *secret_key);
OQS_API OQS_STATUS OQS_SIG_ml_dsa_44_verify_with_ctx_str(const uint8_t *message, size_t message_len, const uint8_t *signature, size_t signature_len, const uint8_t *ctx, size_t ctxlen, const uint8_t *public_key);
/* ext-mu functions: replace (msg, msglen) with mu
* return OQS_ERROR if no implementation of this scheme can do external-mu
*/
OQS_API OQS_STATUS OQS_SIG_ml_dsa_44_sign_extmu(uint8_t *signature, size_t *signature_len, const uint8_t *mu, const uint8_t *secret_key);
OQS_API OQS_STATUS OQS_SIG_ml_dsa_44_verify_extmu(const uint8_t *mu, const uint8_t *signature, size_t signature_len, const uint8_t *public_key);
docs/algorithms/sig/ml_dsa.yml
Outdated
| spec-version: ML-DSA | ||
| primary-upstream: | ||
| source: https://github.com/pq-code-package/mldsa-native/commit/f48f164cefb07f4ffa519ddda7cee670b8ee3517 | ||
| source: https://github.com/abhi-dev-engg/mldsa-native/commit/b3198696c21f4b28508ad2231e130768f622076a |
There was a problem hiding this comment.
I am not comfortable with moving the source of implementation from official PQCA repository to a personal fork.
I can see that you are trying to change the schema of the upstream META.yml file. If a change in the schema of META.yml files is necessary, I would recommend opening a separate pull request to mldsa-native.
Alternatively, you can set only the _extmu implementations to upstream from your fork, which means that in copy_from_upstream.yml there will be a separate upstream (e.g. mldsa-native-fork) from the official PQCA upstream.
There was a problem hiding this comment.
I would be creating a PR to merge this into mldsa-native once this PR is reviewed and then replacing this with the official git repo as currently done. I needed a way to test out this feature hence this change.
My bad, I should have mentioned it earlier. The PR description is updated with this info.
|
Thanks @abhi-dev-engg. I’ll take a more detailed look.
This would be Option 2 from the associated issue #2254. |
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Created PR for mldsa-native/main :pq-code-package/mldsa-native#976. This PR description is updated with this info. Since the upstream of mldsa-native points to v1.0.0-alpha2 tag, I checked with mldsa-native maintainers regarding merging changes to this tag, however they mentioned of a release starting of march. Please find the conversation here : https://github.com/orgs/pq-code-package/discussions/202#discussioncomment-15932113. |
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
|
hi @xuganyu96 , could you pls have a look at the failing TCs, it seems to be not related to changes in this PR and look like generic failure affecting other PRs as well ? |
Unfortunately coveralls.io is down (it's always DNS!). If the site does not come back up in a day or two we will consider some workarounds. |
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
|
Hi @xuganyu96 , @bhess The provided review comments have been addressed, could you pls have a look and resolve if all is ok ? |
|
The mldsa-native PR for the yaml changes is merged. PR : pq-code-package/mldsa-native#976 cc : @dstebila @xuganyu96 @bhess |
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
xuganyu96
left a comment
There was a problem hiding this comment.
I am satisfied with the code change as it is now, with the exception of changing the source of mldsa-native from Abhi'personal fork back to a full release of mldsa-native from pq-code-package. Per @abhi-dev-engg's suggestion, we can wait for an mldsa-native release before merging this pull request.
Thank you @xuganyu96 . could you pls "resolve" the review comments in such a case pls. helps me to differentiate b/w fixed & pending reviews. Hi @bhess , could you pls re-review the changes and resolve if all is ok ? |
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
Signed-off-by: Abhi S <saxena_abhinav@icloud.com>
bhess
left a comment
There was a problem hiding this comment.
Looks good. To confirm I'm following: we wait for the upstream release, update the input location to the upstream repo, and then merge. I'll approve once these changes are in.
|
|
||
|
|
||
| if with_kat: | ||
| if with_kat and not scheme.get('is_extmu', False): |
There was a problem hiding this comment.
Ok, I suppose known-answer tests for externalMu are covered by the ACVP tests?
yes, that's correct. The PR for the yaml changes in mldsa-native repo is already merged. |
yes, ACVP tests cover external mu cases. I have modified vector_sig to use the "OQS_Sig" APIs for external-mu tc as well. |

Add External API support for "External MU" Signature generation and verification.
The following changes were done as a part of this PR:
NOTE : The upstream changes to include the "external mu" APIs in ML-DSA-xx.yml is currently mapped to my fork repo. This would later be changes to official mldsa-native repo once the review in this PR is done. I would be creating PRs for the same - (i) for v1.0.0-alpha2 & (ii) on the latest main
PR for mldsa-native yaml changes : pq-code-package/mldsa-native#976