gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

Test context population #686

Open
bjwtaylor wants to merge 16 commits from bjwtaylor/test-context-population into development
pull from: bjwtaylor/test-context-population
merge into: gh-Mbed-TLS:development
Conversation 10 Commits 16 Files changed 7 +486 -332
bjwtaylor commented 2026-02-09 03:50:51 -06:00 (Migrated from github.com)

Description

Resolves https://github.com/Mbed-TLS/mbedtls/issues/10573

Add in a new helper function validate_pk_context to validate contexts produced by the various parser functions in PK.

PR checklist

  • changelog provided | not required because: TBC
  • framework PR not required
  • mbedtls development PR not required because:
  • mbedtls 3.6 PR not required because:
  • tests provided
## Description Resolves https://github.com/Mbed-TLS/mbedtls/issues/10573 Add in a new helper function validate_pk_context to validate contexts produced by the various parser functions in PK. ## PR checklist - [ ] **changelog** provided | not required because: TBC - [ ] **framework PR** not required - [ ] **mbedtls development PR** not required because: - [ ] **mbedtls 3.6 PR** not required because: - **tests** provided
mpg (Migrated from github.com) reviewed 2026-02-09 03:50:51 -06:00
mpg commented 2026-02-09 04:19:49 -06:00 (Migrated from github.com)

Adds validate_pk_context to validate all contexts and test_pk_context_setup to generate the contexts.

Adding validate_pk_context is indeed what we want, but I don't think we want to be adding test_pk_context_setup. From the issue description:

have a central test helper function that validates all the invariants that a populated PK context should satisfy, and then call that helper in each of the existing test functions for parsing (and other methods of populating a context

That is, something like the following:

diff --git a/tests/suites/test_suite_pkparse.function b/tests/suites/test_suite_pkparse.function
index b2ce02083611..0dbcb78a7cc1 100644
--- a/tests/suites/test_suite_pkparse.function
+++ b/tests/suites/test_suite_pkparse.function
@@ -127,6 +127,7 @@ void pk_parse_keyfile_rsa(char *key_file, char *password, int result)
     TEST_EQUAL(res, result);
 
     if (res == 0) {
+        TEST_EQUAL(0, validate_pk_context(&ctx));
         TEST_ASSERT(mbedtls_pk_can_do(&ctx, MBEDTLS_PK_RSA));
         TEST_EQUAL(mbedtls_pk_get_bitlen(&ctx), ctx.bits);
         TEST_EQUAL(mbedtls_pk_get_len(&ctx), PSA_BITS_TO_BYTES(ctx.bits));
@@ -164,6 +165,7 @@ void pk_parse_public_keyfile_rsa(char *key_file, int result)
     TEST_EQUAL(res, result);
 
     if (res == 0) {
+        TEST_EQUAL(0, validate_pk_context(&ctx));
         TEST_ASSERT(mbedtls_pk_can_do(&ctx, MBEDTLS_PK_RSA));
 
         TEST_EQUAL(mbedtls_pk_get_bitlen(&ctx), ctx.bits);
@@ -196,6 +198,7 @@ void pk_parse_public_keyfile_ec(char *key_file, int result)
     TEST_EQUAL(res, result);
 
     if (res == 0) {
+        TEST_EQUAL(0, validate_pk_context(&ctx));
         TEST_ASSERT(mbedtls_pk_can_do(&ctx, MBEDTLS_PK_ECKEY));
 
 #if defined(MBEDTLS_PSA_CRYPTO_C)

and so on for all of the other test functions pkparse.function.

Creating a test_pk_context_setup function that tests all the code paths that can happen when parsing a key, including PEM vs DER, encrypted or not, public key included or not (it's optional in ECC), etc etc would be a gigantic task, but fortunately you don't have to do it because it's already done and it's called test_suite_pkparse.

We just want to make the existing test cases stricter by having them call the new validate_pk_context() function. As you can see, most of the test are already doing some sanity checks on the parsed context when parsing was successful. All we want to do is also test the new invariants such as pub_raw_len != 0 etc (list in issue description) in addition, via the new function validate_pk_context.

> Adds validate_pk_context to validate all contexts and test_pk_context_setup to generate the contexts. Adding `validate_pk_context` is indeed what we want, but I don't think we want to be adding `test_pk_context_setup`. From the issue description: > have a central test helper function that validates all the invariants that a populated PK context should satisfy, and then call that helper in each of the existing test functions for parsing (and other methods of populating a context That is, something like the following: ```diff diff --git a/tests/suites/test_suite_pkparse.function b/tests/suites/test_suite_pkparse.function index b2ce02083611..0dbcb78a7cc1 100644 --- a/tests/suites/test_suite_pkparse.function +++ b/tests/suites/test_suite_pkparse.function @@ -127,6 +127,7 @@ void pk_parse_keyfile_rsa(char *key_file, char *password, int result) TEST_EQUAL(res, result); if (res == 0) { + TEST_EQUAL(0, validate_pk_context(&ctx)); TEST_ASSERT(mbedtls_pk_can_do(&ctx, MBEDTLS_PK_RSA)); TEST_EQUAL(mbedtls_pk_get_bitlen(&ctx), ctx.bits); TEST_EQUAL(mbedtls_pk_get_len(&ctx), PSA_BITS_TO_BYTES(ctx.bits)); @@ -164,6 +165,7 @@ void pk_parse_public_keyfile_rsa(char *key_file, int result) TEST_EQUAL(res, result); if (res == 0) { + TEST_EQUAL(0, validate_pk_context(&ctx)); TEST_ASSERT(mbedtls_pk_can_do(&ctx, MBEDTLS_PK_RSA)); TEST_EQUAL(mbedtls_pk_get_bitlen(&ctx), ctx.bits); @@ -196,6 +198,7 @@ void pk_parse_public_keyfile_ec(char *key_file, int result) TEST_EQUAL(res, result); if (res == 0) { + TEST_EQUAL(0, validate_pk_context(&ctx)); TEST_ASSERT(mbedtls_pk_can_do(&ctx, MBEDTLS_PK_ECKEY)); #if defined(MBEDTLS_PSA_CRYPTO_C) ``` and so on for all of the other test functions `pkparse.function`. Creating a `test_pk_context_setup` function that tests all the code paths that can happen when parsing a key, including PEM vs DER, encrypted or not, public key included or not (it's optional in ECC), etc etc would be a gigantic task, but fortunately you don't have to do it because it's already done and it's called `test_suite_pkparse`. We just want to make the existing test cases stricter by having them call the new `validate_pk_context()` function. As you can see, most of the test are already doing some sanity checks on the parsed context when parsing was successful. All we want to do is also test the new invariants such as `pub_raw_len != 0` etc (list in issue description) in addition, via the new function `validate_pk_context`.
bjwtaylor commented 2026-02-09 04:38:37 -06:00 (Migrated from github.com)

So, the test_pk_context_setup function is basically just a renamed version of the original test_bitlen_setup. The original function didn't make sense anymore, as bitlen is now tested in validate_pk_context. You're right though. This functionality probably duplicates most of functionality in other parts of the system, though I wasn't confident all of these scenarios were covered. The aim here was to refactor these into a single smaller function, that guarantees it covers all parsing scenarios. Though if we are confident existing tests already do this/do not want to expand the scope to include this. Then I think we can probably just delete this function, add the additional tests from other parsing functions to validate_pk_context and then test all parsing contexts with this function. What do you think @mpg?

So, the `test_pk_context_setup` function is basically just a renamed version of the original `test_bitlen_setup`. The original function didn't make sense anymore, as bitlen is now tested in `validate_pk_context`. You're right though. This functionality probably duplicates most of functionality in other parts of the system, though I wasn't confident all of these scenarios were covered. The aim here was to refactor these into a single smaller function, that guarantees it covers all parsing scenarios. Though if we are confident existing tests already do this/do not want to expand the scope to include this. Then I think we can probably just delete this function, add the additional tests from other parsing functions to `validate_pk_context` and then test all parsing contexts with this function. What do you think @mpg?
mpg (Migrated from github.com) reviewed 2026-02-09 04:45:20 -06:00
mpg (Migrated from github.com) left a comment

Ok, glad we discussed it, because I think there was a misunderstanding about the strategy. To be fair, we had a big side discussion about where to place the new test helper, which was a bit of distraction. Anyway, let me try to sum up the strategy:

  1. Merge test_suite_pkparse.function into test_suite_pk.function, but for the data file we only need to rename test_suite_pkparse.data to test_suite_pk.parse.data (the new name makes it associated to the test_suite_pk.function file).
  2. In test_suite_pk.function, create a new test helper validate_pk_context() which you started here. The checks it should have are listed in the first part of this comment and I'm suggesting a general structure for this helper there.
  3. Then call this new test helper in all the existing test functions that are testing context-populating functions. The places are listed in the 2nd part of this comment and I also left a partial example patch there

Unless I missed something, the content of the .data files should not need to be modified in any way (just one filename at step 1).

Ok, glad we discussed it, because I think there was a misunderstanding about the strategy. To be fair, we had a big side discussion about where to place the new test helper, which was a bit of distraction. Anyway, let me try to sum up the strategy: 1. Merge `test_suite_pkparse.function` into `test_suite_pk.function`, but for the data file we only need to rename `test_suite_pkparse.data` to `test_suite_pk.parse.data` (the new name makes it associated to the `test_suite_pk.function` file). 2. In `test_suite_pk.function`, create a new test helper `validate_pk_context()` which you started here. The checks it should have are listed in the first part of [this comment](https://github.com/Mbed-TLS/mbedtls/issues/10573#issuecomment-3822754905) and I'm suggesting a general structure for this helper [there](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/686#discussion_r2781858075). 3. Then call this new test helper in all the existing test functions that are testing context-populating functions. The places are listed in the 2nd part of [this comment](https://github.com/Mbed-TLS/mbedtls/issues/10573#issuecomment-3822754905) and I also left a partial example patch [there](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/686#issuecomment-3870781732) Unless I missed something, the content of the `.data` files should not need to be modified in any way (just one filename at step 1).
tests/suites/test_suite_pk.data
mpg (Migrated from github.com) commented 2026-02-09 04:23:58 -06:00

I don't think that should be changing.

I don't think that should be changing.
tests/suites/test_suite_pk.data
mpg (Migrated from github.com) commented 2026-02-09 04:27:45 -06:00

I think these should remain in a separate file: test_suite_pk.parse.data. Merging the .function files is useful for having a common helper function that can be used by everyone, but I don't see any reason to merge the .data files too - multiple .data files can be associated with the same .function file.

I think these should remain in a separate file: `test_suite_pk.parse.data`. Merging the `.function` files is useful for having a common helper function that can be used by everyone, but I don't see any reason to merge the `.data` files too - multiple `.data` files can be associated with the same `.function` file.
tests/suites/test_suite_pk.function
@ -578,7 +578,223 @@ exit:
psa_reset_key_attributes(&new_attr);
mpg (Migrated from github.com) commented 2026-02-09 04:33:43 -06:00

I know this is temporary development code, but just in case: in the final code we want to be using TEST_ASSERT().

The proper pattern for structuring test helpers might not be obvious, recently Valerio had to remind me, after being reminder by Gilles, so I'm continuing the chain:

static int validate_pk_context(...)
{
    int ret = -1; 

    TEST_ASSERT(foo);
    if (bar) {
       TEST_ASSERT(baz);
    }
    // and so on, and once all the test are done:
    ret = 0;
exit:
    mbedtls_xxx_free(stuff); // if necessary
    return ret;
}
I know this is temporary development code, but just in case: in the final code we want to be using `TEST_ASSERT()`. The proper pattern for structuring test helpers might not be obvious, recently Valerio had to remind me, after being reminder by Gilles, so I'm continuing the chain: ``` static int validate_pk_context(...) { int ret = -1; TEST_ASSERT(foo); if (bar) { TEST_ASSERT(baz); } // and so on, and once all the test are done: ret = 0; exit: mbedtls_xxx_free(stuff); // if necessary return ret; } ```
tests/suites/test_suite_pk.function
mpg (Migrated from github.com) commented 2026-02-09 04:34:16 -06:00

I see no reason for changing this.

I see no reason for changing this.
mpg commented 2026-02-09 04:54:51 -06:00 (Migrated from github.com)

The original function didn't make sense anymore, as bitlen is now tested in validate_pk_context.

I think we have a misunderstanding again: validate_pk_context is not supposed to test mbedtls_pk_getbitlen(), it is supposed to test the bitlen field in mbedtls_pk_context.

Granted, right now mbedtls_pk_getbitlen() is just a simpler getter for that field, but that might not always be true. In terms of architecture:

  • each public API function gets its own functional tests;
  • the new validate_pk_context is about internal invariants of the mbedtls_pk_context structure.

I agree that when a pubic API function is a simple getter for an existing field in the structure, that results in some redundancy, but I don't mind. As discussed a number of times, we don't have the bandwidth to fully reshape PK testing.

The scope of this task is only to add some validation to some existing tests (and fix any bugs that this extra validation might uncover).

> The original function didn't make sense anymore, as bitlen is now tested in `validate_pk_context`. I think we have a misunderstanding again: `validate_pk_context` is not supposed to test `mbedtls_pk_getbitlen()`, it is supposed to test the `bitlen` field in `mbedtls_pk_context`. Granted, right now `mbedtls_pk_getbitlen()` is just a simpler getter for that field, but that might not always be true. In terms of architecture: - each public API function gets its own functional tests; - the new `validate_pk_context` is about internal invariants of the `mbedtls_pk_context` structure. I agree that when a pubic API function is a simple getter for an existing field in the structure, that results in some redundancy, but I don't mind. As discussed a number of times, we don't have the bandwidth to fully reshape PK testing. The scope of this task is only to add some validation to some existing tests (and fix any bugs that this extra validation might uncover).
bjwtaylor (Migrated from github.com) reviewed 2026-02-09 05:01:04 -06:00
tests/suites/test_suite_pk.data
bjwtaylor (Migrated from github.com) commented 2026-02-09 05:01:04 -06:00

Understood, I'll restore the original function.

Understood, I'll restore the original function.
bjwtaylor commented 2026-02-09 05:04:07 -06:00 (Migrated from github.com)

Understood @mpg. Thanks for taking a look at it, as I expect it saved me jumping into a rabbit hole. I'll update the PR as suggested.

Understood @mpg. Thanks for taking a look at it, as I expect it saved me jumping into a rabbit hole. I'll update the PR as suggested.
👍 1
mpg commented 2026-04-01 03:36:12 -05:00 (Migrated from github.com)

Just doing a design review. The shape of this PR is still pretty different from what I had in mind. As I commented previously: "the content of the .data files should not need to be modified in any way".

The goal is to check self-consistency of the context structure once it has been populated, namely to check the invariants listed here. The goal is not to check correctness by comparing with a reference expected value. I think both the issue title ("invariants") and the comment about what invariants to check, were clear that we are comparing fields against each other (and against known-bad values like NULL or 0), but not against reference values.

In the sample patch I gave previously, the new test helper function validate_pk_context() takes a single argument: the PK context, and that is no coincidence.

I would like this PR to be less invasive than it is now:

  1. No changes to the content of the .data files (just one of them being renamed).
  2. Only changes to the .function files (other than merging) is we have a new static function validate_pk_context() that takes one PK context as its single argument, and is called from the relevant places (2nd part of this comment).
  3. And obviously fix the bugs in the library found by the above - from what I've seen, I'm happy with this part!
Just doing a design review. The shape of this PR is still pretty different from what I had in mind. As I [commented previously](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/686#pullrequestreview-3772439498): "the content of the .data files should not need to be modified in any way". The goal is to check self-consistency of the context structure once it has been populated, namely to check the invariants [listed here](https://github.com/Mbed-TLS/mbedtls/issues/10573#issuecomment-3822754905). The goal is not to check correctness by comparing with a reference expected value. I think both the issue title ("invariants") and the comment about what invariants to check, were clear that we are comparing fields against each other (and against known-bad values like NULL or 0), but not against reference values. In the sample patch [I gave previously](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/686#issuecomment-3870781732), the new test helper function `validate_pk_context()` takes a single argument: the PK context, and that is no coincidence. I would like this PR to be less invasive than it is now: 1. No changes to the content of the `.data` files (just one of them being renamed). 2. Only changes to the `.function` files (other than merging) is we have a new static function `validate_pk_context()` that takes one PK context as its single argument, and is called from the relevant places (2nd part of [this comment](https://github.com/Mbed-TLS/mbedtls/issues/10573#issuecomment-3822754905)). 3. And obviously fix the bugs in the library found by the above - from what I've seen, I'm happy with this part!
bjwtaylor commented 2026-04-13 02:11:50 -05:00 (Migrated from github.com)

@mpg, apologies I missed your comment. Let me look into adjusting this subject to the above changes and I'll re-request the review.

@mpg, apologies I missed your comment. Let me look into adjusting this subject to the above changes and I'll re-request the review.
bjwtaylor commented 2026-04-16 05:22:41 -05:00 (Migrated from github.com)

@mpg, I've updated it to remove the changes to the data files and remove the value checking. I've been looking at the input arguments and I was wondering do we definitely want to have just a single context input. Currently we have two optional contexts a pubic and private one. By keeping this we can do a few additional checks on the public one if supplied and we can do the check_pair. Which although I guess is not strictly needed is probably useful? What do you think do you definitely want to get rid of the public context handling and just check the private ones?

@mpg, I've updated it to remove the changes to the data files and remove the value checking. I've been looking at the input arguments and I was wondering do we definitely want to have just a single context input. Currently we have two optional contexts a pubic and private one. By keeping this we can do a few additional checks on the public one if supplied and we can do the check_pair. Which although I guess is not strictly needed is probably useful? What do you think do you definitely want to get rid of the public context handling and just check the private ones?
This pull request has changes conflicting with the target branch.
  • tests/suites/test_suite_pkparse.function
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'bjwtaylor/test-context-population:bjwtaylor/test-context-population'
git switch 'bjwtaylor/test-context-population'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'
git merge --no-ff 'bjwtaylor/test-context-population'
git switch 'bjwtaylor/test-context-population'
git rebase 'development'
git switch 'development'
git merge --ff-only 'bjwtaylor/test-context-population'
git switch 'bjwtaylor/test-context-population'
git rebase 'development'
git switch 'development'
git merge --no-ff 'bjwtaylor/test-context-population'
git switch 'development'
git merge --squash 'bjwtaylor/test-context-population'
git switch 'development'
git merge --ff-only 'bjwtaylor/test-context-population'
git switch 'development'
git merge 'bjwtaylor/test-context-population'
git push origin 'development'
Sign in to join this conversation.
Reviewers
No reviewers
mpg
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
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

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/TF-PSA-Crypto!686
No description provided.