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

Adapt components with MBEDTLS_PSA_CRYPTO_CONFIG enabled #9185

Merged
merged 7 commits into from
Jun 19, 2024
Merged

Adapt components with MBEDTLS_PSA_CRYPTO_CONFIG enabled #9185

merged 7 commits into from
Jun 19, 2024

Conversation

ronald-cron-arm
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm commented May 27, 2024 •
edited
Loading

Description

Progresses #8153
Associated mbedtls-framework PR: Mbed-TLS/mbedtls-framework#22

PR checklist

@ronald-cron-arm ronald-cron-arm added enhancement needs-ci Needs to pass CI tests component-test Test framework and CI scripts priority-high High priority - will be reviewed soon labels May 27, 2024
This was referenced May 27, 2024
Open
Open
@ronald-cron-arm ronald-cron-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 and removed needs-ci Needs to pass CI tests labels May 28, 2024
@tom-daubney-arm tom-daubney-arm self-requested a review May 28, 2024 08:27
@gilles-peskine-arm gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label May 29, 2024
@ronald-cron-arm ronald-cron-arm mentioned this pull request Jun 4, 2024
Merged
@ronald-cron-arm ronald-cron-arm force-pushed the adapt-components-with-psa-crypto-config-enabled branch from 37029f2 to 84f6aec Compare June 4, 2024 17:36
@ronald-cron-arm
Copy link
Contributor Author

ronald-cron-arm commented Jun 4, 2024 •
edited
Loading

I've rebased because of a conflict following the merge of #9200, move of generate_psa_wrappers.py to the framework repository. Because of the move to the framework this PR is now associated to a framework PR, namely Mbed-TLS/mbedtls-framework#22. Eventually, following the discussion in #9190, I have just removed component_test_full_no_bignum rather than adapting it.

@ronald-cron-arm ronald-cron-arm force-pushed the adapt-components-with-psa-crypto-config-enabled branch from 84f6aec to 85438f6 Compare June 5, 2024 07:21
@ronald-cron-arm
Copy link
Contributor Author

@tom-daubney-arm @davidhorstmann-arm this is now ready for re-review/review. See #9185 (comment) for more information.

@ronald-cron-arm ronald-cron-arm force-pushed the adapt-components-with-psa-crypto-config-enabled branch from 85438f6 to efcd8bf Compare June 7, 2024 08:07
@ronald-cron-arm
Copy link
Contributor Author

ronald-cron-arm commented Jun 7, 2024 •
edited
Loading

I've just amended with no change the last commit to get the DCO check.

@ronald-cron-arm ronald-cron-arm mentioned this pull request Jun 14, 2024
Merged
4 tasks
tom-daubney-arm
tom-daubney-arm previously approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@tom-daubney-arm tom-daubney-arm left a comment

Choose a reason for hiding this comment

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

LGTM - thanks

davidhorstmann-arm
davidhorstmann-arm requested changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm 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.

The tip of MbedTLS/mbedtls-framework#22 doesn't seem to match the submodule update for this PR, have you missed an update to one of them?

The framework commit in this PR seems to be 353f7145f259a6f24f2483395d46e86ae27ae085, whereas the tip of Mbed-TLS/mbedtls-framework#22 is c3d2f019196c1109ed9b5f730eff8b4d09806f75.

The 2 commits are equivalent and this PR will be updated on the merge of the other anyway, so I'll review the other work.

davidhorstmann-arm
davidhorstmann-arm previously approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ronald-cron-arm
Copy link
Contributor Author

The tip of MbedTLS/mbedtls-framework#22 doesn't seem to match the submodule update for this PR, have you missed an update to one of them?

The framework commit in this PR seems to be 353f7145f259a6f24f2483395d46e86ae27ae085, whereas the tip of Mbed-TLS/mbedtls-framework#22 is c3d2f019196c1109ed9b5f730eff8b4d09806f75.

The 2 commits are equivalent and this PR will be updated on the merge of the other anyway, so I'll review the other work.

Yes sorry about that, in #22 I had to retrigger the DCO check and this has resulted in a change of commit id whilst the content of the commit is the same.

@ronald-cron-arm
Adapt test_crypto_full_md_light_only
a67f1be 
Adapt test_crypto_full_md_light_only with
MBEDTLS_PSA_CRYPTO_CONFIG enabled.

No need to disable PSA_WANT_ALG_HKDF as
the PSA implementation of HKDF is independent
of hkdf.c and thus of MAC through md.c.

Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm ronald-cron-arm force-pushed the adapt-components-with-psa-crypto-config-enabled branch from efcd8bf to c9c71e3 Compare June 19, 2024 07:26
@ronald-cron-arm
Copy link
Contributor Author

ronald-cron-arm commented Jun 19, 2024 •
edited
Loading

@davidhorstmann-arm @tom-daubney-arm there was a conflict with development regarding the framework pointer. My understanding is that following the merge of #9247, the framework commit pointed to by this PR was not a descendant of the framework commit pointed to by the head of the development branch anymore. Thus I have rebased. Apart from the framework, the rebase was straightforward. Regarding the framework, the commit "tests: src: Fix PSA test wrappers for PAKE" now points to the rebase of PR22 on top of the merge of PR 26: commit b332327. Note that in PR22, GitHub has kept your approvals after the rebase, I guess this is because the trees before and after the rebase are the same.

@ronald-cron-arm
Adapt component_test_when_no_ciphersuites_have_mac
0417a2c 
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm ronald-cron-arm force-pushed the adapt-components-with-psa-crypto-config-enabled branch from c9c71e3 to 0417a2c Compare June 19, 2024 07:45
@ronald-cron-arm ronald-cron-arm mentioned this pull request Jun 19, 2024
Merged
4 tasks
@gabor-mezei-arm gabor-mezei-arm added priority-very-high Highest priority - prioritise this over other review work and removed priority-high High priority - will be reviewed soon labels Jun 19, 2024
@gabor-mezei-arm gabor-mezei-arm mentioned this pull request Jun 19, 2024
Merged
4 tasks
@ronald-cron-arm
Copy link
Contributor Author

@davidhorstmann-arm @tom-daubney-arm the OpenCI has passed successfully thus the CI can be considered as green. The PR is ready for re-review, thanks.

davidhorstmann-arm
davidhorstmann-arm previously approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

Rebase LGTM, thanks!

@ronald-cron-arm
Update framework submodule to the merge of PR22
390dba6 
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm
Copy link
Contributor Author

@davidhorstmann-arm @tom-daubney-arm last bit, the update of the framework to the merge of PR22.

davidhorstmann-arm
davidhorstmann-arm approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

tom-daubney-arm
tom-daubney-arm approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@tom-daubney-arm tom-daubney-arm left a comment

Choose a reason for hiding this comment

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

LGTM too - thanks for all the rebase work!

@tom-daubney-arm tom-daubney-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 19, 2024
@tom-daubney-arm
Copy link
Contributor

Note: Internal CI failures are spurious and due to timeouts.

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

@tom-daubney-arm tom-daubney-arm tom-daubney-arm approved these changes

@davidhorstmann-arm davidhorstmann-arm davidhorstmann-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-test Test framework and CI scripts enhancement priority-very-high Highest priority - prioritise this over other review work
Projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ronald-cron-arm @tom-daubney-arm @davidhorstmann-arm @gilles-peskine-arm @gabor-mezei-arm