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

Openthread fix crypto psa import key #67473

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Openthread fix crypto psa import key #67473

merged 2 commits into from
Jan 23, 2024

Conversation

canisLupus1313
Copy link
Contributor

According to PSA specification in case of PSA_KEY_TYPE_ECC_KEY_PAIR
function psa_import_key takes private key from key pair as argument.
This commit adds extraction of Private key from ECDSA key pair.

Also removes not needed otPlatCryptoEcdsaGetPublicKey.

@canisLupus1313 canisLupus1313 force-pushed the openthread_fix_crypto_psa_import_key branch from 4d3b831 to 68533f6 Compare January 11, 2024 11:51
@canisLupus1313 canisLupus1313 force-pushed the openthread_fix_crypto_psa_import_key branch from 68533f6 to 8b54d57 Compare January 22, 2024 07:42
@canisLupus1313 canisLupus1313 marked this pull request as ready for review January 22, 2024 07:42
@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 22, 2024 •
edited
Loading

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
openthread zephyrproject-rtos/openthread@4ed44bc zephyrproject-rtos/openthread@00076af (main) zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-openthread DNM This PR should not be merged (Do Not Merge) labels Jan 22, 2024
@canisLupus1313 canisLupus1313 force-pushed the openthread_fix_crypto_psa_import_key branch from 8b54d57 to ea9433f Compare January 22, 2024 07:50
@canisLupus1313 canisLupus1313 force-pushed the openthread_fix_crypto_psa_import_key branch from ea9433f to 2268520 Compare January 22, 2024 08:26
maciejbaczmanski
maciejbaczmanski reviewed Jan 22, 2024
View reviewed changes
Copy link
Member

@maciejbaczmanski maciejbaczmanski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

few more suggestions / questions

modules/openthread/platform/crypto_psa.c Show resolved Hide resolved
modules/openthread/platform/crypto_psa.c Outdated Show resolved Hide resolved
modules/openthread/platform/crypto_psa.c Show resolved Hide resolved
modules/openthread/platform/crypto_psa.c Show resolved Hide resolved
modules/openthread/platform/crypto_psa.c Show resolved Hide resolved
@canisLupus1313 canisLupus1313 force-pushed the openthread_fix_crypto_psa_import_key branch from 2268520 to 9be8885 Compare January 22, 2024 10:15
maciejbaczmanski
maciejbaczmanski previously approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@maciejbaczmanski maciejbaczmanski left a comment

Choose a reason for hiding this comment

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

lgtm

rlubos
rlubos reviewed Jan 22, 2024
View reviewed changes
west.yml Outdated Show resolved Hide resolved
@canisLupus1313
net: openthread: Regular openthread upmerge to 00076af
c908c30 
Regular upmerge.

Signed-off-by: Przemyslaw Bida <[email protected]>
@canisLupus1313
net: openthread: Fix key import in case of ECDSA.
be59a9c 
According to PSA specification in case of `PSA_KEY_TYPE_ECC_KEY_PAIR`
function `psa_import_key` takes private key from key pair as argument.
This commit adds extraction of Private key from ECDSA key pair.

Also removes not needed `otPlatCryptoEcdsaGetPublicKey`.

Signed-off-by: Przemyslaw Bida <[email protected]>
@canisLupus1313 canisLupus1313 force-pushed the openthread_fix_crypto_psa_import_key branch from 9be8885 to be59a9c Compare January 22, 2024 13:35
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Jan 22, 2024
@carlescufi carlescufi merged commit a6184b9 into zephyrproject-rtos:main Jan 23, 2024
21 of 22 checks passed
@Damian-Nordic
Copy link
Collaborator

@canisLupus1313 why did you remove otPlatCryptoEcdsaGetPublicKey function? Now the binary links the weak implementation from crypto_platform.cpp from OpenThread, based on mbedTLS. It's OK if it's not used right now but the platform API requires that it be provided.

@canisLupus1313
Copy link
Contributor Author

@Damian-Nordic otPlatCryptoEcdsaGetPublicKey from plain text key pair is not used anymore in Openthread. The weak implementation was left there to indicate for users that this functionality shouldn't be used. That was the first reason.
Secondly the implementation of the function was never correct (since we haven't used it nobody noticed), the problem with is that even thought the PSA function takes parameters that suggest it should be a key pair it really takes private key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maciejbaczmanski maciejbaczmanski maciejbaczmanski approved these changes

@ArekBalysNordic ArekBalysNordic ArekBalysNordic approved these changes

@rlubos rlubos rlubos approved these changes

@edmont edmont Awaiting requested review from edmont

@mariuszpos mariuszpos Awaiting requested review from mariuszpos

@pdgendt pdgendt Awaiting requested review from pdgendt

Assignees

@rlubos rlubos

Labels
area: OpenThread manifest manifest-openthread
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@canisLupus1313 @zephyrbot @Damian-Nordic @rlubos @ArekBalysNordic @maciejbaczmanski @carlescufi