Tls13 add early data indication #6486

xkqian · 2022-10-26T07:31:02Z

Add fields to mbedtls_ssl_context
Add write early data indication function
Add early data option to ssl_client2
Add test cases for early data

Part of adding early data support in TLS 1.3. No need for a change log.

ronald-cron-arm

Some early feedback. At that point no need to evolve the PR by adding new commits. Please just rewrite a clean commit history.

include/mbedtls/ssl.h

include/mbedtls/build_info.h

include/mbedtls/ssl.h

library/ssl_tls13_generic.c

ronald-cron-arm

Some additional comments. I haven't look at the changes in ssl-opt.sh yet.

include/mbedtls/ssl.h

library/ssl_misc.h

library/ssl_tls13_client.c

library/ssl_tls13_generic.c

yuhaoth

We should provide a parameter to input early data. Also some improvements.

include/mbedtls/ssl.h

library/ssl_misc.h

library/ssl_tls13_client.c

library/ssl_tls13_generic.c

tests/ssl-opt.sh

library/ssl_tls13_generic.c

programs/ssl/ssl_client2.c

yuhaoth

Some improvement and reply.

library/ssl_tls13_client.c

include/mbedtls/ssl.h

library/ssl_tls13_client.c

ronald-cron-arm

A few final minor issues.

library/ssl_tls13_client.c

library/ssl_misc.h

library/ssl_tls13_client.c

tests/ssl-opt.sh

include/mbedtls/build_info.h

yuhaoth

Some minor improvement

library/ssl_tls13_client.c

include/mbedtls/ssl.h

library/ssl_debug_helpers.h

library/ssl_misc.h

tests/ssl-opt.sh

Define the ALLOW_PSK_RESUMPTION and ALLOW_PSK_EPHEMERAL_RESUMPTION to the key exchange mode EXCHANGE_MODE_PSK and EXCHANGE_MODE_PSK_EPHEMERAL to facilate later check. Since they are 1( 1u<<0 ) and 4( 1u<<2 ), so define ALLOW_EARLY_DATA to 8( 1u<<3 ). Signed-off-by: Xiaokang Qian <[email protected]>

Signed-off-by: Xiaokang Qian <[email protected]>

yuhaoth

LGTM

The code is based PR Mbed-TLS#6486. This commit might be adapted after PR Mbed-TLS#6486 merged. Signed-off-by: Pengyu Lv <[email protected]>

ronald-cron-arm

Just some comments about the tests. Otherwise, this looks good to me.

tests/opt-testcases/tls13-misc.sh

ronald-cron-arm · 2022-11-17T08:22:25Z

tests/opt-testcases/tls13-misc.sh

+                             MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_ENABLED
+run_test    "TLS 1.3 m->G: EarlyData: basic check, good" \
+            "$G_NEXT_SRV -d 10 --priority=NORMAL:-VERS-ALL:+VERS-TLS1.3:+CIPHER-ALL:+ECDHE-PSK:+PSK --earlydata --disable-client-cert" \
+            "$P_CLI debug_level=4 force_version=tls13 early_data=1 reco_mode=1 reconnect=1 reco_delay=2" \


We try usually to force the TLS 1.3 version only on the server side thus like in the following test. That way when both TLS 1.2 and TLS 1.3 are enabled in the build the "hybrid" test is run and when only TLS 1.3 is enabled the test corresponding to TLS 1.3 being forced on the client side is run. Thus, I think we should just remove this one.

I just remember in kex exchange mode tests, we remove the force_version=tls13 in client side. To keep aligned, I prefer to remove this case. How about your opinion? @yuhaoth

For this PR, it is okay to remove it for me.
It should be added in Write APP data

https://datatracker.ietf.org/doc/html/rfc8446#appendix-A.2 shows early data can be sent after ClientHello. It is Okay for TLS1.3 only. But for hybrid mode, it might cause problem. That's why suggest add hybrid tests.

And I do not think we should postpone write app data like prototype, it does not match RFC. This point has been raised in GnuTLS https://gitlab.com/gnutls/gnutls/-/issues/1146.

For this PR, it is okay to remove it for me. It should be added in Write APP data

So I will remove the test case with -force_version=tls13, and only leave the test case which you said "hibrid case" as basic case.

https://datatracker.ietf.org/doc/html/rfc8446#appendix-A.2 shows early data can be sent after ClientHello. It is Okay for TLS1.3 only. But for hybrid mode, it might cause problem. That's why suggest add hybrid tests.

We will leave the "hibrid case" there. Can you eleborate the problems?

And I do not think we should postpone write app data like prototype, it does not match RFC. This point has been raised in GnuTLS https://gitlab.com/gnutls/gnutls/-/issues/1146.

Seems it's related with wrte early data, can we leave the comment to this PR #6542

So I will remove the test case with -force_version=tls13, and only leave the test case which you said "hibrid case" as basic case.

Okay.

Seems it's related with wrte early data, can we leave the comment to this PR #6542

Sure.

We will leave the "hibrid case". Can you eleborate the problems?

From RFC 8446, we should send early data after ClientHello immediately. So we must switch outbound traffic after ClientHello ASAP.

If switch in CLIENT_HELLO and send early data, it match RFC. But if the server select TLS 1.2, I am not sure what will happen.

To avoid the issue, I think we should postpone the traffic switch, just after TLS 1.3 is selected at this moment. And in ClientHello, we should only switch traffic if it is configured as tls1.3 only.

For the test side, we should have tls1.3 only and hybrid mode test to cover the cases.

From RFC 8446, we should send early data after ClientHello immediately. So we must switch outbound traffic after ClientHello ASAP.

If switch in CLIENT_HELLO and send early data, it match RFC. But if the server select TLS 1.2, I am not sure what will happen.

To avoid the issue, I think we should postpone the traffic switch, just after TLS 1.3 is selected at this moment. And in ClientHello, we should only switch traffic if it is configured as tls1.3 only.

For the test side, we should have tls1.3 only and hybrid mode test to cover the cases.

Seems it's one issue we should trade off when and in which case to change transform key.
Currently we only support session tikets psk, which means we only send out early data in resumption with tickets, maybe weI can make use of the tls_version in seriealized session to detect whether we only send out tls13 version, and then server will only have one tls13 choice. I am not sure whether the design work or not, maybe we can solve it in some ways.

Yes that's a key point. A TLS 1.3 handshake proposing early data can only be done with a PSK (external or obtained through a ticket) which is TLS 1.3 specific. Thus in that case we are not going to propose TLS 1.2 to the server: "TLS 1.3 only ClientHello".

tests/opt-testcases/tls13-misc.sh

Signed-off-by: Xiaokang Qian <[email protected]>

ronald-cron-arm

LGTM, @yuhaoth please have another look.

yuhaoth

LGTM

ronald-cron-arm · 2022-11-17T09:52:42Z

LGTM

It seems you selected "Request changes" instead of "Approve".

yuhaoth

LGTM

The code is based PR Mbed-TLS#6486. This commit might be adapted after PR Mbed-TLS#6486 merged. Signed-off-by: Pengyu Lv <[email protected]>

xkqian added needs-work needs-ci Needs to pass CI tests component-tls13 labels Oct 26, 2022

xkqian linked an issue Oct 26, 2022 that may be closed by this pull request

TLS 1.3 client: Writing of the early data indication extension #6326

Closed

2 tasks

xkqian force-pushed the tls13_add_early_data_indication branch 6 times, most recently from 9233497 to 039504b Compare October 27, 2022 10:42

daverodgman added the priority-high High priority - will be reviewed soon label Oct 27, 2022

ronald-cron-arm requested changes Oct 28, 2022

View reviewed changes

xkqian force-pushed the tls13_add_early_data_indication branch from a8eeda0 to b29afc8 Compare October 28, 2022 10:10

xkqian mentioned this pull request Oct 28, 2022

Tls13 parse early data indication ee #6490

Merged

xkqian requested a review from ronald-cron-arm October 28, 2022 10:54

ronald-cron-arm requested changes Oct 28, 2022

View reviewed changes

xkqian force-pushed the tls13_add_early_data_indication branch from b29afc8 to fb91dc8 Compare October 31, 2022 10:38

xkqian requested a review from yuhaoth November 1, 2022 10:53

yuhaoth suggested changes Nov 2, 2022

View reviewed changes

xkqian force-pushed the tls13_add_early_data_indication branch 2 times, most recently from 45ade6f to f99b5a5 Compare November 3, 2022 06:43

yuhaoth suggested changes Nov 4, 2022

View reviewed changes

yuhaoth mentioned this pull request Nov 4, 2022

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

Closed

1 task

xkqian requested a review from ronald-cron-arm November 7, 2022 07:08

ronald-cron-arm requested changes Nov 7, 2022

View reviewed changes

xkqian force-pushed the tls13_add_early_data_indication branch from 02ffe41 to d2bf636 Compare November 8, 2022 09:13

xkqian requested review from yuhaoth and ronald-cron-arm November 8, 2022 09:15

ronald-cron-arm requested changes Nov 8, 2022

View reviewed changes

yuhaoth suggested changes Nov 9, 2022

View reviewed changes

xkqian added 2 commits November 16, 2022 08:32

Add EARLY_DATA guard to the early data extension in session ticket

0cc4320

Signed-off-by: Xiaokang Qian <[email protected]>

xkqian requested a review from yuhaoth November 16, 2022 08:53

Update enabled guards for early data cases

e7bab00

Signed-off-by: Xiaokang Qian <[email protected]>

xkqian force-pushed the tls13_add_early_data_indication branch from 802e2c8 to e7bab00 Compare November 16, 2022 10:07

yuhaoth previously approved these changes Nov 16, 2022

View reviewed changes

lpy4105 added a commit to lpy4105/mbedtls that referenced this pull request Nov 17, 2022

Add definations of ticket_flags

d498f4c

The code is based PR Mbed-TLS#6486. This commit might be adapted after PR Mbed-TLS#6486 merged. Signed-off-by: Pengyu Lv <[email protected]>

lpy4105 added a commit to lpy4105/mbedtls that referenced this pull request Nov 17, 2022

Add definations of ticket_flags

a457b4d

The code is based PR Mbed-TLS#6486. This commit might be adapted after PR Mbed-TLS#6486 merged. Signed-off-by: Pengyu Lv <[email protected]>

ronald-cron-arm requested changes Nov 17, 2022

View reviewed changes

Remove the fore_tls13 option case from client side

e9622ac

Signed-off-by: Xiaokang Qian <[email protected]>

xkqian dismissed yuhaoth’s stale review via e9622ac November 17, 2022 09:26

ronald-cron-arm approved these changes Nov 17, 2022

View reviewed changes

yuhaoth suggested changes Nov 17, 2022

View reviewed changes

yuhaoth mentioned this pull request Nov 17, 2022

TLS1.3 write early data #6542

Closed

yuhaoth approved these changes Nov 17, 2022

View reviewed changes

ronald-cron-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, needs-ci Needs to pass CI tests labels Nov 17, 2022

ronald-cron-arm merged commit d12922a into Mbed-TLS:development Nov 17, 2022

xkqian mentioned this pull request Nov 21, 2022

TLS 1.3: SessionTicket: Enable kex change mode check when resumption #6616

Merged

8 tasks

lpy4105 added a commit to lpy4105/mbedtls that referenced this pull request Nov 22, 2022

Add definations of ticket_flags

284a2ce

The code is based PR Mbed-TLS#6486. This commit might be adapted after PR Mbed-TLS#6486 merged. Signed-off-by: Pengyu Lv <[email protected]>

xkqian mentioned this pull request Nov 28, 2023

Propose tls13 only when enable the session ticket resumption #8572

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tls13 add early data indication #6486

Tls13 add early data indication #6486

xkqian commented Oct 26, 2022 •

edited by ronald-cron-arm

Loading

ronald-cron-arm left a comment

ronald-cron-arm left a comment

yuhaoth left a comment

yuhaoth left a comment

ronald-cron-arm left a comment

yuhaoth left a comment

yuhaoth left a comment

ronald-cron-arm left a comment

ronald-cron-arm Nov 17, 2022

xkqian Nov 17, 2022

yuhaoth Nov 17, 2022

xkqian Nov 17, 2022 •

edited

Loading

yuhaoth Nov 17, 2022

xkqian Nov 17, 2022 •

edited

Loading

ronald-cron-arm Nov 17, 2022

ronald-cron-arm left a comment

yuhaoth left a comment

ronald-cron-arm commented Nov 17, 2022

yuhaoth left a comment

Tls13 add early data indication #6486

Tls13 add early data indication #6486

Conversation

xkqian commented Oct 26, 2022 • edited by ronald-cron-arm Loading

ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

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 Nov 17, 2022

Choose a reason for hiding this comment

xkqian Nov 17, 2022

Choose a reason for hiding this comment

yuhaoth Nov 17, 2022

Choose a reason for hiding this comment

xkqian Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

yuhaoth Nov 17, 2022

Choose a reason for hiding this comment

xkqian Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm Nov 17, 2022

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

ronald-cron-arm commented Nov 17, 2022

yuhaoth left a comment

Choose a reason for hiding this comment

xkqian commented Oct 26, 2022 •

edited by ronald-cron-arm

Loading

xkqian Nov 17, 2022 •

edited

Loading

xkqian Nov 17, 2022 •

edited

Loading