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 mbedtls_ssl_is_handshake_over() #5653

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

paul-elliott-arm
Copy link
Member

Description

Add an accessor function so that the user can tell at what point the SSL handshake is over, and thus at what point they should stop calling mbedtls_ssl_handshake_step().

Change over all internal and test checks to use this function.

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

  • Tests
  • Changelog updated

Steps to test or reproduce

test_suite_ssl and ssl_opt.sh should both run clean.

@paul-elliott-arm paul-elliott-arm self-assigned this Mar 21, 2022
@paul-elliott-arm paul-elliott-arm added enhancement needs-review Every commit must be reviewed by at least two team members, component-tls needs-ci Needs to pass CI tests needs: changelog needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) labels Mar 21, 2022
@mprse mprse self-requested a review March 22, 2022 17:03
Add function to query if SSL handshake is over or not, in order to
determine when to stop calling mbedtls_ssl_handshake_step among other
things. Document function, and add warnings that the previous method of
ascertaining if handshake was over is now deprecated, and may break in
future releases.

Signed-off-by: Paul Elliott <[email protected]>
Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

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

Code looks good. Didn't find more occurrences of conditions including MBEDTLS_SSL_HANDSHAKE_OVER that could be handled by mbedtls_ssl_is_handshake_over().

Only found potential style issue. Please check review comment for details.

library/ssl_msg.c Outdated Show resolved Hide resolved
Switch over to using the new function both internally and in tests.

Signed-off-by: Paul Elliott <[email protected]>
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 removed needs-review Every commit must be reviewed by at least two team members, needs: changelog needs-reviewer This PR needs someone to pick it up for review labels Mar 30, 2022
@mpg mpg requested a review from mprse March 30, 2022 08:07
@mpg mpg added the needs-review Every commit must be reviewed by at least two team members, label Mar 30, 2022
Copy link
Contributor

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

mpg commented Mar 30, 2022

OpenCI seems stuck on resources, but the internal CI fully passed, which is enough.

@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, needs-ci Needs to pass CI tests labels Mar 30, 2022
@mpg mpg merged commit 3304f25 into Mbed-TLS:development Mar 30, 2022
*
* \param ssl SSL context
*
* \return \c 1 if handshake is over, \c 0 if it is still ongoing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this only the initial handshake, or also renegotiations? Can this function return 0 during a renegotiation?

@ndilieto
Copy link
Contributor

ndilieto commented Jun 12, 2022

I have noticed that unlike mbedtls_ssl_conf_sni, mbedtls_ssl_conf_cert_cb doesn't allow passing an opaque pointer to the callback. This is very inconvenient when the ssl context is contained in a struct that is needed in the callback. Is it still possible to align the cert callback to accept an opaque pointer, like the sni callback?

Edit: I filed a new issue #5910

@gstrauss
Copy link
Contributor

@ndilieto a more generic solution was committed in #5426

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-tls enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants