Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove MBEDTLS_PSA_CRYPTO_CONFIG configuration option #9771

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Remove MBEDTLS_PSA_CRYPTO_CONFIG configuration option #9771

merged 3 commits into from
Nov 21, 2024

Conversation

ronald-cron-arm
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm commented Nov 12, 2024 •
edited
Loading

Description

Fixes #9622

PR checklist

@ronald-cron-arm ronald-cron-arm added enhancement needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon 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 and removed needs-ci Needs to pass CI tests labels Nov 12, 2024
@minosgalanakis
Copy link
Contributor

Is this PR intended to remove only the configuration options, or the source code that is enabled by it?

Should we consider adjusting the ext configration files?

@ronald-cron-arm
Copy link
Contributor Author

ronald-cron-arm commented Nov 14, 2024 •
edited
Loading

Is this PR intended to remove only the configuration options, or the source code that is enabled by it?

Both.

Should we consider adjusting the ext configration files?

I have on purpose not changed these ones as they are not supposed to be changed, just imported from TF-M project.

@ronald-cron-arm ronald-cron-arm mentioned this pull request Nov 14, 2024
Closed
@ronald-cron-arm ronald-cron-arm force-pushed the remove-psa-crypto-config branch from 615907c to c849b30 Compare November 14, 2024 12:01
Harry-Ramsey
Harry-Ramsey previously approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ronald-cron-arm ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Nov 14, 2024
@ronald-cron-arm ronald-cron-arm mentioned this pull request Nov 15, 2024
Merged
@ronald-cron-arm ronald-cron-arm added needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, labels Nov 15, 2024
minosgalanakis
minosgalanakis previously approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ronald-cron-arm ronald-cron-arm force-pushed the remove-psa-crypto-config branch from e5ef871 to 499e715 Compare November 18, 2024 08:10
@ronald-cron-arm
Copy link
Contributor Author

ronald-cron-arm commented Nov 18, 2024 •
edited
Loading

I've updated the last commit to point to the new version of the framework companion PR. I have then rebased on top of development. That way with 9567 rebased on top of this one we have 9567 on top of 9760(merged) and this one.

@ronald-cron-arm ronald-cron-arm force-pushed the remove-psa-crypto-config branch from 499e715 to 6310d0d Compare November 18, 2024 08:18
@ronald-cron-arm ronald-cron-arm mentioned this pull request Nov 18, 2024
Merged
5 tasks
minosgalanakis
minosgalanakis previously approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ( The Psa-sim failure looks like a CI starvation isssue)

@ronald-cron-arm ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Nov 18, 2024
@ronald-cron-arm
Copy link
Contributor Author

ronald-cron-arm commented Nov 18, 2024 •
edited
Loading

OpenCI is green. ABI-API-checking failure is due to the fact that now PBKDF2 is enabled by default, which is fine. @Harry-Ramsey please have another look and to the new framework companion PR as well: Mbed-TLS/mbedtls-framework#76.

Harry-Ramsey
Harry-Ramsey previously approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ronald-cron-arm ronald-cron-arm removed the needs-review Every commit must be reviewed by at least two team members, label Nov 18, 2024
@ronald-cron-arm ronald-cron-arm mentioned this pull request Nov 20, 2024
Merged
5 tasks
@ronald-cron-arm ronald-cron-arm force-pushed the remove-psa-crypto-config branch from df7f603 to 8cf623d Compare November 20, 2024 12:42
@ronald-cron-arm ronald-cron-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Nov 20, 2024
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Nov 20, 2024
Merged
5 tasks
@ronald-cron-arm
Copy link
Contributor Author

@minosgalanakis @Harry-Ramsey On OpenCI only a glitch in an DTLS test, Internal CI green, thus the CI can be considered green. Please have another look to this PR and its companion PRs #9787 and Mbed-TLS/mbedtls-framework#79.

Harry-Ramsey
Harry-Ramsey previously approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

minosgalanakis
minosgalanakis previously approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ronald-cron-arm ronald-cron-arm force-pushed the remove-psa-crypto-config branch from 8cf623d to 7e56db2 Compare November 21, 2024 12:59
@ronald-cron-arm
Copy link
Contributor Author

Re-running CI to be able to go through the merge queue, OpenCI must be green.

@ronald-cron-arm
Move back *config_test_driver* headers from the framework
e0ebf55 
config_test_driver.h and
crypto_config_test_driver_extension.h are
configuration files thus they better fit in
mbedtls branches than in the framework.

Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm
Remove MBEDTLS_PSA_CRYPTO_CONFIG configuration option
93ba625 
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm
Add change log
42a5c97 
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm ronald-cron-arm force-pushed the remove-psa-crypto-config branch from 7e56db2 to 42a5c97 Compare November 21, 2024 14:53
@ronald-cron-arm
Copy link
Contributor Author

@minosgalanakis @Harry-Ramsey due to the merge of #9794 (I think) this PR got a framework conflict. I rebased and in the first commit changed the framework pointer to the merge of f#79 instead of the head of f#79. Please have another look.

@davidhorstmann-arm davidhorstmann-arm mentioned this pull request Nov 21, 2024
Closed
5 tasks
minosgalanakis
minosgalanakis approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Harry-Ramsey
Harry-Ramsey approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Nov 21, 2024
Merged via the queue into Mbed-TLS:development with commit 28a26ec Nov 21, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm left review comments

@Harry-Ramsey Harry-Ramsey Harry-Ramsey approved these changes

@minosgalanakis minosgalanakis minosgalanakis approved these changes

Assignees
No one assigned
Labels
enhancement needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon
Projects
Roadmap pull requests (new board)
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove MBEDTLS_PSA_CRYPTO_CONFIG configuration option
4 participants
@ronald-cron-arm @minosgalanakis @gilles-peskine-arm @Harry-Ramsey