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

[MBEDTLS_PRIVATE] Add accessor for session and ciphersuite_id #8888

Conversation

minosgalanakis
Copy link
Contributor

@minosgalanakis minosgalanakis commented Mar 4, 2024

Description

The ps is adding accessors for the following members of the mbedtls_ssl_session

  • int MBEDTLS_PRIVATE(ciphersuite) -> mbedtls_ssl_session_get_ciphersuite_id()
  • size_t MBEDTLS_PRIVATE(id_len) -> mbedtls_ssl_session_get_id_len()
    -unsigned char MBEDTLS_PRIVATE(id)[32 -> mbedtls_ssl_session_get_id()

As well as one accessor for the mbedtls_ssl_ciphersuite_t struct

  • uint8_t MBEDTLS_PRIVATE(cipher) -> mbedtls_ssl_ciphersuite_get_id()

Resolves #8529

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog Included
  • backport Not required new API calls.
  • tests provided

@minosgalanakis minosgalanakis added needs-ci Needs to pass CI tests component-tls13 size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Mar 4, 2024
@minosgalanakis minosgalanakis force-pushed the features/add_ssl_session_accessor_8529 branch 5 times, most recently from 0ad9589 to a058c4e Compare March 6, 2024 13:50
@minosgalanakis minosgalanakis force-pushed the features/add_ssl_session_accessor_8529 branch 2 times, most recently from 417517f to 32426cb Compare March 6, 2024 15:08
@minosgalanakis minosgalanakis added 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 and removed needs-ci Needs to pass CI tests labels Mar 7, 2024

/* Test setting a reference id for tls1.3 and tls1.2 */
ciphersuite_info = mbedtls_ssl_ciphersuite_from_id(ciphersuite_id);
if (ciphersuite_info != NULL) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In certain configurations the ssl_ciphersuite_from_id will return NULL. Testing this function is outside of the scope so we only test the getter for the other cases where it will create a ciphersuite_info stucture.

Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

One typo, one nit, LGTM otherwise

ChangeLog.d/add_ssl_session_accessors.txt Outdated Show resolved Hide resolved
ChangeLog.d/add_ssl_session_accessors.txt Outdated Show resolved Hide resolved
@tom-daubney-arm tom-daubney-arm self-requested a review March 7, 2024 15:10
@tom-daubney-arm tom-daubney-arm removed the needs-reviewer This PR needs someone to pick it up for review label Mar 7, 2024
@minosgalanakis minosgalanakis force-pushed the features/add_ssl_session_accessor_8529 branch from 32426cb to 0338122 Compare March 7, 2024 16:02
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-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, thanks!

Copy link
Contributor

@tom-daubney-arm tom-daubney-arm left a comment

Choose a reason for hiding this comment

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

Couple of small typos but otherwise it looks good.

@@ -961,6 +961,14 @@ TLS 1.3: SRV: session serialization: Wrong config
depends_on:MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_SSL_SRV_C
ssl_session_serialize_version_check:0:0:0:1:MBEDTLS_SSL_IS_SERVER:MBEDTLS_SSL_VERSION_TLS1_3

Test Session id & Ciphersuite accesors TLS 1.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Test Session id & Ciphersuite accesors TLS 1.2
Test Session id & Ciphersuite accessors TLS 1.2

depends_on:MBEDTLS_SSL_PROTO_TLS1_2
ssl_session_id_accessors_check:MBEDTLS_SSL_VERSION_TLS1_2

Test Session id & Ciphersuite accesors TLS 1.3
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Test Session id & Ciphersuite accesors TLS 1.3
Test Session id & Ciphersuite accessors TLS 1.3

@minosgalanakis minosgalanakis force-pushed the features/add_ssl_session_accessor_8529 branch from 0338122 to f9a6893 Compare March 11, 2024 10:09
@minosgalanakis minosgalanakis added priority-very-high Highest priority - prioritise this over other review work and removed priority-high High priority - will be reviewed soon labels Mar 11, 2024
Copy link
Contributor

@tom-daubney-arm tom-daubney-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 now - thanks!

Copy link
Contributor

@davidhorstmann-arm davidhorstmann-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, thanks!

@tom-daubney-arm tom-daubney-arm 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 Mar 12, 2024
@daverodgman daverodgman added this pull request to the merge queue Mar 13, 2024
Merged via the queue into Mbed-TLS:development with commit 60c2f47 Mar 13, 2024
6 checks passed
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 priority-very-high Highest priority - prioritise this over other review work size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review requested MBEDTLS_PRIVATE accessors.
4 participants