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.

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

Fix and test MBEDTLS_PSA_INJECT_ENTROPY #7518

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Apr 28, 2023

  • Fix MBEDTLS_PSA_INJECT_ENTROPY breaks the build since 2.26 #7516: fix the build when MBEDTLS_PSA_INJECT_ENTROPY is enabled.
  • Add a test component with MBEDTLS_PSA_INJECT_ENTROPY enabled. This required instrumenting the test framework to perform the necessary entropy injection.
  • We had tests for MBEDTLS_PSA_INJECT_ENTROPY, but they'd never been enabled in any CI build. Make sure they pass and don't leave the system in a state that breaks subsequent tests.
  • Add some checks in the tests that would break if we made an incompatible change to the seed file. Fixes Test stability of the injected entropy file #7517.

Gatekeeper checklist

The build option MBEDTLS_PSA_INJECT_ENTROPY requires some extra platform
functions, for historical reasons. To enable us to test this option, provide
a version of these functions for testing.

(These versions would actually work in production, but providing them in the
library in a way that doesn't break existing users might be slightly tricky,
so it's out of scope of this commit.)

Signed-off-by: Gilles Peskine <[email protected]>
…g tests

The seed file must exist before running tests. Because the location is
somewhat platform- and configuration-dependent, and to be friendly to
developers who run test suites individually and aren't familiar with this
feature, rely on the test framework code rather than on test scripts to
create the seed file.

Signed-off-by: Gilles Peskine <[email protected]>
When MBEDTLS_PSA_INJECT_ENTROPY is enabled, we disable standard entropy
sources, so mbedtls_entropy_func() doesn't work out of the box. Disable
tests that rely on it. MBEDTLS_PSA_INJECT_ENTROPY is intended for PSA-only
environments anyway, so it doesn't matter if some legacy features don't work
normally.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added bug enhancement needs-ci Needs to pass CI tests component-psa PSA keystore/dispatch layer (storage, drivers, …) size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits labels Apr 28, 2023
Until now, we were never enabling this option in any test.

MBEDTLS_PSA_INJECT_ENTROPY requires MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES, so
it cannot be enabled in the full config and it gets its own component.

Test with MBEDTLS_USE_PSA_CRYPTO enabled, since MBEDTLS_PSA_INJECT_ENTROPY
is a very PSA feature (which can break non-PSA applications), and Mbed
OS (for whch MBEDTLS_PSA_INJECT_ENTROPY was designed) enables
MBEDTLS_USE_PSA_CRYPTO when it enables MBEDTLS_PSA_INJECT_ENTROPY.

Signed-off-by: Gilles Peskine <[email protected]>
This function was written before the PSA storage layer switched to the PSA
ITS API as its storage abstraction. Now we can just call PSA ITS functions
unconditionally.

Signed-off-by: Gilles Peskine <[email protected]>
The seed file is part of the stable interface of PSA_CRYPTO_INJECT_ENTROPY,
because it has to survive a library upgrade on a device. So check that its
existence and content are as expected at each point in the tested life cycle.

Signed-off-by: Gilles Peskine <[email protected]>
The seed file UID is part of the library's stable interface.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa_inject_entropy-file-stability branch from feb47d0 to b377229 Compare April 28, 2023 22:28
The test framework leaves the seed file behind (like it does with the
corresponding file in the legacy API, namely seedfile), so ignore it.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-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 2, 2023
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

@ronald-cron-arm ronald-cron-arm self-requested a review May 31, 2023 09:55
@ronald-cron-arm ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label May 31, 2023
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

It looks almost good to me. Just some comments that do not seem to be right to me and one question.

tests/suites/test_suite_psa_crypto_entropy.function Outdated Show resolved Hide resolved
tests/configs/user-config-for-test.h Show resolved Hide resolved
@gilles-peskine-arm
Copy link
Contributor Author

The 2.28 backport is up: #7965

Copy link
Contributor

@ronald-cron-arm ronald-cron-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

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

@tom-cosgrove-arm tom-cosgrove-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 Jul 21, 2023
@gilles-peskine-arm gilles-peskine-arm merged commit 5647d06 into Mbed-TLS:development Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-psa PSA keystore/dispatch layer (storage, drivers, …) enhancement priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Test stability of the injected entropy file MBEDTLS_PSA_INJECT_ENTROPY breaks the build since 2.26
3 participants