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

MBEDTLS_POLY1305_C and MBEDTLS_CHACHA20_C are needed when PSA_WANT_ALG_CHACHA20_POLY1305 is defined #5949

Merged
merged 1 commit into from
Aug 5, 2022
Merged

MBEDTLS_POLY1305_C and MBEDTLS_CHACHA20_C are needed when PSA_WANT_ALG_CHACHA20_POLY1305 is defined #5949

merged 1 commit into from
Aug 5, 2022

Conversation

Summer-ARM
Copy link
Contributor

@Summer-ARM Summer-ARM commented Jun 17, 2022 •
edited by daverodgman
Loading

MBEDTLS_POLY1305_C is needed when PSA_WANT_ALG_CHACHA20_POLY1305 is defined

Signed-off-by: Summer Qin [email protected]

Status

READY

Todos

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

@Summer-ARM Summer-ARM force-pushed the mbedtls-psa-crypto-config branch from e015282 to aed962b Compare June 17, 2022 06:16
@Summer-ARM Summer-ARM changed the title MBEDTLS_CHACHAPOLY_C is needed when PSA_WANT_ALG_CHACHA20_POLY1305 is… MBEDTLS_POLY1305_C is needed when PSA_WANT_ALG_CHACHA20_POLY1305 is defined Jun 17, 2022
@bensze01 bensze01 self-assigned this Jun 22, 2022
gilles-peskine-arm
gilles-peskine-arm reviewed Jun 28, 2022
View reviewed changes
include/mbedtls/config_psa.h
@@ -436,6 +436,7 @@ extern "C" {
#if !defined(MBEDTLS_PSA_ACCEL_ALG_CHACHA20_POLY1305)
#if defined(PSA_WANT_KEY_TYPE_CHACHA20)
#define MBEDTLS_CHACHAPOLY_C
#define MBEDTLS_POLY1305_C
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

MBEDTLS_CHACHA20_C is also missing here, can you please add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@gilles-peskine-arm gilles-peskine-arm added needs-work needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. needs-reviewer This PR needs someone to pick it up for review component-psa PSA keystore/dispatch layer (storage, drivers, …) and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jun 28, 2022
@Summer-ARM Summer-ARM force-pushed the mbedtls-psa-crypto-config branch from aed962b to bdb7005 Compare June 28, 2022 09:55
@Summer-ARM
Add MBEDTLS_POLY1305_C and MBEDTLS_CHACHA20_C
9f2596f 
MBEDTLS_POLY1305_C and MBEDTLS_CHACHA20_C are needed
when PSA_WANT_ALG_CHACHA20_POLY1305 is defined

Signed-off-by: Summer Qin <[email protected]>
@Summer-ARM Summer-ARM force-pushed the mbedtls-psa-crypto-config branch from bdb7005 to 9f2596f Compare June 28, 2022 09:58
@Summer-ARM Summer-ARM changed the title MBEDTLS_POLY1305_C is needed when PSA_WANT_ALG_CHACHA20_POLY1305 is defined MBEDTLS_POLY1305_C and MBEDTLS_CHACHA20_C are needed when PSA_WANT_ALG_CHACHA20_POLY1305 is defined Jun 28, 2022
@daverodgman daverodgman added bug needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon and removed needs-work labels Jul 4, 2022
@AndrzejKurek AndrzejKurek self-requested a review July 8, 2022 12:33
@AndrzejKurek
Copy link
Contributor

Config change to reproduce the problem:

--- a/include/mbedtls/mbedtls_config.h
+++ b/include/mbedtls/mbedtls_config.h
@@ -1802,7 +1802,7 @@
  * This feature is still experimental and is not ready for production since
  * it is not completed.
  */
-//#define MBEDTLS_PSA_CRYPTO_CONFIG
+#define MBEDTLS_PSA_CRYPTO_CONFIG
 
 /**
  * \def MBEDTLS_VERSION_FEATURES
@@ -2140,7 +2140,7 @@
  *
  * Module:  library/chacha20.c
  */
-#define MBEDTLS_CHACHA20_C
+//#define MBEDTLS_CHACHA20_C
 
 /**
  * \def MBEDTLS_CHACHAPOLY_C
@@ -2151,7 +2151,7 @@
  *
  * This module requires: MBEDTLS_CHACHA20_C, MBEDTLS_POLY1305_C
  */
-#define MBEDTLS_CHACHAPOLY_C
+//#define MBEDTLS_CHACHAPOLY_C
 
 /**
  * \def MBEDTLS_CIPHER_C
@@ -2676,7 +2676,7 @@
  * Module:  library/poly1305.c
  * Caller:  library/chachapoly.c
  */
-#define MBEDTLS_POLY1305_C
+//#define MBEDTLS_POLY1305_C

make -> error: #error "MBEDTLS_CHACHAPOLY_C defined, but not all prerequisites"

AndrzejKurek
AndrzejKurek approved these changes Jul 11, 2022
View reviewed changes
Copy link
Contributor

@AndrzejKurek AndrzejKurek 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.

Looks good.
With MBEDTLS_PSA_ACCEL_KEY_TYPE_CHACHA20 and MBEDTLS_CHACHA20_C will also be undefined, so the additional line is needed.
I don't think that a changelog entry is needed, as it seems that it's more of an internal bug in dependencies and if someone defined PSA_WANT_ALG_CHACHA20_POLY1305, MBEDTLS_CHACHA20_C and MBEDTLS_POLY1305_C was probably expected.

@bensze01 bensze01 removed their assignment Jul 19, 2022
paul-elliott-arm
paul-elliott-arm approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-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

@paul-elliott-arm paul-elliott-arm added the approved Design and code approved - may be waiting for CI or backports label Jul 22, 2022
@daverodgman daverodgman removed the needs-review Every commit must be reviewed by at least two team members, label Jul 29, 2022
@daverodgman daverodgman removed the needs-reviewer This PR needs someone to pick it up for review label Jul 29, 2022
@daverodgman daverodgman mentioned this pull request Jul 29, 2022
Merged
@gilles-peskine-arm gilles-peskine-arm merged commit 5740ff5 into Mbed-TLS:development Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm left review comments

@paul-elliott-arm paul-elliott-arm paul-elliott-arm approved these changes

@AndrzejKurek AndrzejKurek AndrzejKurek approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports bug component-psa PSA keystore/dispatch layer (storage, drivers, …) needs-backports Backports are missing or are pending review and approval. priority-high High priority - will be reviewed soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Summer-ARM @AndrzejKurek @gilles-peskine-arm @paul-elliott-arm @daverodgman @bensze01