Remove discussions of MBEDTLS_USE_PSA_CRYPTO in API documentation #9781

yanesca · 2024-11-19T16:37:29Z

Description

Remove discussions of MBEDTLS_USE_PSA_CRYPTO in API documentation. Resolves partially #9632.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

changelog not required because: documentation only
development PR this
framework PR not required
3.6 PR not required because: 4.0 only
2.28 PR not required because: 4.0 only
tests not required because: documentation only

gilles-peskine-arm

This mostly looks good, and I'm satisfied with completeness for the subtask “API documentation”. There are a few problems in mbedtls_config.h. There are conflicts with #9771, so it would be better to wait until that is merged for rework and re-review.

configs/ext/tfm_mbedcrypto_config_profile_medium.h

gilles-peskine-arm · 2024-11-19T17:11:59Z

include/mbedtls/mbedtls_config.h

+ * \note  When this option is enabled, restartable operations in PK, X.509
+ *        and TLS (see above) are not using PSA. On the other hand, ECDH
+ *        computations in TLS are using PSA, and are not restartable. These
+ *        are temporary limitations that should be lifted in the future.


Minor: there is only one limitation now.

I am sorry I am not sure I can see which of these limitations has gone. Could you please elaborate?

Oh, based on your next comment the remaining limitation is the restartable ECDH. As far as I can tell the restartable operations in PK, X509 and TLS are still not using PSA.

Raised #9817 for tracking.

Sorry, I must have forgotten to actually write a sentence here, or I made a mistake when consolidating/separating comments. Grammatically, the current paragraph contains two limitations (one per sentence). The first limitation “… not using PSA” is not really meaningful anymore, since users must assume that everything is “using PSA” — users must call psa_crypto_init and must enable PSA_WANT_xxx. So I thought we should remove that sentence: it's now an implementation detail, no longer a statement about the interface. The sentence isn't misleading though, and it's informative in a non-semantic way, so it can stay.

include/mbedtls/mbedtls_config.h

gilles-peskine-arm · 2024-11-19T18:55:25Z

include/mbedtls/mbedtls_config.h

@@ -853,8 +850,8 @@
 *
 * Enable the ECDH-ECDSA based ciphersuite modes in SSL / TLS.
 *
- * Requires: MBEDTLS_ECDH_C or (MBEDTLS_USE_PSA_CRYPTO and PSA_WANT_ALG_ECDH)
- *           MBEDTLS_ECDSA_C or (MBEDTLS_USE_PSA_CRYPTO and PSA_WANT_ALG_ECDSA)
+ * Requires: MBEDTLS_ECDH_C or PSA_WANT_ALG_ECDH


Partly preexisting (in many places in this file): this isn't quite right, and it wasn't right in 3.6 either. When MBEDTLS_USE_PSA_CRYPTO is enabled and MBEDTLS_PSA_CRYPTO_CONFIG is enabled (i.e. the only thing that will remain in 4.0), defining MBEDTLS_ECDH_C does not automatically enable ECDH in PSA, so here the requirement is on PSA_WANT_ALG_ECDH only.

In 3.6, this would be correct, although not necessarily the most helpful way to put it:

(!MBEDTLS_USE_PSA_CRYPTO and MBEDTLS_ECDH_C) or (MBEDTLS_USE_PSA_CRYPTO and PSA_WANT_ALG_ECDH)

It's not the most helpful way because if MBEDTLS_PSA_CRYPTO_CONFIG is disabled, which is the case by default, then enabling MBEDTLS_ECDH_C works regardless of MBEDTLS_USE_PSA_CRYPTO.

In some cases where there's a direct correspondence between legacy and PSA mechanisms, we have a rule that the PSA mechanism will always be enabled if the legacy mechanism is (unless PSA is completely disabled). This is done in config_adjust_psa_superset_legacy.h. But we only do this for hashes and curves, not for things like ECC/RSA algorithms.

Given that the removal of MBEDTLS_PSA_CRYPTO_CONFIG is happening in parallel (https://github.com/Mbed-TLS/mbedtls/pull/9771/files#r1848889790), I think it would be best to not try to get those statements exactly right on either side, because then we'd have a lot of conflicts. Rather, I propose to file a task to fix these requirement statements once both #9781 and #9771 are merged.

Bug report for 3.6: #9790

Follow-up task for 4.0: #9791

Rather, I propose to file a task to fix these requirement statements once both #9781 and #9771 are merged.

Good idea, and thank you for raising those issues.

MBEDTLS_USE_PSA_CRYPTO is now always enabled we need to update the documentation accordingly. Signed-off-by: Janos Follath <janos.follath@arm.com>

Recent commits have changed these reference configurations and they are not verbatim copies anymore. Signed-off-by: Janos Follath <janos.follath@arm.com>

Now that USA_PSA_CRYPTO is always on, users need to call psa_init() with all protocol versions. Signed-off-by: Janos Follath <janos.follath@arm.com>

Add reference to github issues to give a way for users to track progress and express interest. Signed-off-by: Janos Follath <janos.follath@arm.com>

gilles-peskine-arm

LGTM (given that we'll make a follow-up to handle #9791)

gilles-peskine-arm · 2024-12-02T16:46:55Z

tf-psa-crypto/drivers/builtin/include/mbedtls/pk.h

@@ -165,7 +162,8 @@ typedef struct mbedtls_pk_rsassa_pss_options {
 * which functions are used for various operations. The overall picture looks
 * like this:
 * - if USE_PSA is not defined and ECP_C is defined then use ecp_keypair data
- *   structure and legacy functions
+ *   structure and legacy functions. (MBEDTLS_USE_PSA_CRYPTO is always on and


Cosmetic:

Suggested change

* structure and legacy functions. (MBEDTLS_USE_PSA_CRYPTO is always on and

* structure and legacy functions. (\c MBEDTLS_USE_PSA_CRYPTO is always on and

gilles-peskine-arm · 2024-12-02T16:47:12Z

tf-psa-crypto/drivers/builtin/include/mbedtls/pk.h

- *                  #MBEDTLS_USE_PSA_CRYPTO is defined, the salt length is not
- *                  verified as PSA_ALG_RSA_PSS_ANY_SALT is used.
+ *                  otherwise it must be NULL. Note that the salt length is not
+ *                  verified as contexes have PSA_ALG_RSA_PSS_ANY_SALT as default


Cosmetic, preexisting:

Suggested change

* verified as contexes have PSA_ALG_RSA_PSS_ANY_SALT as default

* verified as contexts have #PSA_ALG_RSA_PSS_ANY_SALT as default

gilles-peskine-arm · 2024-12-02T16:48:42Z

include/mbedtls/mbedtls_config.h

+ * \note  When this option is enabled, restartable operations in PK, X.509
+ *        and TLS (see above) are not using PSA. On the other hand, ECDH


Minor: “not using PSA” is not really meaningful anymore, since users must assume that everything is “using PSA” — users must call psa_crypto_init and must enable PSA_WANT_xxx. So arguably we should remove that sentence: it's now an implementation detail, no longer a statement about the interface. The sentence isn't misleading though, and it's informative in a non-semantic way, so it can stay.

Using or not using PSA might be an implementation detail, but it has user visible consequences. (eg. using or not using drivers)

davidhorstmann-arm

Changes LGTM and there are no documentation mentions of USE_PSA in include or tf-psa-crypto/include, which are the public-facing parts.

gilles-peskine-arm self-requested a review November 19, 2024 16:55

yanesca mentioned this pull request Nov 19, 2024

Remove discussions of MBEDTLS_USE_PSA_CRYPTO in documentation #9632

Closed

gilles-peskine-arm mentioned this pull request Nov 19, 2024

Remove MBEDTLS_PSA_CRYPTO_CONFIG configuration option #9771

Merged

5 tasks

gilles-peskine-arm requested changes Nov 20, 2024

View reviewed changes

gilles-peskine-arm added needs-work needs-preceding-pr Requires another PR to be merged first and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Nov 20, 2024

yanesca added 4 commits December 2, 2024 12:52

Remove USE_PSA references from config documentation

277bba8

MBEDTLS_USE_PSA_CRYPTO is now always enabled we need to update the documentation accordingly. Signed-off-by: Janos Follath <janos.follath@arm.com>

Remove USE_PSA references from SSL documentation

130ece0

MBEDTLS_USE_PSA_CRYPTO is now always enabled we need to update the documentation accordingly. Signed-off-by: Janos Follath <janos.follath@arm.com>

Remove USE_PSA references from X509 documentation

582ecd0

MBEDTLS_USE_PSA_CRYPTO is now always enabled we need to update the documentation accordingly. Signed-off-by: Janos Follath <janos.follath@arm.com>

Remove USE_PSA references from crypto documentation

056cb14

MBEDTLS_USE_PSA_CRYPTO is now always enabled we need to update the documentation accordingly. Signed-off-by: Janos Follath <janos.follath@arm.com>

yanesca force-pushed the remove_USE_PSA_from_API_doc_9632 branch from 77d290e to 056cb14 Compare December 2, 2024 13:04

yanesca removed the needs-preceding-pr Requires another PR to be merged first label Dec 2, 2024

yanesca added 3 commits December 2, 2024 13:10

Adapt configs/ext/README.md

92dfb88

Recent commits have changed these reference configurations and they are not verbatim copies anymore. Signed-off-by: Janos Follath <janos.follath@arm.com>

Move the psa_init() warnings to _CLI and _SRV

745e561

Now that USA_PSA_CRYPTO is always on, users need to call psa_init() with all protocol versions. Signed-off-by: Janos Follath <janos.follath@arm.com>

Add issue reference for ECP_RESTARTABLE limitations

2761d18

Add reference to github issues to give a way for users to track progress and express interest. Signed-off-by: Janos Follath <janos.follath@arm.com>

yanesca 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-work labels Dec 2, 2024

yanesca requested a review from gilles-peskine-arm December 2, 2024 14:01

gilles-peskine-arm approved these changes Dec 2, 2024

View reviewed changes

davidhorstmann-arm self-requested a review December 3, 2024 14:27

davidhorstmann-arm removed the needs-reviewer This PR needs someone to pick it up for review label Dec 3, 2024

davidhorstmann-arm approved these changes Dec 3, 2024

View reviewed changes

davidhorstmann-arm added needs-ci Needs to pass CI tests 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 Dec 3, 2024

yanesca added this pull request to the merge queue Dec 3, 2024

davidhorstmann-arm removed the needs-ci Needs to pass CI tests label Dec 3, 2024

Merged via the queue into Mbed-TLS:development with commit b6860cf Dec 3, 2024
6 checks passed

ronald-cron-arm mentioned this pull request Dec 3, 2024

PSA Crypto Configuration Split #9567

Merged

5 tasks

yanesca mentioned this pull request Dec 4, 2024

Simplify documentation of crypto dependencies between configuration options in 4.0 #9791

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove discussions of MBEDTLS_USE_PSA_CRYPTO in API documentation #9781

Remove discussions of MBEDTLS_USE_PSA_CRYPTO in API documentation #9781

yanesca commented Nov 19, 2024 •

edited by davidhorstmann-arm

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm Nov 19, 2024

yanesca Dec 2, 2024

yanesca Dec 2, 2024

yanesca Dec 2, 2024

gilles-peskine-arm Dec 2, 2024

gilles-peskine-arm Nov 19, 2024

gilles-peskine-arm Nov 20, 2024

yanesca Dec 2, 2024

gilles-peskine-arm left a comment

gilles-peskine-arm Dec 2, 2024

gilles-peskine-arm Dec 2, 2024

gilles-peskine-arm Dec 2, 2024

yanesca Dec 3, 2024

davidhorstmann-arm left a comment

	* structure and legacy functions. (MBEDTLS_USE_PSA_CRYPTO is always on and
	* structure and legacy functions. (\c MBEDTLS_USE_PSA_CRYPTO is always on and

	* verified as contexes have PSA_ALG_RSA_PSS_ANY_SALT as default
	* verified as contexts have #PSA_ALG_RSA_PSS_ANY_SALT as default

		* \note When this option is enabled, restartable operations in PK, X.509
		* and TLS (see above) are not using PSA. On the other hand, ECDH

Remove discussions of MBEDTLS_USE_PSA_CRYPTO in API documentation #9781

Remove discussions of MBEDTLS_USE_PSA_CRYPTO in API documentation #9781

Conversation

yanesca commented Nov 19, 2024 • edited by davidhorstmann-arm Loading

Description

PR checklist

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

yanesca commented Nov 19, 2024 •

edited by davidhorstmann-arm

Loading