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.

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

One minor fix #8171

Merged
Merged

Conversation

ronald-cron-arm
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm commented Sep 7, 2023

Description

Fix one minor issue I came across while working on the psa-crypto repository.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

Fix defined but not used warning when
MBEDTLS_USE_PSA_CRYPTO, MBEDTLS_PK_HAVE_RFC8410_CURVES
and MBEDTLS_PK_HAVE_ECC_KEYS are defined but not
MBEDTLS_PEM_WRITE_C.

Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm ronald-cron-arm added component-psa PSA keystore/dispatch layer (storage, drivers, …) enhancement labels Sep 7, 2023
@ronald-cron-arm ronald-cron-arm added the needs-ci Needs to pass CI tests label Sep 7, 2023
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

The HKDF change is wrong.

Ok for the pkwrite change.

@@ -67,20 +67,26 @@

#if defined(PSA_WANT_ALG_HKDF)
#if !defined(MBEDTLS_PSA_ACCEL_ALG_HKDF)
#define MBEDTLS_HKDF_C
Copy link
Contributor

Choose a reason for hiding this comment

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

No: PSA has its own implementation of HKDF, separate from hkdf.c. The existing code here 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.

This has come up before and we really should have put a comment there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I was indeed wondering why it was not caught by the PSA crypto tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some comments instead.

Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm ronald-cron-arm changed the title Two minor fixes One minor fix Sep 7, 2023
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports needs-backports Backports are missing or are pending review and approval. labels Sep 7, 2023
@ronald-cron-arm ronald-cron-arm removed the needs-backports Backports are missing or are pending review and approval. label Sep 7, 2023
@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Sep 12, 2023
Merged via the queue into Mbed-TLS:development with commit ad2f351 Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-psa PSA keystore/dispatch layer (storage, drivers, …) enhancement needs-ci Needs to pass CI tests
Projects
Development

Successfully merging this pull request may close these issues.

3 participants