gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

Mechanism to error out on removed configuration options #269

Merged
gilles-peskine-arm merged 11 commits from config-error-on-removed-options-crypto into development 2025-09-25 12:26:23 -05:00
Conversation 16 Commits 11 Files changed 8 +263 -25
gilles-peskine-arm commented 2025-04-25 11:40:17 -05:00 (Migrated from github.com)

Set up a mechanism for erroring out if users try to use removed options in their configuration file. Resolves https://github.com/Mbed-TLS/mbedtls/issues/10147.

Specification in https://github.com/Mbed-TLS/mbedtls-framework/pull/184.

Status: ready.

Needed preceding PR:

PR checklist

Set up a mechanism for erroring out if users try to use removed options in their configuration file. Resolves https://github.com/Mbed-TLS/mbedtls/issues/10147. Specification in https://github.com/Mbed-TLS/mbedtls-framework/pull/184. Status: ready. Needed preceding PR: * [x] framework https://github.com/Mbed-TLS/mbedtls-framework/pull/164 * [x] mbedtls https://github.com/Mbed-TLS/mbedtls/pull/10409 ## PR checklist - [x] **TF-PSA-Crypto PR** here - [x] **development PR** https://github.com/Mbed-TLS/mbedtls/pull/10409 (fix problems in test builds); https://github.com/Mbed-TLS/mbedtls/pull/10306 (just doing the same thing in the other project) - [x] **3.6 PR** not required because: new code for use in 1.0/4.0 only - [x] **framework PR** https://github.com/Mbed-TLS/mbedtls-framework/pull/164 - [x] **tests** provided
mpg (Migrated from github.com) reviewed 2025-04-28 04:30:06 -05:00
mpg (Migrated from github.com) left a comment

I've reviewed the general design and the tests, and they both look good to me.

I've reviewed the general design and the tests, and they both look good to me.
include/tf-psa-crypto/config_check_after.h
mpg (Migrated from github.com) commented 2025-04-28 03:55:07 -05:00

If the file is auto-generated and checked in, we probably want some check that the checked-in version is up-to-date. But perhaps the situation is just temporary to facilitate exploration & design review?

If the file is auto-generated and checked in, we probably want some check that the checked-in version is up-to-date. But perhaps the situation is just temporary to facilitate exploration & design review?
gilles-peskine-arm (Migrated from github.com) reviewed 2025-07-01 10:42:13 -05:00
include/tf-psa-crypto/config_check_after.h
gilles-peskine-arm (Migrated from github.com) commented 2025-07-01 10:42:12 -05:00

I expect that this won't change after the 1.0/4.0 releases, since we can't remove configuration options except in major versions. If we decide to expand it (maybe to cover experimental options? or maybe we decide to improve it for some need that we don't see yet?), we can add a check later.

I expect that this won't change after the 1.0/4.0 releases, since we can't remove configuration options except in major versions. If we decide to expand it (maybe to cover experimental options? or maybe we decide to improve it for some need that we don't see yet?), we can add a check later.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-07-14 15:05:41 -05:00
include/tf-psa-crypto/config_check_after.h
gilles-peskine-arm (Migrated from github.com) commented 2025-07-14 15:05:41 -05:00

Having explored the topic further, I've decided to move the checks inside the library directory. I didn't want to open the can of worms of generated public headers, but generated internal headers with no dependencies other than *config.h are no problem. So the files will be generated by make_generated_files.py, not checked in.

Having explored the topic further, I've decided to move the checks inside the library directory. I didn't want to open the can of worms of generated public headers, but generated internal headers with no dependencies other than `*config.h` are no problem. So the files will be generated by `make_generated_files.py`, not checked in.
gilles-peskine-arm commented 2025-07-14 15:06:22 -05:00 (Migrated from github.com)

I have written a specification at https://github.com/Mbed-TLS/mbedtls-framework/pull/184 . Right now the code doesn't match the specification, I'm updating it. I will rewrite the history significantly.

I have written a specification at https://github.com/Mbed-TLS/mbedtls-framework/pull/184 . Right now the code doesn't match the specification, I'm updating it. I will rewrite the history significantly.
gilles-peskine-arm commented 2025-09-22 07:50:11 -05:00 (Migrated from github.com)

This is ready for review. It isn't passing the CI yet, but fixing that should take just a few more commits: one here to fix the libtestdriver1 build and a couple in mbedtls to fix the make build when generated files are missing and to fix the XTS test job.

This is ready for review. It isn't passing the CI yet, but fixing that should take just a few more commits: one here to fix the libtestdriver1 build and a couple in mbedtls to fix the make build when generated files are missing and to fix the XTS test job.
gilles-peskine-arm commented 2025-09-22 08:59:41 -05:00 (Migrated from github.com)

I've pushed a fix for the libtestdriver1 builds here. The other CI failures will require https://github.com/Mbed-TLS/mbedtls/pull/10409 to be merged first.

I've pushed a fix for the libtestdriver1 builds here. The other CI failures will require https://github.com/Mbed-TLS/mbedtls/pull/10409 to be merged first.
minosgalanakis (Migrated from github.com) reviewed 2025-09-24 06:28:51 -05:00
minosgalanakis (Migrated from github.com) left a comment

Looks good overall, some minor comments only

Looks good overall, some minor comments only
core/tf_psa_crypto_config.c
@ -9,0 +18,4 @@
* In a normal build, `LIBTESTDRIVER1_TF_PSA_CRYPTO_MARKER` remains
* undefined so the conditional is false.
*/
#define TF_PSA_CRYPTO_MARKER 1
minosgalanakis (Migrated from github.com) commented 2025-09-24 04:45:57 -05:00

is the TF_PSA_CRYPTO_MARKER used anywhere else? Should we undefine it after the LIBTESTDRIVER1_TF_PSA_CRYPTO_MARKER == TF_PSA_CRYPTO_MARKER

is the TF_PSA_CRYPTO_MARKER used anywhere else? Should we undefine it after the `LIBTESTDRIVER1_TF_PSA_CRYPTO_MARKER == TF_PSA_CRYPTO_MARKER`
core/tf_psa_crypto_config.c
@ -9,0 +20,4 @@
*/
#define TF_PSA_CRYPTO_MARKER 1
#if LIBTESTDRIVER1_TF_PSA_CRYPTO_MARKER == TF_PSA_CRYPTO_MARKER
#define TF_PSA_CRYPTO_WE_ARE_IN_LIBTESTDRIVER1
minosgalanakis (Migrated from github.com) commented 2025-09-24 04:46:50 -05:00

TF_PSA_CRYPTO_WE_ARE_IN_LIBTESTDRIVER1

#define TF_PSA_CRYPTO_LIBTESTDRIVER1_BUILD
TF_PSA_CRYPTO_WE_ARE_IN_LIBTESTDRIVER1 ```suggestion #define TF_PSA_CRYPTO_LIBTESTDRIVER1_BUILD ```
scripts/generate_config_checks.py
@ -9,0 +22,4 @@
new_public = (history.options('tfpsacrypto', '1.0') |
history.options('mbedtls', '4.0'))
internal = history.internal('tfpsacrypto', '1.0')
for option in sorted(old_public - new_public):
minosgalanakis (Migrated from github.com) commented 2025-09-24 06:28:24 -05:00

More pythonic but maybe harder to read?

yield from (
    Internal(option) if option in internal else Removed(option, 'TF-PSA-Crypto 1.0')
    for option in sorted(sorted(old_public - new_public))
)
More pythonic but maybe harder to read? ~~~~~~ yield from ( Internal(option) if option in internal else Removed(option, 'TF-PSA-Crypto 1.0') for option in sorted(sorted(old_public - new_public)) ) ~~~~~~
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-24 08:42:10 -05:00
core/tf_psa_crypto_config.c
@ -9,0 +20,4 @@
*/
#define TF_PSA_CRYPTO_MARKER 1
#if LIBTESTDRIVER1_TF_PSA_CRYPTO_MARKER == TF_PSA_CRYPTO_MARKER
#define TF_PSA_CRYPTO_WE_ARE_IN_LIBTESTDRIVER1
gilles-peskine-arm (Migrated from github.com) commented 2025-09-24 08:42:10 -05:00

“libtestdriver1 build” would be ambiguous: does that mean refer to a build of libtestdriver1, or a build with libtestdriver1? I picked a name that doesn't have this ambiguity.

“libtestdriver1 build” would be ambiguous: does that mean refer to a build _of_ libtestdriver1, or a build _with_ libtestdriver1? I picked a name that doesn't have this ambiguity.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-24 08:44:02 -05:00
scripts/generate_config_checks.py
@ -9,0 +22,4 @@
new_public = (history.options('tfpsacrypto', '1.0') |
history.options('mbedtls', '4.0'))
internal = history.internal('tfpsacrypto', '1.0')
for option in sorted(old_public - new_public):
gilles-peskine-arm (Migrated from github.com) commented 2025-09-24 08:44:02 -05:00

I don't know if it's more pythonic, but it's certainly harder to read, and would only get worse if the logic became more complicated.

I don't know if it's more pythonic, but it's certainly harder to read, and would only get worse if the logic became more complicated.
minosgalanakis (Migrated from github.com) reviewed 2025-09-24 09:40:06 -05:00
minosgalanakis (Migrated from github.com) left a comment
No description provided.
Provisionally LGTM. It will need to be rebased once the framework is merged
gilles-peskine-arm commented 2025-09-24 10:23:01 -05:00 (Migrated from github.com)

Rebased, hopefully for the last time, with the framework updated to the merge of https://github.com/Mbed-TLS/mbedtls-framework/pull/164. The previous head is at https://github.com/gilles-peskine-arm/TF-PSA-Crypto/tree/config-error-on-removed-options-crypto-11

Rebased, hopefully for the last time, with the framework updated to the merge of https://github.com/Mbed-TLS/mbedtls-framework/pull/164. The previous head is at https://github.com/gilles-peskine-arm/TF-PSA-Crypto/tree/config-error-on-removed-options-crypto-11
mpg (Migrated from github.com) reviewed 2025-09-24 10:44:59 -05:00
scripts/generate_config_checks.py
@ -9,0 +22,4 @@
new_public = (history.options('tfpsacrypto', '1.0') |
history.options('mbedtls', '4.0'))
internal = history.internal('tfpsacrypto', '1.0')
for option in sorted(old_public - new_public):
mpg (Migrated from github.com) commented 2025-09-24 10:44:59 -05:00

Yeah I don't think we should aim for maximal code density. Actually I was even on the verge of suggesting

for option in sorted(old_public):
    if option in new_public:
        continue

but I think compared to the current version it's a matter of opinion. (We don't want to be too verbose either.) Or perhaps

no_longer_public = old_public - new_public
for option in sorted(no_longer_public):

which makes the subtraction stand out a bit more without adding too many lines.

Anyway, I'm happy with the code as it is.

Yeah I don't think we should aim for maximal code density. Actually I was even on the verge of suggesting ```python for option in sorted(old_public): if option in new_public: continue ``` but I think compared to the current version it's a matter of opinion. (We don't want to be too verbose either.) Or perhaps ```python no_longer_public = old_public - new_public for option in sorted(no_longer_public): ``` which makes the subtraction stand out a bit more without adding too many lines. Anyway, I'm happy with the code as it is.
mpg (Migrated from github.com) reviewed 2025-09-24 10:52:41 -05:00
core/tf_psa_crypto_config.c
@ -9,0 +20,4 @@
*/
#define TF_PSA_CRYPTO_MARKER 1
#if LIBTESTDRIVER1_TF_PSA_CRYPTO_MARKER == TF_PSA_CRYPTO_MARKER
#define TF_PSA_CRYPTO_WE_ARE_IN_LIBTESTDRIVER1
mpg (Migrated from github.com) commented 2025-09-24 10:52:41 -05:00

“libtestdriver1 build” would be ambiguous: does that mean refer to a build of libtestdriver1, or a build with libtestdriver1?

+100 we use such ambiguous names way too often. Language is not just an unstructured heap of keywords, we also have syntax and function words to show how the keywords relate to each other. Let's use those in identifiers when necessary.

> “libtestdriver1 build” would be ambiguous: does that mean refer to a build of libtestdriver1, or a build with libtestdriver1? +100 we use such ambiguous names way too often. Language is not just an unstructured heap of keywords, we also have syntax and function words to show how the keywords relate to each other. Let's use those in identifiers when necessary.
mpg (Migrated from github.com) reviewed 2025-09-25 05:17:55 -05:00
core/tf_psa_crypto_config.c
@ -9,0 +44,4 @@
#include "tf_psa_crypto_config_check_before.h"
/* The second stage, after including the user config but before doing
* any subsequent adjustment, will be included by build_info.h. */
#define TF_PSA_CRYPTO_INCLUDE_AFTER_RAW_CONFIG "tf_psa_crypto_config_check_user.h"
mpg (Migrated from github.com) commented 2025-09-25 05:17:55 -05:00

Regarding the discussion we had elsewhere on naming: raw_config sounds pretty good to me. (So, tf_psa_crypto_config_check_before_raw_config.h and tf_psa_crypto_config_check_after_raw_config.h). Too late, but just a thought.

Regarding the discussion we had elsewhere on naming: `raw_config` sounds pretty good to me. (So, `tf_psa_crypto_config_check_before_raw_config.h` and `tf_psa_crypto_config_check_after_raw_config.h`). Too late, but just a thought.
mpg (Migrated from github.com) approved these changes 2025-09-25 05:37:31 -05:00
mpg (Migrated from github.com) left a comment

LGTM, thanks!

LGTM, thanks!
minosgalanakis (Migrated from github.com) approved these changes 2025-09-25 10:46:57 -05:00
minosgalanakis (Migrated from github.com) left a comment

LGTM

LGTM
Sign in to join this conversation.
Reviewers
No reviewers
mpg
minosgalanakis
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
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

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
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!269
No description provided.