Add initial support for MLKEM768 (without any new Security Policies) #4816

alexw91 · 2024-10-01T23:02:27Z

Resolved issues:

None

Description of changes:

Part 2 in a multi-part series to add X25519MLKEM768 support to s2n. (Previous PR is: #4810)

This PR includes the following changes:

Add support for new send_kem_first boolean parameter for Hybrid PQ KeyShares (needed for X25519MLKEM768).
Add support for 1 new KEM: MLKEM768
Add support for 2 new Hybrid KemGroups: X25519MLKEM768 and SecP256r1MLKEM768
Add new self-talk unit tests to negotiate X25519MLKEM768 and confirm shared secret calculation is correct.

Call-outs:

No new s2n-tls security policies with X25519MLKEM768 have been added (other than test_all Policy). These new MLKEM-768 security policies will be coming in a later PR.

Testing:

s2n self-talk unit tests with X25519MLKEM768
Unit tests confirm that s2n-tls hybrid shared secret calculation matches 2nd independent Python implementation.

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

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

tls/s2n_kem.h

tls/s2n_kem_preferences.c

tls/s2n_kem.h

crypto/s2n_pq.c

tls/s2n_kem.h

tls/extensions/s2n_client_key_share.c

lrstewart · 2024-10-03T06:26:18Z

tests/unit/s2n_kem_preferences_test.c

        const struct s2n_kem_group *test_kem_groups[] = {
+            &s2n_secp256r1_mlkem_768,
+            &s2n_x25519_mlkem_768,


Any reason this needs to be another separate list and can't be ALL_SUPPORTED_KEM_GROUPS?

Was this missed?

tests/unit/s2n_pq_kem_test.c

tests/unit/s2n_security_policies_test.c

tests/unit/s2n_server_key_share_extension_test.c

goatgoose · 2024-10-04T15:09:30Z

tests/unit/s2n_mlkem_test.c

+    /* MLKEM Support was added to AWSLC when AWSLC_API_VERSION == 29 */
+    if (s2n_libcrypto_is_awslc() && s2n_libcrypto_awslc_api_version() >= 30) {
+        EXPECT_TRUE(s2n_libcrypto_supports_mlkem());


Is this correct? It looks like the NID_MLKEM768 field used in the feature probe was defined in aws/aws-lc#1797, which was API version 30. Or was NID_MLKEM768 somehow exposed earlier?

goatgoose · 2024-10-04T15:14:57Z

crypto/s2n_libcrypto.h

@@ -17,6 +17,8 @@

 #include "utils/s2n_result.h"

+uint64_t s2n_libcrypto_awslc_api_version(void);
+bool s2n_libcrypto_is_awslc();


s2n_libcrypto_is_awslc() is already defined in s2n_openssl.h:

s2n-tls/crypto/s2n_openssl.h

Line 59 in 7324a33

bool s2n_libcrypto_is_awslc();

This header does seem like a more reasonable place to define the libcrypto_is functions, but moving them may be out of scope for this PR.

goatgoose · 2024-10-04T15:47:29Z

tls/extensions/s2n_server_key_share.c

@@ -34,6 +34,22 @@ const s2n_extension_type s2n_server_key_share_extension = {
    .if_missing = s2n_extension_noop_if_missing,
 };

+static int s2n_server_key_share_send_hybrid_partial_ecc(struct s2n_connection *conn, struct s2n_stuffer *out)
+{
+    struct s2n_kem_group_params *server_kem_group_params = &conn->kex_params.server_kem_group_params;


nit

Suggested change

struct s2n_kem_group_params *server_kem_group_params = &conn->kex_params.server_kem_group_params;

POSIX_ENSURE_REF(conn);

struct s2n_kem_group_params *server_kem_group_params = &conn->kex_params.server_kem_group_params;

goatgoose · 2024-10-04T15:49:23Z

tls/extensions/s2n_server_key_share.c

@@ -157,6 +171,27 @@ static int s2n_server_key_share_send(struct s2n_connection *conn, struct s2n_stu
    return S2N_SUCCESS;
 }

+static int s2n_server_key_share_recv_hybrid_partial_ecc(struct s2n_connection *conn, struct s2n_stuffer *extension)
+{
+    struct s2n_kem_params *client_kem_params = &conn->kex_params.client_kem_group_params.kem_params;


nit

Suggested change

struct s2n_kem_params *client_kem_params = &conn->kex_params.client_kem_group_params.kem_params;

POSIX_ENSURE_REF(conn);

struct s2n_kem_params *client_kem_params = &conn->kex_params.client_kem_group_params.kem_params;

goatgoose · 2024-10-04T15:53:21Z

tls/extensions/s2n_server_key_share.c

+{
+    struct s2n_kem_params *client_kem_params = &conn->kex_params.client_kem_group_params.kem_params;
+    struct s2n_kem_group_params *server_kem_group_params = &conn->kex_params.server_kem_group_params;
+    uint16_t expected_ecc_share_size = server_kem_group_params->kem_group->curve->share_size;


nit - I think kem_group can be null:

Suggested change

uint16_t expected_ecc_share_size = server_kem_group_params->kem_group->curve->share_size;

const struct s2n_kem_group *server_kem_group = server_kem_group_params->kem_group;

POSIX_ENSURE_REF(server_kem_group);

uint16_t expected_ecc_share_size = server_kem_group->curve->share_size;

goatgoose · 2024-10-04T17:01:00Z

tests/unit/s2n_kem_preferences_test.c

        const struct s2n_kem_group *test_kem_groups[] = {
+            &s2n_secp256r1_mlkem_768,
+            &s2n_x25519_mlkem_768,


Was this missed?

goatgoose · 2024-10-04T17:12:15Z

tests/unit/s2n_tls13_pq_handshake_test.c

@@ -406,6 +406,27 @@ int main()
        .ecc_preferences = &s2n_ecc_preferences_20201021,
    };

+    const struct s2n_kem_group *mlkem768_test_groups[] = {
+        &s2n_x25519_mlkem_768,


If I'm understanding this test correctly, I think this will only try to handshake with s2n_x25519_mlkem_768? Would it be worth adding a self-talk test here or somewhere else that negotiates both of these?

goatgoose · 2024-10-04T17:40:03Z

tests/features/S2N_LIBCRYPTO_SUPPORTS_MLKEM.c

It looks like our CI might be using an older version of AWS-LC:

-- feature S2N_LIBCRYPTO_SUPPORTS_MLKEM: FALSE

We may want to update that before merging so all of the tests can run.

alexw91 requested review from lrstewart, camshaft and goatgoose October 1, 2024 23:02

alexw91 force-pushed the mlkem-part-2 branch from 7d459c3 to 03f7815 Compare October 1, 2024 23:41

alexw91 changed the title ~~Add initial support for MLKEM768 (without any new Security Policies)~~ [Part 2] Add initial support for MLKEM768 (without any new Security Policies) Oct 1, 2024

alexw91 force-pushed the mlkem-part-2 branch 2 times, most recently from 316df93 to 19fcae8 Compare October 1, 2024 23:51

alexw91 changed the title ~~[Part 2] Add initial support for MLKEM768 (without any new Security Policies)~~ Add initial support for MLKEM768 (without any new Security Policies) Oct 2, 2024

alexw91 force-pushed the mlkem-part-2 branch 3 times, most recently from a4cf925 to d07f333 Compare October 2, 2024 23:42

lrstewart reviewed Oct 3, 2024

View reviewed changes

alexw91 force-pushed the mlkem-part-2 branch from d07f333 to 685727a Compare October 3, 2024 22:01

Add initial support for MLKEM768

0d2ecfc

alexw91 force-pushed the mlkem-part-2 branch 3 times, most recently from eb04c73 to a592e1e Compare October 3, 2024 22:39

Address Feedback

90ec719

alexw91 force-pushed the mlkem-part-2 branch from a592e1e to 90ec719 Compare October 3, 2024 22:47

boquan-fang mentioned this pull request Oct 3, 2024

cmake and make run feature tests with different flags #4187

Open

goatgoose reviewed Oct 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial support for MLKEM768 (without any new Security Policies) #4816

Add initial support for MLKEM768 (without any new Security Policies) #4816

alexw91 commented Oct 1, 2024 •

edited

Loading

lrstewart Oct 3, 2024

alexw91 Oct 3, 2024

goatgoose Oct 4, 2024

goatgoose Oct 4, 2024

goatgoose Oct 4, 2024

goatgoose Oct 4, 2024

goatgoose Oct 4, 2024

goatgoose Oct 4, 2024

goatgoose Oct 4, 2024

goatgoose Oct 4, 2024

goatgoose Oct 4, 2024

	struct s2n_kem_group_params *server_kem_group_params = &conn->kex_params.server_kem_group_params;
	POSIX_ENSURE_REF(conn);
	struct s2n_kem_group_params *server_kem_group_params = &conn->kex_params.server_kem_group_params;

	struct s2n_kem_params *client_kem_params = &conn->kex_params.client_kem_group_params.kem_params;
	POSIX_ENSURE_REF(conn);
	struct s2n_kem_params *client_kem_params = &conn->kex_params.client_kem_group_params.kem_params;

-    uint16_t expected_ecc_share_size = server_kem_group_params->kem_group->curve->share_size;
+    const struct s2n_kem_group *server_kem_group = server_kem_group_params->kem_group;
+    POSIX_ENSURE_REF(server_kem_group);
+    uint16_t expected_ecc_share_size = server_kem_group->curve->share_size;

Add initial support for MLKEM768 (without any new Security Policies) #4816

Are you sure you want to change the base?

Add initial support for MLKEM768 (without any new Security Policies) #4816

Conversation

alexw91 commented Oct 1, 2024 • edited Loading

Resolved issues:

Description of changes:

Call-outs:

Testing:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexw91 commented Oct 1, 2024 •

edited

Loading