Skip to content

fix: restrict mldsa signatures based on certificate#5713

Merged
jmayclin merged 4 commits intoaws:mainfrom
jmayclin:2026-01-27-time-pin
Jan 31, 2026
Merged

fix: restrict mldsa signatures based on certificate#5713
jmayclin merged 4 commits intoaws:mainfrom
jmayclin:2026-01-27-time-pin

Conversation

@jmayclin
Copy link
Copy Markdown
Contributor

@jmayclin jmayclin commented Jan 28, 2026

Goal

Allow us to correctly negotiate ML-DSA signatures.

Why

We fail to negotiate ML-DSA with the new release of OpenSSL

    Updating openssl-src v300.5.4+3.5.4 -> v300.5.5+3.5.5

This change broke our CI.

This OpenSSL release included a stricter, more correct behavior

This broke us because our signature selection was not certificate aware. We were loading ML-DSA-87 certs

        let cert_path = format!("{TEST_PEMS_PATH}mldsa/ML-DSA-87.crt");
        let key_path = format!("{TEST_PEMS_PATH}mldsa/ML-DSA-87-seed.priv");

But the cert verify that the s2n-tls server was sending was not mldsa87

        Handshake(
            CertVerifyTls13(
                CertVerifyTls13 {
                    algorithm: SignatureScheme {
                        value: 2308,
                        description: "mldsa44",
                    },
                    signature: PrefixedBlob(
                        4627,
                    ),
                },
            ),
        ),

How

We updated the signature selection to properly handle MLDSA signatures. I also added an "else" case so any new certificate type will explicitly fail unless we handle it.

Related

Also, I am pinning time to get the CI to pass. That is blocking a different PR to fix the rust toolchains in the Nix stuff.

Testing

CI should pass

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

@github-actions github-actions bot added the s2n-core team label Jan 28, 2026
@jmayclin jmayclin marked this pull request as ready for review January 28, 2026 00:24
@jmayclin jmayclin changed the title chore: pin time to avoid msrv bump fix: restrict mldsa signatures based on certificate Jan 28, 2026
POSIX_GUARD_RESULT(s2n_signature_algorithm_get_pkey_type(sig_scheme->sig_alg, &cert_type));

/* A valid cert must exist for the authentication method. */
struct s2n_cert_chain_and_key *cert = s2n_get_compatible_cert_chain_and_key(conn, cert_type);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to look at all other places where s2n_get_compatible_cert_chain_and_key() is called? That function is used in multiple places.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer to go ahead and move ahead with this PR. I agree that we need to do a deeper dive, but I'd like to get the CI unblocked, and this is very concretely fixing a real bug.

@jmayclin jmayclin requested a review from maddeleine January 30, 2026 18:18
@jmayclin jmayclin added this pull request to the merge queue Jan 31, 2026
Merged via the queue into aws:main with commit bd8206a Jan 31, 2026
53 checks passed
@jmayclin jmayclin deleted the 2026-01-27-time-pin branch January 31, 2026 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants