Skip to content

Fix include in psa compliance tests to mbedtls config file #10041

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
Merged
merged 3 commits into from
Mar 12, 2019
Merged

Fix include in psa compliance tests to mbedtls config file #10041

merged 3 commits into from
Mar 12, 2019

Conversation

netanelgonen
Copy link
Contributor

Description

Fix issue #10035
change the way pal_crypto_config.h includes mbedtls configuration.
in this way, it will take the user file if needed

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@TaniaMirzin @orenc17 @avolinski

Release Notes

@0xc0170 0xc0170 requested a review from a team March 11, 2019 16:56
0xc0170
0xc0170 approved these changes Mar 11, 2019
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Not certain why all these additional spaces added though

@0xc0170 0xc0170 requested a review from a user March 11, 2019 16:58
@netanelgonen netanelgonen changed the title Fix include mbedtls Fix include in psa compliance tests to mbedtls config file Mar 11, 2019
alzix
alzix reviewed Mar 11, 2019
View reviewed changes
@alzix
Copy link
Contributor

alzix commented Mar 11, 2019

@0xc0170,

Not certain why all these additional spaces added though

there is a tab hiding at the begging of the line

remove hidden tad
0f5147c 
Co-Authored-By: netanelgonen <[email protected]>
alzix
alzix reviewed Mar 11, 2019
View reviewed changes
features/frameworks/TARGET_PSA/pal/pal_crypto_config.h
@@ -408,4 +404,4 @@

#include "pal_crypto_config_check.h"

#endif /* _PAL_CRYPTO_CONFIG_H_ */
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reason for this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bug in the code, user config should augment the mbedtls one not replace it

features/frameworks/TARGET_PSA/pal/pal_crypto_config.h
#else
#include MBEDTLS_CONFIG_FILE
#endif
#include "mbedtls/config.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two changes here:

  1. change default header file include path
  2. remove an option to override this file from app sources

I'm fine with the first one, the second one is not clear to me.
What is the reason for removing the ability to use custom config file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

part 2 is actually made inside part 1
mbedtls overwriting the file, we should not do this.

@NirSonnenschein
Copy link
Contributor

starting CI while we wait for reviews

ghost
ghost approved these changes Mar 11, 2019
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Meets criteria, PSA required. Approved.

@ghost ghost added the PM_ACCEPTED label Mar 11, 2019
@mbed-ci
Copy link

mbed-ci commented Mar 11, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

@NirSonnenschein
Copy link
Contributor

NirSonnenschein commented Mar 11, 2019 •
edited
Loading

CI has passed and this is PM approved. @avolinski @TaniaMirzin please review

@cmonr cmonr merged commit 1471b4c into ARMmbed:master Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alzix alzix alzix left review comments

@NirSonnenschein NirSonnenschein NirSonnenschein approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@orenc17 orenc17 orenc17 approved these changes

@alekshex alekshex alekshex approved these changes

@TaniaMirzin TaniaMirzin Awaiting requested review from TaniaMirzin

Assignees
No one assigned
Labels
release-version: 5.12.0-rc2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@netanelgonen @alzix @NirSonnenschein @mbed-ci @0xc0170 @orenc17 @alekshex @cmonr