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: Add definition of mbedtls_ssl_{write,read}_early_data #6621

Merged

Conversation

ronald-cron-arm
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm commented Nov 18, 2022

Description

Add definition of mbedtls_ssl_{write,read}_early_data to write and read early data.
Fix #4902
Fix #6310

Gatekeeper checklist

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

@ronald-cron-arm ronald-cron-arm added enhancement needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests component-tls13 priority-high High priority - will be reviewed soon labels Nov 18, 2022
Copy link
Contributor

@yuhaoth yuhaoth left a comment

Choose a reason for hiding this comment

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

Some nitpick comments. Very details :)

int mbedtls_ssl_write_early_data( mbedtls_ssl_context *ssl,
const unsigned char *buf, size_t len );

#define MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT 0
Copy link
Contributor

Choose a reason for hiding this comment

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

MBEDTLS_SSL_EARLY_DATA_STATUS_CAN_DO means SSL context allow early data, any negative value means not allow.
MBEDTLS_SSL_EARLY_DATA_STATUS_ALLOW_SEND, I am not sure if it should be expose to user. that means mbedtls_ssl_write_early_data can called without error report. If not expose that, we should call mbedtls_ssl_handshake_step inside mbedtls_ssl_write_early_data

Suggested change
#define MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT 0
#define MBEDTLS_SSL_EARLY_DATA_STATUS_CAN_DO 0
#define MBEDTLS_SSL_EARLY_DATA_STATUS_ALLOW_SEND 3

Copy link
Contributor Author

@ronald-cron-arm ronald-cron-arm Nov 21, 2022

Choose a reason for hiding this comment

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

The SSL context is set-up by the application and if it wants to send early data it has to set it up with a proper PSK, enabling early data with mbedtls_ssl_tls13_conf_early_data() ... Thus the application is aware if the SSL context potentially allow early data or not. That's why there is currently no API to tell it so.

Setting-up the SSL context is not trivial though thus we should at least have clear debug logs when early data have been enabled through mbedtls_ssl_tls13_conf_early_data() but eventually early data cannot be sent.

If not expose that, we should call mbedtls_ssl_handshake_step inside mbedtls_ssl_write_early_data

That's the plan like in mbedtls_ssl_write() to call mbedtls_ssl_handshake() in mbedtls_ssl_write_early_data().

Copy link
Contributor

@yuhaoth yuhaoth Nov 21, 2022

Choose a reason for hiding this comment

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

One more question. Is below code expected ?

int mbedtls_ssl_get_early_data_status( mbedtls_ssl_context *ssl )
{
    if( is_server || handshake_is_not_over )
        return( BAD_INPUT_DATA );
    return( ssl->early_data_status );
}

include/mbedtls/ssl.h Show resolved Hide resolved
library/ssl_tls13_client.c Show resolved Hide resolved
include/mbedtls/ssl.h Show resolved Hide resolved
include/mbedtls/ssl.h Show resolved Hide resolved
Comment on lines +4934 to +4941
int mbedtls_ssl_read_early_data( mbedtls_ssl_context *ssl,
unsigned char *buf, size_t len );
Copy link
Contributor

@yuhaoth yuhaoth Nov 20, 2022

Choose a reason for hiding this comment

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

I think we need a internal buffer for early data, before it is called, early data should be saved.

In this case, should we change mbedtls_ssl_read ? If early data buffer is not empty, we should return early data .

Copy link
Contributor Author

@ronald-cron-arm ronald-cron-arm Nov 21, 2022

Choose a reason for hiding this comment

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

The plan is for mbedtls_ssl_read_early_data() to be called when the handshake is not started and for mbedtls_ssl_read_early_data() to initiate it. Thus when we receive early data while the handshake is on-going we have a buffer (the buffer passed to mbedtls_ssl_read_early_data()) to store the early data. See the pseudo-code I have added in tls13-support.md.

Otherwise mbedtls_ssl_read does not and cannot return early data as it reads data when the handshake is over.

Copy link
Contributor

@yuhaoth yuhaoth Nov 21, 2022

Choose a reason for hiding this comment

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

If the early data buffer is not empty when handshake is over, we should free it , right? it must be read by mbedtls_ssl_read_early_data.

yuhaoth
yuhaoth previously approved these changes Nov 21, 2022
Copy link
Contributor

@yuhaoth yuhaoth left a comment

Choose a reason for hiding this comment

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

LGTM.

There are still two questions to confirm the exactly meaning in this PR

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.

I am not sure whether I have fully understood it or not.
I think we need one internal buffer and internal API like mbedtls_ssl_set_early_data() to copy the early data to it.
Our ssl_client2 is one single thread, I think we only have chance to write the early data in the first flight of handshake, which is between early key generated and end of early data received. And at this time, we are during the handshake step. and application will have no chance to transfer the data buffer to it.
Maybe this will be realted with the change of the application, anyway, I am something confused, can you help me? thanks.

@yuhaoth
Copy link
Contributor

yuhaoth commented Nov 23, 2022

I am not sure whether I have fully understood it or not. I think we need one internal buffer and internal API like mbedtls_ssl_set_early_data() to copy the early data to it. Our ssl_client2 is one single thread, I think we only have chance to write the early data in the first flight of handshake, which is between early key generated and end of early data received. And at this time, we are during the handshake step. and application will have no chance to transfer the data buffer to it. Maybe this will be realted with the change of the application, anyway, I am something confused, can you help me? thanks.

Do you want to change something in this PRs?

As the discussion in this PR, handshake_step will be called in mbedtls_ssl_write_early_data that is expected behavior. Inside mbedtls_ssl_write_early_data , if early_data_status == REJECT and ssl->state < SERVER_FINISHED , early data can be sent. That is described in https://docs.google.com/document/d/1h0Uf9ULJA0IgHR2dWhNynatjihQEN5IiFkvmkZD_ikc/edit#heading=h.9do2ve21ciye and https://github.com/hannestschofenig/mbedtls/blob/3a50d364a977a61914c0d34d560c207eb71734d9/library/ssl_msg.c

@xkqian
Copy link
Contributor

xkqian commented Nov 23, 2022

one conflic happens, I will approve it after it's resovled.

@xkqian
Copy link
Contributor

xkqian commented Nov 23, 2022

Do you want to change something in this PRs?

As the discussion in this PR, handshake_step will be called in mbedtls_ssl_write_early_data that is expected behavior. Inside mbedtls_ssl_write_early_data , if early_data_status == REJECT and ssl->state < SERVER_FINISHED , early data can be sent. That is described in https://docs.google.com/document/d/1h0Uf9ULJA0IgHR2dWhNynatjihQEN5IiFkvmkZD_ikc/edit#heading=h.9do2ve21ciye and https://github.com/hannestschofenig/mbedtls/blob/3a50d364a977a61914c0d34d560c207eb71734d9/library/ssl_msg.c

I don't want to change anything, just want to learn more about it and talk abou the application desgin here, seems it's not a right place here.

Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm
Copy link
Contributor Author

I don't want to change anything, just want to learn more about it and talk abou the application desgin here, seems it's not a right place here.

That's the right place, I just haven't found the time to answer yet. I will try to have a look tomorrow morning.

@ronald-cron-arm
Copy link
Contributor Author

ronald-cron-arm commented Nov 23, 2022

I am not sure whether I have fully understood it or not. I think we need one internal buffer and internal API like mbedtls_ssl_set_early_data() to copy the early data to it. Our ssl_client2 is one single thread, I think we only have chance to write the early data in the first flight of handshake, which is between early key generated and end of early data received. And at this time, we are during the handshake step. and application will have no chance to transfer the data buffer to it. Maybe this will be realted with the change of the application, anyway, I am something confused, can you help me? thanks.

Did you have a look to the documentation I added in tls13-support.md which is about how an application would use mbedtls_ssl_write_early_data(). The idea is for the application to call mbedtls_ssl_write_early_data() with an ssl context for which the handshake as not started yet. mbedtls_ssl_write_early_data() starts the handshake and when the handshake is in the right state starts sending the early data that has been passed to it. I know that ssl_client2 is not structured like that: first it does the handshake, then does something else ... To test early data in ssl_client2, we will have to add a different path of execution.

As mentioned by @yuhaoth in https://github.com/hannestschofenig/mbedtls/blob/3a50d364a977a61914c0d34d560c207eb71734d9/library/ssl_msg.c you can find an implementation of mbedtls_ssl_write_early_data() that's probably close to what we will do eventually (if you wonder how mbedtls_ssl_write_early_data() as described in this PR could be implemented).

Copy link
Contributor

@yuhaoth yuhaoth 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
Copy link
Contributor

xkqian commented Nov 24, 2022

Did you have a look to the documentation I added in tls13-support.md which is about how an application would use mbedtls_ssl_write_early_data(). The idea is for the application to call mbedtls_ssl_write_early_data() with an ssl context for which the handshake as not started yet. mbedtls_ssl_write_early_data() starts the handshake and when the handshake is in the right state starts sending the early data that has been passed to it. I know that ssl_client2 is not structured like that: first it does the handshake, then does something else ... To test early data in ssl_client2, we will have to add a different path of execution.

Understand. Thanks for you and Jerry's explanation.

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 4cf77e9 into Mbed-TLS:development Nov 24, 2022
@ronald-cron-arm ronald-cron-arm deleted the tls13-early-data-write branch July 21, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-tls13 enhancement needs-ci Needs to pass CI tests 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