sys/psa_crypto: Add generic HMAC implementation #20758

Wer-Wolf · 2024-06-22T21:36:40Z

Contribution description

This PR adds a generic HMAC implementation for the PSA crypto API. This HMAC implementation can work with all
hashing algorithms already supported by the PSA crypto API.

This means that in order to add support for a new HMAC algorithm, only the hashing algorithm implementation is necessary,
the rest will be handled by the generic HMAC.

In order to support the full PSA MAC API, this PR also adds support for the multi-part MAC API. It also removes support for hardware acceleration of the SHA-256 HMAC, since a full-fledged hardware acceleration is expected to be provided in the near future.

Last but not least, a unittest for the generic HMAC is provided.

Testing procedure

The test-hashes unittest also tests the generic HMAC, so running those tests should be suitable to test the generic HMAC.

This PR depends on #20698, and also depends on a follow-up PR which will reintroduce proper hardware acceleration.

Prepare to support the multi-part MAC API by creating appropriate dispatchers for both algorithm and location backends. Since there are no supported backends at the moment, the dispatcher always returns PSA_ERROR_NOT_SUPPORTED for now. Signed-off-by: Armin Wolf <[email protected]>

The initial implementation was inspired by MbedTLS, with the addition of the MD2 and MD4 algorithms. Signed-off-by: Armin Wolf <[email protected]>

This support macro will be needed by the generic hmac implementation. Signed-off-by: Armin Wolf <[email protected]>

This additional macro will be used by the generic hmac implementation to calculate the size of the internal buffers. Signed-off-by: Armin Wolf <[email protected]>

Add a generic HMAC implementation based on the PSA hashing API. In order to support a specific HMAC algorithm, all what has to be implemented is a backend for the PSA hashing API. Signed-off-by: Armin Wolf <[email protected]>

The generic HMAC implementation can only be used by going through the dispatcher. Do the necessary wire-up so that applications using the PSA crypto API can use the generic HMAC implementation. Signed-off-by: Armin Wolf <[email protected]>

The old HMAC implementation only supported the SHA256 hashing algorithm and only implemented the single-part MAC function. Replace it with the generic HMAC implementation which supports all hashing algorithms and is already used for the multi-part MAC functions. A side effect of this commit is that the cryptocell HMAC implementation is not used anymore. This will be fixed in a later commit which introduces broad hardare-acceleration for HMAC. Signed-off-by: Armin Wolf <[email protected]>

Implement the PSA MAC verification API. Currently only the generic HMAC backend is available for MAC verification, but hardware-accelerated backends can be added later. Signed-off-by: Armin Wolf <[email protected]>

Add some documentation regarding the steps for adding support for new HMAC algorithms to the generic HMAC implementation. Signed-off-by: Armin Wolf <[email protected]>

Add tests for the generic HMAC implementation. Authored-by: Daria Zatokovenko <[email protected]> Signed-off-by: Armin Wolf <[email protected]>

riot-ci · 2024-06-24T09:23:24Z

Murdock results

❌ FAILED

966e3ac fixup! /tests/sys/psa_crypto_mac: made tests compliant to coding conventions

Build failures (1)

Application	Target	Toolchain	Runtime (s)	Worker
tests/sys/psa_crypto_mac	msb-430	gnu	0.83	alien

Artifacts

mguetschow

Thorough implementation, good coding style overall, well done! I have some comments inline below :)

mguetschow · 2024-06-24T09:14:25Z

sys/psa_crypto/Makefile.dep

+ifneq (,$(filter psa_mac_hmac_sha_512_backend_riot,$(USEMODULE)))
+  USEMODULE += psa_hash
+  USEMODULE += psa_hash_sha_512
+  USEMODULE += psa_riot_mac_hmac_generic
 endif


This is (obviously) still missing the logic for SHA-3 support. Will need to be added after #20698 is merged. Just leaving the note here to not forget about it.

Yes, i should have mentioned that.

sys/psa_crypto/doc.txt

sys/psa_crypto/include/psa_hmac.h

mguetschow · 2024-06-24T10:38:11Z

tests/unittests/tests-hashes/tests-hashes-sha384-hmac.c

+ * Copyright (C) 2016 Martin Landsmann <[email protected]>
+ * Copyright 2016 OTA keys S.A.


Please update the copyright accordingly.

Sorry for the wait, done.

tests/unittests/tests-hashes/tests-hashes-sha384-hmac.c

mguetschow · 2024-06-24T10:42:44Z

tests/unittests/tests-hashes/tests-hashes-sha384-hmac.c

+    for (size_t i = 0; i < str_length; i += 2)
+    {


Suggested change

for (size_t i = 0; i < str_length; i += 2)

{

for (size_t i = 0; i < str_length; i += 2) {

and in other places too (except for function definitions): https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#indentation-and-braces

mguetschow · 2024-06-24T10:45:01Z

tests/unittests/tests-hashes/tests-hashes.h

+/**
+ * @brief   Generates tests for hashes/sha512.h - generic hmac
+ *
+ * @return  embUnit tests if successful, NULL if not.
+ */
+Test *tests_hashes_sha512_hmac_tests(void);


We have all the other PSA crypto API tests below tests/sys/psa_crypto*. To not bloat the unittests binary too much, I would propose you move the tests to the already existing psa_crypto_mac test.

Good point, I moved the tests to the existing psa_crypto_mac test from unittests.

Einhornhool

This looks nice and it's a good idea to have a generic implementation.
I added a few comments :)

sys/psa_crypto/psa_hmac.c

sys/psa_crypto/doc.txt

sys/psa_crypto/psa_hmac.c

Signed-off-by: Armin Wolf <[email protected]>

…eric HMAC Signed-off-by: Armin Wolf <[email protected]>

Signed-off-by: Armin Wolf <[email protected]>

Moving the check of MODULE_PSA_RIOT_MAC_HMAC_GENERIC improves the clarity and makes it easier to add hardware acceleration support later. Signed-off-by: Armin Wolf <[email protected]>

Rename the generic HMAC function to signal that they are not part of the user-facing API. Signed-off-by: Armin Wolf <[email protected]>

Add comments to explain which steps the generic HMAC implementation takes to comply with RFC 2104. Signed-off-by: Armin Wolf <[email protected]>

When psa_generic_hmac_compute() is called, the caller wants to use the generic software-based HMAC implementation, so there is no need to call the dispatcher again. Signed-off-by: Armin Wolf <[email protected]>

mguetschow · 2024-07-09T09:22:58Z

Thanks @Wer-Wolf for addressing all the comments, looks good to me from the implementation side. There are still some open change requests on @daria-gauster's test code, would someone of you mind addressing those too?

Wer-Wolf · 2024-07-09T09:49:29Z

I will take care of this too.

Co-authored-by: mguetschow <[email protected]>

… psa_crypto_mac test

…entions

mguetschow

Some more comments below :)

mguetschow · 2024-07-15T09:24:33Z

tests/unittests/tests-hashes/Makefile.include

The changes to all files in tests/unittests should not be needed anymore.

mguetschow · 2024-07-15T09:25:31Z

tests/unittests/tests-hashes/tests-hashes.c

@@ -33,5 +33,4 @@ void tests_hashes(void)
    TESTS_RUN(tests_hashes_sha512_tests());
    TESTS_RUN(tests_hashes_sha512_224_tests());
    TESTS_RUN(tests_hashes_sha512_256_tests());
-    TESTS_RUN(tests_hashes_sha3_tests());


Pretty sure you didn't mean to delete this?

mguetschow · 2024-07-15T09:25:37Z

tests/unittests/tests-hashes/tests-hashes.h

-  /**
- * @brief   Generates tests for hashes/sha3.h
- *
- * @return  embUnit tests if successful, NULL if not.
- */
-Test *tests_hashes_sha3_tests(void);


mguetschow · 2024-07-15T09:26:24Z

tests/sys/psa_crypto_mac/Makefile

 USEMODULE += psa_mac
+USEMODULE += hashes


Suggested change

USEMODULE += hashes

should be automatically selected

mguetschow · 2024-07-15T09:27:36Z

tests/sys/psa_crypto_mac/Makefile

@@ -1,12 +1,21 @@
 include ../Makefile.sys_common

+USEMODULE += embunit


Suggested change

USEMODULE += embunit

I would prefer to keep your tests consistent with the existing ones and not use embUnit for them (simply replace TEST_ASSERT() with the construction used in other tests).

mguetschow · 2024-07-15T09:28:06Z

tests/sys/psa_crypto_mac/Makefile

+USEMODULE += hashes
+USEMODULE += psa_hash
+
+USEMODULE += psa_riot_mac_hmac_generic


Suggested change

USEMODULE += psa_riot_mac_hmac_generic

should be selected automatically, I guess

mguetschow · 2024-07-15T09:28:23Z

tests/sys/psa_crypto_mac/Makefile

+USEMODULE += psa_hash_sha_384
+USEMODULE += psa_mac_hmac_sha_384
+USEMODULE += psa_hash_sha_512
+USEMODULE += psa_mac_hmac_sha_512


Suggested change

USEMODULE += psa_hash_sha_384

USEMODULE += psa_mac_hmac_sha_384

USEMODULE += psa_hash_sha_512

USEMODULE += psa_mac_hmac_sha_512

USEMODULE += psa_mac_hmac_sha_384

USEMODULE += psa_mac_hmac_sha_512

same here

mguetschow · 2024-07-15T09:30:20Z

Also the current test implementation fails on the CI: https://ci.riot-os.org/details/f921c2a1211244a0b9ee984ef457e915

I'd propose to mimic the other tests, then it should work.

Wer-Wolf and others added 10 commits June 14, 2024 12:35

sys/psa_crypto: Implement PSA_HASH_BLOCK_LENGTH()

c0fc64e

The initial implementation was inspired by MbedTLS, with the addition of the MD2 and MD4 algorithms. Signed-off-by: Armin Wolf <[email protected]>

sys/psa_crypto: Implement PSA_MAC_MAX_SIZE()

e8660ef

This support macro will be needed by the generic hmac implementation. Signed-off-by: Armin Wolf <[email protected]>

sys/psa_crypto: Add PSA_HMAC_BLOCK_MAX_SIZE()

a835ea9

This additional macro will be used by the generic hmac implementation to calculate the size of the internal buffers. Signed-off-by: Armin Wolf <[email protected]>

sys/psa_crypto: Add generic HMAC implementation

d5be6a7

Add a generic HMAC implementation based on the PSA hashing API. In order to support a specific HMAC algorithm, all what has to be implemented is a backend for the PSA hashing API. Signed-off-by: Armin Wolf <[email protected]>

sys/psa_crypto: Implement psa_mac_verify()

e7e942b

Implement the PSA MAC verification API. Currently only the generic HMAC backend is available for MAC verification, but hardware-accelerated backends can be added later. Signed-off-by: Armin Wolf <[email protected]>

sys/psa_crypto: Document the generic HMAC implementation

d3e1827

Add some documentation regarding the steps for adding support for new HMAC algorithms to the generic HMAC implementation. Signed-off-by: Armin Wolf <[email protected]>

tests/unittests/tests-hashes: Add tests for generic HMAC implementations

cc1a1ad

Add tests for the generic HMAC implementation. Authored-by: Daria Zatokovenko <[email protected]> Signed-off-by: Armin Wolf <[email protected]>

Wer-Wolf requested a review from miri64 as a code owner June 22, 2024 21:36

github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: sys Area: System labels Jun 22, 2024

waehlisch requested a review from Einhornhool June 22, 2024 23:54

mguetschow self-requested a review June 24, 2024 07:39

mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 24, 2024

mguetschow requested changes Jun 24, 2024

View reviewed changes

Einhornhool reviewed Jun 25, 2024

View reviewed changes

sys/psa_crypto/psa_hmac.c Outdated Show resolved Hide resolved

sys/psa_crypto/doc.txt Outdated Show resolved Hide resolved

sys/psa_crypto/doc.txt Outdated Show resolved Hide resolved

sys/psa_crypto/psa_hmac.c Outdated Show resolved Hide resolved

Wer-Wolf added 9 commits June 26, 2024 10:44

fixup! sys/psa_crypto: Add generic HMAC implementation

d416675

Signed-off-by: Armin Wolf <[email protected]>

fixup! sys/psa_crypto: Replace old SHA256 HMAC implemenation with gen…

dfb1808

…eric HMAC Signed-off-by: Armin Wolf <[email protected]>

fixup! sys/psa_crypto: Add generic HMAC implementation

36d2629

Signed-off-by: Armin Wolf <[email protected]>

fixup! sys/psa_crypto: Add generic HMAC implementation

6a64462

sys/psa_crypto: Make switch-case more clear

d8b8304

Moving the check of MODULE_PSA_RIOT_MAC_HMAC_GENERIC improves the clarity and makes it easier to add hardware acceleration support later. Signed-off-by: Armin Wolf <[email protected]>

sys/psa_crypto: Rename generic HMAC functions

3a52e1a

Rename the generic HMAC function to signal that they are not part of the user-facing API. Signed-off-by: Armin Wolf <[email protected]>

sys/psa_crypto: Add comment to explain the generic HMAC

f6ad8de

Add comments to explain which steps the generic HMAC implementation takes to comply with RFC 2104. Signed-off-by: Armin Wolf <[email protected]>

fixup! sys/psa_crypto: Rename generic HMAC functions

2fb2070

sys/psa_crypto: Use generic HMAC functions for compute and verification

fe7ee4f

When psa_generic_hmac_compute() is called, the caller wants to use the generic software-based HMAC implementation, so there is no need to call the dispatcher again. Signed-off-by: Armin Wolf <[email protected]>

daria-gauster and others added 2 commits July 10, 2024 19:42

Update tests/unittests/tests-hashes/tests-hashes-sha384-hmac.c

4545e61

Co-authored-by: mguetschow <[email protected]>

Update tests/unittests/tests-hashes/tests-hashes-sha384-hmac.c

7105630

Co-authored-by: mguetschow <[email protected]>

daria-gauster requested review from leandrolanzieri, aabadie and MichelRottleuthner as code owners July 12, 2024 20:11

fixup! tests/sys/psa_crypto_mac: Moved generic HMAC tests to existing…

4e1838c

… psa_crypto_mac test

daria-gauster force-pushed the generic_hmac_sha512_test branch from 408da13 to 4e1838c Compare July 12, 2024 20:20

fixup! /tests/sys/psa_crypto_mac: made tests compliant to coding conv…

966e3ac

…entions

mguetschow requested changes Jul 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/psa_crypto: Add generic HMAC implementation #20758

sys/psa_crypto: Add generic HMAC implementation #20758

Wer-Wolf commented Jun 22, 2024 •

edited

Loading

riot-ci commented Jun 24, 2024 •

edited

Loading

mguetschow left a comment

mguetschow Jun 24, 2024

Wer-Wolf Jun 24, 2024

mguetschow Jun 24, 2024

daria-gauster Jul 12, 2024

mguetschow Jun 24, 2024

mguetschow Jun 24, 2024

daria-gauster Jul 12, 2024

Einhornhool left a comment

mguetschow commented Jul 9, 2024

Wer-Wolf commented Jul 9, 2024

mguetschow left a comment

mguetschow Jul 15, 2024

mguetschow Jul 15, 2024

mguetschow Jul 15, 2024

mguetschow Jul 15, 2024

mguetschow Jul 15, 2024

mguetschow Jul 15, 2024

mguetschow Jul 15, 2024

mguetschow commented Jul 15, 2024

		* Copyright (C) 2016 Martin Landsmann <[email protected]>
		* Copyright 2016 OTA keys S.A.

	for (size_t i = 0; i < str_length; i += 2)
	{
	for (size_t i = 0; i < str_length; i += 2) {

		@@ -1,12 +1,21 @@
		include ../Makefile.sys_common

		USEMODULE += embunit

sys/psa_crypto: Add generic HMAC implementation #20758

Are you sure you want to change the base?

sys/psa_crypto: Add generic HMAC implementation #20758

Conversation

Wer-Wolf commented Jun 22, 2024 • edited Loading

Contribution description

Testing procedure

riot-ci commented Jun 24, 2024 • edited Loading

Murdock results

Artifacts

mguetschow 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

Einhornhool left a comment

Choose a reason for hiding this comment

mguetschow commented Jul 9, 2024

Wer-Wolf commented Jul 9, 2024

mguetschow 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

mguetschow commented Jul 15, 2024

Wer-Wolf commented Jun 22, 2024 •

edited

Loading

riot-ci commented Jun 24, 2024 •

edited

Loading