Privatize functions in drivers/builtin/include/mbedtls header files #243

Closed

felixc-arm wants to merge 2 commits from privatize-hash-headers into development

felixc-arm commented

2025-03-31 09:05:42 -05:00

(Migrated from github.com)

Adds the guard #if defined(MBEDTLS_DECLARE_PRIVATE_IDENTIFIERS) around all functions in the files below so that they cannot be accessed by user programs (as they are now private).
~~Also adds this guard to a couple of structs/types that are explicitly marked as internal (in ecp.h, ecdsa.h & block_cipher.h).~~

List of files (for ease of reference, matches the files specified in the linked issues):

aes.h
aria.h
bignum.h
camellia.h
cipher.h
ccm.h
chacha20.h
chachapoly.h
cipher.h
cmac.h
des.h
ecdh.h
ecdsa.h
ecjpake.h
ecp.h
gcm.h
md5.h
poly1305.h
ripemd160.h
sha1.h
sha256.h
sha3.h
sha512.h

resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/220, resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/224, resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/225, resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/226, resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/227

PR checklist

changelog not required because: Changelog and other documentation to be added with this issue: https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/232
framework PR not required
mbedtls development PR not required because: tf-psa-crypto-only change
mbedtls 3.6 PR not required because: new API for 4.0
tests not required because: no internal change to PSA crypto/mbedtls (I guess could have a test that uses the private functions and is expected to fail to compile, but I'm not sure how possible that is with the framework & it would require a 'wrong' example program so may be confusing?)

Adds the guard `#if defined(MBEDTLS_DECLARE_PRIVATE_IDENTIFIERS)` around all functions in the files below so that they cannot be accessed by user programs (as they are now private). ~~Also adds this guard to a couple of structs/types that are explicitly marked as internal (in ecp.h, ecdsa.h & block_cipher.h).~~ List of files (for ease of reference, matches the files specified in the linked issues): - aes.h - aria.h - bignum.h - camellia.h - cipher.h - ccm.h - chacha20.h - chachapoly.h - cipher.h - cmac.h - des.h - ecdh.h - ecdsa.h - ecjpake.h - ecp.h - gcm.h - md5.h - poly1305.h - ripemd160.h - sha1.h - sha256.h - sha3.h - sha512.h resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/220, resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/224, resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/225, resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/226, resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/227 ## PR checklist - [ ] **changelog** not required because: Changelog and other documentation to be added with this issue: https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/232 - [ ] **framework PR** not required - [ ] **mbedtls development PR** not required because: tf-psa-crypto-only change - [ ] **mbedtls 3.6 PR** not required because: new API for 4.0 - **tests** not required because: no internal change to PSA crypto/mbedtls (I guess could have a test that uses the private functions and is expected to fail to compile, but I'm not sure how possible that is with the framework & it would require a 'wrong' example program so may be confusing?)

mpg commented

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

(Migrated from github.com)

resolves #220, resolves #224, resolves #225, resolves #226, resolves #227

I'm sorry, but I don't think doing a single PR for all of them was a good idea. Even though on the surface it looks like the resulting PR is still small (in number of lines change) there is quite a lot to review because the question is exactly what should be hidden - and in some cases there might be non-trivial discussions. We want to avoid too large reviews for several reasons, including quality (experience shows we miss more things in larger PRs) and planning (spreading the load, making incremental progress).

In general, we should not address more than one issue with a single PR. When we're tempting to do so, we should generally ask for opinions - if possible from the person who created the issues, as there's usually a reason why the work was split that way. As always there are exceptions (for example dependencies were missed when splitting tasks, so it turns out they were split in an inconvenient way), but I don't think that's one of them.

As a result, I'm closing this PR, please re-raise one PR per issue in order to facilitate review. (As a reminder, we have a general rule to prioritize reviewer convenience over author convenience, since review is much more often the bottleneck.)

> resolves #220, resolves #224, resolves #225, resolves #226, resolves #227 I'm sorry, but I don't think doing a single PR for all of them was a good idea. Even though on the surface it looks like the resulting PR is still small (in number of lines change) there is quite a lot to review because the question is exactly what should be hidden - and in some cases there might be non-trivial discussions. We want to avoid too large reviews for several reasons, including quality (experience shows we miss more things in larger PRs) and planning (spreading the load, making incremental progress). In general, we should not address more than one issue with a single PR. When we're tempting to do so, we should generally ask for opinions - if possible from the person who created the issues, as there's usually a reason why the work was split that way. As always there are exceptions (for example dependencies were missed when splitting tasks, so it turns out they were split in an inconvenient way), but I don't think that's one of them. As a result, I'm closing this PR, please re-raise one PR per issue in order to facilitate review. (As a reminder, we have a general rule to prioritize reviewer convenience over author convenience, since review is much more often the bottleneck.)

felixc-arm commented

2025-04-09 05:55:09 -05:00

(Migrated from github.com)

Sure, no worries 🙂

Sure, no worries :slightly_smiling_face:

👍 1

gilles-peskine-arm commented

2025-04-09 06:09:20 -05:00

(Migrated from github.com)

the question is exactly what should be hidden - and in some cases there might be non-trivial discussions

Specifically, I think we always want to exclude functions. We mostly want to include types but perhaps not all of them, although that's not a big deal. But macros could go either way and getting that wrong could have subtle consequences (e.g. depending on header inclusion order, different .c files could end up having incompatible opinions on a data structure's layout, because they aren't getting the same #defines).

> the question is exactly what should be hidden - and in some cases there might be non-trivial discussions Specifically, I think we always want to exclude functions. We mostly want to include types but perhaps not all of them, although that's not a big deal. But macros could go either way and getting that wrong could have subtle consequences (e.g. depending on header inclusion order, different `.c` files could end up having incompatible opinions on a data structure's layout, because they aren't getting the same `#define`s).

Pull request closed

This pull request cannot be reopened because the branch was deleted.

No reviewers

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/TF-PSA-Crypto!243

No description provided.

Rows
Columns