Implement psa_hash_compute and psa_hash_compare #327

2019-11-28T13:13:21-06:00

gilles-peskine-arm commented

2019-11-28 13:13:21 -06:00

(Migrated from github.com)

A step on the way to IOTCRYPT-933

If #179 is merged first, this PR will need to be modified to add test case dependencies on !MBEDTLS_SHA512_NO_SHA384.

A step on the way to IOTCRYPT-933 If #179 is merged first, this PR will need to be modified to add test case dependencies on `!MBEDTLS_SHA512_NO_SHA384`.

gilles-peskine-arm commented

2019-12-02 10:48:30 -06:00

(Migrated from github.com)

CI passed except for expected failures (inherited from #325): an ABI change (two external symbols have been renamed) and generated files in TLS (due to the new header).

gilles-peskine-arm commented

2019-12-06 10:51:14 -06:00

(Migrated from github.com)

CI succeeded except for check_generated_files.sh in TLS which is expected to fail until the crypto submodule is updated to include crypto_compat.h in the VS project template.

CI succeeded except for `check_generated_files.sh` in TLS which is expected to fail until the crypto submodule is updated to include `crypto_compat.h` in the VS project template.

mpg (Migrated from github.com) reviewed

2020-01-28 06:03:59 -06:00

mpg (Migrated from github.com) left a comment

I'm happy with what I see, except a few notes and questions.

I'm a bit more concerned about what I don't see: do we have an all.sh component that builds without MD_C an runs the test suite that way? I'm not seeing one but perhaps I missed something. I'm not very comfortable approving the massing changes in test dependencies on MD_C without an automated test that confirms the new dependencies are correct.

I'm happy with what I see, except a few notes and questions. I'm a bit more concerned about what I don't see: do we have an `all.sh` component that builds without `MD_C` an runs the test suite that way? I'm not seeing one but perhaps I missed something. I'm not very comfortable approving the massing changes in test dependencies on `MD_C` without an automated test that confirms the new dependencies are correct.

tests/suites/test_suite_psa_crypto.data

					
				@ -764,3 +764,3 @@

				depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C

				depends_on:MBEDTLS_SHA256_C

				hash_setup:PSA_ALG_HMAC(PSA_ALG_SHA_256):PSA_ERROR_INVALID_ARGUMENT

Does this test really depend on SHA-256? Or are you using it as a convenient shortcut for "at least on hash defined"?

(Same for the next test.)

Does this test really depend on SHA-256? Or are you using it as a convenient shortcut for "at least on hash defined"? (Same for the next test.)

Note: this will interact with https://github.com/ARMmbed/mbed-crypto/pull/179 so whichever PR is merged first will be OK, but the other will need fixing. I don't think there's anything we can do in advance to avoid that, I'm just leaving that note as a reminder.

We seem to be missing a newline before the next test.

tests/suites/test_suite_psa_crypto.function

Question: shouldn't we test psa_hash_verify() with invalid algs as well? If not, why not?

Question: shouldn't we test `psa_hash_verify()` with invalid algs as well? If not, why not?

mpg commented

2020-01-28 06:04:27 -06:00

(Migrated from github.com)

Also, a conflict has developed and needs to be resolved.

gilles-peskine-arm commented

2020-01-28 13:26:06 -06:00

(Migrated from github.com)

I rebased to avoid the merge conflict. I kept the same history except for the missing blank line between two test cases. The previous version is in https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-hash_compute-3

I rebased to avoid the merge conflict. I kept the same history except for the [missing blank line between two test cases](https://github.com/ARMmbed/mbed-crypto/pull/327#discussion_r371757134). The previous version is in https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-hash_compute-3

gilles-peskine-arm (Migrated from github.com) reviewed

2020-01-28 13:26:51 -06:00

tests/suites/test_suite_psa_crypto.data

					
				@ -764,3 +764,3 @@

				depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C

				depends_on:MBEDTLS_SHA256_C

				hash_setup:PSA_ALG_HMAC(PSA_ALG_SHA_256):PSA_ERROR_INVALID_ARGUMENT

I somehow committed it in a later commit. Since I was rebasing due to the merge conflict in check-test-cases.py, I modified the appropriate commit to add the missing newline.

I somehow committed it in a later commit. Since I was rebasing due to the merge conflict in `check-test-cases.py`, I modified the appropriate commit to add the missing newline.

gilles-peskine-arm commented

2020-01-28 13:28:15 -06:00

(Migrated from github.com)

We don't run any tests without MBEDTLS_MD_C. There isn't much you can do without it! I made the changes because I prefer to be more correct, even if we can't be absolutely correct.

We don't run any tests without `MBEDTLS_MD_C`. There isn't much you can do without it! I made the changes because I prefer to be more correct, even if we can't be absolutely correct.

👍 1

gilles-peskine-arm (Migrated from github.com) reviewed

2020-01-28 13:29:42 -06:00

tests/suites/test_suite_psa_crypto.data

					
				@ -764,3 +764,3 @@

				depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C

				depends_on:MBEDTLS_SHA256_C

				hash_setup:PSA_ALG_HMAC(PSA_ALG_SHA_256):PSA_ERROR_INVALID_ARGUMENT

The dependency is wrong. Probably copypasta.

gilles-peskine-arm (Migrated from github.com) reviewed

2020-01-28 13:42:07 -06:00

tests/suites/test_suite_psa_crypto.function

ITYM psa_hash_compare. Indeed, I added some test cases.

ITYM `psa_hash_compare`. Indeed, I added some test cases.

mpg (Migrated from github.com) reviewed

2020-01-29 03:07:26 -06:00

tests/suites/test_suite_psa_crypto.function

I think I meant psa_hash_verify because I was mislead by the title of this PR, the first commit message, and the name of the next test function: hash_compute_verify.

I think I meant `psa_hash_verify` because I was mislead by the title of this PR, the first commit message, and the name of the next test function: `hash_compute_verify`.

mpg (Migrated from github.com) requested changes

2020-01-29 03:10:28 -06:00

mpg (Migrated from github.com) left a comment

Thanks for making the requested changes and fixing the conflict. I'm mostly happy now, except for a naming issue that I failed to notice the first time round.

tests/suites/test_suite_psa_crypto.function

					
				@ -2430,6 +2430,109 @@ exit:

				}

This should probably be hash_compute_compare. Also, the commit message could be amended.

This should probably be `hash_compute_compare`. Also, the commit message could be amended.

gilles-peskine-arm (Migrated from github.com) reviewed

2020-01-29 03:24:44 -06:00

tests/suites/test_suite_psa_crypto.function

					
				@ -2430,6 +2430,109 @@ exit:

				}

Ok. I went and amended the commit that introduced this test function and reworded the commit messages.

mpg (Migrated from github.com) reviewed

2020-01-29 03:42:49 -06:00

mpg (Migrated from github.com) left a comment

No description provided.

Thanks for fixing the naming issue. I'm happy with the result.

mpg commented

2020-01-30 03:34:43 -06:00

(Migrated from github.com)

@gilles-peskine-arm Just to notify you that #179 was just merged, so this will need a few !MBEDTLS_SHA512_NO_SHA384 in relevant places now. Also, there's a conflict in test_suite_psa_crypto.data which is probably related to dependencies being updated on both sides as well.

@gilles-peskine-arm Just to notify you that #179 was just merged, so this will need a few `!MBEDTLS_SHA512_NO_SHA384` in relevant places now. Also, there's a conflict in `test_suite_psa_crypto.data` which is probably related to dependencies being updated on both sides as well.

gilles-peskine-arm commented

2020-01-30 05:35:26 -06:00

(Migrated from github.com)

Rebased again (previous iteration in https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-hash_compute-5):

"Implement and test psa_hash_compute, psa_hash_compare": added !MBEDTLS_SHA512_NO_SHA384 to the one new SHA-384 test case.
"Remove obsolete dependencies on MBEDTLS_MD_C", "Add missing dependencies on MBEDTLS_MD_C": do it to the same test cases, from the updated dependencies.

Rebased again (previous iteration in https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-hash_compute-5): * "Implement and test psa_hash_compute, psa_hash_compare": added `!MBEDTLS_SHA512_NO_SHA384` to the one new SHA-384 test case. * "Remove obsolete dependencies on MBEDTLS_MD_C", "Add missing dependencies on MBEDTLS_MD_C": do it to the same test cases, from the updated dependencies.

yanesca (Migrated from github.com) reviewed

2020-01-30 08:52:42 -06:00

yanesca (Migrated from github.com) left a comment

It looks good to me in general, I just have two questions.

include/psa/crypto.h

					
				@ -933,3 +933,3 @@

				                              size_t input_length,

				                              const uint8_t *hash,

				                              const size_t hash_length);

				                              size_t hash_length);

Does this mean that we need to update the API reference? This is still const in RC2...

tests/scripts/check-test-cases.py

					
				@ -20,6 +20,7 @@

				#

				# This file is part of Mbed TLS (https://tls.mbed.org)

				import argparse

Why are the changes needed in this file/commit needed in this PR?

gilles-peskine-arm (Migrated from github.com) reviewed

2020-01-30 09:13:21 -06:00

tests/scripts/check-test-cases.py

					
				@ -20,6 +20,7 @@

				#

				# This file is part of Mbed TLS (https://tls.mbed.org)

				import argparse

The new option to check-test-cases isn't needed to make psa_hash_compute work. But I made it and used it during the development of the PR to make it easier to see errors (they tend to get lost among the warnings).

The new option to `check-test-cases` isn't needed to make `psa_hash_compute` work. But I made it and used it during the development of the PR to make it easier to see errors (they tend to get lost among the warnings).

gilles-peskine-arm (Migrated from github.com) reviewed

2020-01-30 09:13:39 -06:00

include/psa/crypto.h

					
				@ -933,3 +933,3 @@

				                              size_t input_length,

				                              const uint8_t *hash,

				                              const size_t hash_length);

				                              size_t hash_length);

Indeed, thanks for noticing that.

gilles-peskine-arm commented

2020-01-30 09:14:12 -06:00

(Migrated from github.com)

CI passes except for Mbed OS jobs which is a known preexisting issue.

yanesca (Migrated from github.com) reviewed

2020-01-30 09:19:46 -06:00

yanesca (Migrated from github.com) left a comment

No description provided.

LGTM.

gilles-peskine-arm (Migrated from github.com) reviewed

2020-01-30 09:33:32 -06:00

include/psa/crypto.h

					
				@ -933,3 +933,3 @@

				                              size_t input_length,

				                              const uint8_t *hash,

				                              const size_t hash_length);

				                              size_t hash_length);

And I also fixed this in the similar function for MAC, because it's easier to fix it now than track it for later.

yanesca (Migrated from github.com) approved these changes

2020-01-30 09:38:50 -06:00

athoelke (Migrated from github.com) reviewed

2020-01-30 10:42:54 -06:00

include/psa/crypto.h

					
				@ -933,3 +933,3 @@

				                              size_t input_length,

				                              const uint8_t *hash,

				                              const size_t hash_length);

				                              size_t hash_length);

I'll update the spec - this doesn't actually change the interface, but it is not standard practice to use this in the API declaration.

gilles-peskine-arm (Migrated from github.com) reviewed

2020-01-30 10:45:16 -06:00

include/psa/crypto.h

					
				@ -933,3 +933,3 @@

				                              size_t input_length,

				                              const uint8_t *hash,

				                              const size_t hash_length);

				                              size_t hash_length);

@athoelke https://github.com/ARMmbed/psa-crypto-api/pull/366

athoelke (Migrated from github.com) reviewed

2020-01-30 10:56:28 -06:00

include/psa/crypto.h

					
				@ -933,3 +933,3 @@

				                              size_t input_length,

				                              const uint8_t *hash,

				                              const size_t hash_length);

				                              size_t hash_length);