PK: add `mbedtls_pk_write_pubkey_psa()` #577

valeriosetti · 2025-11-19T11:34:36Z

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

mpg

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

mpg · 2025-12-03T11:08:50Z

include/mbedtls/pk.h

+ *
+ * \param ctx       PK context from which the public key is extracted. It must
+ *                  have been populated.
+ * \param buf       Output buffer where the pubblic key is written. It must not


Typo: pubBlic

mpg · 2025-12-03T11:19:01Z

include/mbedtls/pk.h

+ *                  operation is successful. In case of failure the value is 0.
+ *
+ * \return          0 if successful.
+ * \return          MBEDTLS_ERR_PK_BAD_INPUT_DATA if \p ctx, \p buf or \p buf_len


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!)

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

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?

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.

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.

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!

include/mbedtls/pk.h

mpg · 2025-12-03T11:20:47Z

tests/suites/test_suite_pkwrite.function

+    USE_PSA_INIT();
+    TEST_EQUAL(mbedtls_pk_parse_public_keyfile(&pk, key_file), 0);
+
+    /* TODO: Replace this with mbedtls_pk_get_type() once it will be available */


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

mpg · 2025-12-03T11:22:52Z

tests/suites/test_suite_pkwrite.function

+    TEST_EQUAL(mbedtls_pk_write_pubkey_psa(&pk, buf, sizeof(buf), &buf_len),
+               MBEDTLS_ERR_PK_BAD_INPUT_DATA);
+
+    USE_PSA_INIT();


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

mpg · 2025-12-03T12:23:11Z

tests/suites/test_suite_pkwrite.function

+               MBEDTLS_ERR_PK_BAD_INPUT_DATA);
+
+    USE_PSA_INIT();
+    TEST_EQUAL(mbedtls_pk_parse_public_keyfile(&pk, key_file), 0);


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?

Add function implementation, prototype and documentation. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

Signed-off-by: Valerio Setti <vsetti@baylibre.com>

valeriosetti · 2025-12-04T08:48:05Z

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.

…_psa() It is OK to segfault (or any crash, depending on the platform) if a NULL pointer is passed when the documentation clearly states that this must not be done. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

Since "mbedtls_pk_ecc_set_pubkey_from_prv" and "mbedtls_pk_rsa_set_pubkey_from_prv" are exactly the same they can be merged together. The new function is named "mbedtls_pk_set_pubkey_from_prv" and it's placed in "pk.c" because it's common to all key types. The prototype was already in "pk_internal.h" so this was already fine. Until now these functions has been used for non-opaque context, but given the recent changes to PK, this new function can also be used to handle opaque context in the same way (i.e. exporting its public key and keping it in the PK context). Signed-off-by: Valerio Setti <vsetti@baylibre.com>

Allow the function to be able to return the public key in all the scenarios supported by PK: - If the public key is already available in the PK context, then simply memcpy() it. - If only the private key is available, then export the public key from the private one and then memcpy() it. Advantages are: - The user doesn't need to worry about filling the public part before calling this function. - It allows the function to work also with opaque keys. This functionality wasn't supported before, so it's an improvement that comes for free. The only side effect of this change is that the first parameter of mbedtls_pk_write_pubkey_psa (i.e. mbedtls_pk_context) cannot be const because otherwise it wouldn't be possible to pass it to "mbedtls_pk_set_pubkey_from_prv" without some dirty casting. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

- replace USE_PSA_INIT/USE_PSA_DONE, with PSA_INIT/PSA_DONE - use mbedtls_pk_get_key_type() to get the type of key being tested instead of the temporary solution that was added before when this function was not yet available. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

…e key For the EC keys this is already happening in "pk_parse_key_sec1_der()" even though the procedure is a bit different in that case. For SEC1 the key might or might not be present on the parsed buffer, so it might or might not be derived form the private counterpart. For RSA keys there is no such possibility: the public key should always be exported from the private key. The point here is that at the end of the "mbedtls_pk_parse_key()" we want private and public fields of the PK context to be filled for all the supported key types. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

In this way both private and public key files are parsed and tested with the same test function. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

Since we're going to add new test scenarios for "mbedtls_pk_write_pubkey_psa()" where the PK context is generated in a different way than parsing a key file, we rename "pk_write_pubkey_psa" to "pk_write_pubkey_psa_from_file" to differentiate it from the new test functions that will be added in following commits. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

…write_pubkey_psa_from_file() This will become handy in follow up commits where the same core test function will be used elsewhere. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

PK returns "int" not "psa_status_t" so the return code should have been "0" and not "PSA_SUCCESS". Signed-off-by: Valerio Setti <vsetti@baylibre.com>

After the PK context has been initialized it must be tested (obviously), the same as in "pk_write_pubkey_psa_from_file", but this part was missing. This commit fixes this. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

Add a previously missing check that caused some tests to erroneously fail when "pk->pub_raw_len = 0" and also the current output buffer len was set to 0. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

…_psa Partially reverts commit 63660b7 for what concerns with the following: - keep "const" on the PK parameter being passed in input to the function; - do not try to complete the PK context if the public key is missing. The rationale here is that this is just a debug function meant to get some data from the PK context. It's the caller responsibility to properly fill the PK context before calling it. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

Add a call to "mbedtls_pk_set_pubkey_from_prv" from "mbedtls_pk_wrap_psa" in order to set the public key in PK context when the PSA key pair is wrapped. This aligns "mbedtls_pk_wrap_psa" to all other current ways of generating a PK context from a key pair: "mbedtls_pk_copy_from_psa" and "mbedtls_pk_parse_keyfile". Signed-off-by: Valerio Setti <vsetti@baylibre.com>

This helps reading the ".data" file so that it's immediately clear if a certain file being parsed is supposed to contain a private or public key. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

This completes the testing of mbedtls_pk_write_pubkey_psa() for all possible scenarios. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

This is not needed at all by this PR, but it required to make the CI happy. It's just updating the reference of framework to the current HEAD of main branch. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

valeriosetti · 2025-12-11T17:54:28Z

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

mpg

LGTM, thanks!

valeriosetti added this to Roadmap pull requests (new board) Nov 19, 2025

github-project-automation bot moved this to In Development in Roadmap pull requests (new board) Nov 19, 2025

valeriosetti mentioned this pull request Nov 19, 2025

Remove use of pk_debug() Mbed-TLS/mbedtls#10517

Open

5 tasks

valeriosetti force-pushed the issue529 branch from a9520f0 to d0aa1bd Compare November 20, 2025 14:25

valeriosetti removed the needs-ci Needs to pass CI tests label Nov 20, 2025

bjwtaylor previously approved these changes Nov 21, 2025

View reviewed changes

mpg requested changes Dec 3, 2025

View reviewed changes

valeriosetti dismissed bjwtaylor’s stale review via a9f7d9e December 3, 2025 22:47

valeriosetti removed the needs-review Every commit must be reviewed by at least two team members label Dec 3, 2025

valeriosetti added 3 commits December 4, 2025 09:47

drivers: pk: add mbedtls_pk_write_pubkey_psa()

259cdf3

Add function implementation, prototype and documentation. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

tests: scripts: add tests for mbedtls_pk_write_pubkey_psa()

0ca9ba9

Signed-off-by: Valerio Setti <vsetti@baylibre.com>

changelog: add note for mbedtls_pk_write_pubkey_psa()

dc202d3

Signed-off-by: Valerio Setti <vsetti@baylibre.com>

valeriosetti force-pushed the issue529 branch from a9f7d9e to 06cd968 Compare December 4, 2025 08:47

valeriosetti added 8 commits December 4, 2025 13:12

tests: pkwrite: extend pk_write_pubkey_psa() to test also private keys

1896adf

In this way both private and public key files are parsed and tested with the same test function. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

tests: pkwrite: move testing part of pk_write_pubkey_psa() out of pk_…

47e353d

…write_pubkey_psa_from_file() This will become handy in follow up commits where the same core test function will be used elsewhere. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

valeriosetti force-pushed the issue529 branch 2 times, most recently from 64a93fb to 9533816 Compare December 4, 2025 15:16

valeriosetti removed the needs-reviewer This PR needs someone to pick it up for review label Dec 4, 2025

gilles-peskine-arm removed their request for review December 11, 2025 10:12

bjwtaylor previously approved these changes Dec 11, 2025

View reviewed changes

github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Dec 11, 2025

valeriosetti added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members labels Dec 11, 2025

valeriosetti enabled auto-merge December 11, 2025 15:29

valeriosetti added 8 commits December 11, 2025 18:53

drivers: pk: fix return code in mbedtls_pk_set_pubkey_from_prv()

0943b9d

PK returns "int" not "psa_status_t" so the return code should have been "0" and not "PSA_SUCCESS". Signed-off-by: Valerio Setti <vsetti@baylibre.com>

tests: pkwrite: fix checks in test_pk_write_pubkey_psa

e21b32d

Add a previously missing check that caused some tests to erroneously fail when "pk->pub_raw_len = 0" and also the current output buffer len was set to 0. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

tests: pkwrite: add enum to specify the type of key file being parsed

88f8eef

This helps reading the ".data" file so that it's immediately clear if a certain file being parsed is supposed to contain a private or public key. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

tests: pk: test mbedtls_pk_write_pubkey_psa() on unpopulated PK context

e3faa6e

This completes the testing of mbedtls_pk_write_pubkey_psa() for all possible scenarios. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

framework: update reference

3cdd524

This is not needed at all by this PR, but it required to make the CI happy. It's just updating the reference of framework to the current HEAD of main branch. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

valeriosetti dismissed stale reviews from bjwtaylor and mpg via 3cdd524 December 11, 2025 17:53

valeriosetti force-pushed the issue529 branch from 62e8557 to 3cdd524 Compare December 11, 2025 17:53

valeriosetti added needs-review Every commit must be reviewed by at least two team members and removed approved Design and code approved - may be waiting for CI or backports labels Dec 11, 2025

mpg approved these changes Dec 12, 2025

View reviewed changes

valeriosetti requested a review from bjwtaylor December 12, 2025 13:40

bjwtaylor approved these changes Dec 12, 2025

View reviewed changes

valeriosetti added this pull request to the merge queue Dec 12, 2025

valeriosetti added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members labels Dec 12, 2025

Merged via the queue into Mbed-TLS:development with commit 8fe80c3 Dec 12, 2025
3 checks passed

github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) Dec 12, 2025

valeriosetti mentioned this pull request Dec 10, 2025

Inline get bitlen #575

Open

4 tasks

PK: add mbedtls_pk_write_pubkey_psa() #577

PK: add mbedtls_pk_write_pubkey_psa() #577

Uh oh!

Conversation

valeriosetti commented Nov 19, 2025 • edited by mpg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR checklist

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valeriosetti commented Dec 4, 2025

Uh oh!

valeriosetti commented Dec 11, 2025

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PK: add `mbedtls_pk_write_pubkey_psa()` #577

PK: add `mbedtls_pk_write_pubkey_psa()` #577

valeriosetti commented Nov 19, 2025 •

edited by mpg

Loading