sys/psa_crypto: Update key headers by Wer-Wolf · Pull Request #21046 · RIOT-OS/RIOT

Wer-Wolf · 2024-11-26T19:06:36Z

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.

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]>

mguetschow

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 · 2024-11-27T15:26:58Z

Murdock results

✔️ PASSED

7cabd3e sys/psa_crypto: Split key attributes definitions into separate file

Success	Failures	Total	Runtime
10249	0	10249	18m:08s

Artifacts

Documentation preview

mguetschow · 2024-11-27T15:29:46Z

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

Details

-- 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 · 2024-11-27T19:00:07Z

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 · 2024-11-27T19:06:20Z

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 · 2024-11-27T19:38:19Z

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 · 2024-11-28T10:45:26Z

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

LGTM!

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

Einhornhool · 2024-11-29T18:24:47Z

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 · 2024-11-29T20:47:15Z

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 · 2024-12-01T23:07:43Z

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

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]>

Wer-Wolf · 2024-12-06T08:54:19Z

The tests seems to be happy now, anything else which needs to be changed?

mguetschow

Looks good, thank you!

github-actions bot added the Area: sys Area: System label Nov 26, 2024

benpicco requested review from Einhornhool, PeterKietzmann and mguetschow November 27, 2024 12:31

mguetschow changed the title ~~Psa key headers~~ sys/psa_crypto: Update key headers Nov 27, 2024

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 approved these changes Nov 27, 2024

View reviewed changes

Einhornhool approved these changes Nov 29, 2024

View reviewed changes

Wer-Wolf force-pushed the psa-key-headers branch from c283518 to 7cabd3e Compare December 5, 2024 20:36

mguetschow approved these changes Dec 10, 2024

View reviewed changes

mguetschow enabled auto-merge December 10, 2024 13:04

mguetschow added this pull request to the merge queue Dec 10, 2024

Merged via the queue into RIOT-OS:master with commit c80058e Dec 10, 2024

MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025

mguetschow deleted the psa-key-headers branch October 16, 2025 12:23

Conversation

Wer-Wolf commented Nov 26, 2024

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

riot-ci commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

mguetschow commented Nov 27, 2024

Uh oh!

Wer-Wolf commented Nov 27, 2024

Uh oh!

Wer-Wolf commented Nov 27, 2024

Uh oh!

Wer-Wolf commented Nov 27, 2024

Uh oh!

mguetschow commented Nov 28, 2024

Uh oh!

Einhornhool left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Einhornhool commented Nov 29, 2024

Uh oh!

Wer-Wolf commented Nov 29, 2024

Uh oh!

Einhornhool commented Dec 1, 2024

Uh oh!

Wer-Wolf commented Dec 6, 2024

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

riot-ci commented Nov 27, 2024 •

edited

Loading

Einhornhool left a comment •

edited

Loading