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

mbed TLS: Inconsistent Kconfig option names #22421

Closed
mrfuchs opened this issue Feb 3, 2020 · 2 comments · Fixed by #22503
Closed

mbed TLS: Inconsistent Kconfig option names #22421

mrfuchs opened this issue Feb 3, 2020 · 2 comments · Fixed by #22503
Assignees
Labels
Enhancement Changes/Updates/Additions to existing features

Comments

@mrfuchs
Copy link
Contributor

mrfuchs commented Feb 3, 2020

While working on #22407 I noticed some naming inconsistencies in mbed TLS Kconfig options:

MBEDTLS_CCM_C was translated to CONFIG_MBEDTLS_CIPHER_CCM_ENABLED, but
MBEDTLS_GCM_C was translated to CONFIG_MBEDTLS_CIPHER_MODE_GCM_ENABLED, while
MBEDTLS_CIPHER_MODE_CBC was translated to CONFIG_MBEDTLS_CIPHER_CBC_ENABLED, but MBEDTLS_CIPHER_MODE_XTS was translated to CONFIG_MBEDTLS_CIPHER_MODE_XTS_ENABLED.

Also, some mbed TLS cipher mode options cannot be set by Kconfig, like MBEDTLS_CIPHER_MODE_CFB, MBEDTLS_CIPHER_MODE_OFB and MBEDTLS_CIPHER_MODE_CTR. That's probably the reason why the current version of the mbed TLS shim driver does not support AES-CTR mode:

if (mode != CRYPTO_CIPHER_MODE_CCM &&
mode != CRYPTO_CIPHER_MODE_CBC &&
mode != CRYPTO_CIPHER_MODE_ECB) {
LOG_ERR("Unsupported mode");

case CRYPTO_CIPHER_MODE_CTR:
break;

@mrfuchs mrfuchs added the bug The issue is a bug, or the PR is fixing a bug label Feb 3, 2020
@jhedberg jhedberg added priority: low Low impact/importance bug Enhancement Changes/Updates/Additions to existing features and removed bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug labels Feb 4, 2020
@rlubos
Copy link
Contributor

rlubos commented Feb 5, 2020

I could either prefix each Kconfig with MBEDTLS_CIPHER_MODE or follow the mbedTLS convention (still, I'd prefer to keep the full MBEDTLS_CIPHER prefix). Will the following work for you?

mbedTLS Kconfig
MBEDTLS_CCM_C CONFIG_MBEDTLS_CIPHER_CCM_ENABLED
MBEDTLS_GCM_C CONFIG_MBEDTLS_CIPHER_GCM_ENABLED
MBEDTLS_CIPHER_MODE_CBC CONFIG_MBEDTLS_CIPHER_MODE_CBC_ENABLED
MBEDTLS_CIPHER_MODE_XTS CONFIG_MBEDTLS_CIPHER_MODE_XTS_ENABLED

As for the Kconfig mbedTLS options - everyone is free the extend the Kconfig option list per his/hers requirements. The original file was created to cover the functionalities needed by the TLS sockets implementation and is gradually updated with new features. Feel free to contribute if you find something missing.

@mrfuchs
Copy link
Contributor Author

mrfuchs commented Feb 5, 2020

Looks good. I'm fine with the proposed naming fixes.
I agree that gradually adding required options makes sense.

rlubos added a commit to rlubos/zephyr that referenced this issue May 7, 2020
Update mbedTLS commit along with the following fixes:

* Fix naming inconsistencies in some cipher modes, to match core mbedTLS
  configs
* Add Kconfig to enable CTR cipher mode

Fixes zephyrproject-rtos#22421

Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
galak pushed a commit that referenced this issue May 8, 2020
Update mbedTLS commit along with the following fixes:

* Fix naming inconsistencies in some cipher modes, to match core mbedTLS
  configs
* Add Kconfig to enable CTR cipher mode

Fixes #22421

Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
hakehuang pushed a commit to hakehuang/zephyr that referenced this issue Jun 20, 2020
Update mbedTLS commit along with the following fixes:

* Fix naming inconsistencies in some cipher modes, to match core mbedTLS
  configs
* Add Kconfig to enable CTR cipher mode

Fixes zephyrproject-rtos#22421

Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants