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

sys/psa_crypto: Update key headers #21046

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

sys/psa_crypto: Update key headers #21046

wants to merge 2 commits into from
+1,689 −1,474

Conversation

Wer-Wolf
Copy link
Contributor

Contribution description

This PR is a continuation of PR #20906. The final goal still is to split all definitions into separate header files, so backends can for example use psa_key_attributes_t without pulling in the definition for psa_mac_operation_t.

Testing procedure

The changes where tested using the PSA crypto unit tests and appear to work.

Issues/PRs references

Continuation of PR #20906.

@Wer-Wolf
sys/psa_crypto: Split key definitions into separate files
dd81d18 
Split key definitions into separate files, together with some basic
support macros.

This allows PSA crypto backends to use this definitions without
pulling in all the other type definitions.

Signed-off-by: Armin Wolf <[email protected]>
@Wer-Wolf
sys/psa_crypto: Split key attributes definitions into separate file
c283518 
Split key attributes definitions into separate file, together with
some basic support macros and helper functions.

This allows PSA crypto backends to use this definitions without
pulling in all the other type definitions.

Signed-off-by: Armin Wolf <[email protected]>
@github-actions github-actions bot added the Area: sys Area: System label Nov 26, 2024
@mguetschow mguetschow changed the title Psa key headers sys/psa_crypto: Update key headers Nov 27, 2024
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 27, 2024
mguetschow
mguetschow approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

LGTM from a brief look at the changes, trusting you that you haven't changed details while moving code around :)

Maybe @Einhornhool can give a second look?

@riot-ci
Copy link

riot-ci commented Nov 27, 2024

Murdock results

❌ FAILED

c283518 sys/psa_crypto: Split key attributes definitions into separate file

Success Failures Total Runtime
213 0 9269 01m:51s

Artifacts

@mguetschow
Copy link
Contributor

CI encountered some problem while building tests/sys/psa_crypto_hashes for atmega256rfr2-xpro:

-- running on worker breeze thread 1, build number 31131.
make: Entering directory '/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/tests/sys/psa_crypto_hashes'
 You are going to use the PSA Crypto module, which is only partly implemented and not yet thouroughly tested.\n Please do not use this module in production, as it may introduce security issues!
make: Leaving directory '/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/tests/sys/psa_crypto_hashes'
make: Entering directory '/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/tests/sys/psa_crypto_hashes'
 You are going to use the PSA Crypto module, which is only partly implemented and not yet thouroughly tested.\n Please do not use this module in production, as it may introduce security issues!
Building application "tests_psa_crypto_hashes" for "atmega256rfr2-xpro" with CPU "atmega256rfr2".

In file included from include/psa_crypto_se_management.h:29:0,
                 from psa_crypto_location_dispatch.c:25:
include/psa_crypto_se_driver.h:899:59: error: unknown type name ‘psa_key_attributes_t’
                                                     const psa_key_attributes_t *attributes,
                                                           ^
include/psa_crypto_se_driver.h:941:67: error: unknown type name ‘psa_key_attributes_t’
                                                             const psa_key_attributes_t *attributes,
                                                                   ^
include/psa_crypto_se_driver.h:975:55: error: unknown type name ‘psa_key_attributes_t’
                                                 const psa_key_attributes_t *attributes,
                                                       ^
include/psa_crypto_se_driver.h:1072:59: error: unknown type name ‘psa_key_attributes_t’
                                                     const psa_key_attributes_t *attributes,
                                                           ^
In file included from psa_crypto_location_dispatch.c:25:0:
include/psa_crypto_se_management.h:150:43: error: unknown type name ‘psa_key_attributes_t’
 psa_status_t psa_find_free_se_slot( const psa_key_attributes_t *attributes,
                                           ^
make[3]: *** [/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/Makefile.base:149: /tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/build/psa_crypto/psa_crypto_location_dispatch.o] Error 1
make[3]: *** Waiting for unfinished jobs....
In file included from psa_crypto.c:28:0:
include/psa_crypto_se_driver.h:899:59: error: unknown type name ‘psa_key_attributes_t’
                                                     const psa_key_attributes_t *attributes,
                                                           ^
include/psa_crypto_se_driver.h:941:67: error: unknown type name ‘psa_key_attributes_t’
                                                             const psa_key_attributes_t *attributes,
                                                                   ^
include/psa_crypto_se_driver.h:975:55: error: unknown type name ‘psa_key_attributes_t’
                                                 const psa_key_attributes_t *attributes,
                                                       ^
include/psa_crypto_se_driver.h:1072:59: error: unknown type name ‘psa_key_attributes_t’
                                                     const psa_key_attributes_t *attributes,
                                                           ^
In file included from psa_crypto.c:29:0:
include/psa_crypto_se_management.h:150:43: error: unknown type name ‘psa_key_attributes_t’
 psa_status_t psa_find_free_se_slot( const psa_key_attributes_t *attributes,
                                           ^
make[3]: *** [/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/Makefile.base:149: /tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/build/psa_crypto/psa_crypto.o] Error 1
make[2]: *** [/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/Makefile.base:31: ALL--/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/sys/psa_crypto] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/Makefile.base:31: ALL--/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/sys] Error 2
make: *** [/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/tests/sys/psa_crypto_hashes/../../../Makefile.include:747: application_tests_psa_crypto_hashes.module] Error 2
make: Leaving directory '/tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/tests/sys/psa_crypto_hashes'
cat: /tmp/dwq.0.03155465757497755/3164f11bda841325c9939e3075b5cbb9/build/test-input-hash.sha1: No such file or directory
{"build/": 1216}

@Wer-Wolf
Copy link
Contributor Author

I suspect that this happens because now the definition of psa_key_attributes_t is behind a MODULE_PSA_KEY_MANAGEMENT check.

Previously the definition was an incomplete type in such a case.

I will try to fix this.

@Wer-Wolf
Copy link
Contributor Author

I think that psa_crypto_se_driver.h does not employ a preprocessor check when defining its types, so if the necessary dependencies are not enabled some types are missing.

Should i add the preprocessor check or should i reinstate the incomplete type definition of psa_key_attributes_t?

@Wer-Wolf
Copy link
Contributor Author

We could also solve this problem by always defining psa_key_attributes_t. Since it is a pretty fundamental type i think this might be a vaöid option too.

@mguetschow
Copy link
Contributor

Huu, I think we need @Einhornhool's assessment here regarding the conditional compilation and includes. I think I'd favor having the type defined in any case, since it doesn't cost .text, but only (minimal) compile time.

Einhornhool
Einhornhool approved these changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@Einhornhool Einhornhool 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!

Edit, sorry, did not see the discussion. I'll check that out now

@Einhornhool
Copy link
Contributor

Huu, I think we need @Einhornhool's assessment here regarding the conditional compilation and includes. I think I'd favor having the type defined in any case, since it doesn't cost .text, but only (minimal) compile time.

Ok, my thoughts.
A hash application should not need the key attribute type. And the SE headers should only be included, if SEs are used.
So I'd add

#if IS_USED(MODULE_PSA_SECURE_ELEMENT)
#include "psa_crypto_se_management.h"
#include "psa_crypto_se_driver.h"
#endif

where it applies.

@Wer-Wolf
Copy link
Contributor Author

The problem is that the key types are also used inside the multi-part function prototypes, so we would need a lot if preprocessor checks. I rather suggest that we do not hide the type definitions behind such a preprocessor check.

@Einhornhool
Copy link
Contributor

Aah, okay. Well in that case, go for it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mguetschow mguetschow mguetschow approved these changes

@Einhornhool Einhornhool Einhornhool approved these changes

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees
No one assigned
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Wer-Wolf @riot-ci @mguetschow @Einhornhool