Rename psa_asymmetric_{sign_verify} to psa_{sign,verify}_hash #325

2019-11-26T11:28:08-06:00

gilles-peskine-arm commented

2019-11-26 11:28:08 -06:00

(Migrated from github.com)

Rename psa_asymmetric_sign to psa_sign_hash to make it clearer that this function takes a hash as its input (or something that serves as a hash). Likewise for psa_asymmetric_verify to psa_verify_hash. Rename some macros accordingly. A follow-up will add a new pair of functions psa_sign_message and psa_verify_message.

Keep deprecated aliases. Improve the testing of deprecated features: do test some deprecated features when MBEDTLS_DEPRECATED_REMOVED and MBEDTLS_DEPRECATED_WARNING are both disabled, and add a test component that builds and runs with those tests and MBEDTLS_DEPRECATED_WARNING enabled and with -Werror=deprecated-declarations disabled.

This enacts a change in the PSA specification (private link: https://github.com/ARMmbed/psa-crypto/pull/301). Internal ref: IOTCRYPT-933

Rename `psa_asymmetric_sign` to `psa_sign_hash` to make it clearer that this function takes a hash as its input (or something that serves as a hash). Likewise for `psa_asymmetric_verify` to `psa_verify_hash`. Rename some macros accordingly. A follow-up will add a new pair of functions `psa_sign_message` and `psa_verify_message`. Keep deprecated aliases. Improve the testing of deprecated features: do test some deprecated features when `MBEDTLS_DEPRECATED_REMOVED` and `MBEDTLS_DEPRECATED_WARNING` are both disabled, and add a test component that builds and runs with those tests and `MBEDTLS_DEPRECATED_WARNING` enabled and with `-Werror=deprecated-declarations` disabled. This enacts a change in the PSA specification (private link: https://github.com/ARMmbed/psa-crypto/pull/301). Internal ref: IOTCRYPT-933

gilles-peskine-arm commented

2019-11-28 04:01:59 -06:00

(Migrated from github.com)

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

AndrzejKurek (Migrated from github.com) reviewed

2019-11-29 03:23:43 -06:00

include/psa/crypto_compat.h

					
				@ -0,0 +1,109 @@

				/**

				 * \file psa/crypto_compat.h

				 *

				 * \brief PSA cryptography module: Backward compatibility aliases

I would add a sentence about this file also having deprecation information.

AndrzejKurek (Migrated from github.com) requested changes

2019-11-29 03:23:54 -06:00

AndrzejKurek (Migrated from github.com) left a comment

PSA_ASYMMETRIC_SIGN_OUTPUT_SIZE is still used in test_suite_psa_crypto.function.

`PSA_ASYMMETRIC_SIGN_OUTPUT_SIZE` is still used in test_suite_psa_crypto.function.

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

2019-11-29 03:44:39 -06:00

tests/suites/test_suite_psa_crypto.function

					
				@ -3931,2 +3993,4 @@

				    TEST_EQUAL( actual_status, expected_status );

				#if defined(MBEDTLS_TEST_DEPRECATED)

				    TEST_EQUAL( psa_asymmetric_verify( handle, alg,

This is conditioned by MBEDTLS_TEST_DEPRECATED. Using a deprecated macro outside of a MBEDTLS_TEST_DEPRECATED guard should result in a CI failure in the job with MBEDTLS_DEPRECATED_WARNING set.

This is conditioned by `MBEDTLS_TEST_DEPRECATED`. Using a deprecated macro outside of a `MBEDTLS_TEST_DEPRECATED` guard should result in a CI failure in the job with `MBEDTLS_DEPRECATED_WARNING` set.

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

2019-11-29 03:46:03 -06:00

include/psa/crypto_compat.h

					
				@ -0,0 +1,109 @@

				/**

				 * \file psa/crypto_compat.h

				 *

				 * \brief PSA cryptography module: Backward compatibility aliases

What do you mean? Something like “This header declares alternative names for macros and functions that may be removed in a future version of Mbed Crypto”?

AndrzejKurek (Migrated from github.com) reviewed

2019-11-29 04:59:34 -06:00

tests/suites/test_suite_psa_crypto.function

I was thinking about a potential gap in testing deprecated stuff here:
If psa_asymmetric_sign somehow happens to return PSA_SUCCESS on error, but doesn't modify signature in the process, the next ASSERT_COMPARE will succeed anyway, since there's old data in the buffer.
Not sure if it's worth to add buffer zeroization here, since psa_asymmetric_sign should not behave badly.

I was thinking about a potential gap in testing deprecated stuff here: If `psa_asymmetric_sign` somehow happens to return `PSA_SUCCESS` on error, but doesn't modify `signature` in the process, the next `ASSERT_COMPARE` will succeed anyway, since there's old data in the buffer. Not sure if it's worth to add buffer zeroization here, since `psa_asymmetric_sign` should not behave badly.

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

2019-11-29 05:09:39 -06:00

tests/suites/test_suite_psa_crypto.function

I don't think it's useful to test deprecated identifiers as black boxes. My intent was a gray-box test, where we assume that the deprecated identifiers are defined as aliases but they may be defined as the wrong alias, or pass the wrong parameter, or have syntax errors in a macro expansion. Zeroing signature here is cheap and useful so I'll do it.

I don't think it's useful to test deprecated identifiers as black boxes. My intent was a gray-box test, where we assume that the deprecated identifiers are defined as aliases but they may be defined as the wrong alias, or pass the wrong parameter, or have syntax errors in a macro expansion. Zeroing `signature` here is cheap and useful so I'll do it.

AndrzejKurek (Migrated from github.com) reviewed

2019-11-29 05:12:56 -06:00

include/psa/crypto_compat.h

					
				@ -0,0 +1,109 @@

				/**

				 * \file psa/crypto_compat.h

				 *

				 * \brief PSA cryptography module: Backward compatibility aliases

Or just Backward compatibility aliases and deprecated entities

Or just `Backward compatibility aliases and deprecated entities`

AndrzejKurek (Migrated from github.com) approved these changes

2019-11-29 06:00:23 -06:00

AndrzejKurek (Migrated from github.com) reviewed

2019-11-29 06:01:54 -06:00

AndrzejKurek (Migrated from github.com) left a comment

I've verified that the renamed entities are not used across the library (apart from crypto_compat and deprecation tests). The changes look fine.

I've verified that the renamed entities are not used across the library (apart from `crypto_compat` and deprecation tests). The changes look fine.

gilles-peskine-arm commented

2019-12-02 06:18:50 -06:00

(Migrated from github.com)

Again CI (head, merge) passed except for expected failures: an ABI change (two external symbols have been renamed) and generated files in TLS (due to the new header).

Again CI ([head](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-325-head/4), [merge](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-325-merge/7)) passed except for expected failures: an ABI change (two external symbols have been renamed) and generated files in TLS (due to the new header).

dgreen-arm (Migrated from github.com) approved these changes

2019-12-05 03:12:37 -06:00

Sign in to join this conversation.

No reviewers

AndrzejKurek

dgreen-arm