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

Backport 3.6: Switch generate_psa_test.py to automatic dependencies for positive test cases #9796

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Nov 21, 2024

Mostly happening in the framework, see Mbed-TLS/mbedtls-framework#83.

We expect one set of complaints from the interface stability check: many storage test cases have disappeared, namely the never-executed test cases that are no longer getting generated.

PR checklist

@gilles-peskine-arm gilles-peskine-arm added needs-work component-crypto Crypto primitives and low-level interfaces component-platform Portability layer and build scripts needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Nov 21, 2024
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa-storage-test-cases-never-supported-positive-3.6 branch 2 times, most recently from 6e6cebd to 99afe62 Compare November 27, 2024 22:05
Running `make library/foo`, `make programs/foo` or `make tests/foo` only
rebuilt the given target if it was not an existing file, because the
toplevel makefile does not know the file's dependencies and thus thought
that every such target had empty dependencies. Fix this by always invoking
make recursively.

Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
…s-never-supported-positive-3.6

Update framework submodule to the tip of main.
@gilles-peskine-arm gilles-peskine-arm added needs-ci Needs to pass CI tests and removed 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 labels Dec 24, 2024
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa-storage-test-cases-never-supported-positive-3.6 branch from 1387207 to 89a1b15 Compare December 24, 2024 16:17
@gilles-peskine-arm gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Dec 24, 2024
@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 labels Dec 24, 2024
Don't list mechanisms that are not implemented in
`include/psa/crypto_config.h`, even commented out. Uncommenting them
wouldn't help anyway: they don't work. Having them listed, even commented
out, causes `find_dependencies_not_implemented()` in `psa_test_case.py` to
consider those mechanisms to be implemented, and thus causes
`generate_psa_tests.py` to generate test cases that cannot be executed.

The affected mechanisms are:

* `PSA_ALG_CBC_MAC` (`PSA_WANT_ALG_CBC_MAC`)
* `PSA_ALG_XTS` (`PSA_WANT_ALG_XTS`)
* `PSA_ECC_FAMILY_SECP_K1` 224-bit (`PSA_WANT_ECC_SECP_K1_224`)

Also remove the affected mechanisms from configuration adjustment files,
since that is code that can never be triggered.

There were already no generated test cases for SECP224K1 because
`PSA_WANT_ECC_SECP_K1_224` was already detected as a dependency that cannot
be implemented, because that is not a valid size: PSA defines SECP224K1 as
225-bit, and `crypto_knowledge.py` follows suite, so `generate_psa_tests.py`
saw `PSA_WANT_ECC_SECP_K1_225` in its enumeration but skipped it because it
was never mentioned in `crypto_config.h`.

This causes generated PSA tests to no longer include positive test cases for
`PSA_ALG_CBC_MAC` and `PSA_ALG_XTS`.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa-storage-test-cases-never-supported-positive-3.6 branch from 89a1b15 to b62279d Compare December 24, 2024 19:07
@davidhorstmann-arm davidhorstmann-arm self-requested a review January 6, 2025 10:44
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!

@waleed-elmelegy-arm waleed-elmelegy-arm self-requested a review January 8, 2025 14:42
Copy link
Contributor

@waleed-elmelegy-arm waleed-elmelegy-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

@waleed-elmelegy-arm waleed-elmelegy-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-reviewer This PR needs someone to pick it up for review needs-review Every commit must be reviewed by at least two team members, labels Jan 8, 2025
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue Jan 9, 2025
Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit 9058998 Jan 9, 2025
4 of 6 checks passed
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 component-crypto Crypto primitives and low-level interfaces component-platform Portability layer and build scripts priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Development

Successfully merging this pull request may close these issues.

3 participants