Privatize functions and some types/macros in cipher.h #259

Merged

felixc-arm merged 5 commits from privatize-cipher-functions into development

2025-05-21 05:47:46 -05:00

felixc-arm commented

2025-04-16 09:44:59 -05:00

(Migrated from github.com)

Privatizes all functions and some types/macros (below) by guarding them with MBEDTLS_DECLARE_PRIVATE_IDENTIFIERS. Any types/macros not listed/privatized here will break the build if they are privatized.

Types privatized:

mbedtls_cipher_padding_t
enum that includes MBEDTLS_KEY_LENGTH_<NONE|DES|DES_EDE|DES_EDE3>
mbedtls_cipher_base_t

Macros privatized:

MBEDTLS_MAX_KEY_LENGTH
MBEDTLS_CIPHER_VARIABLE_IV_LEN
MBEDTLS_CIPHER_VARIABLE_KEY_LEN
MBEDTLS_KEY_BITLEN_SHIFT (required unlinking from Doxygen comment)
MBEDTLS_IV_SIZE_SHIFT (required unlinking from Doxygen comment)

Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/220

PR checklist

changelog not required because: will come later encompassing all privatization using MBEDTLS_DECLARE_PRIVATE_IDENTIFIERS
framework PR not required
mbedtls development PR not required because: crypto change only
mbedtls 3.6 PR not required because: 4.0/1.0 work
tests not required because: no functional change

Privatizes all functions and some types/macros (below) by guarding them with `MBEDTLS_DECLARE_PRIVATE_IDENTIFIERS`. Any types/macros not listed/privatized here will break the build if they are privatized. Types privatized: `mbedtls_cipher_padding_t` `enum that includes MBEDTLS_KEY_LENGTH_<NONE|DES|DES_EDE|DES_EDE3>` `mbedtls_cipher_base_t` Macros privatized: `MBEDTLS_MAX_KEY_LENGTH` `MBEDTLS_CIPHER_VARIABLE_IV_LEN` `MBEDTLS_CIPHER_VARIABLE_KEY_LEN` `MBEDTLS_KEY_BITLEN_SHIFT` (required unlinking from Doxygen comment) `MBEDTLS_IV_SIZE_SHIFT` (required unlinking from Doxygen comment) Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/220 ## PR checklist - [x] **changelog** not required because: will come later encompassing all privatization using `MBEDTLS_DECLARE_PRIVATE_IDENTIFIERS` - [x] **framework PR** not required - [x] **mbedtls development PR** not required because: crypto change only - [x] **mbedtls 3.6 PR** not required because: 4.0/1.0 work - **tests** not required because: no functional change

felixc-arm commented

2025-04-17 10:10:09 -05:00

(Migrated from github.com)

A lot more types & macros could probably be privatized too, however doing so would break the CI for other reasons. e.g. MBEDTLS_KEY_BITLEN_SHIFT breaks the doxygen build as it's used in a comment above. So I guess the question is do we want to do any extra work to get more types/macros privatized or just only privatize ones that don't break the CI to keep the amount of work down?

A lot more types & macros could probably be privatized too, however doing so would break the CI for other reasons. e.g. `MBEDTLS_KEY_BITLEN_SHIFT` breaks the doxygen build as it's used in a comment above. So I guess the question is do we want to do any extra work to get more types/macros privatized or just only privatize ones that don't break the CI to keep the amount of work down?

gilles-peskine-arm commented

2025-04-17 10:36:27 -05:00

(Migrated from github.com)

do we want to do any extra work to get more types/macros privatized or just only privatize ones that don't break the CI to keep the amount of work down?

I think we should try to find a reasonable compromise between the two. We don't have time to spend hours analyzing every single macro. But we should take quick wins.

For example, MBEDTLS_KEY_BITLEN_SHIFT is only used in existing Doxygen comments that will not be part of the public documentation (and actually shouldn't be even in 3.x, since it only appears in the documentation of a private field of a structure, and those should not be documented). So here I think the best solution is to unlink the reference in the Doxygen comment, and guard the macro.

I wouldn't insist on doing this kind of analysis for all macros, especially for ones that are used in more places or in more complex ways. But I'd prefer to privatize macros when the analysis and fix is as easy as this case.

> do we want to do any extra work to get more types/macros privatized or just only privatize ones that don't break the CI to keep the amount of work down? I think we should try to find a reasonable compromise between the two. We don't have time to spend hours analyzing every single macro. But we should take quick wins. For example, `MBEDTLS_KEY_BITLEN_SHIFT` is only used in existing Doxygen comments that will not be part of the public documentation (and actually shouldn't be even in 3.x, since it only appears in the documentation of a private field of a structure, and those should not be documented). So here I think the best solution is to unlink the reference in the Doxygen comment, and guard the macro. I wouldn't insist on doing this kind of analysis for all macros, especially for ones that are used in more places or in more complex ways. But I'd prefer to privatize macros when the analysis and fix is as easy as this case.

felixc-arm commented

2025-04-22 09:02:31 -05:00

(Migrated from github.com)

Sounds good - I've re-privatized those two macros then. I can't see any other macros/types that are privatized as easily, so the ones privatized in this PR should be the complete list - on my end at least.

bjwtaylor (Migrated from github.com) reviewed

2025-04-25 07:56:41 -05:00

mpg (Migrated from github.com) reviewed

2025-05-14 05:58:55 -05:00

mpg (Migrated from github.com) left a comment

Looks good to me, but I think now that GCC/CCM are done, we can probably go a bit further here. For example, in a local copy of this branch (with development merged in) I was able to make mbedtls_cipher_mode_t private and things are still compiling (at least in the default config).

Looks good to me, but I think now that GCC/CCM are done, we can probably go a bit further here. For example, in a local copy of this branch (with development merged in) I was able to make `mbedtls_cipher_mode_t` private and things are still compiling (at least in the default config).

felixc-arm commented

2025-05-14 09:21:33 -05:00

(Migrated from github.com)

Ah yes - done that now & updated the PR description to include the newly-privatized types/macros, cheers!

felixc-arm commented

2025-05-14 10:58:37 -05:00

(Migrated from github.com)

...And the CI reminded my why they were left unprivatized (at least mbedtls_cipher_id_t and mbedtls_cipher_mode_t) - they are present in drivers/builtin/src/psa_crypto_cipher.h which is then used to build libtestdriver1 which can't find privatized types. Although mbedtls_cipher_base_t & MBEDTLS_MAX_KEY_LENGTH are now also privatized (presuming CI passes...)

...And the CI reminded my why they were left unprivatized (at least `mbedtls_cipher_id_t` and `mbedtls_cipher_mode_t`) - they are present in `drivers/builtin/src/psa_crypto_cipher.h` which is then used to build `libtestdriver1` which can't find privatized types. Although `mbedtls_cipher_base_t` & `MBEDTLS_MAX_KEY_LENGTH` are now also privatized (presuming CI passes...)

mpg commented

2025-05-15 03:26:39 -05:00

(Migrated from github.com)