ssl_tls12_populate_transform optim

[gstrauss]

Description

ssl_tls12_populate_transform optimizations

• remove redundant calls to retrieve info
  (mbedtls_ssl_cipher_to_psa() and mbedtls_cipher_info_from_type() are also called from mbedtls_ssl_get_mode_from_ciphersuite())
• defer calls to where needed for specific ssl_modes

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

• Changelog updated

[AndrzejKurek]

There are quite some failures in two test families on the CI:

• test_when_no_ciphersuites_have_mac - call ./tests/scripts/all.sh "test_when_no_ciphersuites_have_mac" to reproduce.
• reference configuration config-ccm-psk-dtls1_2.h+PSA - call ./tests/scripts/test-ref-configs.pl config-ccm-psk-dtls1_2.h to reproduce.

[gstrauss]

There are quite some failures in two test families on the CI:

* `test_when_no_ciphersuites_have_mac` - call `./tests/scripts/all.sh "test_when_no_ciphersuites_have_mac"` to reproduce.

* reference configuration `config-ccm-psk-dtls1_2.h+PSA` - call `./tests/scripts/test-ref-configs.pl config-ccm-psk-dtls1_2.h` to reproduce.

I needed to move an else inside an #ifdef. Re-pushed.

[gstrauss]

@AndrzejKurek CI tests now pass. Ready for review. Thanks.

[gstrauss]

FYI: these simplifications in this PR were uncovered while attempting to port hostap to have an option to use mbedtls, and I needed the keyblock size to implement the hostap API tls_connection_get_eap_fast_key https://github.com/gstrauss/hostap/blob/mbedtls.0/src/crypto/tls_mbedtls.c#L1041 for EAP-FAST keys. My tls_mbedtls_ssl_keyblock_size https://github.com/gstrauss/hostap/blob/mbedtls.0/src/crypto/tls_mbedtls.c#L984 copies logic from mbedtls library/ssl_tls.c:ssl_tls12_populate_transform() because keyblock size info is not exposed in mbed TLS 3.x. My current solution does not include support for defined(MBEDTLS_USE_PSA_CRYPTO), and that might be an issue in the future with #5156 which aims to make use of PSA crypto non-optional. Perhaps mbedtls_ssl_cipher_to_psa() could be made public and modified to take const mbedtls_ssl_ciphersuite_t * param instead of mbedtls_cipher_type_t so that the proper psa_algorithm_t can be filled in. All current callers besides mbedtls_ssl_ticket_setup() already have the const mbedtls_ssl_ciphersuite_t *

[gstrauss]

branch has been rebased on HEAD of 'development'

[AndrzejKurek]

I agree with everything but the part that moves taglen setting and makes it so that there are two calls to mbedtls_ssl_cipher_to_psa in case of AEAD ciphersuites. What's the gain here?

[gstrauss]

I agree with everything but the part that moves taglen setting and makes it so that there are two calls to mbedtls_ssl_cipher_to_psa in case of AEAD ciphersuites. What's the gain here?

Previously, there were always two calls to mbedtls_ssl_cipher_to_psa() for PSA (including from a subroutine, IIRC). Now, the second call occurs only for AEAD.

Edit: The PR replaces a call to mbedtls_ssl_get_mode_from_ciphersuite(), which was the first of two calls to mbedtls_ssl_cipher_to_psa(). The second call to mbedtls_ssl_cipher_to_psa() is now avoided, except for AEAD.

[gstrauss]

Separately, and not included in this PR, library/ssl_tls12_server.c:ssl_write_encrypt_then_mac_ext() is the remaining caller of mbedtls_ssl_get_mode_from_ciphersuite() and could avoid some work by checking if ( ssl->session_negotiate->encrypt_then_mac == MBEDTLS_SSL_ETM_ENABLED ) before doing anything else.

--- a/library/ssl_tls12_server.c
+++ b/library/ssl_tls12_server.c
@@ -2351,8 +2351,11 @@ static int ssl_write_server_hello( mbedtls_ssl_context *ssl )
 #endif
 
 #if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC_ETM)
-    ssl_write_encrypt_then_mac_ext( ssl, p + 2 + ext_len, &olen );
-    ext_len += olen;
+    if( ssl->session_negotiate->encrypt_then_mac == MBEDTLS_SSL_ETM_ENABLED )
+    {
+        ssl_write_encrypt_then_mac_ext( ssl, p + 2 + ext_len, &olen );
+        ext_len += olen;
+    }
 #endif
 
 #if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET)

Then, ssl_write_encrypt_then_mac_ext() could be simplified some and mbedtls_ssl_get_mode_from_ciphersuite() could be specialized to a bool return mbedtls_ssl_ciphersuite_mode_is_cbc_etm()

[gstrauss]

@AndrzejKurek you approved this 7 weeks ago. Would you please nominate a second approver? Thank you.

[AndrzejKurek]

@AndrzejKurek you approved this 7 weeks ago. Would you please nominate a second approver? Thank you.

I'm sorry, but that's not my authority. This PR is on our "todo" board for any volunteer to pick up and review, but the decision on what to review is governed by personal preference and higher-level priorities. Sorry that you're waiting for so long! I'll bring our team's attention to it.

[gilles-peskine-arm]

The change is correct, but now that I've reviewed it I don't understand why it's desirable. I don't find it particularly more or less readable than the previous code. This is presented as an optimization, but what is being optimized? There's very little computation going on, so performance is irrelevant. Memory usage isn't changing. Code size might be changing, but with in the configuration of tests/scripts/all.sh build_arm_none_eabi_gcc_m0plus I get:

commit	size ssl_tls.o
52f83dc	24715
92d69b9	24727
1e1bf341fb6283f11bab1c7e1c0f0be56492bbd6	24719

So where's the optimization? Is this for a specific configuration with a reduced feature set where the new code makes a difference?

[gilles-peskine-arm]

Why recalculate alg, key_type and key_bits here? Above you've already calculated them, but possibly with a different tag length, which only influences the algorithm. So here key_type and key_bits are correct but alg is the full-tag-length version of the algorithm.

Suggested change

	mbedtls_ssl_cipher_to_psa( ciphersuite_info->cipher, transform->taglen,
	&alg, &key_type, &key_bits );
	alg = PSA_ALG_AEAD_WITH_SHORTENED_TAG( alg, transform->taglen );

or

    transform->taglen = 16;
    if( ciphersuite_info->flags & MBEDTLS_CIPHERSUITE_SHORT_TAG )
    {
        transform->taglen = 8;
        alg = PSA_ALG_AEAD_WITH_SHORTENED_TAG( alg, 8 );
    }

This isn't just or even mainly about optimization, it's also about reducing the complexity of the code by having fewer mutated variables.

[gstrauss]

Yes, as noted below, the code changes highlight where the code could be simplied.

I did not specify sufficiently that the optimization is more a code-flow simplification, rather than a performance optimization.

If I understand the rest of the code correctly, the change you suggested above applies only to AEAD.

[gstrauss]

🚀

[gstrauss]

As noted in the original post and also above the code changes aim to make the code simpler, disentangling some previously entangled code flows: separating some of the PSA-specific code from the non-PSA mbedtls_cipher code, collecting some fo the CBC-ETM code, and separating some work that needs to be done for AEAD (for which you suggested improvements).

[gstrauss]

@AndrzejKurek you last reviewed this over 3 months ago, and I made the small changes requested then, over 3 months ago.

I also responded the same day in #6072 (comment) to @gilles-peskine-arm question, repeating information already provided in the original post.

If removing redundant function calls, untangling PSA and non-PSA code to reduce ifdefs, and moving work to scopes where it is needed is not deemed to be cleaner code, then maybe I do not understand what @gilles-peskine-arm considers cleaner code. Maybe @gilles-peskine-arm had an issue with my use of the abbreviation "optim" for removed redundancies, localized work, and more cleanly structured code?

[AndrzejKurek]

I think that you mean well, but in my experience that statement has no correlation to the reality I have experienced in my PRs to mbedtls. See also #6072 which you approved over 3 months ago, and where I subsequently made a small change requested by a second reviewer. You have not revisited that PR since I made the two line change requested by the second reviewer.

I'm sorry for missing this PR, I'll re-review this today. Please re-request a review once you think that the PR is ready (as you did in other PRs), as otherwise my "review requested" list won't show it.

[AndrzejKurek]

I actually think that his version of the code (lines 8214 - 8253) is less readable and does more work to get the same result as before, at the benefit of not calling mbedtls_ssl_cipher_to_psa twice. I can approve this PR if this is a performance optimization or if a second reviewer will determine that this code is better this way, but I would like to voice my concerns.

[AndrzejKurek]

@gstrauss - have you done any performance tests? As I believe that this was the reason you added this as an optimization?

[gstrauss]

The reason I submitted this PR was a result of porting hostap to use mbedtls where I needed to get a specific piece of data. I had to trace through what I considered convoluted code in ssl_tls.c in order to find out how to obtain this data.

No, I do not have specific performance numbers. My visual analysis is that the cleaner code flow should be faster, and more importantly, should not be appreciably slower. Any performance improvement would come from avoiding redundant calls or caching effects of doing work in a tighter scope where the results are used, but I am more interested in slightly cleaner code, which may contribute to later code cleanup once PSA is the default in mbedtls 3.x and the older interface is deprecated/removed.

[gstrauss]

BTW, the work done in the current patch is done for PSA_ALG_CCM and is the same work that would be done by calling mbedtls_ssl_cipher_to_psa(), but only doing the work for PSA_ALG_CCM and without the mbedtls_ssl_cipher_to_psa() switch() and without filling in variables that are then not used.

Please let me know whatever you and the second reviewer think is best.

[gstrauss]

This part of the code is a few more lines than the previous, but not only avoids the second call to mbedtls_ssl_cipher_to_psa(), but also separates out MBEDTLS_SSL_SOME_SUITES_USE_CBC_ETM and whether or not PSA is used, rather than having a preprocessor conditional for an extra arg to a function whether or not MBEDTLS_SSL_SOME_SUITES_USE_CBC_ETM is defined.

[gilles-peskine-arm]

As I've mentioned before, I do not find the new code more readable, less redundant, or otherwise cleaner. So I won't block it, but as far as I'm concerned, this is taking review bandwidth away from more important things.

We're likely to change how TLS code calculates hashes soon, following some refactoring of legacy interfaces (design not yet done for cipher). I don't think the changes in this pull request will particularly help or hinder, but they will likely conflict.

[gstrauss]

@gilles-peskine-arm this PR was filed almost 8 months ago, whereas #7120 was created two weeks ago. My crystal ball must be broken. 🤷

[daverodgman]

It looks like this is unlikely to be accepted - closing.