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

Remove ENABLE_DILITHIUM flag #2070

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jakemas
Copy link
Contributor

@jakemas jakemas commented Dec 20, 2024

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 an experimental flag (based on the deprecated flag) to indicate the external APIs may still be subject to change as the standards space develops.
  • Renamed EVP_PKEY_pqdsa_new_raw_private_key to EVP_PKEY_pqdsa_new_raw_secret_key to better match the equivalent API for KEMs (EVP_PKEY_kem_new_raw_secret_key).

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.

Once we are happy with the name, including the decision around PQDSA, then we can remove the experimental flags. Personally, I'd advocate for NISTDSA as all NIST signature schemes are matching these API conventions (sign, pre-hash-sign, context-sign -- see ed25519, ML-DSA, SLH-DSA, FN-DSA as examples). Most of this naming is internal, with the exception of the three external functions: EVP_PKEY_CTX_pqdsa_set_params, EVP_PKEY_pqdsa_new_raw_secret_key, EVP_PKEY_pqdsa_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.

@jakemas jakemas requested a review from a team as a code owner December 20, 2024 01:02
@jakemas
Copy link
Contributor Author

jakemas commented Dec 20, 2024

Alternative option is that we place the three APIs in a new file within https://github.com/aws/aws-lc/tree/main/include/openssl/experimental. Open to thoughts from the team.

@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 98.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.92%. Comparing base (f45bc34) to head (4c6b994).

Files with missing lines Patch % Lines
...o/dilithium/pqcrystals_dilithium_ref_common/poly.c 96.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2070      +/-   ##
==========================================
+ Coverage   78.76%   78.92%   +0.15%     
==========================================
  Files         598      610      +12     
  Lines      103747   105266    +1519     
  Branches    14735    14921     +186     
==========================================
+ Hits        81718    83076    +1358     
- Misses      21375    21539     +164     
+ Partials      654      651       -3     

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

@jakemas
Copy link
Contributor Author

jakemas commented Dec 20, 2024

will merge #2072 first to split this PR into smaller sized additions

@jakemas
Copy link
Contributor Author

jakemas commented Dec 20, 2024

Seems to be multiple definitions of crypto_sign_keypair and crypto_sign_open when integration testing against libmariadb who seem to use the same names for their ed25519 implementation. Addressed in #2072

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.

2 participants