[WIP]Re-introduce support of TLS 1.3 #290
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch is to confirm how to merge tls1.3 prototype branch Change-Id: I821e2fa75ffcacd03671e54faab0d19d2c8bd952 CustomizedGitHooks: yes Signed-off-by: Jerry Yu <[email protected]>
hanno-becker
changed the base branch from
tls13-prototype
to
tls13-prototype-backup180520
hanno-becker
changed the base branch from
tls13-prototype-backup180520
to
tls13-prototype
| #if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL) | ||
| static inline int check_version_config(const mbedtls_ssl_config *conf) | ||
| { | ||
| if(conf->max_minor_ver==MBEDTLS_SSL_MINOR_VERSION_4 |
There was a problem hiding this comment.
Minor: Please make sure to adhere to the Mbed TLS coding style.
Comment on lines
+4422
to
+4428
| if(conf->max_minor_ver==MBEDTLS_SSL_MINOR_VERSION_4 | ||
| && conf->min_minor_ver == MBEDTLS_SSL_MINOR_VERSION_4) | ||
| return 0; | ||
| if(conf->max_minor_ver!=MBEDTLS_SSL_MINOR_VERSION_4 | ||
| && conf->min_minor_ver != MBEDTLS_SSL_MINOR_VERSION_4) | ||
| return 0; | ||
| return 1; |
There was a problem hiding this comment.
Suggested change
| if(conf->max_minor_ver==MBEDTLS_SSL_MINOR_VERSION_4 | |
| && conf->min_minor_ver == MBEDTLS_SSL_MINOR_VERSION_4) | |
| return 0; | |
| if(conf->max_minor_ver!=MBEDTLS_SSL_MINOR_VERSION_4 | |
| && conf->min_minor_ver != MBEDTLS_SSL_MINOR_VERSION_4) | |
| return 0; | |
| return 1; | |
| if( conf->max_minor_ver == MBEDTLS_SSL_MINOR_VERSION_4 && | |
| conf->min_minor_ver == MBEDTLS_SSL_MINOR_VERSION_4 ) | |
| { | |
| return( 0 ); | |
| } | |
| if( conf->max_minor_ver != MBEDTLS_SSL_MINOR_VERSION_4 && | |
| conf->min_minor_ver != MBEDTLS_SSL_MINOR_VERSION_4 ) | |
| { | |
| return( 0 ); | |
| } | |
| return( 1 ); |
| @@ -4416,7 +4416,18 @@ static void ssl_mps_free( mbedtls_ssl_context *ssl ) | |||
| mps_alloc_free( &ssl->mps.alloc ); | |||
| } | |||
| #endif /* MEDTLS_SSL_USE_MPS */ | |||
|
|
|||
| #if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL) | |||
| static inline int check_version_config(const mbedtls_ssl_config *conf) | |||
There was a problem hiding this comment.
Can you give this function a more descriptive name?
| @@ -4426,6 +4437,11 @@ int mbedtls_ssl_setup( mbedtls_ssl_context *ssl, | |||
| { | |||
| int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; | |||
|
|
|||
| #if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL) | |||
| if(check_version_config(conf)) | |||
| return( MBEDTLS_ERR_SSL_BAD_CONFIG ); | |||
There was a problem hiding this comment.
MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE seems more appropriate I think.
| @@ -4426,6 +4437,11 @@ int mbedtls_ssl_setup( mbedtls_ssl_context *ssl, | |||
| { | |||
| int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; | |||
|
|
|||
| #if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL) | |||
| if(check_version_config(conf)) | |||
There was a problem hiding this comment.
Can you add a short comment on what we're checking here and how it is going to change in the future?
| { | ||
| #if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL) | ||
|
|
||
| if(ssl->conf->max_major_ver==MBEDTLS_SSL_MAJOR_VERSION_3 |
There was a problem hiding this comment.
Trivia: Coding style
| mbedtls_ssl_conf_min_version( &conf, MBEDTLS_SSL_MAJOR_VERSION_3, | ||
| opt.min_version ); | ||
|
|
||
| if( opt.max_version != DFL_MAX_VERSION ) | ||
| // TAG for Jerry Yu, This is important for TLS1.3 now |
There was a problem hiding this comment.
Can this comment be removed?
hanno-becker
requested changes
Jul 20, 2021
There was a problem hiding this comment.
The approach looks good to me, thank you @yuhaoth
If I understand correctly, the PR still needs to rename the handshake state machine functions in ssl_tls13_{client,server}.c?
|
Closing in favor of #293 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is to confirm how to introduce support of TLS 1.3