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

Remove MBEDTLS_TEST_NULL_ENTROPY config option. #4450

Merged
merged 3 commits into from
May 20, 2021
Merged

Remove MBEDTLS_TEST_NULL_ENTROPY config option. #4450

merged 3 commits into from
May 20, 2021

Conversation

mstarzyk-mobica
Copy link
Contributor

resolves #4388

@mstarzyk-mobica mstarzyk-mobica self-assigned this Apr 30, 2021
@mstarzyk-mobica mstarzyk-mobica added mbedtls-3 needs-ci Needs to pass CI tests 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 Apr 30, 2021
chris-jones-arm
chris-jones-arm previously requested changes Apr 30, 2021
View reviewed changes
Copy link
Contributor

@chris-jones-arm chris-jones-arm 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.

Changelog needs a slight improvement and also needs a migration guide entry although I can confirm I can no longer find any references to the removed option.

ChangeLog.d/remove_null_entropy.txt Outdated Show resolved Hide resolved
@chris-jones-arm chris-jones-arm added needs-work and removed needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, labels Apr 30, 2021
@ronald-cron-arm ronald-cron-arm linked an issue May 5, 2021 that may be closed by this pull request
Remove MBEDTLS_TEST_NULL_ENTROPY #4388
Closed
@mstarzyk-mobica mstarzyk-mobica changed the base branch from development_3.0 to development May 11, 2021 11:11
@mstarzyk-mobica
Remove MBEDTLS_TEST_NULL_ENTROPY config option.
72f60df 
Building the library without entropy sources negates any and all security
provided by the library.
This option was originally requested a relatively long time ago and it
does not provide any tangible benefit for users any more.

Signed-off-by: Mateusz Starzyk <mateusz.starzyk@mobica.com>
@mstarzyk-mobica
Copy link
Contributor Author

Rebased to development branch (fixes conflict with removed psa-crypto config).
Amended the requested change in the changelog file.

@mstarzyk-mobica
Copy link
Contributor Author

This PR needs two more reviewers

@ronald-cron-arm ronald-cron-arm self-requested a review May 12, 2021 07:51
ronald-cron-arm
ronald-cron-arm requested changes May 12, 2021
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

You need a migration guide entry thus an *.md file in docs/3.0-migration-guide.d.

mpg
mpg approved these changes May 14, 2021
View reviewed changes
Copy link
Contributor

@mpg mpg 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, except as Ronald pointer out, we want a migration guide entry too.

@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label May 14, 2021
mpg
mpg requested changes May 14, 2021
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Err, sorry, I meant to select "request changes".

@mpg mpg dismissed chris-jones-arm’s stale review May 14, 2021 11:11

Chris will not be re-reviewing

ronald-cron-arm
ronald-cron-arm requested changes May 18, 2021
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

To address the typos please amend the latest commit and do not create another commit.

docs/3.0-migration-guide.d/remove-null-entropy.md Outdated Show resolved Hide resolved
docs/3.0-migration-guide.d/remove-null-entropy.md Outdated Show resolved Hide resolved
docs/3.0-migration-guide.d/remove-null-entropy.md Outdated Show resolved Hide resolved
@mstarzyk-mobica
Add migration guide for removed null entropy config option
ef80a9c 
Signed-off-by: Mateusz Starzyk <mateusz.starzyk@mobica.com>
@mstarzyk-mobica
Copy link
Contributor Author

Fixed typos and alignment in migration file

@ronald-cron-arm ronald-cron-arm added the needs-review Every commit must be reviewed by at least two team members, label May 18, 2021
ronald-cron-arm
ronald-cron-arm previously approved these changes May 18, 2021
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

I am not competent to assert the proposed migration path but I count on @mpg for that. LGTM otherwise.

@mstarzyk-mobica
Copy link
Contributor Author

I am not competent to assert the proposed migration path but I count on @mpg for that. LGTM otherwise.

Thanks. Actually me neither, I used what chris proposed in #4388 description.

mpg
mpg requested changes May 19, 2021
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

I've suggested an improvement to the migration guide in order to be clearer about security requirements.

docs/3.0-migration-guide.d/remove-null-entropy.md Outdated Show resolved Hide resolved
@mstarzyk-mobica
Provide more in-depth migration guide after removal of null entropy.
2396b21 
Signed-off-by: Mateusz Starzyk <mateusz.starzyk@mobica.com>
mpg
mpg approved these changes May 20, 2021
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

@mpg mpg dismissed davidhorstmann-arm’s stale review May 20, 2021 07:19

Feedback has been addressed and marked as resolved

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels May 20, 2021
@mpg mpg merged commit 729fa5b into Mbed-TLS:development May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhorstmann-arm davidhorstmann-arm davidhorstmann-arm left review comments

@chris-jones-arm chris-jones-arm chris-jones-arm left review comments

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

@mpg mpg mpg approved these changes

Assignees

@mstarzyk-mobica mstarzyk-mobica

Labels
approved Design and code approved - may be waiting for CI or backports
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove MBEDTLS_TEST_NULL_ENTROPY
5 participants
@mstarzyk-mobica @mpg @ronald-cron-arm @chris-jones-arm @davidhorstmann-arm