Document driver testing status #8566

mpg · 2023-11-24T11:53:09Z

Description

This PR's main goal is to document the strategy used to test the PSA Crypto driver interface, as well as the current implementation status of that strategy.

Along the way, a couple of minor things are modified, but the PR's goal is not to fix things, or even to provide a complete plan for action - only to document the current state of things (which could be the first step towards an action plan).

Main gaps identified:

Opaque is mostly not tested at all (only a few key management entry points are tested but not even all of them (edit: and now asymmetric encrypt/decrypt), and not simulating an opaque driver with internal storage).
Key derivation is not tested at all.
Fallback to other entry points for operations that support it (like using mulit-part entry points for one-short API when the driver doesn't provide one-shot) is not tested at all - see also Support PSA drivers without verify entry points #4570
~~Two gaps in driver-only testing (compared to what we're supposed to support).~~ Fixed in the meantime: 8616, 8734.
A number of gaps in test_suite_psa_crypto_driver_wrapper.data.

PR checklist

changelog not required - only documentation and testing
backport not required - only useful for new development
tests not required - only documentation and testing changes

mpg · 2023-11-24T11:56:48Z

@gilles-peskine-arm @ronald-cron-arm Since you're probably the most familiar with this, I'd appreciate if you could take a look - maybe not a complete review but at high-level view to see if this looks reasonable or if you notice any misunderstanding or obvious omissions here.

valeriosetti

I only found 1 typo, the rest is fine for me. However the CI is not fully happy about the change made to generate_ec_key

tests/include/test/drivers/crypto_config_test_driver_extension.h

docs/architecture/testing/driver-interface-test-strategy.md

valeriosetti

LGTM

valeriosetti · 2023-12-20T09:19:18Z

I think that at least 3 out of 4 errors found in test_psa_crypto_config_accel_cipher_aead belong to the fact that CTR-DRBG is using AES through PSA now:

[2023-12-18T11:12:54.941Z] generate_ec_key through transparent driver: fake .................. FAILED
[2023-12-18T11:12:54.941Z]   mbedtls_test_driver_key_management_hooks.hits == 1
[2023-12-18T11:12:54.941Z]   at line 791, suites/test_suite_psa_crypto_driver_wrappers.function
[2023-12-18T11:12:54.941Z]   lhs = 0x0000000000000005 = 5
[2023-12-18T11:12:54.941Z]   rhs = 0x0000000000000001 = 1
[2023-12-18T11:12:54.941Z] generate_ec_key through transparent driver: in-driver ............. FAILED
[2023-12-18T11:12:54.941Z]   mbedtls_test_driver_key_management_hooks.hits == 1
[2023-12-18T11:12:54.941Z]   at line 791, suites/test_suite_psa_crypto_driver_wrappers.function
[2023-12-18T11:12:54.941Z]   lhs = 0x0000000000000008 = 8
[2023-12-18T11:12:54.941Z]   rhs = 0x0000000000000001 = 1
[2023-12-18T11:12:54.941Z] generate_ec_key through transparent driver: fallback .............. FAILED
[2023-12-18T11:12:54.941Z]   mbedtls_test_driver_key_management_hooks.hits == 1
[2023-12-18T11:12:54.941Z]   at line 791, suites/test_suite_psa_crypto_driver_wrappers.function
[2023-12-18T11:12:54.941Z]   lhs = 0x0000000000000008 = 8
[2023-12-18T11:12:54.941Z]   rhs = 0x0000000000000001 = 1

I think we should fix this (as we did recently) by adding a specific hit counter instead of mbedtls_test_driver_key_management_hooks.hits. Wdyt?

mpg · 2023-12-20T09:32:15Z

I think we should fix this (as we did recently) by adding a specific hit counter instead of mbedtls_test_driver_key_management_hooks.hits. Wdyt?

Sounds good, thanks for the suggestion! I hadn't even looked at the CI results yet, thanks for analyzing the failures!

valeriosetti

I only found 1 typo which I didn't see before. The rest is fine for me :)

valeriosetti · 2023-12-20T09:35:28Z

tests/configs/user-config-for-test.h

@@ -37,34 +37,76 @@
 #endif

 /* Use the accelerator driver for all cryptographic mechanisms for which
- * the test driver implemented. */
+ * the test driver is implemented. This is copied from psa/crypto_config.h
+ * with the parts not implmented by the test driver commented out. */


Suggested change

* with the parts not implmented by the test driver commented out. */

* with the parts not implemented by the test driver commented out. */

mpg · 2023-12-29T09:02:36Z

Note to self: update names of cipher/aead components when #8641 has been merged.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Improved by Mbed-TLS#8616 - closing 8553. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Been merged in the meantime. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Mbed-TLS#8700 merged in the meantime. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2024-04-12T10:46:52Z

@gilles-peskine-arm Thanks for your review! I believe I have addressed your feedback.

I've also had to rebase to resolve a conflict. Turns out the commit "Fix failure in some configs by being more precise" was no longer needed as something similar had been done in development in the meantime.

gilles-peskine-arm

LGTM except for a typo, and maybe one bit of explanation to add.

As before, I haven't reviewed the details of which entry points are covered, and I intend to approve regardless. I'm confident that even if there are mistakes, they're minor, and no worse than what this document is likely to become over time when people improve the tests and forget to update the document.

gilles-peskine-arm · 2024-04-12T11:42:36Z

docs/architecture/testing/driver-interface-test-strategy.md

+implementing the operation. This will remain hard to test until we have proper
+support for JSON-defined drivers with auto-generation of dispatch code.
+(The `MBEDTLS_PSA_ACCEL_xxx` macros we currently use are not expressive enough
+to specify which entry points are support for a given mechanism.)


Suggested change

to specify which entry points are support for a given mechanism.)

to specify which entry points are supported for a given mechanism.)

gilles-peskine-arm · 2024-04-12T11:50:27Z

docs/architecture/testing/driver-interface-test-strategy.md

-operation: entry points always return `NOT_SUPPORTED`.
+The opaque driver implements the declared entry points, and can use any
+backend: internal or libtestdriver1. However it does not implement the
+instrumentation (hits, forced output/status), as this [was not an immediate


From a chat with Ronald: hits is actually probably an obsolete concern. With the internal backend, hits were how we knew that the driver was called, as opposed to directly calling the built-in code. With libtestdriver1, we check that by ensuring that the built-in code is not present, so if the operation gives the correct result, only a driver call can have calculated that result. So there is low value in checking the hit count. There is still some value for hit counts, e.g. checking that we don't call a multipart entry point when we intended to call the one-shot entry point, but it's limited.

And fix a typo while at it. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

gilles-peskine-arm

LGTM

valeriosetti

LGTM ;)

mpg 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 size-s Estimated task size: small (~2d) labels Nov 24, 2023

mpg self-assigned this Nov 24, 2023

mpg requested a review from valeriosetti November 24, 2023 11:53

valeriosetti reviewed Nov 27, 2023

View reviewed changes

tests/include/test/drivers/crypto_config_test_driver_extension.h Show resolved Hide resolved

docs/architecture/testing/driver-interface-test-strategy.md Outdated Show resolved Hide resolved

mpg force-pushed the driver-status branch from ab14624 to 6d57c1b Compare November 28, 2023 13:58

mpg requested a review from valeriosetti November 29, 2023 08:38

valeriosetti previously approved these changes Nov 30, 2023

View reviewed changes

gilles-peskine-arm self-requested a review November 30, 2023 09:26

mpg removed the needs-reviewer This PR needs someone to pick it up for review label Dec 1, 2023

mpg dismissed valeriosetti’s stale review via b5ef178 December 18, 2023 10:37

mpg force-pushed the driver-status branch from 6d57c1b to b5ef178 Compare December 18, 2023 10:37

mpg requested a review from valeriosetti December 18, 2023 10:38

valeriosetti reviewed Dec 20, 2023

View reviewed changes

mpg force-pushed the driver-status branch 3 times, most recently from be21b16 to 9cf821c Compare December 22, 2023 10:01

mpg added the needs-work label Jan 4, 2024

mpg force-pushed the driver-status branch from 9cf821c to 774d444 Compare January 10, 2024 11:54

mpg removed the needs-work label Jan 10, 2024

mpg requested a review from valeriosetti January 10, 2024 11:55

valeriosetti previously approved these changes Jan 11, 2024

View reviewed changes

mpg dismissed valeriosetti’s stale review via 8f34db1 March 18, 2024 09:13

mpg force-pushed the driver-status branch from 774d444 to 8f34db1 Compare March 18, 2024 09:13

mpg added 12 commits April 12, 2024 12:40

Add note about fallback to other entry points

b18bc80

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Fix copy-pasta in top-of-file comment

4c81c34

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Improve comment in a header file

70cd911

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Update status of RSA testing

f2089da

Improved by Mbed-TLS#8616 - closing 8553. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Fix a typo in a comment

45fe86d

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Update names of components renamed in the meantime

98f8da1

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Update for HMAC testing

6c45361

Been merged in the meantime. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Update of opaque asymmetric encrypt/decrypt

dde1abd

Mbed-TLS#8700 merged in the meantime. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Update libtestdriver1 vs internal

0ca2fd0

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Refine paragraphs about incomplete entry points

ae22f04

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Rephrase description of the KDF situation

a47a3c4

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Misc fixes & improvements to driver testing doc

432e3b4

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg force-pushed the driver-status branch from ebff30f to 432e3b4 Compare April 12, 2024 10:45

mpg requested review from gilles-peskine-arm and ronald-cron-arm and removed request for ronald-cron-arm April 12, 2024 10:46

mpg removed the needs-work label Apr 12, 2024

gilles-peskine-arm requested changes Apr 12, 2024

View reviewed changes

Add a note on hits usefulness

4575d23

And fix a typo while at it. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg requested a review from gilles-peskine-arm April 15, 2024 08:57

gilles-peskine-arm approved these changes Apr 15, 2024

View reviewed changes

valeriosetti approved these changes Apr 16, 2024

View reviewed changes

mpg 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 Apr 17, 2024

mpg added this pull request to the merge queue Apr 17, 2024

Merged via the queue into Mbed-TLS:development with commit 68deadd Apr 17, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document driver testing status #8566

Document driver testing status #8566

mpg commented Nov 24, 2023 •

edited

Loading

mpg commented Nov 24, 2023

valeriosetti left a comment

valeriosetti left a comment

valeriosetti commented Dec 20, 2023

mpg commented Dec 20, 2023

valeriosetti left a comment

valeriosetti Dec 20, 2023

mpg commented Dec 29, 2023

mpg commented Apr 12, 2024

gilles-peskine-arm left a comment •

edited

Loading

gilles-peskine-arm Apr 12, 2024

gilles-peskine-arm Apr 12, 2024

gilles-peskine-arm left a comment

valeriosetti left a comment

	* with the parts not implmented by the test driver commented out. */
	* with the parts not implemented by the test driver commented out. */

	to specify which entry points are support for a given mechanism.)
	to specify which entry points are supported for a given mechanism.)

Document driver testing status #8566

Document driver testing status #8566

Conversation

mpg commented Nov 24, 2023 • edited Loading

Description

PR checklist

mpg commented Nov 24, 2023

valeriosetti left a comment

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

valeriosetti commented Dec 20, 2023

mpg commented Dec 20, 2023

valeriosetti left a comment

Choose a reason for hiding this comment

valeriosetti Dec 20, 2023

Choose a reason for hiding this comment

mpg commented Dec 29, 2023

mpg commented Apr 12, 2024

gilles-peskine-arm left a comment • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm Apr 12, 2024

Choose a reason for hiding this comment

gilles-peskine-arm Apr 12, 2024

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

mpg commented Nov 24, 2023 •

edited

Loading

gilles-peskine-arm left a comment •

edited

Loading