-
Notifications
You must be signed in to change notification settings - Fork 462
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
Add ML-DSA / FIPS 204 final #1919
base: main
Are you sure you want to change the base?
Conversation
10a5487
to
0989e9d
Compare
1f89068
to
9347125
Compare
Add wrapper for default ctx recommended in FIPS 204 Update suppression files. Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
cfd2cd3
to
69a80f8
Compare
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
A progress update:
The context parameter is needed by some use cases like for composite signatures.
I plan to wrap up this PR this week, also adding tests for the context API. The initial plan was to also add ACVP tests for the external API and also adding pre-hash variants. Since the publication of these vectors was postponed to within November, I'd suggest to do this as a follow-up after this PR. |
The API definition looks good to me. I do wonder if instead of setting |
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
…pointers Signed-off-by: Basil Hess <[email protected]>
This PR represents a break with what This is clearly visible by the huge amount of code this PR adds just to introduce structures all basically returning "OQS_ERROR". This is a parallel to what is proposed in #1985 -- with the difference that in this PR, an (one!) upstream code base actually supports the feature. In my opinion, this addition makes OQS even harder to maintain, more fragile (see #1919 (comment)) and harder to support than it already is. For example, the only reason all this empty boilerplate code stands a chance of being OK is the code generation logic built into "copy_from_upstream": This duplicates easily a lot of code, but also makes In sum, this PR
I see 2 alternatives to counter these problems:
Option 1) would retain the nature of |
Thanks for your feedback @baentsch, I agree with your concerns about maintainability when adding this extra API.
I think we could justify adapting the generic OQS sign/verify API to include the context strings. In my view context strings are not specific to the internals of the ML-KEM/SLH-DSA algorithms but were added as a good practice for signature schemes and allow domain separation between applications. I didn't do option 1) right away, indeed because of the API break and downstream code updates needed. However, at least oqs-provider needs to be updated anyways because composite-signatures will use context strings. So I'd go for option 1) for this issue. |
Thanks for going along with option 1, @bhess. The core logic update to That said, I'm now seeing another shortcoming of |
Good suggestion. I think it's worth doing as a separate PR so that we can get eyes focused specifically on that change and think about its implementation. |
@bhess In today's OQS status call we talked about a timeline for the liboqs 0.12.0 release. The main thing to be included is ML-DSA. The hope was to be able to make a release around two weeks from now. What timeline do you anticipate for landing this PR? |
For option 1, does "amend the existing sig API" mean to have two signature APIs always available, one without the context string (i.e., the current API) and one with the context string (i.e., a new API), or does it mean having only one signature API, but it being the one with the context string? If it means having only one signature API, being the one with the context string, then that will be a breaking change for all existing applications calling the OQS signature API, which seems problematic. Unless there's a way in C to allow optional arguments; I know there's |
That's my understanding.
I don't see this as problematic: Yes, it requires adaptations by users, but truly trivial ones for people who don't need the new feature -- and essential ones for people who want to use the standard algs. The big advantage I see is that this change minimizes support and maintenance effort for the OQS team, minimizes code changes in OQS PRs, minimizes library size. Also, it's a change supported by more mature versioning support. |
Taking a more critical/in-depth look at the API updates after today's meeting, here are my thoughts: Based on the fact that both SLH-DSA and ML-DSA share the same context string--supporting API, it seems likely (to me) that future NIST standards (e.g., for Falcon and potential selections from the signature on-ramp process) will follow this API. Does that seem like a correct assessment? If so, then I think it's desirable to have our main signature API ( Other notes:
|
Signed-off-by: Basil Hess <[email protected]>
4a59d11
to
0f22a1d
Compare
… the SIG API [full tests] Signed-off-by: Basil Hess <[email protected]>
0f22a1d
to
1b728aa
Compare
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
The API has now been updated to include a single sign/verify interface that incorporates the context string:
Note that the SIG_STFL API also needed to be updated, as the API signatures for sign/verify must be identical to the ones of SIG.
I believe the primary open issue is reaching consensus on the API. So far, the discussion seems to favor a unified sign/verify API, which has now been added. Beyond that, there are two smaller tasks to address: creating a tracker branch in the OQS provider and updating the constant-time tests. I should be able to handle these this week. |
Signed-off-by: Basil Hess <[email protected]>
I don't think it's quite that simple. We have two levels of public APIs -- the "object-oriented" API in the I am also worried that it is a substantial breaking change. I think it's especially problematic to change the behaviour of an existing function name. I'm a bit rusty on linking, but what happens when a binary that was compiled against the old sign API is dynamically linked against a new build of liboqs with the new sign API? Will linking fail? Or will it silently go through at link time and then fail at runtime? Or will it silently go through at link and runtime, but just use the wrong arguments somehow? If the latter, this seems very bad, as the 5th argument in the old sign API is the secret key, and the 5th argument in the new sign API is the context string, which suggests a risk that the secret key will now be used in an insecure way. My understanding of linking may be horribly wrong -- apologies -- but I would like to make sure these factors are considered. If the proposal was to rename the function name, that would at least be less risky, because things would clearly fail at any stage (compilation-time, link-time, runtime). Given that all of the public APIs are in portions of code created by code generation, I don't see it as particularly hard to generate the second API for the algorithms that support it. (Note that the code generation metadata will need to be updated to say which algorithms support it regardless of which approach we use, unless we change the function signature of all the underlying implementations as I mentioned as the last option two paragraphs above.) |
I whole-heartedly agree with this line of thought, @SWilson4.
Agreed again.
Sure -- or rather, actually, there's no particular plan required for this; one can simply use the OSSL_PARAM mechanism for this (via the existing "_ex" function variants); the SLH-DSA PR proposes "OSSL_SIGNATURE_PARAM_CONTEXT_STRING", for example. |
Well, @bhess just did it before you wrote your comment :)
Shouldn't an SOVERSION mismatch make this impossible in the case of shared libs (for static builds there's obviously no problem at all)? |
Adds ML-DSA from FIPS204
Pulls ML-DSA from pq-crystals/dilithium upstream: https://github.com/pq-crystals/dilithium
Removes ML-DSA-ipd alias in favor of ML-DSA.
TODOs:
NIST ACVP vectors against the external API (that will be exposed to the user), expected by October: ML-DSA: integrate final standard #1891 (comment)Postponed for ACVP availabilityPre-hash variants / Algorithm 4/5 in FIPS 204Postponed for ACVP availabilityFIPS 204 further defines a context parameter 'ctx' passed to the sign/verify API. This is currently not supported with the OQS API. It also defines
This PR makes only this default case available. Asking for feedback if we should adapt/add to the OQS Sign API to accomodate such a 'ctx' string, or if we restrict support to the default case.
Closes #1891