Skip to content

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

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

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

felixc-arm
Copy link
Contributor

@felixc-arm felixc-arm commented Apr 16, 2025

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>

Macros privatized:

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 #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

@felixc-arm felixc-arm 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 priority-high High priority - will be reviewed soon api-break This issue/PR breaks the API and must wait for a new major version and removed priority-high High priority - will be reviewed soon labels Apr 17, 2025
@felixc-arm felixc-arm marked this pull request as ready for review April 17, 2025 15:06
@felixc-arm
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break This issue/PR breaks the API and must wait for a new major version 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make cipher.h functions private
3 participants