Implement psa_hash_compute and psa_hash_compare #327

gilles-peskine-arm · 2019-11-28T19:13:21Z

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 · 2019-12-02T16:48:30Z

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 · 2019-12-06T16:51:14Z

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

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

tests/suites/test_suite_psa_crypto.function

mpg · 2020-01-28T12:04:27Z

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

gilles-peskine-arm · 2020-01-28T19:26:06Z

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

gilles-peskine-arm · 2020-01-28T19:28:15Z

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.

mpg

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

mpg

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

mpg · 2020-01-30T09:34:43Z

@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.

The PSA implementation of hash algorithms, HMAC algorithms and KDF algorithms using HMAC no longer use the MD module.

The PSA implementations of deterministic ECDSA, of all RSA signatures and of RSA OAEP use the MD module.

Whether a parameter should be const is an implementation detail of the function, so don't declare a parameter of psa_hash_compare as const. (This only applies to parameters themselves, not to objects that pointer parameters points to.)

psa_hash_compare is tested for good cases and invalid-signature cases in hash_compute_compare. Also test invalid-argument cases. Also run a few autonomous test cases with valid arguments.

gilles-peskine-arm · 2020-01-30T11:35:26Z

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

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

include/psa/crypto.h

tests/scripts/check-test-cases.py

gilles-peskine-arm · 2020-01-30T15:14:12Z

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

yanesca

LGTM.

An earlier commit fixed this for psa_hash_compare. psa_mac_verify had the same flaw.

mpg

LGTM.

mpg · 2020-01-31T08:31:05Z

After the latest commit, the CI is still passing except for Mbed OS tests which are a known issue independent from this PR.

Previously in d875285: * ARMmbed#333: Streamline PSA key type encodings: prepare * ARMmbed#323: Initialise return values to an error Previously in dbcb442: * ARMmbed#291: Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY * ARMmbed#334: Fix some pylint warnings Previously in ceceedb: * ARMmbed#348: Bump version to Mbed TLS 2.20.0 and crypto SO version to 4 * ARMmbed#354: Fix incrementing pointer instead of value In this commit: * ARMmbed#349: Fix minor defects found by Coverity * ARMmbed#179: Add option to build SHA-512 without SHA-384 * ARMmbed#327: Implement psa_hash_compute and psa_hash_compare * ARMmbed#330: Streamline PSA key type and curve encodings

gilles-peskine-arm added needs: preceding PR Requires another PR to be merged first needs: ci Needs a passing full CI run needs: work The pull request needs rework before it can be merged. labels Nov 28, 2019

gilles-peskine-arm force-pushed the psa-hash_compute branch from 362f8d8 to 375142c Compare December 2, 2019 13:33

gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: ci Needs a passing full CI run needs: work The pull request needs rework before it can be merged. labels Dec 2, 2019

gilles-peskine-arm force-pushed the psa-hash_compute branch from 375142c to 6d949ec Compare December 6, 2019 13:15

gilles-peskine-arm added needs: ci Needs a passing full CI run and removed needs: preceding PR Requires another PR to be merged first labels Dec 6, 2019

mpg self-requested a review January 28, 2020 11:33

mpg reviewed Jan 28, 2020

View reviewed changes

mpg added the needs: work The pull request needs rework before it can be merged. label Jan 28, 2020

gilles-peskine-arm force-pushed the psa-hash_compute branch from 6d949ec to de3f24e Compare January 28, 2020 19:25

gilles-peskine-arm force-pushed the psa-hash_compute branch from 6815077 to fc85c17 Compare January 28, 2020 19:44

gilles-peskine-arm removed the needs: work The pull request needs rework before it can be merged. label Jan 28, 2020

gilles-peskine-arm requested a review from mpg January 28, 2020 19:45

mpg mentioned this pull request Jan 29, 2020

Add option to build SHA-512 without SHA-384 #179

Merged

mpg suggested changes Jan 29, 2020

View reviewed changes

tests/suites/test_suite_psa_crypto.function Outdated Show resolved Hide resolved

gilles-peskine-arm force-pushed the psa-hash_compute branch from fc85c17 to a31c20a Compare January 29, 2020 09:24

mpg previously approved these changes Jan 29, 2020

View reviewed changes

gilles-peskine-arm added 3 commits January 30, 2020 12:27

Implement and test psa_hash_compute, psa_hash_compare

0a749c8

Remove obsolete dependencies on MBEDTLS_MD_C

aead02c

The PSA implementation of hash algorithms, HMAC algorithms and KDF algorithms using HMAC no longer use the MD module.

Add missing dependencies on MBEDTLS_MD_C

7b8efaf

The PSA implementations of deterministic ECDSA, of all RSA signatures and of RSA OAEP use the MD module.

gilles-peskine-arm added 5 commits January 30, 2020 12:27

Use psa_hash_compute in psa_hmac_setup_internal

84b8fc8

Add command line option to hide warnings

1fb7aea

Don't declare a parameter as const

fa710f5

Whether a parameter should be const is an implementation detail of the function, so don't declare a parameter of psa_hash_compare as const. (This only applies to parameters themselves, not to objects that pointer parameters points to.)

Remove some spurious dependencies on MBEDTLS_SHA256_C

29eb80d

Add dedicated test cases for psa_hash_compare

88e0846

psa_hash_compare is tested for good cases and invalid-signature cases in hash_compute_compare. Also test invalid-argument cases. Also run a few autonomous test cases with valid arguments.

gilles-peskine-arm dismissed mpg’s stale review via 88e0846 January 30, 2020 11:33

gilles-peskine-arm force-pushed the psa-hash_compute branch from a31c20a to 88e0846 Compare January 30, 2020 11:33

gilles-peskine-arm requested a review from mpg January 30, 2020 11:33

yanesca reviewed Jan 30, 2020

View reviewed changes

include/psa/crypto.h Show resolved Hide resolved

tests/scripts/check-test-cases.py Show resolved Hide resolved

gilles-peskine-arm changed the title ~~Implement psa_hash_compute and psa_hash_verify~~ Implement psa_hash_compute and psa_hash_compare Jan 30, 2020

yanesca previously approved these changes Jan 30, 2020

View reviewed changes

Don't declare a parameter as const

13faa2d

An earlier commit fixed this for psa_hash_compare. psa_mac_verify had the same flaw.

gilles-peskine-arm dismissed yanesca’s stale review via 13faa2d January 30, 2020 15:33

gilles-peskine-arm requested a review from yanesca January 30, 2020 15:33

yanesca approved these changes Jan 30, 2020

View reviewed changes

mpg approved these changes Jan 31, 2020

View reviewed changes

mpg added ready for merge Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only. and removed needs: ci Needs a passing full CI run needs: review The pull request is ready for review. This generally means that it has no known issues. labels Jan 31, 2020

mpg merged commit 350d4c3 into ARMmbed:development Jan 31, 2020

gilles-peskine-arm mentioned this pull request Feb 17, 2020

Update crypto submodule 2020-02-06 Mbed-TLS/mbedtls#3037

Merged

This was referenced Apr 27, 2020

Some of the Crypto APIs of PSA Crypto 1.0 Beta-3 are unimplemented mbedcrypto-3.0.1 #381

Closed

Implement one-shot hash, MAC and cipher Mbed-TLS/mbedtls#3258

Closed

Implement psa_hash_compute and psa_hash_compare #327

Implement psa_hash_compute and psa_hash_compare #327

Uh oh!

Conversation

gilles-peskine-arm commented Nov 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm commented Dec 2, 2019

Uh oh!

gilles-peskine-arm commented Dec 6, 2019

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpg commented Jan 28, 2020

Uh oh!

gilles-peskine-arm commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm commented Jan 28, 2020

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

mpg commented Jan 30, 2020

Uh oh!

gilles-peskine-arm commented Jan 30, 2020

Uh oh!

yanesca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm commented Jan 30, 2020

Uh oh!

yanesca left a comment

Choose a reason for hiding this comment

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

mpg commented Jan 31, 2020

Uh oh!

Uh oh!

gilles-peskine-arm commented Nov 28, 2019 •

edited

Loading

gilles-peskine-arm commented Jan 28, 2020 •

edited

Loading