feature: add support for configuring (but not yet using) ml-dsa certs #5263

lrstewart · 2025-04-22T04:15:45Z

Release Summary:

Resolved issues:

related to #5257

Description of changes:

I update s2n-tls to successfully parse and load ML-DSA certificates. It's mostly minor changes to the certificate-loading logic, but it touches a lot of different files so I wanted to keep it separate from the actual signing logic PR. I left the signing methods, which are triggered by s2n_pkey_match, as no-ops for now.

Testing:

I added a unit test that successfully loads all combinations of ML-DSA private + public keys.

The version of awslc in our CI is recent enough to support ML-DSA. You can see the "-- feature S2N_AWSLC_SUPPORTS_MLDSA: TRUE" message in awslc builds like this AddressSanitizer one.

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

lrstewart · 2025-04-22T04:16:53Z

crypto/s2n_certificate.c

+    int type_hint = 0;
+    POSIX_GUARD(s2n_stuffer_private_key_from_pem(key_in_stuffer, key_out_stuffer, &type_hint));


Changing "type" to "type_hint" isn't strictly necessary, but I think it makes the code much clearer. We're really just guessing at the type.

lrstewart · 2025-04-22T04:24:00Z

crypto/s2n_pkey_mldsa.c

+
+bool s2n_pkey_mldsa_supported()


I'm wondering if I should name this "s2n_mldsa_supported" instead, despite the name of this module.

lrstewart · 2025-04-22T04:25:41Z

crypto/s2n_signature.h

+    /* For now, ML-DSA does not have a public value */
+    S2N_SIGNATURE_MLDSA,


I'm avoiding adding any publicly visible values (the S2N_TLS_SIGNATURE_ values are all defined in s2n.h) until this feature is complete.

tests/features/S2N_AWSLC_SUPPORTS_MLDSA.c

lrstewart · 2025-04-22T04:27:24Z

tests/testlib/s2n_testlib.h

-#define S2N_MAX_TEST_PEM_SIZE        8192
+#define S2N_MAX_TEST_PEM_SIZE        12000


The largest test ML-DSA file was like 11k.

tests/unit/s2n_pkey_mldsa_test.c

lrstewart · 2025-04-22T07:09:28Z

crypto/s2n_openssl_x509.c

+     * Note: this method may fail for ML-DSA-44 and ML-DSA-87, depending on the
+     * version of AWS-LC. See https://github.com/aws/aws-lc/issues/2347
+     * In order to handle this bug, we intentionally do not check the return
+     * value of OBJ_find_sigid_algs. Since OBJ_find_sigid_algs should only fail
+     * if signature_nid is unsupported by the libcrypto, and we retrieve signature_nid
+     * from the libcrypto, OBJ_find_sigid_algs should only fail for library bugs
+     * like the linked issue.
     */
-    RESULT_GUARD_OSSL(OBJ_find_sigid_algs(info->signature_nid, &info->signature_digest_nid, NULL),
-            S2N_ERR_CERT_TYPE_UNSUPPORTED);
+    OBJ_find_sigid_algs(info->signature_nid, &info->signature_digest_nid, NULL);


I've got a PR out to fix the awslc bug: aws/aws-lc#2348

But even if I fix it, we won't be able to assume that we're building against a fixed version of awslc :/ So I think we're stuck with handling this case. If just ignoring the error isn't acceptable, I could instead skip the method call for the MLDSA nids, since we don't actually need it. That's a pretty ugly check though: I'll need to specifically list the MLDSA nids to skip. And it'll need an if/def since the nids don't exist in awslc before MLDSA support was added.

We could also ask awslc to bump their API version, and then gate MLDSA support on the newer API version. I'm not sure that's any better than just ignoring the bug and failing to load the cert though: in both cases, a customer would build s2n-tls with a version of awslc that they would reasonably expect supports MLDSA, but be unable to load an MLDSA cert. The only difference would be the "S2N_AWSLC_MLDSA_SUPPORT" feature flag printed during the build.

I think my only problem with this is that it doesn't seem like we should trust any returned signature_digest_nid if the call to OBJ_find_sigid_algs fails. If we don't actually need this value anyway for ML-DSA, could we just not update info->signature_digest_nid unless the call succeeds?

Hmmm so like check for failure, but if the failure occurs, just explicitly set signature_digest_nid to 0 in case OBJ_find_sigid_algs set it to something else? I don't think that's super useful, but I'm probably biased by having looked at the implementation of OBJ_find_sigid_algs :P I can do that.

Okay so the current plan:

Initially, follow Sam's suggestion and reset signature_digest_nid on failure, but otherwise ignore failures.

Update AWSLC's API version: Bump AWSLC_API_VERSION to account for OBJ_find_sigid_algs bug aws-lc#2349

Wait for the next AWSLC release (probably later this week)

Rebuild our codebuild images

Make ML-DSA support depend on the new API version and revert the changes to this logic.

bindings/rust/extended/s2n-tls/src/config.rs

goatgoose · 2025-04-22T14:54:16Z

crypto/s2n_openssl_x509.c

+     * Note: this method may fail for ML-DSA-44 and ML-DSA-87, depending on the
+     * version of AWS-LC. See https://github.com/aws/aws-lc/issues/2347
+     * In order to handle this bug, we intentionally do not check the return
+     * value of OBJ_find_sigid_algs. Since OBJ_find_sigid_algs should only fail
+     * if signature_nid is unsupported by the libcrypto, and we retrieve signature_nid
+     * from the libcrypto, OBJ_find_sigid_algs should only fail for library bugs
+     * like the linked issue.
     */
-    RESULT_GUARD_OSSL(OBJ_find_sigid_algs(info->signature_nid, &info->signature_digest_nid, NULL),
-            S2N_ERR_CERT_TYPE_UNSUPPORTED);
+    OBJ_find_sigid_algs(info->signature_nid, &info->signature_digest_nid, NULL);


I think my only problem with this is that it doesn't seem like we should trust any returned signature_digest_nid if the call to OBJ_find_sigid_algs fails. If we don't actually need this value anyway for ML-DSA, could we just not update info->signature_digest_nid unless the call succeeds?

crypto/s2n_pkey_mldsa.c

tests/features/S2N_AWSLC_SUPPORTS_MLDSA.c

tests/unit/s2n_pkey_mldsa_test.c

jakemas · 2025-04-22T20:37:44Z

Just watching! Feel free to ping me regarding any questions around ML-DSA.

crypto/s2n_openssl_x509.c

crypto/s2n_pkey_mldsa.c

github-actions bot added the s2n-core team label Apr 22, 2025

lrstewart force-pushed the mldsa_1b_3 branch 4 times, most recently from c7573f3 to e28e94d Compare April 22, 2025 06:06

feature: add support for configuring (but not yet using) ml-dsa certs

bdaf721

lrstewart force-pushed the mldsa_1b_3 branch from e28e94d to bdaf721 Compare April 22, 2025 07:05

lrstewart commented Apr 22, 2025

View reviewed changes

lrstewart marked this pull request as ready for review April 22, 2025 07:22

lrstewart requested review from goatgoose and CarolYeh910 April 22, 2025 07:22

goatgoose reviewed Apr 22, 2025

View reviewed changes

lrstewart added 3 commits April 22, 2025 11:48

s/S2N_AWSLC_SUPPORTS/S2N_LIBCRYPTO_SUPPORTS

44f7492

Better handling of OBJ_find_sigid_algs failures

43896dc

assert test pkey is mldsa

ded9ad2

lrstewart requested a review from goatgoose April 22, 2025 19:01

goatgoose approved these changes Apr 22, 2025

View reviewed changes

crypto/s2n_openssl_x509.c Outdated Show resolved Hide resolved

crypto/s2n_pkey_mldsa.c Outdated Show resolved Hide resolved

pr comments

4e02e4e

CarolYeh910 approved these changes Apr 23, 2025

View reviewed changes

lrstewart added this pull request to the merge queue Apr 23, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 23, 2025

lrstewart added this pull request to the merge queue Apr 23, 2025

Merged via the queue into aws:main with commit 9fd38bd Apr 23, 2025
46 checks passed

lrstewart deleted the mldsa_1b_3 branch April 23, 2025 21:58

BrewTestBot mentioned this pull request May 1, 2025

s2n 1.5.18 Homebrew/homebrew-core#222175

Merged

		int type_hint = 0;
		POSIX_GUARD(s2n_stuffer_private_key_from_pem(key_in_stuffer, key_out_stuffer, &type_hint));

		/* For now, ML-DSA does not have a public value */
		S2N_SIGNATURE_MLDSA,

		#define S2N_MAX_TEST_PEM_SIZE 8192
		#define S2N_MAX_TEST_PEM_SIZE 12000


		bool s2n_pkey_mldsa_supported()

feature: add support for configuring (but not yet using) ml-dsa certs #5263

feature: add support for configuring (but not yet using) ml-dsa certs #5263

Uh oh!

Conversation

lrstewart commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Summary:

Resolved issues:

Description of changes:

Testing:

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lrstewart Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakemas commented Apr 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrstewart commented Apr 22, 2025 •

edited

Loading

lrstewart Apr 22, 2025 •

edited

Loading