TLS 1.3: Refine and test client early data status #8755

ronald-cron-arm · 2024-01-26T17:49:19Z

Description

First 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: Ronald Cron <ronald.cron@arm.com>

Fix cases where the client was sending two CCS, no harm but better to send only one. Prevent to send even more CCS when early data are involved without having to add conditional state transitions. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

The main purpose of the change is to know from the status, at any point in the handshake, if early data can be sent or not and why. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

mpg

Looks pretty good to me! I only have minor suggestions for improvements (mostly comments, naming, etc.) but everything looks correct and well-tested to me.

mpg · 2024-02-13T09:16:30Z

library/ssl_tls13_client.c

-            if (ret == 0) {
-                mbedtls_ssl_handshake_set_state(ssl, MBEDTLS_SSL_CLIENT_HELLO);
+            ret = 0;
+            if (ssl->handshake->ccs_count == 0) {


Suggestion: I think we could put that check inside mbedtls_ssl_tls13_write_change_cipher_spec() (which would do nothing and return 0 if CCS has already been sent). This would avoid having to repeat this at each call site, saving a few bytes of code size but more importantly making sure we don't forget the check.

Done in #8760 commit "tls13: client: Improve CCS handling"

mpg · 2024-02-13T09:19:15Z

library/ssl_misc.h

+     *  one CCS per handshake without having to complicate the handshake state
+     *  transitions.
+     */
+    uint8_t ccs_count;


I find the name slightly misleading, since we expect to send no more than once. I'd prefer ccs_sent. (Said otherwise, if an integer variable's only legal values are 0 and 1, I prefer to think of it as a boolean rather than a counter.)

I agree with Manuel, but minor, can be done in a follow-up PR

I agree as well, done in #8760 commit "tls13: Use a flag not a counter for CCS and HRR handling".

mpg · 2024-02-13T09:27:33Z

include/mbedtls/ssl.h

-#define MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT  0
-#define MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED  1
-#define MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED  2
+#define MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT  1


Pre-exisiting: it would be good to have comment here like we have for the other values defined in ssl_misc.h. In particular, out of context it might not be clear if STATUS_NOT_SENT means the extension has not been sent, or early data itself has not been sent.

Btw, maybe renaming to STATUS_NOT_OFFERED or STATUS_EXT_NOT_SENT would make that clearer? (In addition to comments I mean.)

Edit, after reading more of the code: IIUC this actually means "we decided not to send it" as opposed to "we haven't sent it yet but might send it in the future". This only strengthens my point about adding a comment to make that clear.

(pre-existing, minor) feels like this should be an enum

see #8755 (comment)

mpg · 2024-02-13T09:29:36Z

library/ssl_tls13_client.c

-        ssl_tls13_early_data_has_valid_ticket(ssl) &&
-        ssl->conf->early_data_enabled == MBEDTLS_SSL_EARLY_DATA_ENABLED &&
-        ssl->handshake->hello_retry_request_count == 0) {
+    if (ssl->handshake->hello_retry_request_count == 0) {


Pre-existing, but: can there really be more that on HRR per handshake? If not, I'd also suggest making that a boolean instead as I think that count may mislead readers into thinking there may be more than one. (I'll agree if you say that's out of scope.)

I agree, done in #8760 commit "tls13: Use a flag not a counter for CCS and HRR handling".

mpg · 2024-02-13T09:36:44Z

library/ssl_tls13_client.c

-        ssl->conf->early_data_enabled == MBEDTLS_SSL_EARLY_DATA_ENABLED &&
-        ssl->handshake->hello_retry_request_count == 0) {
+    if (ssl->handshake->hello_retry_request_count == 0) {
+        if (mbedtls_ssl_conf_tls13_is_some_psk_enabled(ssl) &&


[edit: skip to last paragraph, only requesting adding a comment after all]

I'm not sure I understand or agree with the logic here:

if no_hrr: if other_conditions: send it and mark as sent else: mark as not sent

So, it there was an HRR in this handshake, we will not send the extension and not update the status to "not sent" either? Is that intentional? I'd have expected:

if no_hrr and other_conditions: send it and mark as sent else: mark as not sent

What am I missing?

Edit, after reading more of the code: I think what I was missing is that if there was an HRR earlier, status has already been updated to "rejected" and we want to leave it at that. So, now I think I understand and agree with the logic here, but a comment would be welcome.

Done in #8760 commit "tls13: client: Add comment about early data in 2nd ClientHello".

mpg · 2024-02-13T09:48:36Z

library/ssl_misc.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.
+ */
+#define MBEDTLS_SSL_EARLY_DATA_STATUS_UNKNOWN 0


Note: the documentation for the early_data_status field still says Reset to #MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT - does it need updating?

Fixed in #8760, commit "tls13: Fix/Improve comments"

mpg · 2024-02-13T09:49:17Z

library/ssl_misc.h

@@ -2145,6 +2145,38 @@ int mbedtls_ssl_tls13_write_early_data_ext(mbedtls_ssl_context *ssl,
                                           unsigned char *buf,
                                           const unsigned char *end,
                                           size_t *out_len);
+
+#if defined(MBEDTLS_SSL_CLI_C)


Note: the definitions in ssl.h are not guarded by this, should they be?

(minor) I'd prefer to see them defined in one place, even if values are defined that aren't used under some circumstances - they are well-enough namespaced

see #8755 (comment)

tests/suites/test_suite_ssl.function

tom-cosgrove-arm

LGTM - I only have minor comments, and given @mpg's review is similar, and these commits are in another PR that is under review, it seems best to merge this as-is and address those comments in a follow-on PR

tom-cosgrove-arm · 2024-02-13T16:42:58Z

include/mbedtls/ssl.h

-#define MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT  0
-#define MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED  1
-#define MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED  2
+#define MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT  1


(pre-existing, minor) feels like this should be an enum

tom-cosgrove-arm · 2024-02-13T16:43:31Z

library/ssl_misc.h

+     *  one CCS per handshake without having to complicate the handshake state
+     *  transitions.
+     */
+    uint8_t ccs_count;


I agree with Manuel, but minor, can be done in a follow-up PR

tom-cosgrove-arm · 2024-02-13T16:44:37Z

library/ssl_misc.h

+
+#if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE)
+    /**
+     *  Number of dummy change_cipher_spec (CCS) record sent. Used to send only


(minor, fix in follow-up) should be plural records sent. Also, there seem to be two spaces after the * on each line :)

comment changed in #8760

tom-cosgrove-arm · 2024-02-13T16:48:48Z

library/ssl_misc.h

+ * ClientHello, the server has accepted them and the client has received the
+ * server Finished message. It cannot send early data to the server anymore.
+ */
+#define MBEDTLS_SSL_EARLY_DATA_STATUS_SERVER_FINISHED_RECEIVED 6


(for follow-up) If these are all possible values of early_data_status i.e. return values from mbedtls_ssl_get_early_data_status() could they be defined together, ideally in an enum?

mbedtls_ssl_get_early_data_status() returns only NOT_SENT or ACCEPTED or REJECTED. The other values are for internal use only. See also #8755 (comment).

tom-cosgrove-arm · 2024-02-13T16:50:06Z

library/ssl_misc.h

@@ -2145,6 +2145,38 @@ int mbedtls_ssl_tls13_write_early_data_ext(mbedtls_ssl_context *ssl,
                                           unsigned char *buf,
                                           const unsigned char *end,
                                           size_t *out_len);
+
+#if defined(MBEDTLS_SSL_CLI_C)


(minor) I'd prefer to see them defined in one place, even if values are defined that aren't used under some circumstances - they are well-enough namespaced

tom-cosgrove-arm · 2024-02-13T16:51:23Z

tests/include/test/ssl_helpers.h

+    mbedtls_test_handshake_test_options *client_options,
+    mbedtls_test_handshake_test_options *server_options,
+    mbedtls_ssl_session *session);
+#endif /* MBEDTLS_SSL_CLI_C && MBEDTLS_SSL_SRV_C &&


(minor) we don't really need the list of conditions in the #endif since it's so close to the #if - it clutters the text, and if the condition needs changing in the future it's an unnecessary thing to have to keep in sync

Done in #8760, commit "tls13: Fix/Improve comments"

tom-cosgrove-arm · 2024-02-13T16:58:40Z

tests/suites/test_suite_ssl.function

+                            TEST_EQUAL(client_ep.ssl.early_data_status,
+                                       MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED);
+                        }
+                        break;


I wonder if we should have default: TEST_FAIL("Missing scenario"); in these in case we extend the set of scenarios and forget to update one of the switches in the future?

Yes that should reduce the risk of missing something when adding a scenario. Done in #8760, last commit.

tom-cosgrove-arm · 2024-02-13T20:49:49Z

Thank you @mpg!

ronald-cron-arm · 2024-02-15T14:31:53Z

@mpg and @tom-cosgrove-arm thanks for the review.

I have addressed your comments in five additional commits in #8760 (thanks for its rebase). Regarding the definition of MBEDTLS_SSL_EARLY_DATA_STATUS_xyz (various comments about it) I've tried to improve things (penultimate commit). MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT/ACCEPTED/REJECTED documentation is still in the documentation of mbedtls_ssl_get_early_data_status() as they are the possible values (apart from errors) returned by this API function. MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT/ACCEPTED/REJECTED are now defined in the mbedtls_ssl_early_data_status enum along with the other values that are for internal use only. The enum itself is also for internal use only.

ronald-cron-arm added enhancement needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first component-tls13 labels Jan 26, 2024

ronald-cron-arm force-pushed the tls13-client-early-data-status branch from 80120b7 to 60c27aa Compare January 26, 2024 18:51

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

TLS 1.3: Add mbedtls_ssl_write_early_data() API #8760

Merged

3 tasks

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 priority-high High priority - will be reviewed soon and removed needs-ci Needs to pass CI tests 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-client-early-data-status branch from 60c27aa to 90f50c4 Compare February 5, 2024 07:53

ronald-cron-arm added needs-ci Needs to pass CI tests and removed needs-preceding-pr Requires another PR to be merged first labels Feb 5, 2024

ronald-cron-arm force-pushed the tls13-client-early-data-status branch 2 times, most recently from 5ca8eae to b9127dc Compare February 6, 2024 08:14

mpg self-requested a review February 6, 2024 11:09

mpg removed the needs-ci Needs to pass CI tests label Feb 6, 2024

mpg requested a review from tom-cosgrove-arm February 6, 2024 11:11

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

ronald-cron-arm added 7 commits February 6, 2024 16:43

tests: ssl: Move group list to options

fb53647

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

tests: ssl: First reset to all zeroes options in init

b4ad3e7

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

tests: ssl: Add early data handshake option

ced99be

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

tests: ssl: Add helper function to get a TLS 1.3 ticket

1f6e4e4

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

tests: ssl: Use get TLS 1.3 ticket helper for early data test

bfcdc06

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

tls13: Send dummy CCS only once

fe59ff7

Fix cases where the client was sending two CCS, no harm but better to send only one. Prevent to send even more CCS when early data are involved without having to add conditional state transitions. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

tls13: cli: Refine early data status

90e2233

The main purpose of the change is to know from the status, at any point in the handshake, if early data can be sent or not and why. Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm added 5 commits February 6, 2024 16:43

tests: ssl: Add early data status unit test

a7f94e4

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

tests: ssl: Add scenario param to early data status testing function

5c208d7

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

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

265273e

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

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

d6dba67

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

tests: early data status: Add HRR scenario

2261ab2

Signed-off-by: Ronald Cron <ronald.cron@arm.com>

ronald-cron-arm force-pushed the tls13-client-early-data-status branch from b9127dc to 2261ab2 Compare February 6, 2024 15:44

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

mpg reviewed Feb 13, 2024

View reviewed changes

tom-cosgrove-arm approved these changes Feb 13, 2024

View reviewed changes

mpg approved these changes Feb 13, 2024

View reviewed changes

mpg added this pull request to the merge queue Feb 13, 2024

Merged via the queue into Mbed-TLS:development with commit e6c80bc Feb 13, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS 1.3: Refine and test client early data status #8755

TLS 1.3: Refine and test client early data status #8755

ronald-cron-arm commented Jan 26, 2024 •

edited

Loading

mpg left a comment

mpg Feb 13, 2024

ronald-cron-arm Feb 15, 2024

mpg Feb 13, 2024

tom-cosgrove-arm Feb 13, 2024

ronald-cron-arm Feb 15, 2024

mpg Feb 13, 2024

tom-cosgrove-arm Feb 13, 2024

ronald-cron-arm Feb 15, 2024

mpg Feb 13, 2024

ronald-cron-arm Feb 15, 2024

mpg Feb 13, 2024 •

edited

Loading

ronald-cron-arm Feb 15, 2024 •

edited

Loading

mpg Feb 13, 2024

ronald-cron-arm Feb 15, 2024

mpg Feb 13, 2024

tom-cosgrove-arm Feb 13, 2024

ronald-cron-arm Feb 15, 2024

tom-cosgrove-arm left a comment •

edited

Loading

tom-cosgrove-arm Feb 13, 2024

tom-cosgrove-arm Feb 13, 2024

tom-cosgrove-arm Feb 13, 2024

ronald-cron-arm Feb 15, 2024

tom-cosgrove-arm Feb 13, 2024

ronald-cron-arm Feb 15, 2024

tom-cosgrove-arm Feb 13, 2024

tom-cosgrove-arm Feb 13, 2024

ronald-cron-arm Feb 15, 2024

tom-cosgrove-arm Feb 13, 2024

ronald-cron-arm Feb 15, 2024 •

edited

Loading

tom-cosgrove-arm commented Feb 13, 2024

ronald-cron-arm commented Feb 15, 2024 •

edited

Loading

TLS 1.3: Refine and test client early data status #8755

TLS 1.3: Refine and test client early data status #8755

Conversation

ronald-cron-arm commented Jan 26, 2024 • edited Loading

Description

PR checklist

mpg left a 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

Choose a reason for hiding this comment

mpg Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm Feb 15, 2024 • edited Loading

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

tom-cosgrove-arm left a comment • edited Loading

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

ronald-cron-arm Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

tom-cosgrove-arm commented Feb 13, 2024

ronald-cron-arm commented Feb 15, 2024 • edited Loading

ronald-cron-arm commented Jan 26, 2024 •

edited

Loading

mpg Feb 13, 2024 •

edited

Loading

ronald-cron-arm Feb 15, 2024 •

edited

Loading

tom-cosgrove-arm left a comment •

edited

Loading

ronald-cron-arm Feb 15, 2024 •

edited

Loading

ronald-cron-arm commented Feb 15, 2024 •

edited

Loading