modules: mbedtls: make key exchange Kconfigs depend on, not select #89200
Conversation
github-actions
bot
requested review from
ceolin,
d3zd3z,
ithinuel,
valeriosetti and
wearyzen
tejlmand
left a comment
tejlmand
left a comment
There was a problem hiding this comment.
depends on is generally recommended over select.
See: https://docs.zephyrproject.org/latest/build/kconfig/tips.html#select-pitfalls
So approving.
tomi-font
force-pushed
the
mbedtls_fix_key_exchange_config
branch
from
c97368c to
11e8dd5
Compare
github-actions
bot
added
area: Networking
area: Samples
github-actions
bot
requested review from
MaochenWang1,
jukkar,
kartben,
krish2718,
nashif,
pdgendt,
rlubos,
ssharks and
tbursztyka
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
modules/mbedtls/Kconfig.mbedtls
Outdated
| select MBEDTLS_PKCS1_V15 | ||
| select MBEDTLS_PKCS1_V21 | ||
| def_bool y | ||
| depends on MBEDTLS_RSA_C && MBEDTLS_PKCS1_V15 && MBEDTLS_PKCS1_V21 |
There was a problem hiding this comment.
Based on this comment I don´t know if MBEDTLS_RSA_FULL still makes sense. It was OK when it was selecting all the other RSA related features, but now it makes less sense IMO. Since MBEDTLS_PKCS1_V15 and MBEDTLS_PKCS1_V21 are already gated by MBEDTLS_RSA_C we can just use
depends on MBEDTLS_PKCS1_V15 || MBEDTLS_PKCS1_V21
to express a dependency on legacy RSA support. Wdyt?
modules/mbedtls/Kconfig.mbedtls
Outdated
| config MBEDTLS_SOME_AEAD_CIPHER_ENABLED | ||
| bool | ||
| default y | ||
| def_bool y |
There was a problem hiding this comment.
Minor comment.
While the change is technically OK, the scope of the commit is different so it would have been better to place it in a separate commits IMO. The same holds also for MBEDTLS_SOME_CIPHER_ENABLED and MBEDTLS_PSA_CRYPTO_CLIENT.
valeriosetti
left a comment
valeriosetti
left a comment
There was a problem hiding this comment.
Although I agree that depends on is to be preferred over select for Kconfig, I think we should draw a line between what the user/other subsystems need to imply/select and what should be automatically inferred by Kconfig.mbedtls.
Having Kconfigs as depends on instead of select-ed allows for maximum configurability, but OTOH the user/subsystem willing to use some key exchange will have to manually select all the required algorithms and key types. This is OK to me, but I've seen people complaining about this kind of complexity during meetings, so it maybe a bit overkilling.
tomi-font
force-pushed
the
mbedtls_fix_key_exchange_config
branch
from
6598caf to
5d21e9e
Compare
Turn the MBEDTLS_RSA_FULL selects into depends on. This is how the other MBEDTLS_KEY_EXCHANGE_* Kconfig options are defined. This is done to avoid circular dependencies. At the same time update uses of the affected MBEDTLS_KEY_EXCHANGE_* Kconfig options to enable/disable the dependencies which used to be automatically handled. Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
Update Zephyr fork of MCUboot to revision:
990b1fcb367e27056b282f183e819964fdbfe907
- f76fba70 boot: bootutil: swap_scratch: Fix issue with bricking
device
- 7253f01c boot: bootutil: Refactor erase functionality to fix
watchdog feeding
- a98bff9f boot: zephyr: kconfig: Fix issues and re-order
- 1b2d261d boot: zephyr: flash_map: Fix unused argument
- 413eb384 boot: zephyr: flash_map: Fix missing include
- 15b36f91 boot: zephyr: kconfig: enable dependencies of Mbed TLS
Kconfig option
- f6e8e88a boot: bootutil: Move erase function location
- 5ef87c79 boot: zephyr: kconfig: Fix BOOT_SWAP_USING_MOVE
description
- bc18d7da boot: boot_serial: Fix issue with CBOR and setting
image state
Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
tomi-font
force-pushed
the
mbedtls_fix_key_exchange_config
branch
from
5d21e9e to
660e183
Compare
|
|
@valeriosetti please revisit this PR, I dismissed your NACK because it was stale. |
I know he is on vacation this week. I don't think he'll oppose in any way to merging this if @d3zd3z or @ceolin approves. |
|
|
||
| config MBEDTLS_PKCS1_V15 | ||
| bool "RSA PKCS1 v1.5" | ||
| default y if UOSCORE || UEDHOC |
There was a problem hiding this comment.
I feel like this UOSCORE logic is an inverting dependencies. Shouldn't UOSCORE select MBEDTLS_PKCS1_V15 ?
I won't block it since there are other symbols following the same logic, but it is looking inverted to me.
There was a problem hiding this comment.
Agreed. @valeriosetti was saying he might take care of changing this.
There was a problem hiding this comment.
Hi, sorry but I was out of office last week. Yes, I started some work to fix this dependency. It's not ready yet, but I hope to finalize it soon.
11 participants
Turn the MBEDTLS_RSA_FULL selects into depends on. This is how the other MBEDTLS_KEY_EXCHANGE_* Kconfig options are defined.
This is done to avoid circular dependencies.
At the same time update uses of the affected MBEDTLS_KEY_EXCHANGE_* Kconfig options to enable/disable the dependencies which used to be automatically handled.