PK: add mbedtls_pk_write_pubkey_psa() #577

Merged
valeriosetti merged 21 commits from issue529 into development 2025-12-12 12:09:11 -06:00
valeriosetti commented 2025-11-19 05:34:36 -06:00 (Migrated from github.com)

Description

Resolves #529

PR checklist

  • changelog provided
  • framework PR not required
  • mbedtls development PR not required because: no change there
  • mbedtls 3.6 PR not required because: no backport
  • tests provided
## Description Resolves #529 ## PR checklist - [x] **changelog** provided - [x] **framework PR** not required - [x] **mbedtls development PR** not required because: no change there - [x] **mbedtls 3.6 PR** not required because: no backport - **tests** provided
bjwtaylor (Migrated from github.com) reviewed 2025-11-21 05:14:49 -06:00
mpg (Migrated from github.com) requested changes 2025-12-03 06:24:38 -06:00
mpg (Migrated from github.com) left a comment

Looks pretty good to me, thanks for the nice documentation, but I want more tests :)

Looks pretty good to me, thanks for the nice documentation, but I want more tests :)
mpg (Migrated from github.com) commented 2025-12-03 05:08:50 -06:00

Typo: pubBlic

Typo: pubBlic
mpg (Migrated from github.com) commented 2025-12-03 05:19:01 -06:00

I don't think we need to check for that or promise that we do in the documentation. Generally speaking when passed NULL pointers that shouldn't be NULL, we're OK with getting whatever behaviour we get from the platform (like segfault on Linux etc).

Some of the old code in PK has more checks than our current policy, but I don't think we should copy that in new code.

(Though thanks for documenting every error condition here!)

I don't think we need to check for that or promise that we do in the documentation. Generally speaking when passed NULL pointers that shouldn't be NULL, we're OK with getting whatever behaviour we get from the platform (like segfault on Linux etc). Some of the old code in PK has more checks than our current policy, but I don't think we should copy that in new code. (Though thanks for documenting every error condition here!)
@ -880,2 +880,4 @@
int mbedtls_pk_write_key_pem(const mbedtls_pk_context *ctx, unsigned char *buf, size_t size);
#endif /* MBEDTLS_PEM_WRITE_C */
/**
mpg (Migrated from github.com) commented 2025-12-03 05:19:49 -06:00

Thanks! This is an example of how we should always write documentation about buffer sizes :)

Thanks! This is an example of how we should always write documentation about buffer sizes :)
@ -158,0 +230,4 @@
} else if (key_bits == 521) {
priv_key = test_ec_secp521r1_priv;
priv_key_len = sizeof(test_ec_secp521r1_priv);
}
mpg (Migrated from github.com) commented 2025-12-03 05:20:47 -06:00

It has now been merged, so this can be updated :)

It has now been merged, so this can be updated :)
mpg (Migrated from github.com) commented 2025-12-03 05:22:52 -06:00

Post 1.0, we can (and should) just call PSA_INIT(). (Same with PSA_DONE().)

Post 1.0, we can (and should) just call `PSA_INIT()`. (Same with `PSA_DONE()`.)
mpg (Migrated from github.com) commented 2025-12-03 06:23:11 -06:00

Ok, that's one was to populate a context. As discussed elsewhere, we need to test other ways of populating the context. Also as discussed, that might either be done with more test functions, or my handling multiple methods in a single function.

Off the top of my head, I'd say:

  • one function for parse public (this one)
  • for parse keypair: possibly the same function, with an extra param for public/private, as I think the only thing that's going to change is the parse function being called
  • possibly a single function from all 3 of wrap_psa() , copy_from_psa() and copy_public_from_psa(), as they all start with a PSA key?
Ok, that's one was to populate a context. As discussed elsewhere, we need to test other ways of populating the context. Also as discussed, that might either be done with more test functions, or my handling multiple methods in a single function. Off the top of my head, I'd say: - one function for parse public (this one) - for parse keypair: possibly the same function, with an extra param for public/private, as I think the only thing that's going to change is the parse function being called - possibly a single function from all 3 of `wrap_psa()` , `copy_from_psa()` and `copy_public_from_psa()`, as they all start with a PSA key?
valeriosetti (Migrated from github.com) reviewed 2025-12-03 08:29:00 -06:00
valeriosetti (Migrated from github.com) commented 2025-12-03 08:29:00 -06:00

While addressing comments I realized that the implementation I provided completely misses the support for opaque keys! This is not a regression compared to what there before since opaque contexts (EC and RSA) do not have the debug support. So the question: is this something we foresee that might be useful in the future? Or are we OK with the current implementation?
I think that extending the test coverage as you suggested will tell if at least we're fine with non-opaque keys, I guess

While addressing comments I realized that the implementation I provided completely misses the support for opaque keys! This is not a regression compared to what there before since opaque contexts ([EC](https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/cb9d0ed64831da3e7b85ea8741a57fdc27c010e6/drivers/builtin/src/pk_wrap.c#L712) and [RSA](https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/cb9d0ed64831da3e7b85ea8741a57fdc27c010e6/drivers/builtin/src/pk_wrap.c#L785)) do not have the debug support. So the question: is this something we foresee that might be useful in the future? Or are we OK with the current implementation? I think that extending the test coverage as [you suggested](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/577#discussion_r2584906619) will tell if at least we're fine with non-opaque keys, I guess
valeriosetti (Migrated from github.com) reviewed 2025-12-03 08:53:08 -06:00
valeriosetti (Migrated from github.com) commented 2025-12-03 08:53:08 -06:00

On a second thought I have a proposal/question as follow-up. How bad is it for opaque keys in PK to have their public part in the PK context (like non-opaque EC/RSA)? I cannot think of any drawback to this, but you know for sure better than me.

Rationale.
I found that mbedtls_pk_ecc_set_pubkey_from_prv and mbedtls_pk_rsa_set_pubkey_from_prv are basically the same, so instead of duplicating the code I can move them in pk.c with their prototype being in pk_internal.h. Then in mbedtls_pk_write_pubkey_psa I can check if pk->pub_raw_len != 0 and memcpy from that; however in case it's empty BUT pk->priv_id is not empty, I can try to export the public key from the private one (using the merge of mbedtls_pk_ecc_set_pubkey_from_prv & mbedtls_pk_rsa_set_pubkey_from_prv ) as last resort.
To me this makes mbedtls_pk_write_pubkey_psa able to work in all scenarios without end user intervention and being able to support opaque keys as well!
Wdyt?

On a second thought I have a proposal/question as follow-up. How bad is it for opaque keys in PK to have their public part in the PK context (like non-opaque EC/RSA)? I cannot think of any drawback to this, but you know for sure better than me. Rationale. I found that `mbedtls_pk_ecc_set_pubkey_from_prv` and `mbedtls_pk_rsa_set_pubkey_from_prv` are basically the same, so instead of duplicating the code I can move them in `pk.c` with their prototype being in `pk_internal.h`. Then in `mbedtls_pk_write_pubkey_psa` I can check if `pk->pub_raw_len != 0` and `memcpy` from that; however in case it's empty BUT `pk->priv_id` is not empty, I can try to export the public key from the private one (using the merge of `mbedtls_pk_ecc_set_pubkey_from_prv` & `mbedtls_pk_rsa_set_pubkey_from_prv` ) as last resort. To me this makes `mbedtls_pk_write_pubkey_psa` able to work in all scenarios without end user intervention and being able to support opaque keys as well! Wdyt?
gilles-peskine-arm (Migrated from github.com) reviewed 2025-12-03 08:57:53 -06:00
gilles-peskine-arm (Migrated from github.com) commented 2025-12-03 08:57:53 -06:00

How bad is it for opaque keys in PK to have their public part in the PK context (like non-opaque EC/RSA)?

It's just a memory-vs-performance compromise. Given that the current trend in pk is to reduce the number of cases when manipulating key representations, I think it would make sense to always store the public key in the context.

> How bad is it for opaque keys in PK to have their public part in the PK context (like non-opaque EC/RSA)? It's just a memory-vs-performance compromise. Given that the current trend in pk is to reduce the number of cases when manipulating key representations, I think it would make sense to always store the public key in the context.
valeriosetti (Migrated from github.com) reviewed 2025-12-03 16:49:06 -06:00
valeriosetti (Migrated from github.com) commented 2025-12-03 16:49:05 -06:00

I just added a couple of commits ([1] and [2]) to show what I had in mind.
I haven't extended testing though, so please wait a bit before reviewing the PR again.

I just added a couple of commits ([[1]](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/577/commits/31c3c0c6c08de01437d4c974d5b0e734d6a4e19c) and [[2]](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/577/commits/a9f7d9e5e5a25e1968c2f08376520404803a720a)) to show what I had in mind. I haven't extended testing though, so please wait a bit before reviewing the PR again.
mpg (Migrated from github.com) reviewed 2025-12-04 02:14:57 -06:00
mpg (Migrated from github.com) commented 2025-12-04 02:14:57 -06:00

As Gilles said, and as I was trying to say on the bitlen PR, I think we want to avoid as much as possible having fields in the pk_context that are sometimes set, sometimes not, and as maintainers we have to remember about them and what condition to check to know if the field is set or not, and that adds a lot of cognitive load when working with the code.

So, yes, every time we can reduce the number of cases we have to consider and move to a situation where the same information is always present in the same way not matter how the key was populated, I'll be in favour of doing so!

As Gilles said, and as I was trying to say on the bitlen PR, I think we want to avoid as much as possible having fields in the `pk_context` that are sometimes set, sometimes not, and as maintainers we have to remember about them and what condition to check to know if the field is set or not, and that adds a lot of cognitive load when working with the code. So, yes, every time we can reduce the number of cases we have to consider and move to a situation where the same information is always present in the same way not matter how the key was populated, I'll be in favour of doing so!
valeriosetti commented 2025-12-04 02:48:05 -06:00 (Migrated from github.com)

Sorry I had to rebase on development because I needed mbedtls_pk_get_key_type() in test code, but this has only been added recently in tf-psa-cryto.

Sorry I had to rebase on `development` because I needed `mbedtls_pk_get_key_type()` in test code, but this has only been added recently in tf-psa-cryto.
bjwtaylor (Migrated from github.com) reviewed 2025-12-08 04:33:35 -06:00
mpg (Migrated from github.com) requested changes 2025-12-08 05:04:15 -06:00
mpg (Migrated from github.com) left a comment

Thanks for adding the bunch of tests I requested, I like how your structured them too :)

However in addition to small suggestions I have two blocker comments:

  • Possibly mutating the context seems dirty: please either remove this, or convince me it's the right way to go (but I'll be hard to convince, fair warning).
  • Apparently missing actual testing for contexts creating from PSA - or am I just not seeing it?

Also, a few other comments/suggestions that are optional.

Thanks for adding the bunch of tests I requested, I like how your structured them too :) However in addition to small suggestions I have two blocker comments: - Possibly mutating the context seems dirty: please either remove this, or convince me it's the right way to go (but I'll be hard to convince, fair warning). - Apparently missing actual testing for contexts creating from PSA - or am I just not seeing it? Also, a few other comments/suggestions that are optional.
@ -137,6 +143,23 @@ int mbedtls_pk_setup(mbedtls_pk_context *ctx, const mbedtls_pk_info_t *info)
return 0;
mpg (Migrated from github.com) commented 2025-12-08 04:08:04 -06:00

Minor: since the function returns int, I'd find return 0 more natural here.

Minor: since the function returns `int`, I'd find `return 0` more natural here.
@ -475,6 +475,26 @@ int mbedtls_pk_write_key_der(const mbedtls_pk_context *key, unsigned char *buf,
return MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE;
}
/******************************************************************************
mpg (Migrated from github.com) commented 2025-12-08 04:13:47 -06:00

This feels a bit hacky: having to remove the const is a bad smell. I'd rather make sure all functions that populate a context always set the public key field (presumably by calling mbedtls_pk_set_pubkey_from_prv().

This goes with my general wish that once a context is populated, all the fields from pk_context are set and usable (except for priv_id when the context is only holding public key material).

Unless you think there's a reason this can't be done?

This feels a bit hacky: having to remove the `const` is a bad smell. I'd rather make sure all functions that populate a context always set the public key field (presumably by calling `mbedtls_pk_set_pubkey_from_prv()`. This goes with my general wish that once a context is populated, all the fields from `pk_context` are set and usable (except for `priv_id` when the context is only holding public key material). Unless you think there's a reason this can't be done?
@ -153,3 +153,86 @@ pk_write_public_from_private:"../framework/data_files/ec_x25519_prv.der":"../fra
Derive public key EC X448
depends_on:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY:PSA_WANT_ECC_MONTGOMERY_448
mpg (Migrated from github.com) commented 2025-12-08 04:21:32 -06:00

Minor: can we have symbolic names, like MBEDTLS_PK_TEST_{PUBLIC,PRIVATE} (either using #defines or an enum)? We already have too many test with 0/1 parameters, and as long as there's only one we can often guess, but when there's more than one it's uncomfortable to read.

In the spirit that you suggested of raising the bar for readability in newly added tests :)

Minor: can we have symbolic names, like `MBEDTLS_PK_TEST_{PUBLIC,PRIVATE}` (either using `#define`s or an enum)? We already have too many test with 0/1 parameters, and as long as there's only one we can often guess, but when there's more than one it's uncomfortable to read. In the spirit that you suggested of raising the bar for readability in newly added tests :)
mpg (Migrated from github.com) commented 2025-12-08 04:58:13 -06:00

Note: this is likely to be also useful for other test functions, so it would make sense turning that into its own helper function. (Can be done either in this PR, or in the next PR that needs something similar - I think perhaps #575 could use something similar.)

Note: this is likely to be also useful for other test functions, so it would make sense turning that into its own helper function. (Can be done either in this PR, or in the next PR that needs something similar - I think perhaps #575 could use something similar.)
mpg (Migrated from github.com) commented 2025-12-08 04:55:37 -06:00

Shouldn't we call test_pk_write_pubkey_psa()? Am I missing something obvious or did you?

Shouldn't we call `test_pk_write_pubkey_psa()`? Am I missing something obvious or did you?
mpg (Migrated from github.com) reviewed 2025-12-08 05:05:43 -06:00
@ -153,3 +153,86 @@ pk_write_public_from_private:"../framework/data_files/ec_x25519_prv.der":"../fra
Derive public key EC X448
mpg (Migrated from github.com) commented 2025-12-08 05:05:43 -06:00

I thought I left a comment here but it seems github lost it: I'd like a symbolic name instead of 0/1 for the new argument, like you did for pk_write_pubkey_psa_from_psa().

I thought I left a comment here but it seems github lost it: I'd like a symbolic name instead of 0/1 for the new argument, like you did for `pk_write_pubkey_psa_from_psa()`.
valeriosetti (Migrated from github.com) reviewed 2025-12-09 04:20:26 -06:00
@ -475,6 +475,26 @@ int mbedtls_pk_write_key_der(const mbedtls_pk_context *key, unsigned char *buf,
return MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE;
}
/******************************************************************************
valeriosetti (Migrated from github.com) commented 2025-12-09 04:20:26 -06:00

This feels a bit hacky: having to remove the const is a bad smell

TBH I was skeptical as well about this change. I checked current calls to mbedtls_pk_debug in TLS code and in all the cases a const pointer is being passed. It makes sense of course, because we're only supposed to get some data out of the PK context, not to modify it.
Also I knew that this change was "requesting" a bit too many weird castings in the TLS code and that this was not nice as well.

I'd rather make sure all functions that populate a context always set the public key field (presumably by calling mbedtls_pk_set_pubkey_from_prv().

I'll look into improving this.

> This feels a bit hacky: having to remove the const is a bad smell TBH I was skeptical as well about this change. I checked current calls to `mbedtls_pk_debug` in TLS code and in all the cases a `const` pointer is being passed. It makes sense of course, because we're only supposed to get some data out of the PK context, not to modify it. Also I knew that this change was "requesting" a bit too many weird castings in the TLS code and that this was not nice as well. > I'd rather make sure all functions that populate a context always set the public key field (presumably by calling mbedtls_pk_set_pubkey_from_prv(). I'll look into improving this.
valeriosetti (Migrated from github.com) reviewed 2025-12-09 04:23:20 -06:00
@ -153,3 +153,86 @@ pk_write_public_from_private:"../framework/data_files/ec_x25519_prv.der":"../fra
Derive public key EC X448
depends_on:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY:PSA_WANT_ECC_MONTGOMERY_448
valeriosetti (Migrated from github.com) commented 2025-12-09 04:23:20 -06:00

Sure, that makes completely sense. I usually hate when I have to switch back and forth between .data and .function files because there are sequences of 1 and 0 in the test data and my bad short term memory cannot remember the order of parameters in test functions :)

Sure, that makes completely sense. I usually hate when I have to switch back and forth between `.data` and `.function` files because there are sequences of `1` and `0` in the test data and my bad short term memory cannot remember the order of parameters in test functions :)
gilles-peskine-arm (Migrated from github.com) reviewed 2025-12-09 05:08:53 -06:00
@ -153,3 +153,86 @@ pk_write_public_from_private:"../framework/data_files/ec_x25519_prv.der":"../fra
Derive public key EC X448
depends_on:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY:PSA_WANT_ECC_MONTGOMERY_448
gilles-peskine-arm (Migrated from github.com) commented 2025-12-09 05:08:53 -06:00

I usually hate when I have to switch back and forth between .data and .function files because there are sequences of 1 and 0 in the test data and my bad short term memory cannot remember the order of parameters in test functions

That calls for IDE support! Emacs users can use mbedtls-test-data-mode. Contributions for other editors are welcome.

> I usually hate when I have to switch back and forth between .data and .function files because there are sequences of 1 and 0 in the test data and my bad short term memory cannot remember the order of parameters in test functions That calls for IDE support! Emacs users can use [mbedtls-test-data-mode](https://github.com/Mbed-TLS/mbedtls-docs/blob/main/tools/emacs/mbedtls-test-data-mode.el). Contributions for other editors are welcome.
mpg (Migrated from github.com) reviewed 2025-12-09 05:31:13 -06:00
@ -153,3 +153,86 @@ pk_write_public_from_private:"../framework/data_files/ec_x25519_prv.der":"../fra
Derive public key EC X448
depends_on:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY:PSA_WANT_ECC_MONTGOMERY_448
mpg (Migrated from github.com) commented 2025-12-09 05:31:13 -06:00

I thought @davidhorstmann-arm has something from vim using a small special-purpose LSP, but I can't seem to find it again.

Still, I think we can all agree that symbolic names have value regardless of the level of editor/IDE support :)

I thought @davidhorstmann-arm has something from vim using a small special-purpose LSP, but I can't seem to find it again. Still, I think we can all agree that symbolic names have value regardless of the level of editor/IDE support :)
valeriosetti (Migrated from github.com) reviewed 2025-12-09 08:16:45 -06:00
valeriosetti (Migrated from github.com) commented 2025-12-09 08:16:45 -06:00

Oops, bad mistake, sorry

Oops, bad mistake, sorry
valeriosetti (Migrated from github.com) reviewed 2025-12-09 08:45:20 -06:00
valeriosetti (Migrated from github.com) commented 2025-12-09 08:45:20 -06:00

Since we also collaborated on previous PRs about PK I guess you saw that this was a duplication of the test helper function that's already used in test_suite_pk.function, so I agree that creating a common helper makes sense.

Since we also collaborated on previous PRs about PK I guess you saw that this was a duplication of the test helper function that's already used in [test_suite_pk.function](https://github.com/Mbed-TLS/TF-PSA-Crypto/blob/348d0eeb40bfc053225d4f1da69b60644c1d3110/tests/suites/test_suite_pk.function#L179), so I agree that creating a common helper makes sense.
valeriosetti (Migrated from github.com) reviewed 2025-12-09 08:46:52 -06:00
valeriosetti (Migrated from github.com) commented 2025-12-09 08:46:52 -06:00

The only thing I'm not sure about is if it's better to do this in this PR or in a quick follow-up one. This one is going to be in the range of 20 commits already, so perhaps a quick follow-up one is better.

The only thing I'm not sure about is if it's better to do this in this PR or in a quick follow-up one. This one is going to be in the range of 20 commits already, so perhaps a quick follow-up one is better.
valeriosetti (Migrated from github.com) reviewed 2025-12-09 08:50:50 -06:00
valeriosetti (Migrated from github.com) commented 2025-12-09 08:50:50 -06:00
https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/598
mpg (Migrated from github.com) requested changes 2025-12-11 03:01:22 -06:00
mpg (Migrated from github.com) left a comment

Thanks for addressing my comments. Looks almost all good to me, but I think we're missing a test for calling the new function on an unpopulated context:

unsigned char buf[PSA_EXPORT_PUBLIC_KEY_MAX_SIZE];
mbedtls_pk_init(&ctx);
TEST_EQUAL(MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_write_pubkey_psa(&ctx, buf, sizeof(buf), &buflen));

(or similar, without the silly mistake I probably made).

I'd suggest adding a small dedicated test function for that. That way, looking at the .data file it's very apparent that we're testing all the cases for this function: unpopulated context (negative), then all 5 methods of populating the context (4 positive cases each).

Thanks for addressing my comments. Looks almost all good to me, but I think we're missing a test for calling the new function on an unpopulated context: ```c unsigned char buf[PSA_EXPORT_PUBLIC_KEY_MAX_SIZE]; mbedtls_pk_init(&ctx); TEST_EQUAL(MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_write_pubkey_psa(&ctx, buf, sizeof(buf), &buflen)); ``` (or similar, without the silly mistake I probably made). I'd suggest adding a small dedicated test function for that. That way, looking at the `.data` file it's very apparent that we're testing all the cases for this function: unpopulated context (negative), then all 5 methods of populating the context (4 positive cases each).
mpg (Migrated from github.com) reviewed 2025-12-11 04:07:15 -06:00
mpg (Migrated from github.com) left a comment
No description provided.
I'm finally satisfied with the amount of testing ;)
bjwtaylor (Migrated from github.com) reviewed 2025-12-11 09:27:20 -06:00
valeriosetti commented 2025-12-11 11:54:28 -06:00 (Migrated from github.com)

@mpg @bjwtaylor I had to rebase to resolve a conflict on the framework. Can you please re-check?

@mpg @bjwtaylor I had to rebase to resolve a conflict on the `framework`. Can you please re-check?
mpg (Migrated from github.com) approved these changes 2025-12-12 02:30:54 -06:00
mpg (Migrated from github.com) left a comment

LGTM, thanks!

LGTM, thanks!
bjwtaylor (Migrated from github.com) approved these changes 2025-12-12 08:40:16 -06:00
Sign in to join this conversation.
No reviewers
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/TF-PSA-Crypto!577
No description provided.