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

TinyCrypt deprecation - library's usage removal part 1 #79653

Merged

Conversation

valeriosetti
Copy link
Collaborator

@valeriosetti valeriosetti commented Oct 10, 2024

This is the first step toward the removal of TinyCrypt library usage from the Zephyr's codebase. This is the continuation of the initial PR (#79566) that started the deprecation of the TinyCrypt library.

The idea is to do basic/simple removals here and keep BT stuff for a follow-up separated PR.

This PR depends on #79566. Initial commit is taken from there.

@dkalowsk
Copy link
Contributor

@ceolin and @tomi-font isn't this needed for the 4.0 release? AKA we're in RC1 right now, what needs to be done to make this into RC2?

@ceolin
Copy link
Member

ceolin commented Oct 29, 2024

You have to mark the shim driver as deprecated for now and remove it together with the library. The changes in jwt and random can get in.

@ceolin Are you sure about the latter part? I can imagine users currently using TinyCrypt for those may face breaking changes when they are removed (which means as soon as 4.0 if we remove them right away) due to e.g. using TinyCrypt and not Mbed TLS (and being already tight on size constraints). I don't know. Just trying to make sure this is done right.

Giving my 2 cents here while waiting for Flavio's reply. I think the answer is "yes" and the reason is (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

Yep. We have to keep it for external use cases but internally we can remove it.

ceolin
ceolin previously approved these changes Oct 29, 2024
@ceolin
Copy link
Member

ceolin commented Oct 29, 2024

@ceolin and @tomi-font isn't this needed for the 4.0 release? AKA we're in RC1 right now, what needs to be done to make this into RC2?

These changes look good to me :)

Copy link
Collaborator

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

I only have one blocking comment: #79653 (comment)

doc/releases/migration-guide-4.0.rst Outdated Show resolved Hide resolved
doc/releases/migration-guide-4.0.rst Outdated Show resolved Hide resolved
doc/releases/migration-guide-4.0.rst Outdated Show resolved Hide resolved
doc/releases/migration-guide-4.0.rst Outdated Show resolved Hide resolved
doc/releases/migration-guide-4.0.rst Show resolved Hide resolved
doc/releases/migration-guide-4.0.rst Outdated Show resolved Hide resolved
doc/releases/migration-guide-4.0.rst Outdated Show resolved Hide resolved
subsys/jwt/Kconfig Outdated Show resolved Hide resolved
subsys/jwt/Kconfig Outdated Show resolved Hide resolved
subsys/jwt/jwt_psa.c Outdated Show resolved Hide resolved
Following the deprecation of TinyCrypt (zephyrproject-rtos#79566) we remove
TinyCrypt usage in random generators. This basically only affects
the CTR-DRBG random generator which from now only will only make
use of Mbed TLS.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti valeriosetti dismissed stale reviews from ceolin and d3zd3z via 0637861 October 30, 2024 14:08
@valeriosetti valeriosetti force-pushed the deprecate-tinycrypt-part1 branch 2 times, most recently from 0637861 to a44100c Compare October 30, 2024 14:10
As part of TinyCrypt deprecation process (zephyrproject-rtos#79566) this commit
removes usage of this library from the JWT subsystem and its
related tests.

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

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

Looks good to me! I realized we have one more reason to want this in for 4.0. (@dkalowsk @mmahadevan108)
This is replacing Kconfig options in JWT that themselves replaced older Kconfig options, within this release.
What this means if we only merge this for 4.1, is that there will be a few Kconfig options that are introduced in 4.0 and removed in 4.1. Really not great. Merging this for 4.0 makes this double change atomic, so I hope we can do that.
Also, this helps with the deprecation of TinyCrypt as this deprecates the shim driver (among others), which makes sense to have in 4.0 as well.
@ceolin @d3zd3z please re-review ASAP

@mmahadevan108 mmahadevan108 added TSC Topics that need TSC discussion and removed TSC Topics that need TSC discussion labels Oct 30, 2024
@mmahadevan108
Copy link
Collaborator

I mentioned this PR in the Oct 30th TSC meeting for an exception so we can merge and include in the 4.0 release. I have also posted this PR for discussion in the release channel.

Copy link
Contributor

@dkalowsk dkalowsk left a comment

Choose a reason for hiding this comment

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

Looks right to me. @ceolin looking for your review as well.

One minor question, should the MAINTAINERS.yml file status be updated to also note that TinyCrypt is deprecated and not getting "odd fixes"?

@dkalowsk dkalowsk added this to the v4.0.0 milestone Oct 30, 2024
Copy link
Member

@ceolin ceolin left a comment

Choose a reason for hiding this comment

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

re-approving it

@mmahadevan108 mmahadevan108 added the TSC Topics that need TSC discussion label Nov 1, 2024
Copy link
Contributor

@dkalowsk dkalowsk left a comment

Choose a reason for hiding this comment

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

I don't see anything in the PR that should block it (and it has my approval just not giving it in GitHub yet due the next steps). My understanding from conversations in Discord has been that this part 1 really needs to be merged with part 2. Even then there are some concerns that BT isn't quite ready for Tinycrypt to go away. @jhedberg can you confirm my understanding here?

@jhedberg
Copy link
Member

jhedberg commented Nov 4, 2024

My understanding from conversations in Discord has been that this part 1 really needs to be merged with part 2. Even then there are some concerns that BT isn't quite ready for Tinycrypt to go away. @jhedberg can you confirm my understanding here?

I'd say it's actually three PRs that go together, starting with #79566 which was already merged. Ultimately this is something for the release engineering team and possibly the TSC to decide, i.e. whether to make an exception or not to the practice of not having upstream users of deprecated APIs when a release is made. If an exception isn't made, then my interpretation is that #79566 should be reverted.

@mmahadevan108 mmahadevan108 removed the TSC Topics that need TSC discussion label Nov 5, 2024
@mmahadevan108
Copy link
Collaborator

After discussion in the release meeting on Nov 5th, it was decided to merge this PR for the 4.0 release.

@mmahadevan108 mmahadevan108 merged commit 7f55748 into zephyrproject-rtos:main Nov 5, 2024
29 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Crypto / RNG area: JSON area: Random Random subsystem area: Samples Samples platform: NXP NXP platform: STM32 ST Micro STM32 Release Notes To be mentioned in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants