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:finalize tls13 serialize session save and load #6123

Conversation

yuhaoth
Copy link
Contributor

@yuhaoth yuhaoth commented Jul 23, 2022

Description

Fix #6122

Address left comments in #6087

Status

READY

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

@yuhaoth yuhaoth added component-tls13 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 labels Jul 23, 2022
@yuhaoth yuhaoth changed the title Pr/finialize tls13 serialize session save load TLS 1.3:finialize tls13 serialize session save load Jul 23, 2022
ronald-cron-arm added a commit that referenced this pull request Jul 23, 2022
…save_load

TLS 1.3: Add serialize session save load
I can see that #6087 (comment) and #6087 (comment) are addressed in  #6123. Thus I am ok to merge it as it is.
@yuhaoth yuhaoth force-pushed the pr/finialize-tls13-serialize_session_save_load branch from 0c26766 to e363e22 Compare July 23, 2022 07:43
#endif

return( used );
return( 0 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Curius as to why we setting the MBEDTLS_CHECK_RETURN_CRITICAL attribute and then returning always 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MBEDTLS_SSL_CHK_BUF_PTR will return buffer too small error.

Copy link
Contributor

Choose a reason for hiding this comment

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

So aligned it with the behaviour of ssl_tls13_session_save of always returning 0 and if anything fails it will be caught on the ssl_session_save. Got it thanks.

@yuhaoth yuhaoth force-pushed the pr/finialize-tls13-serialize_session_save_load branch from e363e22 to aff7737 Compare July 29, 2022 02:38
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
tests/suites/test_suite_ssl.function Outdated Show resolved Hide resolved
@yuhaoth yuhaoth removed the needs-reviewer This PR needs someone to pick it up for review label Jul 29, 2022
@yuhaoth yuhaoth requested a review from xkqian July 29, 2022 15:22
minosgalanakis
minosgalanakis previously approved these changes Aug 1, 2022
@yuhaoth
Copy link
Contributor Author

yuhaoth commented Aug 10, 2022

Rebased

library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
@yuhaoth yuhaoth requested a review from xkqian August 11, 2022 07:38
minosgalanakis
minosgalanakis previously approved these changes Aug 12, 2022
Copy link
Contributor

@minosgalanakis minosgalanakis 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
xkqian previously approved these changes Aug 12, 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

@yuhaoth yuhaoth added the approved Design and code approved - may be waiting for CI or backports label Aug 14, 2022
@ronald-cron-arm ronald-cron-arm changed the title TLS 1.3:finialize tls13 serialize session save load TLS 1.3:finalize tls13 serialize session save and load Aug 16, 2022
library/ssl_tls.c Outdated Show resolved Hide resolved
@yuhaoth yuhaoth dismissed stale reviews from xkqian and minosgalanakis via 6f6030b August 17, 2022 13:44
@yuhaoth yuhaoth force-pushed the pr/finialize-tls13-serialize_session_save_load branch from cb21a0f to 6f6030b Compare August 17, 2022 13:44
@yuhaoth
Copy link
Contributor Author

yuhaoth commented Aug 17, 2022

Rewrite base on @ronald-cron-arm 's comments

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth yuhaoth force-pushed the pr/finialize-tls13-serialize_session_save_load branch from 6f6030b to e36fdd6 Compare August 17, 2022 13:50
@yuhaoth yuhaoth removed the approved Design and code approved - may be waiting for CI or backports label Aug 18, 2022
@yuhaoth yuhaoth force-pushed the pr/finialize-tls13-serialize_session_save_load branch from 12f8e96 to 35b1730 Compare August 18, 2022 03:12
From RFC 8446 and the definition of session, we
should check the length.

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth yuhaoth force-pushed the pr/finialize-tls13-serialize_session_save_load branch from 35b1730 to 5b7c7ca Compare August 18, 2022 03:29
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.

A few coding style issues. Otherwise this looks good to me.

library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
library/ssl_tls.c Outdated Show resolved Hide resolved
tests/suites/test_suite_ssl.function Outdated Show resolved Hide resolved
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
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 a look to this version.

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

@ronald-cron-arm ronald-cron-arm merged commit f3f6b0a into Mbed-TLS:development Aug 19, 2022
@yuhaoth yuhaoth deleted the pr/finialize-tls13-serialize_session_save_load branch August 30, 2022 01:49
yuhaoth added a commit to yuhaoth/mbedtls1.3 that referenced this pull request Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-tls13 needs-review Every commit must be reviewed by at least two team members,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLS 1.3: PR 6087 follow-up
4 participants