Break down mbedtls_test_ssl_perform_handshake #10197

gilles-peskine-arm · 2025-05-28T17:25:03Z

Break down mbedtls_test_ssl_perform_handshake() and make mbedtls_test_ssl_endpoint_init() more consistent with it.

This is a preliminary for #10160. I struggled a lot with the SSL helper code, and which function does what. In particular, I want to be able to customize what is passed to mbedtls_ssl_conf_own_cert(), and that was difficult when it was buried very deep down into the call stack.

There is a lot of further refactoring that should ideally be done, but are out of scope here, including:

Better documentation for the existing functions.
Break down mbedtls_test_ssl_perform_connection() further, to allow several test functions to start a handshake, do weird stuff, and finally let the common function finish the handshake, verifying that it reaches the expected state and that the resulting connection is usable.

Regarding backports: This pull request mostly touches ssl_helpers.c, which it changes considerably. This file had already diverged from 3.6 a little, but now backports to this file will be very hard. However, I don't propose to backport this pull request, because ssl_helpers.c should be in the framework. Instead, at this point, I think we're better off moving the file from development to the framework, and restoring the things that are needed in 3.6 (I think mostly changing back PSA_WANT_xxx guards to MBEDTLS_SSL_HAVE_xxx).

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

changelog provided (I made one library function const)
development PR here
TF-PSA-Crypto PR not required because: TLS only
framework PR not required
3.6 PR not required because: better move the file to the framework than replay all the patches in 3.6
tests provided | not required because:

Regexp replacement then `code_style.py --fix`. Signed-off-by: Gilles Peskine <[email protected]>

No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

A future commit will test it on its own instead of as part of every positive test. Signed-off-by: Gilles Peskine <[email protected]>

No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Link the ciphersuite list that's passed to mbedtls_ssl_conf_ciphersuites(), and needs to survive in memory as long as the configuration object is live, in the endpoint structure. This way it doesn't have to be a local variable in mbedtls_test_ssl_do_handshake_with_endpoints(). Signed-off-by: Gilles Peskine <[email protected]>

This reflects the fact that the library will not modify the list, and allows the list to be read from a const buffer. Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

This is a step towards making mbedtls_test_ssl_endpoint_init() and mbedtls_test_ssl_endpoint_free() more self-contained. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Create an auxiliary function to perform some endpoint setup that involves both the client and the server. This is only needed for DTLS. The code that will eventually be in this function is currently mostly in mbedtls_test_ssl_endpoint_init(). This commit adds the new function to the control flow; a subsequent commit will move the relevant code. Signed-off-by: Gilles Peskine <[email protected]>

This allows mbedtls_test_ssl_endpoint_init() to no longer interact with the other endpoint. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

The DTLS context and the queues now conveyed inside the endpoint object. Remove the unused parameters. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

This is made possible by the endpoint init simplification. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

This will facilitate future refactoring that breaks out code into auxiliary functions. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Stop calling mbedtls_test_set_step() in mbedtls_test_ssl_perform_handshake(). This leaves the caller free to use the test step as they wish. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Split mbedtls_test_ssl_perform_connection() out of mbedtls_test_ssl_perform_handshake(). No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Applying SSL configuration settings recorded in the `mbedtls_test_handshake_test_options` structure to an `mbedtls_test_ssl_endpoint` object was split between `mbedtls_test_ssl_endpoint_init()` and `mbedtls_test_ssl_perform_handshake()`. This was surprising, and made it harder to use `mbedtls_test_ssl_endpoint_init()` for custom behavior. It also meant some code duplication in `mbedtls_test_ssl_perform_handshake()`. Move most configuration setup from `mbedtls_test_ssl_perform_handshake()` to `mbedtls_test_ssl_endpoint_init()`. This changes the behavior in two ways: * `mbedtls_test_ssl_endpoint_init()` now takes some options into account that it previously ignored. This is ok because we don't set these options in any of the existing tests. * When calling `mbedtls_test_ssl_perform_handshake()`, some SSL configuration settings are now set (calls to `mbedtls_ssl_conf_xxx()`) before the call to `mbedtls_ssl_setup()` instead of after. This should be ok since it is forbidden to change the configuration after `mbedtls_ssl_setup()`, although the previous test code was getting away with it. This commit does not move all configuration before `mbedtls_ssl_setup()`, that would be out of scope of the current series of patches. Thus there are some internal behavior changes, but they should not affect any relevant aspect of the tests' behavior. Signed-off-by: Gilles Peskine <[email protected]>

There was a discrepancy between how `mbedtls_test_ssl_endpoint_init()` and `mbedtls_test_ssl_perform_handshake()` handled client authentication: `mbedtls_test_ssl_endpoint_init()` defaulted to `MBEDTLS_SSL_VERIFY_REQUIRED` on both sides, whereas `mbedtls_test_ssl_perform_handshake()` obeyed `options->srv_auth_mode` which defaulted to no verification of the client certificate. Make this more uniform. Now `mbedtls_test_ssl_endpoint_init()` obeys `options->srv_auth_mode` on servers (still forcing verification on clients, which is the library default anyway). Also, `options->srv_auth_mode` is now enabled by default. Thus: * Tests that call `mbedtls_test_ssl_perform_handshake()` now perform client certificate verification, unless they disable it explicitly. * Tests that call `mbedtls_test_ssl_endpoint_init()` on a server are unchanged. (They would change if they were setting `options->srv_auth_mode` explicitly, which previously was ignored, but no test function did this.) This means that a few test functions now perform client certificate verification whereas they previously don't. This is harmless except in `handshake_ciphersuite_select`, where one test case `Handshake, select ECDH-RSA-WITH-AES-256-CBC-SHA384, opaque` fails with client authentication because the test code doesn't deal with the weirdness of static ECDH correctly with respect to client authentication. So keep the previous behavior in `handshake_ciphersuite_select`, by explicitly turning off client authentication. Signed-off-by: Gilles Peskine <[email protected]>

No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

felixc-arm

Generally looks good to me, with a couple of fairly minor points/questions.

tests/src/test_helpers/ssl_helpers.c

felixc-arm · 2025-05-30T13:02:06Z

tests/suites/test_suite_ssl.function


    /* Test that the first half of the message is valid */
-    TEST_ASSERT(memcmp(message, received, MSGLEN/2) == 0);
+    TEST_EQUAL(memcmp(message, received, MSGLEN/2),  0);


Minor nit - there seems to be 2 spaces after the comma in all the new TEST_EQUALs in this file, although it's a very minor code style thing so not super important.

tests/src/test_helpers/ssl_helpers.c

In `mbedtls_test_ssl_endpoint_init()`, don't change the SSL configuration object (`mbedtls_ssl_config`) after setting up an SSL context by calling `mbedtls_ssl_setup()`. This works in practice, but is officially forbidden. No intended behavior change. The test code calls the library slightly differently, but this shouldn't make any difference in practice. If it does make a difference, it fixes a bug in the test code. Signed-off-by: Gilles Peskine <[email protected]>

This will allow splitting the configuration and setup stages of `mbedtls_test_ssl_endpoint_init()`, while still checking that the value is carried over from the configuration to the session context. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Split `mbedtls_test_ssl_endpoint_init()` into two separate stages: constructing the SSL configuration, and setting up an SSL session context with that configuration. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm · 2025-06-01T19:55:16Z

Apologies, I thought this was a good stopping point for refactoring, but as I adapt my patch for #10160 on this, I realize that I want to change this a bit:

In mbedtls_test_ssl_endpoint_init, I thought I could get away with keeping the interleaving between setting the configuration and setting up the context. But no, that's still a bit too messy, so I'm adding this cleanup here. Fortunately it turns out to be pretty straightforward.
25582d5 "Split off mbedtls_test_ssl_endpoint_make_key_opaque" is not really the interface I need. The way I did it in this commit is a bit of a hack where it takes the existing pk object and fiddles with it, and that doesn't work well with a subsequent change where I want to allow endpoints to have multiple pk objects. So I'm removing this commit from the history.

The previous version is at https://github.com/gilles-peskine-arm/mbedtls/tree/ssl_helpers-split_perform_handshake-dev-3. The new version branches one commit before the end of that, at 6e4d245.

felixc-arm

LGTM 👍

gilles-peskine-arm added this to Roadmap pull requests (new board) May 28, 2025

gilles-peskine-arm added component-tls needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels May 28, 2025

github-project-automation bot moved this to In Development in Roadmap pull requests (new board) May 28, 2025

gilles-peskine-arm added 17 commits May 28, 2025 20:22

Use TEST_EQUAL(a,b) instead of TEST_ASSERT(a==b)

353eb33

Regexp replacement then `code_style.py --fix`. Signed-off-by: Gilles Peskine <[email protected]>

Flatten out mbedtls_test_ssl_endpoint_certificate structure

b6bb3fb

No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Remove testing of mbedtls_ssl_conf_own_cert(NULL)

35a2d9b

A future commit will test it on its own instead of as part of every positive test. Signed-off-by: Gilles Peskine <[email protected]>

Move timer into the endpoint structure

0677e02

No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

mbedtls_ssl_conf_alpn_protocols: declare list elements as const

c4949d1

This reflects the fact that the library will not modify the list, and allows the list to be read from a const buffer. Signed-off-by: Gilles Peskine <[email protected]>

mbedtls_test_ssl_perform_handshake: declare options as const

9b99368

Signed-off-by: Gilles Peskine <[email protected]>

Move DTLS context into the endpoint structure

2996959

This is a step towards making mbedtls_test_ssl_endpoint_init() and mbedtls_test_ssl_endpoint_free() more self-contained. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Move queue management into mbedtls_test_ssl_dtls_join_endpoints

6c154e7

This allows mbedtls_test_ssl_endpoint_init() to no longer interact with the other endpoint. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Remove unused parameters to endpoint init/free

ca8a9ac

The DTLS context and the queues now conveyed inside the endpoint object. Remove the unused parameters. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Unify identical code

07432b9

This is made possible by the endpoint init simplification. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

mbedtls_test_ssl_perform_handshake: make client, server pointers

e30b5c7

This will facilitate future refactoring that breaks out code into auxiliary functions. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Move renegotiation testing into its own function

78df6ae

No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Move serialization testing into its own function

e23a6d1

No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Unify SSL version checks between client and server

bd95340

Stop calling mbedtls_test_set_step() in mbedtls_test_ssl_perform_handshake(). This leaves the caller free to use the test step as they wish. No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

Separate test function to perform an SSL connection

7a8fd46

Split mbedtls_test_ssl_perform_connection() out of mbedtls_test_ssl_perform_handshake(). No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm force-pushed the ssl_helpers-split_perform_handshake-dev branch from 81b92c4 to 5f85fca Compare May 28, 2025 18:25

gilles-peskine-arm added 3 commits May 28, 2025 21:36

Move certificate and key parsing to auxiliary functions

6e4d245

No behavior change. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm force-pushed the ssl_helpers-split_perform_handshake-dev branch from 5f85fca to 25582d5 Compare May 28, 2025 19:37

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels May 28, 2025

gilles-peskine-arm added the needs-reviewer This PR needs someone to pick it up for review label May 28, 2025

felixc-arm reviewed May 30, 2025

View reviewed changes

gilles-peskine-arm added 3 commits June 1, 2025 21:34

gilles-peskine-arm force-pushed the ssl_helpers-split_perform_handshake-dev branch from 25582d5 to 6edb76c Compare June 1, 2025 19:55

gilles-peskine-arm added the needs-ci Needs to pass CI tests label Jun 1, 2025

gilles-peskine-arm requested a review from felixc-arm June 2, 2025 07:07

gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Jun 2, 2025

felixc-arm approved these changes Jun 2, 2025

View reviewed changes

davidhorstmann-arm self-requested a review June 9, 2025 16:28

gilles-peskine-arm mentioned this pull request Jun 12, 2025

Test mbedtls_ssl_conf_own_cert #10217

Draft

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Break down mbedtls_test_ssl_perform_handshake #10197

Break down mbedtls_test_ssl_perform_handshake #10197

Uh oh!

gilles-peskine-arm commented May 28, 2025 •

edited

Loading

Uh oh!

felixc-arm left a comment

Uh oh!

Uh oh!

felixc-arm May 30, 2025

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm commented Jun 1, 2025

Uh oh!

felixc-arm left a comment

Uh oh!

Uh oh!

Break down mbedtls_test_ssl_perform_handshake #10197

Are you sure you want to change the base?

Break down mbedtls_test_ssl_perform_handshake #10197

Uh oh!

Conversation

gilles-peskine-arm commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

felixc-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felixc-arm May 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm commented Jun 1, 2025

Uh oh!

felixc-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilles-peskine-arm commented May 28, 2025 •

edited

Loading