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

Jump to bottom

psa_asymmetric_encrypt() doesn't work with opaque driver #8700

Merged
merged 8 commits into from
Jan 22, 2024
Merged

psa_asymmetric_encrypt() doesn't work with opaque driver #8700

merged 8 commits into from
Jan 22, 2024

Conversation

valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented Jan 15, 2024 •
edited by minosgalanakis
Loading

Description

This PR enabled asymmetric encryption/decryption with opaque keys.

Resolves #8461

PR checklist

  • changelog provided
  • backport not required
  • tests provided

@valeriosetti
test_suite_psa_crypto: test asymmetric encryption/decryption also wit…
f202c29 
…h opaque keys

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
psa_crypto: allow asymmetric encryption/decryption also with opaque keys
5bb454a 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Jan 15, 2024
@valeriosetti valeriosetti self-assigned this Jan 15, 2024
@valeriosetti
test_driver_key_management: make opaque [un]wrapping functions public
66a827f 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
Copy link
Contributor Author

Note to reviewers

In addition to allowing usage of opaque keys with psa_asymmetric_[en|de]crypt, this PR also fixes limitations in test_driver_asymmetric_[en|de]cryption. So far only a single test is performed in test_suite_psa_crypto.data with opaque key. However if the overall pattern looks reasonably good, I can add more test for it (different key sizes, failing cases, etc)

@valeriosetti
test_driver_asymmetric_encryption: implement opaque [en/de]cryption f…
62b6f10 
…unctions

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
test_suite_psa_crypto: revert known failing checks for [en|de]cryptio…
4860a6c 
…n with opaque keys

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@gilles-peskine-arm
Copy link
Contributor

Thanks for adding tests!

I think one positive test case (for each API function) is enough. We want to validate that the dispatch code has a case for opaque keys. That's a non-regression test for the current bug fix, and it's the thing that's most likely to go wrong. In an ideal world we'd also test that parameters and errors are correctly propagated, but there's not much risk that they would work correctly for transparent drivers but not for opaque drivers, so it's very low priority.

@minosgalanakis minosgalanakis removed the needs-ci Needs to pass CI tests label Jan 16, 2024
minosgalanakis
minosgalanakis previously approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM.

I do not feel a changelog is needed in this case. This pr is fixing the opaque driver support.

Since @gilles-peskine-arm is happy with adding the single test at this stage, we are good to go.

@gilles-peskine-arm
Copy link
Contributor

I do not feel a changelog is needed in this case. This pr is fixing the opaque driver support.

That's a library bug (or missing feature), which has been reported by a user, so we do need a changelog entry.

@valeriosetti
add changelog
584dc80 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
minosgalanakis
minosgalanakis previously approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-daubney-arm tom-daubney-arm self-requested a review January 17, 2024 13:03
@tom-daubney-arm tom-daubney-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jan 17, 2024
tom-daubney-arm
tom-daubney-arm requested changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@tom-daubney-arm tom-daubney-arm left a comment

Choose a reason for hiding this comment

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

Small typo in the changelog and a question in the comments.

ChangeLog.d/8461.txt Outdated
@@ -0,0 +1,4 @@
Bugfix
* Fix unsupported PSA asymmetric encryption and dectryption
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Fix unsupported PSA asymmetric encryption and dectryption
* Fix unsupported PSA asymmetric encryption and decryption

tests/src/drivers/test_driver_key_management.c Outdated
@@ -125,7 +125,7 @@ static size_t mbedtls_test_opaque_get_base_size()
* The argument wrapped_key_buffer_length is filled with the wrapped
* key_size on success.
* */
static psa_status_t mbedtls_test_opaque_wrap_key(
psa_status_t mbedtls_test_opaque_wrap_key(
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not totally sure why the wrap function is no longer static. It is not used in the tests I don't think. Is this just to keep it consistent with the unwrap function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you are totally right. I probably moved both functions from static to public, but actually only the unwrap function is needed for opaque drivers. I'm reverting the change for the wrapping key.

@valeriosetti
changelog: fix typo
43ff242 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
test_driver_key_management: keep mbedtls_test_opaque_wrap_key() private
4f34b15 
Only mbedtls_test_opaque_unwrap_key() is actually needed by other
test drivers to deal with opaque keys. mbedtls_test_opaque_wrap_key()
can be kept private to test_driver_key_management.c.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
tom-daubney-arm
tom-daubney-arm approved these changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@tom-daubney-arm tom-daubney-arm left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. LGTM now.

minosgalanakis
minosgalanakis approved these changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

@valeriosetti valeriosetti mentioned this pull request Jan 22, 2024
Closed
3 tasks
@mpg mpg added this pull request to the merge queue Jan 22, 2024
Merged via the queue into Mbed-TLS:development with commit 34c6e8a Jan 22, 2024
1 check passed
@valeriosetti valeriosetti mentioned this pull request Mar 18, 2024
Merged
3 tasks
mpg added a commit to mpg/mbedtls that referenced this pull request Apr 9, 2024
@mpg
Update of opaque asymmetric encrypt/decrypt
7494467 
Mbed-TLS#8700 merged in the meantime.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg added a commit to mpg/mbedtls that referenced this pull request Apr 12, 2024
@mpg
Update of opaque asymmetric encrypt/decrypt
dde1abd 
Mbed-TLS#8700 merged in the meantime.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@valeriosetti valeriosetti deleted the issue8461 branch December 6, 2024 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minosgalanakis minosgalanakis minosgalanakis approved these changes

@tom-daubney-arm tom-daubney-arm tom-daubney-arm approved these changes

Assignees

@valeriosetti valeriosetti

Labels
needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
No open projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

psa_asymmetric_encrypt() doesn't work with opaque driver
5 participants
@valeriosetti @gilles-peskine-arm @minosgalanakis @tom-daubney-arm @mpg