Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PK tests: use PSA to generate keypairs when USE_PSA is enabled #7393

Merged
merged 6 commits into from
Apr 11, 2023
Merged

PK tests: use PSA to generate keypairs when USE_PSA is enabled #7393

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6891b1
pk: add alternate function for keypair generation using PSA
valeriosetti Apr 3, 2023
37194b9
gen_key: limit EC key generation to when USE_PSA is disabled
valeriosetti Apr 3, 2023
0b30442
ecp: revert changes to ECP module and related tests/programs
valeriosetti Apr 4, 2023
12a063a
test: use proper macros for PSA init/done
valeriosetti Apr 4, 2023
b3f20da
test: fix error handling in the new pk_genkey_ec() function
valeriosetti Apr 4, 2023
7816c24
test: fix guards position in test_suite_pk
valeriosetti Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 64 additions & 4 deletions tests/suites/test_suite_pk.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,62 @@
* unconditionally (https://github.com/Mbed-TLS/mbedtls/issues/2023). */
#include "psa/crypto.h"

/* Used for properly sizing the key buffer in pk_genkey_ec() */
#include "mbedtls/psa_util.h"

#define RSA_KEY_SIZE 512
#define RSA_KEY_LEN 64

#if defined(MBEDTLS_USE_PSA_CRYPTO) && defined(MBEDTLS_ECP_C)
static int pk_genkey_ec(mbedtls_ecp_group *grp,
mbedtls_mpi *d, mbedtls_ecp_point *Q)
{
psa_status_t status;
psa_key_attributes_t key_attr = PSA_KEY_ATTRIBUTES_INIT;
mbedtls_svc_key_id_t key_id = MBEDTLS_SVC_KEY_ID_INIT;
size_t curve_bits;
psa_ecc_family_t curve = mbedtls_ecc_group_to_psa(grp->id,
&curve_bits);
unsigned char key_buf[MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH];
size_t key_len;
int ret;

psa_set_key_type(&key_attr, PSA_KEY_TYPE_ECC_KEY_PAIR(curve));
psa_set_key_bits(&key_attr, curve_bits);
psa_set_key_usage_flags(&key_attr, PSA_KEY_USAGE_EXPORT);

status = psa_generate_key(&key_attr, &key_id);
if (status != PSA_SUCCESS) {
return MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only test but I think we should also destroy key if key generation fails in the middle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to me that this is the only point in that function for which there is a return instead of goto exit. However this seems ok to me since if psa_generate_key fails then there should be nothing to destroy. Other failures destroy the key. What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is all true. I'm not sure if we shouldn't also destroy key here. But looking at the description of psa_generate_key:

On success, an identifier for the newly created key.

It seems that current version is correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I would expect psa_generate_key() to act in an atomic way: either it succeeds, or it leave the key slot unchanged, so that callers don't need to call destroy on failure. However I checked the documentation and it doesn't say explicitly. I checked other uses in the library, and they seem to only call destroy when generate succeeded but an error occurred later. So I think the code is fine as it is, but out of precaution I'll ask on the PSA Crypto API channel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However I checked the documentation and it doesn't say explicitly.

Well, I apparently didn't get enough sleep last night, because the documentation does say: "key On success, an identifier for the newly created key. PSA_KEY_ID_NULL on failure." So, we don't need to call destroy() on failure here. (It would however be legal, as psa_destroy_key(PSA_KEY_ID_NULL) is guaranteed to do nothing an return success.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a general principle that a PSA function doesn't affect the system state on failure. (It might leave its in-out arguments in a changed state on failure, however: multipart operation functions put the operation object in an error state when they fail.)

psa_generate_key and the other key creation functions are guaranteed to set the key_id output argument to 0 on failure. So it's guaranteed that calling psa_destroy_key(key_id) is always correct, but it's also guaranteed that it's a no-op if the creation function fails.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I should refresh the page more often when replying to comments :) I hadn't seen your replies when I wrote mines.

}

status = psa_export_key(key_id, key_buf, sizeof(key_buf), &key_len);
if (status != PSA_SUCCESS) {
ret = MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE;
goto exit;
}

ret = mbedtls_mpi_read_binary(d, key_buf, key_len);
if (ret != 0) {
goto exit;
}

status = psa_export_public_key(key_id, key_buf, sizeof(key_buf),
&key_len);
if (status != PSA_SUCCESS) {
ret = MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE;
goto exit;
}

ret = mbedtls_ecp_point_read_binary(grp, Q, key_buf, key_len);

exit:
psa_destroy_key(key_id);

return ret;
}
#endif /* MBEDTLS_USE_PSA_CRYPTO && MBEDTLS_ECP_C */

/** Generate a key of the desired type.
*
* \param pk The PK object to fill. It must have been initialized
Expand Down Expand Up @@ -53,12 +106,18 @@ static int pk_genkey(mbedtls_pk_context *pk, int parameter)
return ret;
}

#if defined(MBEDTLS_USE_PSA_CRYPTO)
return pk_genkey_ec(&mbedtls_pk_ec(*pk)->grp,
&mbedtls_pk_ec(*pk)->d,
&mbedtls_pk_ec(*pk)->Q);
#else /* MBEDTLS_USE_PSA_CRYPTO */
return mbedtls_ecp_gen_keypair(&mbedtls_pk_ec(*pk)->grp,
&mbedtls_pk_ec(*pk)->d,
&mbedtls_pk_ec(*pk)->Q,
mbedtls_test_rnd_std_rand, NULL);
#endif /* MBEDTLS_USE_PSA_CRYPTO */
}
#endif
#endif /* MBEDTLS_ECP_C */
return -1;
}

Expand Down Expand Up @@ -462,6 +521,7 @@ void pk_utils(int type, int parameter, int bitlen, int len, char *name)
{
mbedtls_pk_context pk;

USE_PSA_INIT();
mbedtls_pk_init(&pk);

TEST_ASSERT(mbedtls_pk_setup(&pk, mbedtls_pk_info_from_type(type)) == 0);
Expand All @@ -475,6 +535,7 @@ void pk_utils(int type, int parameter, int bitlen, int len, char *name)

exit:
mbedtls_pk_free(&pk);
USE_PSA_DONE();
}
/* END_CASE */

Expand Down Expand Up @@ -1234,9 +1295,8 @@ void pk_psa_sign(int parameter_arg,
mbedtls_pk_init(&pk);
TEST_ASSERT(mbedtls_pk_setup(&pk,
mbedtls_pk_info_from_type(MBEDTLS_PK_ECKEY)) == 0);
TEST_ASSERT(mbedtls_ecp_gen_key(grpid,
(mbedtls_ecp_keypair *) pk.pk_ctx,
mbedtls_test_rnd_std_rand, NULL) == 0);
TEST_ASSERT(pk_genkey(&pk, grpid) == 0);

alg_psa = PSA_ALG_ECDSA(PSA_ALG_SHA_256);
} else
#endif /* MBEDTLS_PK_CAN_ECDSA_SIGN */
Expand Down