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

add tls13 client dummy state handlers and improve dispatch test #4942

Merged

Conversation

yuhaoth
Copy link
Contributor

@yuhaoth yuhaoth commented Sep 14, 2021

Description

This PR includes two small changes

  • Improve TLS1.3 dispatch test
    The test is to confirm client/server call correct handshake function. Originally, it depends on the return value, this PR adds two debug output message for client and server. And check if the message exists.

  • Add dummy handler functions for client states. I am not very sure if it is valuable
    This PR adds all states and handlers needed by MVP. The states are come from Fix compile errors without 0-RTT, MPS and compatible mode hannestschofenig/mbedtls#359, that PR disable options unneeded by MVP .

I think this PR can reduce conflicts in future and make sure we did not miss anything.

All PR will change mbedtls_ssl_tls13_handshake_client_step . So I create them with dummy handlers, future PRs can fill the dummy handlers to reduce conflicts. And if all handlers are filled, that means everything done and a real test for MVP should be added.

Difference with prototype:

  • Create static handler functions in ssl_tls13_client.c for all states. prototype did not create local functions, some functions are not in ssl_tls13_client.c
  • Some handlers name are changed. It follows ruler ssl_tls13_<direction><state_name>

Status

READY

library/ssl_tls13_client.c Outdated Show resolved Hide resolved
@yuhaoth yuhaoth added component-tls13 needs-review Every commit must be reviewed by at least two team members, size-s Estimated task size: small (~2d) labels Sep 15, 2021
@yuhaoth yuhaoth linked an issue Sep 15, 2021 that may be closed by this pull request
@gilles-peskine-arm gilles-peskine-arm requested review from gilles-peskine-arm and removed request for mpg September 16, 2021 08:38
@yuhaoth yuhaoth force-pushed the pr/add-tls13-client-dummy-state-handlers branch 5 times, most recently from 227a321 to 59352da Compare September 19, 2021 12:37
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.

Just two things I am not sure about.

library/ssl_tls13_client.c Show resolved Hide resolved
tests/ssl-opt.sh Outdated Show resolved Hide resolved
tests/ssl-opt.sh Outdated Show resolved Hide resolved
@yuhaoth yuhaoth force-pushed the pr/add-tls13-client-dummy-state-handlers branch 4 times, most recently from 8f6329c to 56f9b87 Compare September 23, 2021 05:50
library/ssl_tls13_client.c Outdated Show resolved Hide resolved
library/ssl_tls13_client.c Outdated Show resolved Hide resolved
library/CMakeLists.txt Outdated Show resolved Hide resolved
@yuhaoth yuhaoth force-pushed the pr/add-tls13-client-dummy-state-handlers branch 2 times, most recently from ef76e5e to 311716b Compare September 23, 2021 10:29
@yuhaoth
Copy link
Contributor Author

yuhaoth commented Sep 23, 2021

Only Travis CI raise random fail .

It is temporary check. If any change on `mbedtls_ssl_states`, please
double check those tests

Signed-off-by: Jerry Yu <[email protected]>
@yuhaoth yuhaoth force-pushed the pr/add-tls13-client-dummy-state-handlers branch from dce27af to e86cd65 Compare September 27, 2021 08:33
@yuhaoth
Copy link
Contributor Author

yuhaoth commented Sep 27, 2021

Rebased

mpg
mpg previously approved these changes Sep 27, 2021
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Looks good to me, considering this is only temporary and things are expected to change in follow-up PRs.

library/ssl_tls13_client.c Show resolved Hide resolved
library/ssl_tls13_client.c Show resolved Hide resolved
@yuhaoth
Copy link
Contributor Author

yuhaoth commented Sep 28, 2021

double underscore fail has been fixed with no-check-names mark

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.

Only one consistency point other this looks good to me.

library/ssl_tls13_client.c Show resolved Hide resolved
Signed-off-by: Jerry Yu <[email protected]>
@ronald-cron-arm ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 28, 2021
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 29, 2021
@mpg
Copy link
Contributor

mpg commented Sep 29, 2021

I've checked that the feedback from Gilles either has been addressed, or is no longer relevant for this PR (generate_states.py moved to another PR).

@mpg mpg dismissed gilles-peskine-arm’s stale review September 29, 2021 08:45

Feedback has been adressed

@mpg mpg merged commit 1869377 into Mbed-TLS:development Sep 29, 2021
@yuhaoth yuhaoth deleted the pr/add-tls13-client-dummy-state-handlers branch January 20, 2022 02:38
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 size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upstream TLS 1.3 state machine logic
5 participants