subsys: jwt: add PSA crypto alternative to legacy libraries (TinyCrypt for ECDSA and Mbed TLS for RSA) #78243

valeriosetti · 2024-09-10T14:52:13Z

Following the discussion of the Security WG meeting of September, 9th, TinyCrypt should be deprecated soon and jwt is the only place in Zephyr's codebase where there is no PSA/Mbed TLS alternative to TinyCrypt to perform ECDSA.
The main goal of this PR is to solve this limitation, but it also add some extra improvements:

a little reshape of subsys/jwt code, isolating signature functions;
improving the test coverage;
fix test's conf file.

d3zd3z

Looks good. Probably worth going through the clang-format suggestions just to keep the warnings down. Most seem to have to do with line breaks in function argument lists.

d3zd3z · 2024-09-10T18:35:13Z

subsys/jwt/Kconfig

+
+if JWT_SIGN_ECDSA
+
+config JWT_SIGN_ECDSA_PSA


Just curious, does the PSA configuration basically assume/assert that there is a proper CSPRNG enabled? Otherwise, should we check that. Signing multiple messages with the same nonce in EDCSA is a devastating failure.

I would say that the answer is yes, but it might depend on your configuration/platform capabilities.
I've recently done some change on how the Mbed TLS's PSA can handle random generation. You have basically 2 choices:

legacy, where psa_generate_random() calls to the legacy Mbed TLS modules (i.e. entropy + ctr/hmac drbg) to get random values; in this case the entropy module calls back to Zephyr to poll some random values. It first tries with CS sources, then it falls back to non-CS ones.

external, where psa_generate_random() calls to CS-only sources. In this case there is the possibility to use non-CS sources (via CONFIG_MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG_ALLOW_NON_CSPRNG), but there are warning during the build that clearly state that this is meant to be used only as test.

valeriosetti · 2024-09-11T05:56:51Z

Probably worth going through the clang-format suggestions just to keep the warnings down

I should have addressed most of them. There should be only complains about adding a space after casts, ex: (void) variable instead of (void)variable. Current clang-format wants the latter , but I prefer the former version. Wdyt?

In case my preference makes sense, can I edit the current .clang-format file adding SpaceAfterCStyleCast: True?

tomi-font

Looks good. Just some minor comments.

subsys/jwt/Kconfig

subsys/jwt/jwt.c

subsys/jwt/jwt_ecdsa.c

subsys/jwt/Kconfig

subsys/jwt/jwt_ecdsa.c

tests/subsys/jwt/testcase.yaml

tomi-font · 2024-09-13T07:18:33Z

subsys/jwt/Kconfig

+	prompt "Select crypto library to be used"
+
+config JWT_USE_PSA
+	bool "PSA crypto API library"


Don't you want to add some depends on here?

Right. I implemented what you proposed here

As this is changing the default behavior (now defaulting to PSA, enabling what's needed) we may want a migration guide entry.

subsys/jwt/Kconfig

subsys/jwt/jwt.c

subsys/jwt/CMakeLists.txt

subsys/jwt/jwt.c

subsys/jwt/jwt_psa.c

subsys/jwt/jwt_ecdsa.c

subsys/jwt/Kconfig

tomi-font · 2024-09-13T10:16:42Z

subsys/jwt/Kconfig

+	prompt "Select crypto library to be used"
+
+config JWT_USE_PSA
+	bool "PSA crypto API library"


As this is changing the default behavior (now defaulting to PSA, enabling what's needed) we may want a migration guide entry.

doc/releases/migration-guide-4.0.rst

valeriosetti · 2024-09-23T15:12:10Z

In the last forced-push I just updated the documentation, so I assume the CI will still be fine and the PR is now ready for reviews again :)

doc/releases/migration-guide-4.0.rst

valeriosetti · 2024-09-27T08:32:27Z

@d3zd3z @ceolin I think this is ready for reviews if/when you have some spare review bandwidth :)

dkalowsk

Minor comment, with the rest looking overall good to me.

dkalowsk · 2024-09-30T16:37:00Z

subsys/jwt/jwt.h

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+int jwt_sign_impl(struct jwt_builder *builder, const unsigned char *der_key,


you may want some header guards here. Additionally for this to be stand-alone the include where jwt_builder is defined should be added to the file... or forward declared as it's a pointer to force the compiler to warn about transitive includes.

You're absolutely right. Thanks for both suggestions!

dkalowsk · 2024-09-30T16:43:45Z

doc/releases/migration-guide-4.0.rst

+JWT (JSON Web Token)
+====================
+
+* By default signature is now computed through PSA Crypto API for both RSA and ECDSA.


I think you're missing a word here... By default, <something> signature is now...

Rather than a word I would say an article, "the". JWT assumes that the JSON content is signed and "signature" is the subject of this sentence, so I would say that "by default, the signature" should be fine.
However I'm not a native english speaker, so I might miss something so please I apologize for any mistake.

To be even more precise you could add JWT: the JWT signature is now

d3zd3z · 2024-10-04T18:50:05Z

Probably worth going through the clang-format suggestions just to keep the warnings down

I should have addressed most of them. There should be only complains about adding a space after casts, ex: (void) variable instead of (void)variable. Current clang-format wants the latter , but I prefer the former version. Wdyt?

In case my preference makes sense, can I edit the current .clang-format file adding SpaceAfterCStyleCast: True?

Not using a space between a cast and the value is almost universal across programming conventions. I'd suggest getting used to it.

d3zd3z

I think you should fix the spacing on casts, and use the ARG_UNUSED() macro. The others from clang-format are not good, and maybe we want to propose fixing that in another PR.

d3zd3z · 2024-10-04T18:51:05Z

subsys/jwt/jwt_legacy_ecdsa.c

+	sys_rand_get(entropy, sizeof(entropy));
+
+	int res = tc_ctr_prng_init(&prng_state, (const uint8_t *) &entropy, sizeof(entropy),
+				   personalize, sizeof(personalize));


I'd suggest fixing this, not using a space after a cast is near universal in C style conventions.

d3zd3z · 2024-10-04T18:52:02Z

subsys/jwt/jwt_legacy_ecdsa.c

+	struct tc_sha256_state_struct ctx;
+	uint8_t hash[32];
+	int res;
+	(void) sig_size;


Please use ARG_UNUSED(sig_size);

d3zd3z · 2024-10-04T18:52:58Z

subsys/jwt/jwt_psa.c

+	psa_set_key_type(&attr, PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1));
+	psa_set_key_algorithm(&attr, PSA_ALG_ECDSA(PSA_ALG_SHA_256));
+	alg = PSA_ALG_ECDSA(PSA_ALG_SHA_256);
+#else /* CONFIG_JWT_SIGN_RSA */


This is rather silly and picky of it, but might as well comply.

d3zd3z · 2024-10-04T18:53:50Z

subsys/jwt/jwt_psa.c

+		return -EINVAL;
+	}
+
+	status = psa_sign_message(key_id, alg,


I care less about this, since I think your formatting is slightly more readable, as it groups the things that go together.

d3zd3z · 2024-10-04T18:55:32Z

tests/subsys/jwt/src/jwt-test-private.c

+ *   $ openssl ecparam -name prime256v1 -genkey | openssl ec -outform der | xxd -ps
+ * - copy the content to https://lapo.it/asn1js/
+ * - get the first OCTECT STRING, which is the private key, and copy it here
+ */

 unsigned char jwt_test_private_der[] = {


In this case, the clang format output is actively terrible. If you have ideas on improving the config for this one, perhaps that should be proposed. But, not in this PR, just leave it.

This commit: - creates 2 new files, jwt_ecdsa.c and jwt_rsa.c, to hold the implementations of the corresponding ECDSA/RSA signature algorithms; - RSA signature is stil done through Mbed TLS's PK module which can optionally make use of PSA (if enabled); - ECDSA signature will instead use PSA, if possible, or TinyCrypt as fallback. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

- Some files (PEM/DER files, jwt-test-cert.c, user-tls-conf.h) were removed because they are not used or no longer necessary. Private keys used in the test are now in the jwt-test-private.c file as arrays. - testcase.yaml has been improved in order to test all possible use cases: ECDSA with TC, ECDSA with PSA, RSA; - unnecessary Kconfigs were removed from prj.conf. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

Update JWT subsystem documentation concerning the changes related to: - default library used - new Kconfigs added Signed-off-by: Valerio Setti <vsetti@baylibre.com>

valeriosetti requested review from d3zd3z, ceolin and tomi-font September 10, 2024 14:52

zephyrbot added the area: JSON label Sep 10, 2024

zephyrbot requested review from mrfuchs and Sir-Branch September 10, 2024 14:52

zephyrbot assigned d3zd3z Sep 10, 2024

valeriosetti force-pushed the jwt-replace-tc branch 3 times, most recently from 9ed82e0 to ddd8ee5 Compare September 10, 2024 15:33

d3zd3z reviewed Sep 10, 2024

View reviewed changes

valeriosetti force-pushed the jwt-replace-tc branch from ddd8ee5 to 13c38f9 Compare September 11, 2024 05:53

tomi-font reviewed Sep 11, 2024

View reviewed changes

valeriosetti force-pushed the jwt-replace-tc branch 6 times, most recently from 7e409ee to 51d5e09 Compare September 12, 2024 14:37

tomi-font requested changes Sep 13, 2024

View reviewed changes

valeriosetti force-pushed the jwt-replace-tc branch from 51d5e09 to 7ff86ab Compare September 13, 2024 10:00

valeriosetti requested a review from tomi-font September 13, 2024 10:00

tomi-font requested changes Sep 13, 2024

View reviewed changes

valeriosetti force-pushed the jwt-replace-tc branch from 7ff86ab to 37bbf5f Compare September 13, 2024 11:44

valeriosetti changed the title ~~subsys: jwt: add PSA crypto alternative to TinyCrypt to perform ECDSA~~ subsys: jwt: add PSA crypto alternative to legacy libraries (TinyCrypt for ECDSA and Mbed TLS for RSA) Sep 13, 2024

zephyrbot added the Release Notes To be mentioned in the release notes label Sep 13, 2024

zephyrbot requested review from dkalowsk, kartben and mmahadevan108 September 13, 2024 12:06

dkalowsk reviewed Sep 18, 2024

View reviewed changes

doc/releases/migration-guide-4.0.rst Outdated Show resolved Hide resolved

valeriosetti dismissed tomi-font’s stale review via 2b01a46 September 23, 2024 11:09

valeriosetti force-pushed the jwt-replace-tc branch from 2e146b4 to 2b01a46 Compare September 23, 2024 11:09

valeriosetti requested review from tomi-font and dkalowsk September 23, 2024 11:09

tomi-font requested changes Sep 24, 2024

View reviewed changes

doc/releases/migration-guide-4.0.rst Outdated Show resolved Hide resolved

valeriosetti force-pushed the jwt-replace-tc branch from 2b01a46 to 7a7a0c8 Compare September 24, 2024 07:26

valeriosetti requested a review from tomi-font September 24, 2024 07:26

tomi-font previously approved these changes Sep 24, 2024

View reviewed changes

dkalowsk reviewed Sep 30, 2024

View reviewed changes

valeriosetti dismissed tomi-font’s stale review via 528c004 October 2, 2024 07:57

valeriosetti force-pushed the jwt-replace-tc branch from 7a7a0c8 to 528c004 Compare October 2, 2024 07:57

valeriosetti requested review from dkalowsk and tomi-font October 2, 2024 08:01

tomi-font previously approved these changes Oct 3, 2024

View reviewed changes

d3zd3z approved these changes Oct 4, 2024

View reviewed changes

d3zd3z requested changes Oct 4, 2024

View reviewed changes

valeriosetti dismissed tomi-font’s stale review via 53c4ec4 October 7, 2024 07:38

valeriosetti force-pushed the jwt-replace-tc branch from 528c004 to 53c4ec4 Compare October 7, 2024 07:38

valeriosetti requested a review from d3zd3z October 7, 2024 07:38

valeriosetti added 3 commits October 7, 2024 09:46

doc: update JWT documentation

67a8d8e

Update JWT subsystem documentation concerning the changes related to: - default library used - new Kconfigs added Signed-off-by: Valerio Setti <vsetti@baylibre.com>

valeriosetti force-pushed the jwt-replace-tc branch from 53c4ec4 to 67a8d8e Compare October 7, 2024 07:47

tomi-font approved these changes Oct 7, 2024

View reviewed changes

d3zd3z approved these changes Oct 8, 2024

View reviewed changes

nashif merged commit 64ecbea into zephyrproject-rtos:main Oct 8, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subsys: jwt: add PSA crypto alternative to legacy libraries (TinyCrypt for ECDSA and Mbed TLS for RSA) #78243

subsys: jwt: add PSA crypto alternative to legacy libraries (TinyCrypt for ECDSA and Mbed TLS for RSA) #78243

valeriosetti commented Sep 10, 2024 •

edited

Loading

d3zd3z left a comment

d3zd3z Sep 10, 2024

valeriosetti Sep 11, 2024

valeriosetti commented Sep 11, 2024

tomi-font left a comment

tomi-font Sep 13, 2024

valeriosetti Sep 13, 2024

tomi-font Sep 13, 2024

tomi-font Sep 13, 2024

valeriosetti commented Sep 23, 2024

valeriosetti commented Sep 27, 2024

dkalowsk left a comment

dkalowsk Sep 30, 2024

valeriosetti Oct 2, 2024

dkalowsk Sep 30, 2024

valeriosetti Oct 2, 2024

tomi-font Oct 3, 2024

d3zd3z commented Oct 4, 2024

d3zd3z left a comment

d3zd3z Oct 4, 2024

d3zd3z Oct 4, 2024

d3zd3z Oct 4, 2024

d3zd3z Oct 4, 2024

d3zd3z Oct 4, 2024

subsys: jwt: add PSA crypto alternative to legacy libraries (TinyCrypt for ECDSA and Mbed TLS for RSA) #78243

subsys: jwt: add PSA crypto alternative to legacy libraries (TinyCrypt for ECDSA and Mbed TLS for RSA) #78243

Conversation

valeriosetti commented Sep 10, 2024 • edited Loading

d3zd3z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriosetti commented Sep 11, 2024

tomi-font 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

valeriosetti commented Sep 23, 2024

valeriosetti commented Sep 27, 2024

dkalowsk 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

d3zd3z commented Oct 4, 2024

d3zd3z 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

valeriosetti commented Sep 10, 2024 •

edited

Loading