ML-DSA parameter refactor by jakemas · Pull Request #1910 · aws/aws-lc

jakemas · 2024-10-08T18:58:49Z

Issues:

CryptoAlg-2724

Description of changes:

Parameterization of ML-DSA

Previous to this change, ML-DSA 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-DSA-65 parameter set you would #define DILITHIUM_MODE 3).

The consequence of this was that we had to compile the code three times for three ML-DSA parameter sets. We do this by adding a C file for each parameter set where we first define the corresponding DILITHIUM_MODE value and then include all the ML-DSA 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-DSA 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 DILITHIUM_MODE. To do this, in params.h we define a structure that holds those ML-DSA 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, polyvecl_add function was previously implemented as:

void polyvecl_add(polyvecl *w,
                  const polyvecl *u, 
                  const polyvecl *v) {
  unsigned int i;

  for(i = 0; i < L; ++i)
    poly_add(&w->vec[i], &u->vec[i], &v->vec[i]);
}

Is now changed to:

void polyvecl_add(ml_dsa_params *params,
                  polyvecl *w,
                  const polyvecl *u,
                  const polyvecl *v) {
  unsigned int i;

  for(i = 0; i < params->l; ++i)
    poly_add(&w->vec[i], &u->vec[i], &v->vec[i]);
}

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


int crypto_sign_keypair(ml_dsa_params *params, uint8_t *pk, uint8_t *sk);

int crypto_sign_signature(ml_dsa_params *params,
                          uint8_t *sig, size_t *siglen,
                          const uint8_t *m, size_t mlen,
                          const uint8_t *ctx, size_t ctxlen,
                          const uint8_t *sk);

int crypto_sign(ml_dsa_params *params,
                uint8_t *sm, size_t *smlen,
                const uint8_t *m, size_t mlen,
                const uint8_t *ctx, size_t ctxlen,
                const uint8_t *sk);

int crypto_sign_verify(ml_dsa_params *params,
                       const uint8_t *sig, size_t siglen,
                       const uint8_t *m, size_t mlen,
                       const uint8_t *ctx, size_t ctxlen,
                       const uint8_t *pk);

int crypto_sign_open(ml_dsa_params *params,
                     uint8_t *m, size_t *mlen,
                     const uint8_t *sm, size_t smlen,
                     const uint8_t *ctx, size_t ctxlen,
                     const uint8_t *pk);

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

int ml_dsa_65_keypair(uint8_t *public_key /* OUT */,
                       uint8_t *secret_key /* OUT */) {
  ml_dsa_params params;
  ml_dsa_65_params_init(&params);
  return crypto_sign_keypair(&params, public_key, secret_key);
}

int ml_dsa_65_sign(uint8_t *sig               /* OUT */,
                    size_t *sig_len            /* OUT */,
                    const uint8_t *message     /* IN */,
                    size_t message_len         /* IN */,
                    const uint8_t *ctx         /* IN */,
                    size_t ctx_len             /* IN */,
                    const uint8_t *secret_key  /* IN */) {
  ml_dsa_params params;
  ml_dsa_65_params_init(&params);
  return crypto_sign_signature(&params, sig, sig_len, message, message_len,
                                             ctx, ctx_len, secret_key);
}

int ml_dsa_65_verify(const uint8_t *message    /* IN */,
                      size_t message_len        /* IN */,
                      const uint8_t *sig        /* IN */,
                      size_t sig_len            /* IN */,
                      const uint8_t *ctx        /* IN */,
                      size_t ctx_len            /* IN */,
                      const uint8_t *public_key /* IN */) {
  ml_dsa_params params;
  ml_dsa_65_params_init(&params);
  return crypto_sign_verify(&params, sig, sig_len, message, message_len,
                                          ctx, ctx_len, public_key);
}

As such, files:

dilithium3r3_ref.c
api.h
config.h

are no longer required, and have been removed.

Other Changes

Also modified in this PR are the KAT test framework in p_dilithium_test.cc. This KAT framework has been modified to support multiple levels of ML-DSA (to be added in a later PR).

Call-outs:

See similar changes to ML-KEM in #1763

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.

…sa-refactor

codecov-commenter · 2024-10-08T19:31:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.50%. Comparing base (9ff8458) to head (ea859d1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1910      +/-   ##
==========================================
- Coverage   78.51%   78.50%   -0.02%     
==========================================
  Files         585      585              
  Lines       99681    99681              
  Branches    14271    14271              
==========================================
- Hits        78265    78253      -12     
- Misses      20782    20792      +10     
- Partials      634      636       +2

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

crypto/dilithium/pqcrystals_dilithium_ref_common/packing.h

crypto/dilithium/pqcrystals_dilithium_ref_common/params.c

crypto/dilithium/pqcrystals_dilithium_ref_common/packing.c

crypto/dilithium/pqcrystals_dilithium_ref_common/params.c

crypto/dilithium/sig_dilithium3.c

crypto/dilithium/pqcrystals_dilithium_ref_common/params.h

crypto/dilithium/p_dilithium_test.cc

crypto/dilithium/pqcrystals_dilithium_ref_common/params.c

crypto/dilithium/pqcrystals_dilithium_ref_common/rounding.c

torben-hansen · 2024-10-10T22:15:26Z

crypto/dilithium/p_dilithium_test.cc

-  md_ctx.Reset();
-}
+static const struct ML_DSA parameterSet[] = {
+  {"MLDSA67", NID_DILITHIUM3_R3, 1952, 4032, 3309,  "dilithium/kat/mldsa65.txt"},


Suggested change

{"MLDSA67", NID_DILITHIUM3_R3, 1952, 4032, 3309, "dilithium/kat/mldsa65.txt"},

{"MLDSA65", NID_DILITHIUM3_R3, 1952, 4032, 3309, "dilithium/kat/mldsa65.txt"},

Thanks, fixed in ea859d1.

torben-hansen · 2024-10-10T22:17:55Z

crypto/dilithium/p_dilithium_test.cc

+    // constant DILITHIUM3_SIGNATURE_BYTES.
+
+    std::vector<uint8_t> signature(sig_len);
+    //uint8_t signature[sig_len];


Thanks, fixed in ea859d1.

torben-hansen · 2024-10-10T22:21:42Z

crypto/dilithium/p_dilithium_test.cc

+
+    EXPECT_TRUE(EVP_PKEY_keygen_init(dilithium_pkey_ctx));
+    EXPECT_TRUE(EVP_PKEY_keygen(dilithium_pkey_ctx, &dilithium_pkey));
+    const DILITHIUM3_KEY *dilithium3Key = (DILITHIUM3_KEY *)(dilithium_pkey->pkey.ptr); //called dilithium3 but is generic


Renaming everything to ML-DSA lingo at some point?

Yes, will be switching over to ML-DSA lingo in subsequent PRs (including real OIDs). It makes more sense to do this when I add the other 2 parameter sets for ML-DSA, to keep this PR focused on just one addition.

torben-hansen · 2024-10-10T22:31:26Z

crypto/dilithium/p_dilithium_test.cc

+    EVP_PKEY *dilithium_pkey = EVP_PKEY_new();
+    ASSERT_NE(dilithium_pkey, nullptr);
+
+    EXPECT_TRUE(EVP_PKEY_keygen_init(dilithium_pkey_ctx));


This EXPECT_TRUE and line below should prob be ASSERT_TRUE. Any failure is fatal, but EXPECT_TRUE doesn't cancel.

Thanks, fixed in ea859d1.

torben-hansen · 2024-10-10T22:32:41Z

crypto/dilithium/pqcrystals_dilithium_ref_common/README.md

 The source code was imported from a branch of the official repository of the Crystals-Dilithium team: https://github.com/pq-crystals/dilithium. The code was taken at [commit](https://github.com/pq-crystals/dilithium/commit/cbcd8753a43402885c90343cd6335fb54712cda1) as of 10/01/2024. At the moment, only the reference C implementation is imported.

-The `api.h`, `fips202.h` and `params.h` header files were modified to support our [prefixed symbols build](https://github.com/awslabs/aws-lc/blob/main/BUILDING.md#building-with-prefixed-symbols).
+The code was refactored in [this PR](https://github.com/aws/aws-lc/pull/1910) 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 `DILITHIUM_MODE`. To do this, in `params.h` we defined a structure that holds those ML-DSA parameters and functions


Suggested change

The code was refactored in [this PR](https://github.com/aws/aws-lc/pull/1910) 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 `DILITHIUM_MODE`. To do this, in `params.h` we defined a structure that holds those ML-DSA parameters and functions

The code was refactored in [this PR](https://github.com/aws/aws-lc/pull/1910) by parameterizing all functions that depend on values that are specific to a parameter set, i.e., that directly or indirectly depend on the value of `DILITHIUM_MODE`. To do this, in `params.h` we defined a structure that holds those ML-DSA parameters and functions

I think...

Thanks, fixed in ea859d1.

torben-hansen · 2024-10-10T22:42:48Z

crypto/dilithium/pqcrystals_dilithium_ref_common/packing.c

-  sk += K*POLYETA_PACKEDBYTES;
+  for(i=0; i < params->k; ++i)
+    polyeta_unpack(params, &s2->vec[i], sk + i * params->poly_eta_packed_bytes);
+  sk += params->k*params->poly_eta_packed_bytes;


Suggested change

sk += params->k*params->poly_eta_packed_bytes;

sk += params->k * params->poly_eta_packed_bytes;

Thanks, fixed in ea859d1.

torben-hansen · 2024-10-10T22:42:59Z

crypto/dilithium/pqcrystals_dilithium_ref_common/packing.c

-  sk += L*POLYETA_PACKEDBYTES;
+  for(i=0; i < params->l; ++i)
+    polyeta_unpack(params, &s1->vec[i], sk + i * params->poly_eta_packed_bytes);
+  sk += params->l*params->poly_eta_packed_bytes;


Suggested change

sk += params->l*params->poly_eta_packed_bytes;

sk += params->l * params->poly_eta_packed_bytes;

Thanks, fixed in ea859d1.

torben-hansen · 2024-10-10T22:43:28Z

crypto/dilithium/pqcrystals_dilithium_ref_common/packing.c

-  sig += L*POLYZ_PACKEDBYTES;
+  for(i = 0; i < params->l; ++i)
+    polyz_pack(params, sig + i * params->poly_z_packed_bytes, &z->vec[i]);
+  sig += params->l*params->poly_z_packed_bytes;


Suggested change

sig += params->l*params->poly_z_packed_bytes;

sig += params->l * params->poly_z_packed_bytes;

Thanks, fixed in ea859d1.

torben-hansen · 2024-10-10T22:43:53Z

crypto/dilithium/pqcrystals_dilithium_ref_common/packing.c

-  sig += L*POLYZ_PACKEDBYTES;
+  for(i = 0; i < params->l; ++i)
+    polyz_unpack(params, &z->vec[i], sig + i * params->poly_z_packed_bytes);
+  sig += params->l*params->poly_z_packed_bytes;


Suggested change

sig += params->l*params->poly_z_packed_bytes;

sig += params->l * params->poly_z_packed_bytes;

Thanks, fixed in ea859d1.

crypto/dilithium/pqcrystals_dilithium_ref_common/rounding.c

ml-dsa parameter refactor

3281adf

jakemas requested a review from a team as a code owner October 8, 2024 18:58

jakemas added 3 commits October 8, 2024 11:59

Merge branch 'main' into ml-dsa-refactor

68154aa

updated readme with changes

d16c2b4

Merge branch 'ml-dsa-refactor' of github.com:jakemas/aws-lc into ml-d…

f4da396

…sa-refactor

andrewhop reviewed Oct 8, 2024

View reviewed changes

jakemas added 5 commits October 8, 2024 14:10

removed all DILITHIUM_NAMESPACE-ing

64cc605

CR fixes

1d896b1

refact params.c and add gen-src

c37f75b

more documentation and typo fix

c7e58db

Merge branch 'main' into ml-dsa-refactor

77ec0e6

torben-hansen reviewed Oct 10, 2024

View reviewed changes

CR fixes, spaces, typos

ea859d1

dkostic approved these changes Oct 11, 2024

View reviewed changes

andrewhop approved these changes Oct 11, 2024

View reviewed changes

andrewhop merged commit a748793 into aws:main Oct 11, 2024

	{"MLDSA67", NID_DILITHIUM3_R3, 1952, 4032, 3309, "dilithium/kat/mldsa65.txt"},
	{"MLDSA65", NID_DILITHIUM3_R3, 1952, 4032, 3309, "dilithium/kat/mldsa65.txt"},

	The code was refactored in [this PR](https://github.com/aws/aws-lc/pull/1910) 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 `DILITHIUM_MODE`. To do this, in `params.h` we defined a structure that holds those ML-DSA parameters and functions
	The code was refactored in [this PR](https://github.com/aws/aws-lc/pull/1910) by parameterizing all functions that depend on values that are specific to a parameter set, i.e., that directly or indirectly depend on the value of `DILITHIUM_MODE`. To do this, in `params.h` we defined a structure that holds those ML-DSA parameters and functions

	sk += params->k*params->poly_eta_packed_bytes;
	sk += params->k * params->poly_eta_packed_bytes;

	sk += params->l*params->poly_eta_packed_bytes;
	sk += params->l * params->poly_eta_packed_bytes;

	sig += params->l*params->poly_z_packed_bytes;
	sig += params->l * params->poly_z_packed_bytes;

Conversation

jakemas commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues:

Description of changes:

Parameterization of ML-DSA

Other Changes

Call-outs:

Uh oh!

codecov-commenter commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jakemas commented Oct 8, 2024 •

edited

Loading

codecov-commenter commented Oct 8, 2024 •

edited

Loading