Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TLS 1.3: Refactor early data configuration interface. #6537

Conversation

yuhaoth
Copy link
Contributor

@yuhaoth yuhaoth commented Nov 4, 2022

Description

fix #6338 ,
fix #6347

preceding #6621

  • Add mbedtls_ssl_tls13_conf_max_early_data_size .
    • Config max_early_data_size parameter

Gatekeeper checklist

  • changelog not required, part of adding early data support in TLS 1.3.
  • backport not required
  • tests not required at this stage

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

@yuhaoth yuhaoth added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review component-tls13 priority-high High priority - will be reviewed soon labels Nov 4, 2022
@yuhaoth yuhaoth force-pushed the pr/tls13-refactor-early-data-configuration-interface branch from 8786ba1 to 1bc6ea2 Compare November 5, 2022 15:15
@yuhaoth yuhaoth force-pushed the pr/tls13-refactor-early-data-configuration-interface branch 7 times, most recently from 408eccd to 60647a6 Compare November 12, 2022 02:14
@yuhaoth yuhaoth force-pushed the pr/tls13-refactor-early-data-configuration-interface branch from 60647a6 to f5d4b3b Compare November 16, 2022 02:19
@yuhaoth yuhaoth force-pushed the pr/tls13-refactor-early-data-configuration-interface branch from f5d4b3b to a392b0a Compare November 17, 2022 10:05
@xkqian
Copy link
Contributor

xkqian commented Nov 18, 2022

Seems some conflict files, would you rebase it firstly?

@xkqian xkqian self-requested a review November 18, 2022 09:11
@yuhaoth yuhaoth force-pushed the pr/tls13-refactor-early-data-configuration-interface branch from a392b0a to 4f66b6e Compare November 19, 2022 12:08
@yuhaoth yuhaoth removed the needs-ci Needs to pass CI tests label Nov 20, 2022
@yuhaoth
Copy link
Contributor Author

yuhaoth commented Nov 21, 2022

It's for early review. @xkqian , please pay attention what we really needed .
And it is conflict with #6621. Please evaluate the impact on client side and if it is reasonable.

@yuhaoth
Copy link
Contributor Author

yuhaoth commented Nov 21, 2022

OpenCI interrupt some test cases and raise channel unavailable. NOT code relative fail.

@yuhaoth yuhaoth force-pushed the pr/tls13-refactor-early-data-configuration-interface branch 3 times, most recently from a473fc0 to fa24cc4 Compare November 22, 2022 06:02
include/mbedtls/mbedtls_config.h Outdated Show resolved Hide resolved
include/mbedtls/mbedtls_config.h Show resolved Hide resolved
include/mbedtls/mbedtls_config.h Show resolved Hide resolved
include/mbedtls/ssl.h Outdated Show resolved Hide resolved
include/mbedtls/ssl.h Outdated Show resolved Hide resolved
programs/ssl/ssl_server2.c Outdated Show resolved Hide resolved
programs/ssl/ssl_server2.c Outdated Show resolved Hide resolved
programs/ssl/ssl_server2.c Outdated Show resolved Hide resolved
programs/ssl/ssl_server2.c Outdated Show resolved Hide resolved
include/mbedtls/ssl.h Outdated Show resolved Hide resolved
@yuhaoth yuhaoth requested a review from xkqian December 5, 2022 07:47
xkqian
xkqian previously approved these changes Dec 5, 2022
Copy link
Contributor

@xkqian xkqian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

programs/ssl/ssl_server2.c Outdated Show resolved Hide resolved
include/mbedtls/ssl.h Outdated Show resolved Hide resolved
include/mbedtls/mbedtls_config.h Outdated Show resolved Hide resolved
include/mbedtls/mbedtls_config.h Outdated Show resolved Hide resolved
include/mbedtls/check_config.h Outdated Show resolved Hide resolved
include/mbedtls/ssl.h Outdated Show resolved Hide resolved
include/mbedtls/ssl.h Outdated Show resolved Hide resolved
include/mbedtls/ssl.h Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
- disable reuse of max_early_data_size.
- make conf_early_data available for server.
- various comment issues

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
This reverts commit a693477.

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth yuhaoth force-pushed the pr/tls13-refactor-early-data-configuration-interface branch from 2a248f3 to 3a8c593 Compare December 6, 2022 04:53
@yuhaoth yuhaoth force-pushed the pr/tls13-refactor-early-data-configuration-interface branch from 3a8c593 to 0ad50d2 Compare December 6, 2022 05:49
`conf_max_early_data_size` does not reuse as en/disable. When
call it, we should call `conf_early_data()` also.

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth yuhaoth force-pushed the pr/tls13-refactor-early-data-configuration-interface branch from 0ad50d2 to d146a37 Compare December 6, 2022 06:57
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. One last thing on my side.

library/ssl_tls.c Outdated Show resolved Hide resolved
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
- early_data default to disable
- max_early_data_size default to built-in value

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth yuhaoth force-pushed the pr/tls13-refactor-early-data-configuration-interface branch from 18415e8 to 6ee56aa Compare December 6, 2022 10:01
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @xkqian please have another look.

@@ -129,6 +129,7 @@ int main( void )
#define DFL_SNI NULL
#define DFL_ALPN_STRING NULL
#define DFL_CURVES NULL
#define DFL_MAX_EARLY_DATA_SIZE 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See line 430, Should it be -1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument max_early_data_size is reused for disable/enable , that's different with conf->max_early_data_size and opt.max_early_data_size.
I add tls13_early_data_enabled to represent disable/enabled and opt.max_early_data_size is same meaning with conf->max_early_data_size. So here should be 0

else if( strcmp( p, "max_early_data_size" ) == 0 )
{
long long value = atoll( q );
tls13_early_data_enabled =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common question, in server side, it seems we check the max_early_data_size to decide whether we enable early data or not, is that what we need?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this point has been changed. mbetls_ssl_tls13_conf_early_data is available for server also now.

Copy link
Contributor

@xkqian xkqian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yuhaoth yuhaoth added the approved Design and code approved - may be waiting for CI or backports label Dec 7, 2022
@yuhaoth
Copy link
Contributor Author

yuhaoth commented Dec 7, 2022

OpenCI cancel all tests without reason.

Internal CI and travisCI pass

@ronald-cron-arm ronald-cron-arm merged commit fbba0e9 into Mbed-TLS:development Dec 7, 2022
@yuhaoth yuhaoth deleted the pr/tls13-refactor-early-data-configuration-interface branch December 6, 2023 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-tls13 needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon
Projects
None yet
3 participants