Remove ENABLE_DILITHIUM flag

[jakemas]

Issues:

It's time. (also we need to do this to add ML-DSA to the FIPS module)

Description of changes:

• Removed the enable_dilithium flag
• Added a description to APIs to indicate the external APIs may still be subject to change as the standards space develops. (We don't envision them changing, but they are extremely new).

Call-outs:

Removing the flag has little consequence -- other than it makes the APIs we expose in include/openssl/evp.h that much more "final". We should consider how much we like them before we commit to them. We made a point to refer to asymmetric keypairs as public and private keys, rather than public and secret keys. However, we haven't always been consistant with this, so there is a mix of both in the library. Users will find the consistency between EVP_PKEY_pqdsa_new_raw_secret_key and EVP_PKEY_kem_new_raw_secret_key more satisfying.

However, after internal discussions, we'd also like to change the name of EVP_PKEY_kem_new_raw_public_key and EVP_PKEY_kem_new_raw_secret_key to EVP_PKEY_kem_new_raw_encapsulation_key and EVP_PKEY_kem_new_raw_decapsulation_key to match the names within FIPS 203. Thus, we should stick with secret.

We did consider the use of an experimental flag (by using the OPENSSL_DEPRECATED alias), but, this seems a little over the top. If there are any discrepancy with the API down the line, we will make a change as we are proposing with the kem APIs above. Alternatively, we could place the EVP APIs in include/openssl/experimental/. The arguments of EVP_PKEY_pqdsa_new_raw_secret_key, EVP_PKEY_pqdsa_new_raw_public_key, and EVP_PKEY_CTX_pqdsa_set_params are stable and consistant with KEM and EC variants (see EVP_PKEY_CTX_kem_set_params, EVP_PKEY_CTX_set_dh_paramgen_prime_len, and EVP_PKEY_CTX_set_ec_param_enc, EVP_PKEY_new_raw_private_key, EVP_PKEY_new_raw_public_key, EVP_PKEY_kem_new_raw_public_key).

Testing:

To celebrate the removal of this flag, enjoy this haiku:

feature flag removed,
post-quantum strength stands alone,
simpler code prevails.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.94%. Comparing base (4243a79) to head (3c9fde8).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2082      +/-   ##
==========================================
+ Coverage   78.75%   78.94%   +0.18%     
==========================================
  Files         598      610      +12     
  Lines      103650   105189    +1539     
  Branches    14720    14906     +186     
==========================================
+ Hits        81633    83040    +1407     
- Misses      21365    21497     +132     
  Partials      652      652              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jakemas]

sign_message and verify_message variants are never used (we use sign/verify without the appended message on the end, so that signature sizes are constant)-- should we just remove them from the library?

[WillChilds-Klein]

I usually take the position that if

1. a function is never used and
2. it is not exposed as part of our public API

we should remove it.

[jakemas]

sign_message and verify_message variants are never used (we use sign/verify without the appended message on the end, so that signature sizes are constant)-- should we just remove them from the library?

[WillChilds-Klein]

Holding off on full review until PR 2072, which this PR holds the contents of, has been merged.

However, we haven't always been consistant with this, so there is a mix of both in the library. Users will find the consistency between EVP_PKEY_pqdsa_new_raw_secret_key and EVP_PKEY_kem_new_raw_secret_key more satisfying.

I associate "private key" with asymmetric keys and "secret key" with symmetric keys for HMAC, AES, etc. We have portions of the library that refer to asymmetric private keys as "secret keys"?

---

Why do we need new EVP APIs for ML-DSA? What inputs/outputs differ from other signature schemes supported by EVP_DigestSign?

[jakemas]

Holding off on full review until PR 2072, which this PR holds the contents of, has been merged.

However, we haven't always been consistant with this, so there is a mix of both in the library. Users will find the consistency between EVP_PKEY_pqdsa_new_raw_secret_key and EVP_PKEY_kem_new_raw_secret_key more satisfying.

I associate "private key" with asymmetric keys and "secret key" with symmetric keys for HMAC, AES, etc. We have portions of the library that refer to asymmetric private keys as "secret keys"?

Yes, see EVP_PKEY_kem_new_raw_secret_key.

Why do we need new EVP APIs for ML-DSA? What inputs/outputs differ from other signature schemes supported by EVP_DigestSign?

We need them for the PKEY and ASN.1 functions, not for the DigestSign part. The APIs are:

// EVP_PKEY_CTX_pqdsa_set_params sets in |ctx| the parameters associated with
// the signature scheme defined by the given |nid|. It returns one on success
// and zero on error. This API is marked as EXPERIMENTAL (using the deprecated
// warning) to indicate that this API may change as the standards around the
// signature schemes finalize.
OPENSSL_EXPORT int EVP_PKEY_CTX_pqdsa_set_params(EVP_PKEY_CTX *ctx, int nid);

// EVP_PKEY_pqdsa_new_raw_public_key generates a new EVP_PKEY object of type
// EVP_PKEY_PQDSA, initializes the PQDSA key based on |nid| and populates the
// public key part of the PQDSA key with the contents of |in|. It returns the
// pointer to the allocated PKEY on sucess and NULL on error. This API is marked
// as EXPERIMENTAL to indicate that this API may change as the standards around
// the signature schemes finalize.
OPENSSL_EXPORT EVP_PKEY *EVP_PKEY_pqdsa_new_raw_public_key(int nid, const uint8_t *in, size_t len);

// EVP_PKEY_pqdsa_new_raw_private_key generates a new EVP_PKEY object of type
// EVP_PKEY_PQDSA, initializes the PQDSA key based on |nid| and populates the
// secret key part of the PQDSA key with the contents of |in|. It returns the
  // pointer to the allocated PKEY on sucess and NULL on error. This API is marked
// as EXPERIMENTAL to indicate that this API may change as the standards around
// the signature schemes finalize.
OPENSSL_EXPORT EVP_PKEY *EVP_PKEY_pqdsa_new_raw_private_key(int nid, const uint8_t *in, size_t len);

These are analog to the KEM, DH, and EC PKEY utility function APIs, for example:

• EVP_PKEY_CTX_kem_set_params
• EVP_PKEY_CTX_set_dh_paramgen_prime_len
• EVP_PKEY_CTX_set_ec_param_enc
• EVP_PKEY_kem_new_raw_public_key
• EVP_PKEY_kem_new_raw_secret_key

We need these so we don't have to have distinct ASN.1 functions for every single ML-DSA parameter set, instead, we put all signature schemes (that support the NIST API framework, so all the PQ ones) into the PQDSA PKEY struct and use these utility functions to set a specific NID.

[WillChilds-Klein]

Thanks for the explanation @jakemas, all makes sense. The only (mild) concern I still have is whether the "pqdsa" naming might be too general here. Are we using "pqdsa" instead of "mldsa" because we plan to re-use these functions for SLH-DSA?

If so, with the current signature of EVP_PKEY_CTX_pqdsa_set_params, we'd need a distinct SLH-DSA NID for each of the 12 parameter sets listed in Section 11 of FIPS 205. This doable, but a bit unwieldy.

It might be better to have a distinct EVP_PKEY_CTX_slhdsa_set_params that takes in relevant hash NIDs, in which case we'd probably want to rename EVP_PKEY_CTX_pqdsa_set_params to EVP_PKEY_CTX_mldsa_set_params to disambiguate. Alternatively, we could add the 2 additional hash NIDs as parameters to EVP_PKEY_CTX_pqdsa_set_params and instruct callers to specify NID_undef there for ML-DSA.

Lastly, what are the implications of XMSS for this API? Is XMSS considered part of NIST's "DSA" family of signature algorithms?

[jakemas]

Thanks for the explanation @jakemas, all makes sense. The only (mild) concern I still have is whether the "pqdsa" naming might be too general here. Are we using "pqdsa" instead of "mldsa" because we plan to re-use these functions for SLH-DSA?

Yes, the idea is to support any signature scheme without having to add an ASN.1 functionality for each algorithm specific parameter set. This would include ML-DSA, HashML-DSA, ExtMuML-DSA, FN-DSA, HashFN-DSA, SLH-DSA, HashSLH-DSA, even ED25519 would fit in the API. I wanted to call it NISTDSA, because DSA is taken, and the API format is not PQ specific, it is NIST specific -- but I got push back in review on that name and have changed it to please the reviewers already. This isn't a one way decision, since we are still now considering changing the name of the analogous KEM API years later after it's first release -- so I'd rather not flip flop back over this decision too many times. Once were through validation we can comb back over any naming decisions that may require lengthy discussion.

If so, with the current signature of EVP_PKEY_CTX_pqdsa_set_params, we'd need a distinct SLH-DSA NID for each of the 12 parameter sets listed in Section 11 of FIPS 205. This doable, but a bit unwieldy.

I don't understand how this would be considered unwieldy, the PQDSA API supports this very simply. Demo:

// Generate a key-pair for the desired PQDSA algorithm.
int generate_key_pair(/* OUT */ EVP_PKEY **key,
                      /* IN  */ int pqdsa_nid) {

    EVP_PKEY_CTX *ctx = NULL;

    // Create the PQDSA contex.
    ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_PQDSA, NULL);
    if (!ctx ||
        !EVP_PKEY_CTX_pqdsa_set_params(ctx, pqdsa_nid) ||
        !EVP_PKEY_keygen_init(ctx) ||
        !EVP_PKEY_keygen(ctx, key)) {
        EVP_PKEY_free(*key);
        EVP_PKEY_CTX_free(ctx);
        return 0;
        }
    // Note: a single context can be used to generate many keys.
    EVP_PKEY_CTX_free(ctx);
    return 1;
}

You just set the particular algorithm parameter set using EVP_PKEY_CTX_pqdsa_set_params(ctx, pqdsa_nid).

Your proposal for EVP_PKEY_CTX_slhdsa_set_params would work the exact same way? We'd still have to select the particular parameter set from the OID. The down side would be that we'd now have more EVP APIs, one for ML-DSA and one for SLH-DSA, and then presumably more for each other algorithm? Why would we want that over a single algorithm setter function?

I don't foresee a single API being a problem, no matter how many parameter sets SLH-DSA has, we don't have to support them all. We'd likely choose a sane sub-set for our particular application. This is exactly how we plan to support more KEMs in the KEM API should we ever plan to add more. A change here suggests that this is a larger decision that would include the KEM API naming conventions.

This is all documented in the PR were we added the PQDSA API #1963. I'll be adding a demo readme once functionality is complete.

I could see an argument for replacing all mentions of PQDSA with MLDSA and have it be the ML-DSA API, until we come to a point in the future in which we want to add another algorithm, then we can either rename it then to a generic name -- or create a new XDSA version for the new algorithm, as you suggest.

It might be better to have a distinct EVP_PKEY_CTX_slhdsa_set_params that takes in relevant hash NIDs, in which case we'd probably want to rename EVP_PKEY_CTX_pqdsa_set_params to EVP_PKEY_CTX_mldsa_set_params to disambiguate. Alternatively, we could add the 2 additional hash NIDs as parameters to EVP_PKEY_CTX_pqdsa_set_params and instruct callers to specify NID_undef there for ML-DSA.

Why are you talking about hash NIDs here? Are you talking about pre-hash modes here? This has nothing to do with signing yet still, we're talking about EVP PKEY stuff. The pre-hash NIDs come in through the signing context.

Lastly, what are the implications of XMSS for this API? Is XMSS considered part of NIST's "DSA" family of signature algorithms?

No plans to implement XMSS, stateful signatures are a different use-case to the general-purpose signature schemes we are discussing here. No need to consider its support.

[jakemas]

It looks like the same issue we saw in #1332 is happening in this PR too. Perhaps the removal of the flag within bcm.c caused the offset to change and is now out of range?

bcm.c:7987: Error: pc-relative address offset out of range
bcm.c:7989: Error: pc-relative address offset out of range
bcm.c:7991: Error: pc-relative address offset out of range

[WillChilds-Klein]

I wanted to call it NISTDSA, because DSA is taken, and the API format is not PQ specific, it is NIST specific -- but I got push back in review on that name and have changed it to please the reviewers already.

If there's pre-existing consensus on this naming convention, then please disregard my concern. I don't want to reconsider a decision that's already been made.

My goal here is to make sure we've discussed alternative interfaces because by removing this flag, as in doing so we're committing to a public API that will require breaking changes to modify.

I don't understand how this would be considered unwieldy

I wasn't specific enough here. Agreed that the current interface is easy for the caller to use. I meant that it would be "unwieldy" for us to add so many NIDs (one for each of 12 parameter sets) when the hash NID could be specified independently in EVP_PKEY_CTX_pqdsa_set_params. I didn't realize supporting a subset of parameter sets was on the table.

And of course, adding a few extra NIDs is not a significant cost.

Your proposal for EVP_PKEY_CTX_slhdsa_set_params would work the exact same way? We'd still have to select the particular parameter set from the OID. The down side would be that we'd now have more EVP APIs, one for ML-DSA and one for SLH-DSA, and then presumably more for each other algorithm? Why would we want that over a single algorithm setter function?

Not suggesting we move forward with this, but here's a sketch of the interface I was thinking of:

int EVP_PKEY_CTX_mldsa_set_params(EVP_PKEY_CTX ctx, int mldsa_nid);
int EVP_PKEY_CTX_slhdsa_set_params(EVP_PKEY_CTX ctx, int hash_nid);

Caller would specify which ML-DSA security level they want via mldsa_nid or SLH-DSA parameter set they want via hash_nid. But this is awkward (no dedicated SLH-DSA NIDs?) and inconsistent with the KEM API, with no discernible benefit other than reducing number of NIDs for us to provide -- a marginal benefit at best.

I agree that a single API with per-algorithm, per-parameter-set NIDs is preferable.

I could see an argument for replacing all mentions of PQDSA with MLDSA and have it be the ML-DSA API, until we come to a point in the future in which we want to add another algorithm, then we can either rename it then to a generic name

I don't mean to suggest that you're advocating for this, but IMO we should avoid future renames at ~all costs. They're expensive in places where we have downstream telemetry (internal consumers), and very very difficult to do safely where we don't have downstream telemetry (external consumers).

I'm convinced that the current interface is the right one, and likely won't require future revisions.

[WillChilds-Klein]

can we remove these? they're not used anywhere.

[jakemas]

removed in 3c9fde8

[andrewhop]

The change looks good, but you'll need to fix the FIPS delaactor issue for ubuntu2204_gcc11x_aarch_fips.

[andrewhop]

Functions not actually marked as experimental/deprecated.

[WillChilds-Klein]

What's the right mechanism for this? OPENSSL_DEPRECATED? Or should we create a similar macro OPENSSL_EXPERIMENTAL (out of scope of this PR)?