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

TinyCrypt deprecation - library's usage removal part 2 (bluetooth) #79931

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

TinyCrypt deprecation - library's usage removal part 2 (bluetooth) #79931

wants to merge 13 commits into from
+587 −1,219

Conversation

valeriosetti
Copy link
Collaborator

@valeriosetti valeriosetti commented Oct 16, 2024 •
edited
Loading

This is the second step toward the removal of TinyCrypt library usage from the Zephyr's codebase. This is the continuation of #79566 and #79653.
In particular this PR takes care of removing TinyCrypt usage from the bluetooth subsystem.

Important note

Some commits are taken from the following PRs (i.e. this PR depends on):

To be done

  • fix bsim failures with tests/bsim/bluetooth/audio/test_scripts/cap_broadcast_ac_14.sh
  • merge all prerequisite PRs
  • reorganize commits that belong to this PR (now the split into commits is not great)

@valeriosetti valeriosetti mentioned this pull request Oct 16, 2024
Merged
@valeriosetti valeriosetti force-pushed the deprecate-tinycrypt-part2 branch 5 times, most recently from 9f3f671 to 98b97ea Compare October 18, 2024 04:30
@valeriosetti valeriosetti force-pushed the deprecate-tinycrypt-part2 branch 5 times, most recently from 946f924 to c9cf39d Compare October 23, 2024 08:26
@rettichschnidi
Copy link
Contributor

Any chance this will be ready for Zephyr 4.0?

@valeriosetti
Copy link
Collaborator Author

Any chance this will be ready for Zephyr 4.0?

As far as I know codefreeze is today for 4.0 and I still need to resolve some failures in this PR + it also depends on 2 other PRs which should be merged before this one. I'm sorry but I think the answer is "no"

@valeriosetti valeriosetti force-pushed the deprecate-tinycrypt-part2 branch 6 times, most recently from cf8d840 to 91c5e79 Compare October 30, 2024 05:16
@PavelVPV PavelVPV self-requested a review October 30, 2024 06:31
@PavelVPV PavelVPV mentioned this pull request Oct 30, 2024
Merged
@dkalowsk dkalowsk mentioned this pull request Oct 30, 2024
Open
5 tasks
@mmahadevan108
Copy link
Collaborator

@valeriosetti , do we need both Part 1 and Part 2 to be merged into 4.0.
Can we merge only Part 1 if Part 2 is not ready in time before the release?

@jhedberg
Copy link
Member

@valeriosetti , do we need both Part 1 and Part 2 to be merged into 4.0. Can we merge only Part 1 if Part 2 is not ready in time before the release?

The challenge as I see it is this part from https://docs.zephyrproject.org/latest/develop/api/api_lifecycle.html#deprecated

Code using the deprecated API needs to be modified to remove usage of said API

Which means that we can't make a release and still have code in the upstream tree using a deprecated API.

@valeriosetti
Copy link
Collaborator Author

@valeriosetti , do we need both Part 1 and Part 2 to be merged into 4.0. Can we merge only Part 1 if Part 2 is not ready in time before the release?

While part 1 is ready to be merged, part 2 absolutely not:

Which means that we can't make a release and still have code in the upstream tree using a deprecated API.

That's what I thought at the beginning of this activity as well, but then I've been asked to keep the following support for TinyCrypt in the codebase for at least the next 2 releases:

I'm not an expert of BT, but based on the number of changes I've done so far in this PR to make the CI happy, it seems that BT is way more affected by TinyCrypt removal than other subsystems. So I agree with @alxelax that giving the user some time to deal with TinyCrypt deprecation wouldn't be that bad. But this is my feeling, of course, so I'll be glad to hear what's reviewers opinion about this :)

@PavelVPV PavelVPV mentioned this pull request Oct 31, 2024
Closed
@Thalley Thalley mentioned this pull request Nov 1, 2024
Closed
This was referenced Nov 4, 2024
Open
Open
@valeriosetti
bt: use PSA for HCI ECC and rename BT_TINYCRYPT_ECC
23662a5 
As part of the deprecation process of TinyCrypt in Zephyr codebase
(zephyrproject-rtos#79566) this commit sets PSA Crypto API as the default library
to perform ECC-DH in HCI, replacing TinyCrypt. Therefore the
symbol BT_TINYCRYPT_ECC is renamed as BT_SEND_ECC_EMULATION.
Rreference in samples/tests are also fixed.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
bt-mesh: remove BT_MESH_USES_TINYCRYPT
b0c61a2 
As part of the deprecation process of TinyCrypt in Zephyr codebase
(zephyrproject-rtos#79566) this commit removes TinyCrypt usage from BT mesh and
also the related CONFIG_BT_MESH_USES_TINYCRYPT symbol and it sets
PSA Crypto APIs as the default library for crypto operations.
Tests are also updated in this commit.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
bt: change dependencies for BT_RPA
3355dd2 
As part of the deprecation process of TinyCrypt in Zephyr codebase
(zephyrproject-rtos#79566) this commit modified the dependencies of BT_RPA. Instead
of selecting TinyCrypt, it relies on BT_HOST_CRYPTO or
BT_CTLR_CRYPTO. This helps both in the deprecation process of
TinyCrypt, but also this is more correct dependency since these
2 symbols are extensively used in the rpa.c source code.

Signed-off-by: Valerio Setti <[email protected]>
@tomi-font
Copy link
Collaborator

As discussed in the Release meeting today, we want to consider the deprecation of Tinycrypt a release blocker item for 4.0. That would mean Part1, Part2, and the mbedTLS changes would all need to be merged in.

@tomi-font @valeriosetti any objection to this course of action? The point was raised that Tinycrypt has been "just about to be deprecated" for 5 years now, but never seems to make it. We'd like to make it happen now.

No objection from me. I'll review this and the other needed PRs today.

@valeriosetti
samples: bt: some RAM footprint reductions
e5cf4b7 
As part of the work of deprecating TinyCrypt in favor of
PSA crypto API, this commit fixes some kconfigs in order to
reduce the RAM footprint. In all involved samples RAM usage
was extremely high even with TinyCrypt (usually > 98%) and
of course it got overflowed once TinyCrypt got replaced by
Mbed TLS's PSA Crypto API.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
tests: bluetooth: replace TinyCrypt with PSA in unit tests
8e6c022 
Replace all references to TinyCrypt with PSA Crypto API in BT unit
tests.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
bt: audio: comment failing test case
7d82ba6 
Comment out "Execute_AC_14 32_2_1" in "cap_broadcast_ac_14.sh"
because it's failing. The reason seems to be related to some
timing issue, but it has not be correctly identified yet.
We comment it out for the sake of not delaying upcoming PRs.
It will be re-enabled once the proper solution is found.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
tests: bt: add Mbed TLS headers to the build
cde6b95 
Include Mbed TLS headers to the build system. This is required
because these tests do not follow the "standard build pattern"
of Mbed TLS in Zephyr, otherwise include files would be already
available after the library has been linked. In these examples
some BT source files and Kconfigs are manually added to the
CmakeLists.txt file bypassing the standard library build pattern,
so Mbed TLS headers must also be added manually.

Signed-off-by: Valerio Setti <[email protected]>
alxelax
alxelax reviewed Nov 6, 2024
View reviewed changes
subsys/bluetooth/mesh/Kconfig
select BT_HOST_CCM
help
Use TinyCrypt library to perform crypto operations.

config BT_MESH_USES_MBEDTLS_PSA
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix help description. This is not relevant anymore.

subsys/bluetooth/mesh/Kconfig
config BT_MESH_USES_MBEDTLS_PSA
bool "mbed TLS PSA [EXPERIMENTAL]"
select EXPERIMENTAL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did anybody run mesh with mbedtls on real devices?
Also, to remove experimental tag bsim should use pure mbedtls without its emulation.

It looks to me like, nobody run mesh with mbedtls PSA without technological stubs, but EXPERIMENTAL tag is removed.
Kind of disagree with this.

subsys/bluetooth/mesh/Kconfig
help
Crypto library selection for mesh security.

config BT_MESH_USES_TINYCRYPT
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this should be moved back and marked as [DEPRECATED]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, and a release notes should be added that this option is deprecated.

@jhedberg jhedberg added this to the v4.0.0 milestone Nov 6, 2024
@valeriosetti
tests: bt: increase PSA key slots count when necessary
644364c 
Increase the number of key slots in the PSA Crypto core
for some tests using more keys than the default (16).

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti valeriosetti force-pushed the deprecate-tinycrypt-part2 branch 2 times, most recently from d58375b to 41cf00c Compare November 6, 2024 13:24
@valeriosetti
tests: bt: enable entropy sources where missing
3933503 
Moving from TinyCrypt to PSA Crypto API caused an entropy source
to be always required, so this commit adds it in tests where
necessary.

Signed-off-by: Valerio Setti <[email protected]>
ceolin added a commit to ceolin/zephyr that referenced this pull request Nov 7, 2024
@ceolin
Revert "tinycrypt: deprecate the library"
f5b07a8 
This reverts commit 5e225e0.

Based on zephyrproject-rtos#79931 and TSC discussions, it was decided that TinyCrypt
will be deprecated *AFTER* 4.0.

Signed-off-by: Flavio Ceolin <[email protected]>
@ceolin ceolin mentioned this pull request Nov 7, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alwa-nordic alwa-nordic alwa-nordic left review comments

@alxelax alxelax alxelax left review comments

@hermabe hermabe hermabe left review comments

@jhedberg jhedberg jhedberg left review comments

@PavelVPV PavelVPV PavelVPV left review comments

@Vudentz Vudentz Awaiting requested review from Vudentz Vudentz is a code owner

@asbjornsabo asbjornsabo Awaiting requested review from asbjornsabo

@Casper-Bonde-Bose Casper-Bonde-Bose Awaiting requested review from Casper-Bonde-Bose

@kartben kartben Awaiting requested review from kartben

@MariuszSkamra MariuszSkamra Awaiting requested review from MariuszSkamra

@nashif nashif Awaiting requested review from nashif

@rugeGerritsen rugeGerritsen Awaiting requested review from rugeGerritsen

@sjanc sjanc Awaiting requested review from sjanc

@Thalley Thalley Awaiting requested review from Thalley

@theob-pro theob-pro Awaiting requested review from theob-pro

At least 2 approving reviews are required to merge this pull request.

Assignees

@jhedberg jhedberg

@alwa-nordic alwa-nordic

Labels
DNM This PR should not be merged (Do Not Merge) manifest manifest-mbedtls TSC Topics that need TSC discussion
Projects
TSC Attention Needed
Status: Todo
Milestone
v4.0.0
Development

Successfully merging this pull request may close these issues.