Change early data flag to input file #8596

xkqian · 2023-12-04T08:52:18Z

Description

Originally we have one input option to control whether enable early data or not. Currently we need one extra parameter to input various length of early data. This pr will integrate them together.

This is part of #6542

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

changelog ~~provided, or~~ not required
backport ~~done, or~~ not required
tests provided ~~, or not required~~

tom-cosgrove-arm · 2023-12-04T09:27:29Z

@xkqian Should this PR be part of the Early Data epic?

xkqian · 2023-12-04T10:53:22Z

@xkqian Should this PR be part of the Early Data epic?

Yes, sure, this is part of #6542 I just want to split it into several small PRs to accelerate the review, and this is the the first one.

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

ronald-cron-arm

This looks almost good to me, just some minor suggestions.

programs/ssl/ssl_client2.c

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

yuhaoth

Some comments. I think the implementation looks too complex and miss some features for tests.

programs/ssl/ssl_client2.c

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

yuhaoth

Some new comments. And previous early_data option feedback is not addressed

programs/ssl/ssl_client2.c

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

ronald-cron-arm

Some minor comments

programs/ssl/ssl_client2.c

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

ronald-cron-arm · 2023-12-07T08:47:28Z

I am fine with the two options solution. I put a comment in one of Jerry's suggestion.

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

yuhaoth

LGTM

ronald-cron-arm

While thinking about what option to add to ssl_client2 for early data testing I think we have lost (at least that's my case) sight of its scope. It is mainly about testing the various TLS options and the compatibility with GnuTLS and OpenSSL. Negative and edge cases testing is not really in scope.

Thus I'd say let's keep things simple, for the time being at least:
one option early_data_file whose default value is "". If a valid file path is set to early_data_file, we enable early data on the client and in the reconnection case, before to call mbedtls_ssl_handshake() to reconnect we call mbedtls_ssl_write_early_data() that tries to write the early_data_file file data.

When we add more testing for early data we will see if they best fit in ssl-opt.sh or test_suite_ssl and we need to add other options in ssl_client2.

yuhaoth · 2023-12-08T02:46:14Z

While thinking about what option to add to ssl_client2 for early data testing I think we have lost (at least that's my case) sight of its scope. It is mainly about testing the various TLS options and the compatibility with GnuTLS and OpenSSL. Negative and edge cases testing is not really in scope.

Thus I'd say let's keep things simple, for the time being at least: one option early_data_file whose default value is "". If a valid file path is set to early_data_file, we enable early data on the client and in the reconnection case, before to call mbedtls_ssl_handshake() to reconnect we call mbedtls_ssl_write_early_data() that tries to write the early_data_file file data.

My target is to simple this PR. Let's make final decision in next PRs .
But if only one option, we have to do more things in this PR. early_data option come from development, it must be removed. I do not thinks we should remove or change it in this PR. That make things complex.

For the two options from your previous comments, I do not think empty file and invalid file are same. If they are same, we have to check the file length before call mbedtls_ssl_{handshake, write_early_data}() and reduce possible test case. And I do not mean we MUST add the test in ssl-opt.sh. But the amount of code is less and the test coverage is more, why not do like that?

When we add more testing for early data we will see if they best fit in ssl-opt.sh or test_suite_ssl and we need to add other options in ssl_client2.

Agree.
I think this PR just do two things enough.

add early_data_file option
add a empty function for writing early data in next PRs .

xkqian · 2023-12-08T07:32:03Z

Thus I'd say let's keep things simple, for the time being at least:
one option early_data_file whose default value is "". If a valid file path is set to early_data_file, we enable early data on the client and in the reconnection case, before to call mbedtls_ssl_handshake() to reconnect we call mbedtls_ssl_write_early_data() that tries to write the early_data_file file data.

Seems this is the first way which only provide one option, am I right? if yes, then the code before this commit: 57db590 can satisfy the requirements, of course, need addressing some comments.

My target is to simple this PR. Let's make final decision in next PRs . But if only one option, we have to do more things in this PR. early_data option come from development, it must be removed. I do not thinks we should remove or change it in this PR. That make things complex.

This comment prefer to reserve the early_data option and introduce another option named "early_data_file" as the source file read early data from. The latest code is trying to satisfy this requirement.

Seems we should agree with each other that which way we should take. and then I will revert code or keep the code accordingly.
(my original plan is: one option is enough for this case, but I am fine with two options. )

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

xkqian · 2023-12-08T10:50:20Z

I just revert the code to use the one option, which uses early_data as the input early data file.
In this PR, we can only replace the option, and in next PR, we can add the test code including: ssl_write_early_data(), and how to call it.
I think we can review it base one current version.

ronald-cron-arm

If with this changes you have what you need for your coming PRs implementing mbedtls_ssl_write_early_data() this is good enough to me.

yuhaoth

It has got CI failure
One change request and one nitpick comments.

yuhaoth · 2023-12-08T15:38:16Z

programs/ssl/ssl_client2.c

+    if (strlen(opt.early_data) > 0) {
+        if ((early_data_fp = fopen(opt.early_data, "rb")) == NULL) {
+            mbedtls_printf("failed\n  ! Cannot open '%s' for reading.\n",
+                           opt.early_data);
+            goto exit;
+        }
+        early_data_enabled = MBEDTLS_SSL_EARLY_DATA_ENABLED;
+    }


To fix the CI failure

Suggested change

if (strlen(opt.early_data) > 0) {

if ((early_data_fp = fopen(opt.early_data, "rb")) == NULL) {

mbedtls_printf("failed\n ! Cannot open '%s' for reading.\n",

opt.early_data);

goto exit;

}

early_data_enabled = MBEDTLS_SSL_EARLY_DATA_ENABLED;

}

if (opt.early_data) {

if ((early_data_fp = fopen(opt.early_data, "rb")) == NULL) {

mbedtls_printf("failed\n ! Cannot open '%s' for reading.\n",

opt.early_data);

goto exit;

}

early_data_enabled = MBEDTLS_SSL_EARLY_DATA_ENABLED;

}

The CI failure is caused by the uninitialized file pointer early_data_fp.
opt.early_data will be default to "", this check maybe useless...

yuhaoth · 2023-12-08T15:44:14Z

programs/ssl/ssl_client2.c

+    if (strlen(opt.early_data) > 0) {
+        if ((early_data_fp = fopen(opt.early_data, "rb")) == NULL) {
+            mbedtls_printf("failed\n  ! Cannot open '%s' for reading.\n",
+                           opt.early_data);
+            goto exit;
+        }
+        early_data_enabled = MBEDTLS_SSL_EARLY_DATA_ENABLED;
+    }


Nitpick comment : move it to option check like previous version

Suggested change

if (strlen(opt.early_data) > 0) {

if ((early_data_fp = fopen(opt.early_data, "rb")) == NULL) {

mbedtls_printf("failed\n ! Cannot open '%s' for reading.\n",

opt.early_data);

goto exit;

}

early_data_enabled = MBEDTLS_SSL_EARLY_DATA_ENABLED;

}

mbedtls_ssl_conf_early_data(&conf, early_data_fp ?MBEDTLS_SSL_EARLY_DATA_ENABLE: MBEDTLS_SSL_EARLY_DATA_DISABLE);

I just see there are file processings like context_file, will treat it like what I have done, so I prefer that way.

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

tom-cosgrove-arm · 2023-12-11T07:54:59Z

@yuhaoth are you happy with this now, too?

tom-cosgrove-arm · 2023-12-11T11:12:21Z

programs/ssl/ssl_client2.c

+        }
+        early_data_enabled = MBEDTLS_SSL_EARLY_DATA_ENABLED;
+    }
+    mbedtls_ssl_conf_early_data(&conf, early_data_enabled);


(nit) presumably early data defaults to disabled, so we could move this into the if() and remove the variable?

tom-cosgrove-arm

LGTM

xkqian added needs-ci Needs to pass CI tests component-tls13 priority-high High priority - will be reviewed soon labels Dec 4, 2023

xkqian force-pushed the tls13_early_data_input_file branch from 0c8d22f to 4c909e6 Compare December 4, 2023 08:58

xkqian requested a review from yuhaoth December 4, 2023 08:58

xkqian linked an issue Dec 4, 2023 that may be closed by this pull request

TLS 1.3 client: Add support for early data writing #6340

Closed

1 task

xkqian requested a review from ronald-cron-arm December 4, 2023 09:00

xkqian force-pushed the tls13_early_data_input_file branch from 4c909e6 to 0183914 Compare December 4, 2023 09:04

xkqian added needs-review Every commit must be reviewed by at least two team members, and removed needs-review Every commit must be reviewed by at least two team members, labels Dec 4, 2023

Change early data flag to input file

70fbdcf

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

xkqian force-pushed the tls13_early_data_input_file branch from 0183914 to 70fbdcf Compare December 5, 2023 05:51

xkqian added the needs-review Every commit must be reviewed by at least two team members, label Dec 5, 2023

xkqian mentioned this pull request Dec 5, 2023

TLS1.3 early data test code #8606

Closed

3 tasks

ronald-cron-arm requested changes Dec 5, 2023

View reviewed changes

programs/ssl/ssl_client2.c Outdated Show resolved Hide resolved

programs/ssl/ssl_client2.c Outdated Show resolved Hide resolved

programs/ssl/ssl_client2.c Outdated Show resolved Hide resolved

programs/ssl/ssl_client2.c Outdated Show resolved Hide resolved

Improve the comments of early data input

6c678d7

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Rename the generic read functions to ssl_read_file_text

b1db729

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

yuhaoth suggested changes Dec 6, 2023

View reviewed changes

xkqian added 2 commits December 6, 2023 02:55

Refine the detect code to enable early data or not

eaebedb

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Remove the generic file read functions and simply the early data read

f8fe11d

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

xkqian requested review from ronald-cron-arm and yuhaoth December 6, 2023 07:44

yuhaoth suggested changes Dec 6, 2023

View reviewed changes

programs/ssl/ssl_client2.c Outdated Show resolved Hide resolved

programs/ssl/ssl_client2.c Outdated Show resolved Hide resolved

xkqian added 2 commits December 6, 2023 09:24

Sync the early_data option with internal parameters in ssl_client2

611c717

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Enable early data depend on whether the early data file exist

ae95217

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

ronald-cron-arm requested changes Dec 6, 2023

View reviewed changes

programs/ssl/ssl_client2.c Outdated Show resolved Hide resolved

programs/ssl/ssl_client2.c Outdated Show resolved Hide resolved

programs/ssl/ssl_client2.c Outdated Show resolved Hide resolved

Open the file once read in the file path

daddfb5

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

xkqian added 2 commits December 7, 2023 09:19

Add the test framework of early data

9634680

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

Remove the duplicate cases and add early_data_file option

dce183f

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

xkqian requested a review from yuhaoth December 7, 2023 09:51

yuhaoth previously approved these changes Dec 7, 2023

View reviewed changes

ronald-cron-arm requested changes Dec 7, 2023

View reviewed changes

Revert to ae95217 and addressing some comments

aedfc09

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

xkqian dismissed yuhaoth’s stale review via aedfc09 December 8, 2023 10:45

xkqian requested a review from ronald-cron-arm December 8, 2023 10:50

ronald-cron-arm previously approved these changes Dec 8, 2023

View reviewed changes

yuhaoth reviewed Dec 8, 2023

View reviewed changes

Fix CI failure of uninitialized fp

a9581d2

Signed-off-by: Xiaokang Qian <xiaokang.qian@arm.com>

xkqian dismissed ronald-cron-arm’s stale review via a9581d2 December 11, 2023 01:56

ronald-cron-arm self-requested a review December 11, 2023 07:32

ronald-cron-arm approved these changes Dec 11, 2023

View reviewed changes

ronald-cron-arm requested a review from yuhaoth December 11, 2023 07:32

tom-cosgrove-arm reviewed Dec 11, 2023

View reviewed changes

tom-cosgrove-arm approved these changes Dec 11, 2023

View reviewed changes

tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Dec 11, 2023

gilles-peskine-arm added this pull request to the merge queue Dec 11, 2023

Merged via the queue into Mbed-TLS:development with commit a211bb7 Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change early data flag to input file #8596

Change early data flag to input file #8596

xkqian commented Dec 4, 2023 •

edited by minosgalanakis

Loading

tom-cosgrove-arm commented Dec 4, 2023

xkqian commented Dec 4, 2023

ronald-cron-arm left a comment

yuhaoth left a comment

yuhaoth left a comment

ronald-cron-arm left a comment

ronald-cron-arm commented Dec 7, 2023

yuhaoth left a comment

ronald-cron-arm left a comment

yuhaoth commented Dec 8, 2023

xkqian commented Dec 8, 2023

xkqian commented Dec 8, 2023

ronald-cron-arm left a comment

yuhaoth left a comment •

edited

Loading

yuhaoth Dec 8, 2023 •

edited

Loading

xkqian Dec 11, 2023

yuhaoth Dec 8, 2023 •

edited

Loading

xkqian Dec 11, 2023

tom-cosgrove-arm commented Dec 11, 2023

tom-cosgrove-arm Dec 11, 2023

tom-cosgrove-arm left a comment

Change early data flag to input file #8596

Change early data flag to input file #8596

Conversation

xkqian commented Dec 4, 2023 • edited by minosgalanakis Loading

Description

PR checklist

tom-cosgrove-arm commented Dec 4, 2023

xkqian commented Dec 4, 2023

ronald-cron-arm left a comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm commented Dec 7, 2023

yuhaoth left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

yuhaoth commented Dec 8, 2023

xkqian commented Dec 8, 2023

xkqian commented Dec 8, 2023

ronald-cron-arm left a comment

Choose a reason for hiding this comment

yuhaoth left a comment • edited Loading

Choose a reason for hiding this comment

yuhaoth Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

xkqian Dec 11, 2023

Choose a reason for hiding this comment

yuhaoth Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

xkqian Dec 11, 2023

Choose a reason for hiding this comment

tom-cosgrove-arm commented Dec 11, 2023

tom-cosgrove-arm Dec 11, 2023

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

xkqian commented Dec 4, 2023 •

edited by minosgalanakis

Loading

yuhaoth left a comment •

edited

Loading

yuhaoth Dec 8, 2023 •

edited

Loading

yuhaoth Dec 8, 2023 •

edited

Loading