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'

[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.