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

Switch generate_psa_test.py to automatic dependencies for positive test cases #83

Conversation

gilles-peskine-arm
Copy link
Contributor

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

This is mostly refactoring around generate_psa_tests.py and supporting Python libraries. Create a class for test case generation that can automatically determine test case dependencies. Use this for generate_psa_tests.py for positive test cases.

Practical effect:

  • Never-executed test cases are now easier to work with, because they're commented out and have the reason in a comment.
  • Fix some test cases that were not properly detected as never-executed.

This is a step on the forward port of Mbed-TLS/mbedtls#9025. Here, I handle the positive test cases of generate_psa_tests.py, which brings me to a natural cutoff point.

Follow-up: handling the negative test cases (KeyTypeNotSupported, OpFail).

Review recommendations

The commits are broken down in a way to make their impact on the generated files easy to understand. Only three commits change the output, the others are pure refactoring:

To validate my claims about the impact on the generated files, I use mbedtls-trace-files.py from Mbed-TLS/mbedtls-docs#23 combined with the diff-pairs script attached here.
diff-pairs.txt

From the framework subdirectory with this pull request checked out, with Mbed-TLS/mbedtls#9796 (3.6) checked out in the parent directory:

mbedtls-trace-files.py -b .. -f 1 $(git rev-parse ':/Create a framework')..HEAD $(./scripts/generate_psa_tests.py --list)
diff-pairs [0-9][0-9]-*/
ls -l [0-9][0-9]*.diff

Then for each NN-SHA.diff file, check that the content makes sense given the commit message for SHA.

With https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/ checked out in the parent, the following invocation of mbedtls-trace-files.py works:

mbedtls-trace-files.py --skip-make -r 'cd .. && framework/scripts/generate_psa_tests.py' -b .. -f 1 $(git rev-parse ':/Create a framework')..HEAD $(./scripts/generate_psa_tests.py --list)
diff-pairs [0-9][0-9]-*/
ls -l [0-9][0-9]*.diff

PR checklist

@gilles-peskine-arm gilles-peskine-arm added needs-work needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Nov 21, 2024
@gilles-peskine-arm gilles-peskine-arm force-pushed the dev/gilles-peskine-arm/psa-storage-test-cases-never-supported-positive-framework branch from 18e5920 to b2ea59e Compare November 27, 2024 18:21
@davidhorstmann-arm
Copy link
Contributor

Needs rebase atop #67

The new class `psa_test_case.TestCase` will automatically infer dependencies
from the test data. The dependency inference is not done yet, this will be
implemented in subsequent commits.

No change to any generated file since this new module is not used yet.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Allow "skipping" a test case, meaning that the test case is generated
commented out. This is useful when systematically generating test cases
according to certain rules, where some generated tests cannot be executed
but we still want them to be visible when auditing the generation output.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
But for now, fully override its automatic dependency inference. We will
switch to using the automatic dependencies in future commits.

No change to the generated files.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
No change to the generated files.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
…anisms

No change to the generated output.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
To determine PSA mechanisms that are not implemented, also read `PSA_WANT_`
symbols that cannot (or are not intended to) be configured independently,
and thus are not listed in `psa/crypto_config.h`. Find those symbols in
the config adjustment header `psa/crypto_adjust_config_synonyms.h`.

No impact on generated files yet, because `find_dependencies_not_implemented`
is currently only used on key types that have explicit dependencies. This
will allow using hack_dependencies_not_implemented in other places, for
example to handle algorithm variants like `PSA_WANT_ALG_ECDSA_ANY` which is
inferred from `PSA_WANT_ALG_ECDSA`.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
We can't even attempt to generate DSA test cases because
`asymmetric_key_data.py` doesn't have test data for DSA.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
When we generate a test case for a mechanism that is not implemented,
comment out the test case rather than giving it a never-fulfilled
dependency. That way we don't create test cases that cannot be executed.

This changes the generated output in the following ways:

* No longer emit test cases with a dependency on
  `DEPENDENCY_NOT_IMPLEMENTED_YET`. All removed lines that start with
  `depends_on:` contain `DEPENDENCY_NOT_IMPLEMENTED_YET.
* Emit commented-out test cases instead: all the new lines are comment lines.

There is no change in which test cases actually get executed. This removes
many test cases from the list of available test cases, which causes some of
the exceptions in `analyze_outcomes.py` to no longer be useful.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
No semantic change. In the generated files, `depends_on:` lines have entries
that are reordered.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
No change to the generated files (the new code isn't used yet).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In `psa_test_cases.TestCase`:

* Implement basic support for automatic dependencies, by calling
  `psa_information.automatic_dependencies`.
* Support an alternative dependency prefix.

No changes to the generated file.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This fixes the dependencies for DH group and elliptic curve families.

No changes to the generated output (the new functionality isn't used yet).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This fixes the dependencies for key pair types, which have finer-grained
dependencies for different operations (BASIC, GENERATE, EXPORT, ...).

No changes to the generated output (the new functionality isn't used yet).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
No changes to the generated output (the new functionality isn't used yet).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Automatically skip test cases with not-implemented automatic dependencies.

No changes to the generated output.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This causes more test cases to be commented out due to mechanisms that are
not implemented, because the code `generate_psa_tests.StorageFormat` was not
trying to skip never-supported dependencies.

To review for correctness, filter the diff of the generated files as
follows to find new skip reasons:
```
grep -E '^\+## # skipped because' | sort -u
```
And check that none of the appearing mechanisms are implemented.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm force-pushed the dev/gilles-peskine-arm/psa-storage-test-cases-never-supported-positive-framework branch from b2ea59e to e1f38eb Compare December 11, 2024 10:38
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Dec 11, 2024
@gilles-peskine-arm gilles-peskine-arm added the needs-reviewer This PR needs someone to pick it up for review label Dec 11, 2024
not dep.lstrip('!').startswith('PSA_WANT'))
for dep in dependencies):
dependencies.append('DEPENDENCY_NOT_IMPLEMENTED_YET')

def tweak_key_pair_dependency(dep: str, usage: str):
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

psa_information.tweak_key_pair_dependency, and thus psa_test_case.TestCase.infer_dependencies, silently suppresses dependencies on key pairs if a usage is specified and is not BASIC or GENERATE (for example IMPORT or DERIVE). Furthermore, if no usage is specified with set_key_pair_usage() and a key pair type is present, then infer_dependencies ends up generating the deprecated dependency symbol PSA_WANT_KEY_TYPE_xxx_KEY_PAIR.

Right now, this ends up working, because we always call either set_key_pair_usage('GENERATE') or set_key_pair_usage('BASIC'). This could be considered in scope since set_key_pair_usage() is a new method and it isn't working correctly. This could also be considered out of scope since it doesn't break anything and the goal of this pull request is to forward-port some improvements, not to fix every problem we can find. I propose to consider it out-of-scope here, and handle it in the follow-up that handles negative test cases, for which the current behavior does introduce a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should consider it out of scope. (Just commenting, not reviewing.)

@gilles-peskine-arm
Copy link
Contributor Author

With this branch as it is now (e1f38eb), the CI on 3.6 passes but the CI on
mbedtls/development post-split
fails (it passed pre-split). This is due to unrelated changes in the framework that are required to get past the split.

I've switched the mbedtls/development and crypto pull requests to a rebased framework branch, and I expect the CI to pass. We can either merge this PR as it is or add a merge commit with a recent main or rebase it on top of a recent main, to get the same content that (I expect) will pass the CI.

@yanesca
Copy link
Contributor

yanesca commented Dec 18, 2024

I think that if the PRs with the rebased framework branch pass, we should just merge this one as it is.

@gabor-mezei-arm gabor-mezei-arm self-requested a review December 18, 2024 11:42
Copy link
Contributor

@gabor-mezei-arm gabor-mezei-arm left a comment

Choose a reason for hiding this comment

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

Looks good to me. Ci is passing on all of the three using repos.

IMO the changes make the test generation code more readable.

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.

@yanesca yanesca merged commit 4e3d7d8 into main Dec 20, 2024
1 of 2 checks passed
@yanesca
Copy link
Contributor

yanesca commented Dec 20, 2024

Merging as the crypto and TLS PRs passed CI (see #83 (comment))

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jan 2, 2025
Update TF-PSA-Crypto to have the latest framework with
Mbed-TLS/mbedtls-framework#83 .

Update the framework to match.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jan 2, 2025
Update TF-PSA-Crypto to have the latest framework with
Mbed-TLS/mbedtls-framework#83 .

Update the framework to match.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jan 2, 2025
Update TF-PSA-Crypto to have the latest framework with
Mbed-TLS/mbedtls-framework#83 .

Update the framework to match.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@waleed-elmelegy-arm waleed-elmelegy-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, needs-reviewer This PR needs someone to pick it up for review labels Jan 8, 2025
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jan 8, 2025
Update TF-PSA-Crypto to have the latest framework with
Mbed-TLS/mbedtls-framework#83 .

Update the framework to match.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jan 8, 2025
Update TF-PSA-Crypto to have the latest framework with
Mbed-TLS/mbedtls-framework#83 .

Update the framework to match.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jan 9, 2025
Update TF-PSA-Crypto to have the latest framework with
Mbed-TLS/mbedtls-framework#83 .

Update the framework to match.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
waleed-elmelegy-arm pushed a commit to waleed-elmelegy-arm/mbedtls that referenced this pull request Jan 24, 2025
Update TF-PSA-Crypto to have the latest framework with
Mbed-TLS/mbedtls-framework#83 .

Update the framework to match.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
waleed-elmelegy-arm pushed a commit to waleed-elmelegy-arm/mbedtls that referenced this pull request Jan 24, 2025
Update TF-PSA-Crypto to have the latest framework with
Mbed-TLS/mbedtls-framework#83 .

Update the framework to match.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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 priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Development

Successfully merging this pull request may close these issues.

5 participants