Skip to content
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

Draft
wants to merge 36 commits into
base: main
Choose a base branch
from
Draft

Add ML-DSA / FIPS 204 final #1919

wants to merge 36 commits into from

Conversation

bhess
Copy link
Member

@bhess bhess commented Sep 11, 2024

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:

FIPS 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

By default, the context is the empty string, though applications may specify the use of a non-empty context string.

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.

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

Closes #1891

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]>
@bhess
Copy link
Member Author

bhess commented Nov 5, 2024

A progress update:

  1. The "internal" API was integrated in PQ-Crystals upstream last Friday: pq-crystals/dilithium@444cdcc. These are algorithms 7&8 in FIPS 204.
  2. This PR makes the internal API available for the ACVP tests. Tests are integrated and are passing.
  3. Added a new API for signing and verifying that allows to pass an optional "context" string as specified in FIPS 204 (Algorithms 2&3).

The context parameter is needed by some use cases like for composite signatures.
For this purpose this PR introduces two APIs:

  • liboqs/src/sig/sig.h

    Lines 307 to 324 in 444bccf

    /**
    * Signature generation algorithm, with custom context string.
    *
    * Caller is responsible for allocating sufficient memory for `signature`,
    * based on the `length_*` members in this object or the per-scheme
    * compile-time macros `OQS_SIG_*_length_*`.
    *
    * @param[in] sig The OQS_SIG object representing the signature scheme.
    * @param[out] signature The signature on the message represented as a byte string.
    * @param[out] signature_len The actual length of the signature. May be smaller than `length_signature` for some algorithms since some algorithms have variable length signatures.
    * @param[in] message The message to sign represented as a byte string.
    * @param[in] message_len The length of the message to sign.
    * @param[in] ctx_str The context string for the signature.
    * @param[in] ctx_str_len The length of the context string.
    * @param[in] secret_key The secret key represented as a byte string.
    * @return OQS_SUCCESS or OQS_ERROR
    */
    OQS_API OQS_STATUS OQS_SIG_sign_with_ctx_str(const OQS_SIG *sig, uint8_t *signature, size_t *signature_len, const uint8_t *message, size_t message_len, const uint8_t *ctx_str, size_t ctx_str_len, const uint8_t *secret_key);
  • liboqs/src/sig/sig.h

    Lines 339 to 352 in 444bccf

    /**
    * Signature verification algorithm, with custom context string.
    *
    * @param[in] sig The OQS_SIG object representing the signature scheme.
    * @param[in] message The message represented as a byte string.
    * @param[in] message_len The length of the message.
    * @param[in] signature The signature on the message represented as a byte string.
    * @param[in] signature_len The length of the signature.
    * @param[in] ctx_str The context string for the signature.
    * @param[in] ctx_str_len The length of the context string.
    * @param[in] public_key The public key represented as a byte string.
    * @return OQS_SUCCESS or OQS_ERROR
    */
    OQS_API OQS_STATUS OQS_SIG_verify_with_ctx_str(const OQS_SIG *sig, const uint8_t *message, size_t message_len, const uint8_t *signature, size_t signature_len, const uint8_t *ctx_str, size_t ctx_str_len, const uint8_t *public_key);

    The API will also be needed for FIPS 205, formerly SPHINCS+.
    For algorithms that don't support context strings, the API returns an error code.
    The PR also leaves the existing API without context string to (1) avoid breaking the API, (2) provide an easy way to support the empty context string, which is the default in FIPS 204, (3) support other algorithms that don't have context strings (i.e., the official API for NIST-PQC submissions doesn't have a context string).
    Seeking feedback from @open-quantum-safe/liboqs-committers and @johngray-dev especially on the new API and if it fulfils our needs.

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.

@SWilson4
Copy link
Member

SWilson4 commented Nov 6, 2024

The API definition looks good to me. I do wonder if instead of setting sig->sign_with_ctx_str to NULL we ought to set it to a default function that returns an error. That would ensure users don't get a segfault if they call sig->sign_with_ctx_str directly instead of via the OQS_SIG_ function.

@baentsch
Copy link
Member

baentsch commented Nov 9, 2024

This PR represents a break with what liboqs aimed to be, namely a generic collection of PQ algorithms, by adding an API only of use for some algorithms (1 right now, possibly 2 if/as someone introduces & adds this also to SLH-DSA).

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 liboqs ever larger and less efficient. It also makes it less and less likely that code reviews are done diligently (I bet no-one will spend time in review to vet all 243 files changed are OK).

In sum, this PR

  • invalidates the concept of having one API equally supported for all PQ algs
  • makes the library harder to maintain and more fragile
  • reduces the likelihood that code gets reviewed
  • makes the library even larger/less efficient

I see 2 alternatives to counter these problems:

  1. amend the existing sig APIs to also take a context string (returning OQS_ERROR if not supported)
  2. make this new API compile-time selectable, eg., when building for FIPS compliance

Option 1) would retain the nature of liboqs as a generic PQ library, reduce the amount of boilerplate code substantially and avoid code growth but necessitate downstream code changes. Option 2) would be an alternative approach beginning to technically tackle the internal split of the library into a "productive use/standards track" and "experimental/research track" code base allowing downstream use(rs) to properly differentiate between pretty much best-effort/unmaintained research code and (hopefully, also at the upstreams) more well-maintained standards-track code.

@bhess
Copy link
Member Author

bhess commented Nov 11, 2024

Thanks for your feedback @baentsch, I agree with your concerns about maintainability when adding this extra API.

Option 1) would retain the nature of liboqs as a generic PQ library, reduce the amount of boilerplate code substantially and avoid code growth but necessitate downstream code changes. Option 2) would be an alternative approach beginning to technically tackle the internal split of the library into a "productive use/standards track" and "experimental/research track" code base allowing downstream use(rs) to properly differentiate between pretty much best-effort/unmaintained research code and (hopefully, also at the upstreams) more well-maintained standards-track code.

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.

@baentsch
Copy link
Member

Thanks for going along with option 1, @bhess. The core logic update to oqsprovider indeed is trivial (composite notwithstanding).

That said, I'm now seeing another shortcoming of liboqs: AFAIK there is no simple library version define that a downstream could utilize to adapt to API changes in liboqs: The only define there is is "OQS_VERSION_TEXT" and I'm not sure that's making it sufficiently easy to adapt a downstream's code to liboqs capabilities: The goal of (and one purpose for) oqsprovider is to build against all versions of liboqs, so a simple-to-use code-level library version test facility should be there, e.g. similar to what we have in openssl. Worthwhile a separate issue/PR or simple enough to do as part of this PR? If the latter, the corresponding oqsprovider update could be done as a "-tracker" update as this is an API-changing PR (either way).

@dstebila
Copy link
Member

That said, I'm now seeing another shortcoming of liboqs: AFAIK there is no simple library version define that a downstream could utilize to adapt to API changes in liboqs: The only define there is is "OQS_VERSION_TEXT" and I'm not sure that's making it sufficiently easy to adapt a downstream's code to liboqs capabilities: The goal of (and one purpose for) oqsprovider is to build against all versions of liboqs, so a simple-to-use code-level library version test facility should be there, e.g. similar to what we have in openssl. Worthwhile a separate issue/PR or simple enough to do as part of this PR? If the latter, the corresponding oqsprovider update could be done as a "-tracker" update as this is an API-changing PR (either way).

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.

@dstebila
Copy link
Member

@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?

@dstebila
Copy link
Member

I see 2 alternatives to counter these problems:

  1. amend the existing sig APIs to also take a context string (returning OQS_ERROR if not supported)
  2. make this new API compile-time selectable, eg., when building for FIPS compliance

Option 1) would retain the nature of liboqs as a generic PQ library, reduce the amount of boilerplate code substantially and avoid code growth but necessitate downstream code changes. Option 2) would be an alternative approach beginning to technically tackle the internal split of the library into a "productive use/standards track" and "experimental/research track" code base allowing downstream use(rs) to properly differentiate between pretty much best-effort/unmaintained research code and (hopefully, also at the upstreams) more well-maintained standards-track code.

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 va_list and va_arg in C, but I'm a bit rusty on how those work.

@baentsch
Copy link
Member

does it mean having only one signature API, but it being the one with the context string

That's my understanding.

that will be a breaking change for all existing applications calling the OQS signature API, which seems problematic

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.

@SWilson4
Copy link
Member

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 (OQS_SIG_sign) conform to the FIPS documents. In order to accomplish that, breaking backwards compatibility will be required at some point. There will never be a good time to do this, but if it must be done, then it would be best if it were done quickly (i.e., now). Users have a strong incentive to upgrade to version 0.12.0 (and hence adopt the new API). It also seems to me that many (maybe most?) users of liboqs don't actually use the API directly but instead access it via oqs-provider, in which case they will see no change at all. (Please correct me if I'm wrong on this point, @baentsch.)

Other notes:

  • I think it would be good to get input from @Martyrshot here as a downstream consumer of the "un-providered" liboqs API.
  • I imagine we would have to update all of the language wrappers (at least the ones we wish to support). For some of them (e.g., Java, C++, Python) we might be able to take advantage of overloading and optional/default arguments.
  • Are you aware of a plan for OpenSSL to handle this @baentsch? It's not immediately clear to me how one would support the context string using the existing EVP_DigestSign API.

Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
@bhess
Copy link
Member Author

bhess commented Nov 12, 2024

The API has now been updated to include a single sign/verify interface that incorporates the context string:

OQS_API OQS_STATUS OQS_SIG_sign(const OQS_SIG *sig, uint8_t *signature, size_t *signature_len, const uint8_t *message, size_t message_len, const uint8_t *ctx_str, size_t ctx_str_len, const uint8_t *secret_key);
OQS_API OQS_STATUS OQS_SIG_verify(const OQS_SIG *sig, const uint8_t *message, size_t message_len, const uint8_t *signature, size_t signature_len, const uint8_t *ctx_str, size_t ctx_str_len, const uint8_t *public_key);

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.

@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?

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]>
@dstebila
Copy link
Member

that will be a breaking change for all existing applications calling the OQS signature API, which seems problematic

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.

I don't think it's quite that simple. We have two levels of public APIs -- the "object-oriented" API in the OQS_SIG struct (OQS_SIG->sign) and then per-algorithm public API (e.g., OQS_SIG_ml_dsa_44_ipd_sign). We would need to make changes for both of these APIs across all algorithms. Now admittedly all of this is done via code generation, but it's still non-trivial; and the code generation will have to know whether the underlying implementation it's calling takes a context argument or not, unless the plan is to also change the function signatures of all the underlying implementations, which I think is a non-starter because then we're touching dozens of implementations, many of them in upstreams.

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.)

@baentsch
Copy link
Member

it's desirable to have our main signature API (OQS_SIG_sign) conform to the FIPS documents. In order to accomplish that, breaking backwards compatibility will be required at some point. There will never be a good time to do this, but if it must be done, then it would be best if it were done quickly (i.e., now).

I whole-heartedly agree with this line of thought, @SWilson4.

in which case they will see no change at all. (Please correct me if I'm wrong on this point, @baentsch.)

Agreed again.

Are you aware of a plan for OpenSSL to handle this @baentsch? It's not immediately clear to me how one would support the context string using the existing EVP_DigestSign API.

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.

@baentsch
Copy link
Member

but it's still non-trivial

Well, @bhess just did it before you wrote your comment :)

Or will it silently go through at link and runtime, but just use the wrong arguments somehow?

Shouldn't an SOVERSION mismatch make this impossible in the case of shared libs (for static builds there's obviously no problem at all)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ML-DSA: integrate final standard
4 participants