TLS 1.3: Add mbedtls_ssl_write_early_data() API #8760

ronald-cron-arm · 2024-01-29T08:37:21Z

Description

Second PR of two addressing #6340 .

PR checklist

changelog not required, will add one for TLS 1.3 early data feature as a whole
backport not required, new feature
tests provided

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

No need to define specific early data, the idea is rather to just send the usual request data as early data instead of standard application data. Signed-off-by: Ronald Cron <[email protected]>

Move code to build http request into a dedicated function. Signed-off-by: Ronald Cron <[email protected]>

Switch from int to size_t for some data lengths and counter local variables. Signed-off-by: Ronald Cron <[email protected]>

Add buffer overflow check to build_http_request(). Signed-off-by: Ronald Cron <[email protected]>

Signed-off-by: Ronald Cron <[email protected]>

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

Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm · 2024-02-22T06:38:59Z

@mpg I have addressed your comments, the Open CI is green, please have another look, thanks.

mpg · 2024-02-22T08:35:03Z

Btw, out of scope for this PR, but I think the early data APIs are a bit complex (not saying they're bad, I think 0-RTT is just a fairly complex feature of 1.3 so this reflects in the APIs), and a knowledge base article explaining everything a user needs to know (in general, client-side and server-side) would be helpful if we can write one at some point.

ronald-cron-arm · 2024-02-22T08:43:11Z

Btw, out of scope for this PR, but I think the early data APIs are a bit complex (not saying they're bad, I think 0-RTT is just a fairly complex feature of 1.3 so this reflects in the APIs), and a knowledge base article explaining everything a user needs to know (in general, client-side and server-side) would be helpful if we can write one at some point.

We have already something at the end of tls13-support.md. The read part is outdated but the write part not that much it seems. I've planned to update/fix it when addressing the wrap-up issue.

mpg · 2024-02-22T08:47:35Z

We have already something at the end of tls13-support.md. The read part is outdated but the write part not that much it seems. I've planned to update/fix it when addressing the wrap-up issue.

Great! Though I'll note as a user I would probably not be looking under docs/architecture. Probably worth splitting the document into two parts: one for developers (same location), one for users (probably just under docs?).

mpg

Thanks for addressing my feedback! Looking pretty good, only minor points left.

Also, it seems to me we can now remove the hack used for testing server before we had client support. Btw, now that we have a tls13_write_early_data test function, perhaps tls13_early_data should be renamed to tls13_read_early_data. Also, why doesn't that one have a NO_INDICATION_SENT scenario like the other two? (I'm OK with considering this whole paragraph out of scope as long as it's tracked somewhere.)

include/mbedtls/ssl.h

tests/suites/test_suite_ssl.function

library/ssl_tls13_client.c

tests/suites/test_suite_ssl.function

tests/opt-testcases/tls13-misc.sh

Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm · 2024-02-22T12:36:59Z

Also, it seems to me we can now remove the hack used for testing server before we had client support.

I've removed it in this PR (not used anymore) but I will reintroduced it in max_early_data_size PRs for negative testing purposes.

Btw, now that we have a tls13_write_early_data test function, perhaps tls13_early_data should be renamed to tls13_read_early_data.

Done

Also, why doesn't that one have a NO_INDICATION_SENT scenario like the other two? (I'm OK with considering this whole paragraph out of scope as long as it's tracked somewhere.)

Because not much happens I guess, but better with it thus I've added it.

ronald-cron-arm · 2024-02-22T19:40:45Z

@mpg @waleed-elmelegy-arm this is ready for the next round of reviews, thanks.

mpg

LGTM, thanks for addressing my feedback and answering my questions!

ronald-cron-arm · 2024-02-23T13:12:24Z

LGTM, thanks for addressing my feedback and answering my questions!

Thanks for the review and the questions. @waleed-elmelegy-arm please have a look, thanks.

mpg · 2024-02-26T07:56:00Z

Also, it seems to me we can now remove the hack used for testing server before we had client support.

I've removed it in this PR (not used anymore) but I will reintroduced it in max_early_data_size PRs for negative testing purposes.

Perhaps we shouldn't remove it then. #8854 will no longer build (when merged with development) once this is merged.

ronald-cron-arm · 2024-02-26T08:06:19Z

Also, it seems to me we can now remove the hack used for testing server before we had client support.

I've removed it in this PR (not used anymore) but I will reintroduced it in max_early_data_size PRs for negative testing purposes.

Perhaps we shouldn't remove it then. #8854 will no longer build (when merged with development) once this is merged.

I think if I keep it here the CI will fail on this PR because the function will not be used thus I guess we don't have really the choice.

mpg · 2024-02-26T08:30:36Z

Ah, good point. We'll just need to rebase 8854 when this one gets merged, then.

waleed-elmelegy-arm

Thanks for the PR, just some comments for better clarity but nothing really wrong.

waleed-elmelegy-arm · 2024-02-28T11:34:01Z

include/mbedtls/ssl.h

+ * The client has not sent the first ClientHello yet, it is unknown if the
+ * client will send an early data indication extension or not.
+ */
+    MBEDTLS_SSL_EARLY_DATA_STATUS_UNKNOWN,


I think this gives the idea that this is a state that track the actual early data not the "early data indication extension" and later on when reading the documentation or the code you realise that it's not, so maybe it's clearer to mention it in the name like with adding "EXT" or "IND" or in any other way that indicates it.

waleed-elmelegy-arm · 2024-02-28T11:36:27Z

include/mbedtls/ssl.h

+ * HelloRetryRequest) from the server yet. The transform to protect early data
+ * has been set and early data can be written now.
+ */
+    MBEDTLS_SSL_EARLY_DATA_STATUS_CAN_WRITE,


From the first glance it seems that "MBEDTLS_SSL_EARLY_DATA_STATUS_SENT" is the same as "MBEDTLS_SSL_EARLY_DATA_STATUS_CAN_WRITE" but it's not because of sending changeCipher compatibility so maybe it's better to mention it here in the comments why it's not the case since I was only able to know from the ticket discussion.

waleed-elmelegy-arm · 2024-02-28T11:42:08Z

include/mbedtls/ssl.h

+ */
+    MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT,
+    MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED,
+    MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED,


It seems to me that those three states are overlapping with the rest of the states for example "MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED" and "MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED" are both the same state as "MBEDTLS_SSL_EARLY_DATA_STATUS_SERVER_FINISHED_RECEIVED" but we still need to know the information of if the server accepted early data or not so maybe this can be split to maybe "Result" and "State" for example? or something close maybe.

Actually should we just use different enums for the return value of this function and the internal state? I think that would be clearer.

ronald-cron-arm · 2024-02-28T16:48:03Z

@waleed-elmelegy-arm thanks for your comments. The PR is already quite big and I was thinking that I could address your comments in a follow-up PR. @mpg @waleed-elmelegy-arm would you be fine with that?

mpg · 2024-02-29T07:18:24Z

Fine by me!

ronald-cron-arm added enhancement needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first component-tls13 priority-high High priority - will be reviewed soon labels Jan 29, 2024

ronald-cron-arm mentioned this pull request Jan 29, 2024

TLS1.3 write early data #6542

Closed

ronald-cron-arm force-pushed the tls13-write-early-data branch 2 times, most recently from 875bb4a to 2384374 Compare January 29, 2024 15:22

ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Jan 29, 2024

ronald-cron-arm force-pushed the tls13-write-early-data branch from 2384374 to 32b504f Compare February 5, 2024 07:59

ronald-cron-arm added the needs-ci Needs to pass CI tests label Feb 5, 2024

mpg requested review from mpg and waleed-elmelegy-arm February 6, 2024 11:10

mpg removed the needs-reviewer This PR needs someone to pick it up for review label Feb 6, 2024

ronald-cron-arm force-pushed the tls13-write-early-data branch from 32b504f to 04d82b1 Compare February 6, 2024 13:51

ronald-cron-arm linked an issue Feb 6, 2024 that may be closed by this pull request

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

Closed

1 task

xkqian and others added 11 commits February 7, 2024 08:06

tls13: cli: Add mbedtls_ssl_write_early_data() API

b62732e

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

ssl_client2: Simplify early_data option

54a3829

No need to define specific early data, the idea is rather to just send the usual request data as early data instead of standard application data. Signed-off-by: Ronald Cron <[email protected]>

ssl_client2: Move code to build http request

4e1bd47

Move code to build http request into a dedicated function. Signed-off-by: Ronald Cron <[email protected]>

ssl_client2: Switch from int to size_t

ccfaefa

Switch from int to size_t for some data lengths and counter local variables. Signed-off-by: Ronald Cron <[email protected]>

ssl_client2: Add buffer overflow check

2fe0ec8

Add buffer overflow check to build_http_request(). Signed-off-by: Ronald Cron <[email protected]>

ssl_client2: Add support for early data writing

a556189

Signed-off-by: Ronald Cron <[email protected]>

Add test case for early data writing

30bb7ce

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

tests: ssl: Add write early data unit test

2fbbba9

Signed-off-by: Ronald Cron <[email protected]>

tests: write early data: Add "not sent" scenario

8fe2b01

Signed-off-by: Ronald Cron <[email protected]>

tests: write early data: Add "server rejects" scenario

05600e2

Signed-off-by: Ronald Cron <[email protected]>

tests: write early data: Add HRR scenario

b3d42fd

Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm force-pushed the tls13-write-early-data branch from 04d82b1 to b3d42fd Compare February 7, 2024 07:07

mpg reviewed Feb 22, 2024

View reviewed changes

ronald-cron-arm mentioned this pull request Feb 22, 2024

TLS 1.3 early data: ssl-opt.sh tests with GnuTLS and MbedTLS #6362

Closed

5 tasks

ronald-cron-arm added 6 commits February 22, 2024 11:28

tests: ssl: Rename tls13_early_data to tls13_read_early_data

86d288c

Signed-off-by: Ronald Cron <[email protected]>

tests: read early data: Add no early data indication sent scenario

110303f

Signed-off-by: Ronald Cron <[email protected]>

tests: read early data: Use write API to send early data

7d158f4

Signed-off-by: Ronald Cron <[email protected]>

tls13: Improve documentation

8f1de7e

Signed-off-by: Ronald Cron <[email protected]>

tls13: Improve sanity check in get_early_data_status

f19989d

Signed-off-by: Ronald Cron <[email protected]>

tests: write early data: Improve get_early_data_status testing

dcb09ca

Signed-off-by: Ronald Cron <[email protected]>

ronald-cron-arm mentioned this pull request Feb 23, 2024

TLS 1.3: Enforce max_early_data_size on client #8857

Merged

3 tasks

mpg approved these changes Feb 23, 2024

View reviewed changes

waleed-elmelegy-arm reviewed Feb 28, 2024

View reviewed changes

waleed-elmelegy-arm approved these changes Feb 29, 2024

View reviewed changes

ronald-cron-arm added this pull request to the merge queue Feb 29, 2024

Merged via the queue into Mbed-TLS:development with commit 9b4e964 Feb 29, 2024

This was referenced Mar 1, 2024

TLS 1.3: Enforce max_early_data_size on server #8854

Merged

TLS 1.3: CLI: Split early data user status and internal state #8884

Merged

ronald-cron-arm mentioned this pull request Mar 21, 2024

TLS1.3 early data test code #8606

Closed

3 tasks

TLS 1.3: Add mbedtls_ssl_write_early_data() API #8760

TLS 1.3: Add mbedtls_ssl_write_early_data() API #8760

Uh oh!

Conversation

ronald-cron-arm commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR checklist

Uh oh!

ronald-cron-arm commented Feb 22, 2024

Uh oh!

mpg commented Feb 22, 2024

Uh oh!

ronald-cron-arm commented Feb 22, 2024

Uh oh!

mpg commented Feb 22, 2024

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronald-cron-arm commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronald-cron-arm commented Feb 22, 2024

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

ronald-cron-arm commented Feb 23, 2024

Uh oh!

mpg commented Feb 26, 2024

Uh oh!

ronald-cron-arm commented Feb 26, 2024

Uh oh!

mpg commented Feb 26, 2024

Uh oh!

waleed-elmelegy-arm left a comment

Choose a reason for hiding this comment

Uh oh!

waleed-elmelegy-arm Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

waleed-elmelegy-arm Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

waleed-elmelegy-arm Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

mpg Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

ronald-cron-arm commented Feb 28, 2024

Uh oh!

mpg commented Feb 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ronald-cron-arm commented Jan 29, 2024 •

edited

Loading

ronald-cron-arm commented Feb 22, 2024 •

edited

Loading