aws / aws-lc

AWS-LC is a general-purpose cryptographic library maintained by the AWS Cryptography team for AWS and their customers. It іs based on code from the Google BoringSSL project and the OpenSSL project.
Other
398 stars 118 forks source link

ML-KEM refactor #1763

Closed dkostic closed 2 months ago

dkostic commented 3 months ago

Issues:

CryptoAlg-2614

Description of changes:

Previous to this change, ML-KEM was implemented such that the code for a parameter set was selected by defining the correct C pre-processor flag (for example, if you want to compile the code for ML-KEM 512 parameter set you would #define KYBER_K 2). The consequence of this was that we had to compile the code three times for three ML-KEM parameter sets. We do this by adding a C file for each parameter set where we first define the corresponding KYBER_K value and then include all the ML-KEM C files. Besides being an awkward way to handle multiple parameter sets, this will not work for the FIPS module where we bundle all C files inside bcm.c and compile it as a single compilation unit.

In this change we refactor ML-KEM by parametrizing all functions that depend on values that are specific to a parameter set, i.e., that directly or indirectly depend on the value of KYBER_K. To do this, in params.h we define a structure that holds those ML-KEM parameters and functions that initialize a given structure with values corresponding to a parameter set. This structure can then be passed to every function that requires it. For example, polyvec_ntt function was previously implemented like this:

void polyvec_ntt(polyvec *r) {
  unsigned int i;
  for(i=0;i<KYBER_K;i++)
    poly_ntt(&r->vec[i]);
}

We now change that to:

void polyvec_ntt(ml_kem_params *params, polyvec *r) {
  unsigned int i;
  for(i=0;i<params->k;i++)
    poly_ntt(&r->vec[i]);
}

Of course, now we have to change all callers of polyvec_ntt to also have ml_kem_params as an input argument, and then callers of the caller, etc. These changes bubble up to the highest level API defined in kem.h where we now have:

int crypto_kem_keypair(ml_kem_params *params, ...);
int crypto_kem_enc(ml_kem_params * params, ...);
int crypto_kem_dec(ml_kem_params * params, ...);
int crypto_kem_keypair_derand(ml_kem_params *params, ...);
int crypto_kem_enc_derand(ml_kem_params * params, ...);

Then we can easily implement these functions for specific parameter sets, which can be seen in ml_kem.c file. For example:

int ml_kem_512_ipd_keypair(...) {
  ml_kem_params params;
  ml_kem_512_params_init(&params);
  return ml_kem_keypair_ref(&params, ...);
}
int ml_kem_768_ipd_keypair(...) {
  ml_kem_params params;
  ml_kem_768_params_init(&params);
  return ml_kem_keypair_ref(&params, ...);
}

Call-outs:

Point out areas that need special attention or support during the review process. Discuss architecture or design changes.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

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 commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.32%. Comparing base (79d5d16) to head (9c591df). Report is 18 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1763 +/- ## ========================================== - Coverage 78.44% 78.32% -0.13% ========================================== Files 580 581 +1 Lines 96780 97176 +396 Branches 13863 13930 +67 ========================================== + Hits 75921 76113 +192 - Misses 20243 20440 +197 - Partials 616 623 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.