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 redundant ifdef DEVICE_TRNG from DeviceKey #9576

Merged
merged 1 commit into from
Feb 13, 2019
Merged

Remove redundant ifdef DEVICE_TRNG from DeviceKey #9576

merged 1 commit into from
Feb 13, 2019

Conversation

yossi2le
Copy link
Contributor

Description

Removing the ifdef DEVICE_TRNG from the code which prevents the use of device key with pseudo-random generator supported by mbedtls entropy function.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ciarmcom ciarmcom requested review from a team January 31, 2019 16:00
@ciarmcom
Copy link
Member

@yossi2le, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@cmonr cmonr requested a review from a team January 31, 2019 16:42
cmonr
cmonr approved these changes Jan 31, 2019
View reviewed changes
Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

LGTM, though seems a bit odd.

yanesca
yanesca suggested changes Feb 1, 2019
View reviewed changes
features/device_key/source/DeviceKey.cpp
@@ -260,7 +260,6 @@ int DeviceKey::generate_key_by_random(uint32_t *output, size_t size)
return DEVICEKEY_INVALID_PARAM;
}

#if DEVICE_TRNG
Copy link
Contributor

Choose a reason for hiding this comment

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

This section of code is only safe if either the device has a TRNG, or MBEDTLS_ENTROPY_NV_SEED is turned on. The mbedtls_entropy_func() might catch it if this is not the case and return an error, but it depends on the Mbed TLS configuration. I think it would be safer to add && defined(MBEDTLS_ENTROPY_NV_SEED) to the condition than remove the macro completely. Would that solve the problem?

Copy link

Choose a reason for hiding this comment

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

@yanesca What will happen if there is no entropy seed? Will it be better || defined(MBEDTLS_ENTROPY_NV_SEED)?

Copy link
Contributor

Choose a reason for hiding this comment

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

If a component registers its own weak entropy sources as strong or enables the HAVEGE source, then this function returns low entropy keys. Yes, I think that would be better: it would make explicit that we only provide output if TRNG or entropy seed is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yanesca thanks.
I have fixed it as you suggested.

@adbridge
Copy link
Contributor

adbridge commented Feb 7, 2019

@yossi2le please address @yanesca's review comment

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 11, 2019

@yossi2le Please review

@yogpan01
Copy link
Contributor

@adbridge @0xc0170 Sorry this was missed from our requested PR list but can we take this for 5.11.4 RC.
If we dont get this fix for 5.11.4 it will block PDMC major release for next 2 weeks.
@JanneKiiskila @teetak01 @jenia81 @neilwjackson FYI.

@cmonr
Copy link
Contributor

cmonr commented Feb 12, 2019

@yogpan01 There's a slim chance we might be able to bring this into something unscheduled, but regardless, we need @yossi2le to reply to the review concern...

@cmonr
Copy link
Contributor

cmonr commented Feb 12, 2019

CI started, whilst pending review comment is addressed

@mbed-ci
Copy link

mbed-ci commented Feb 13, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@cmonr
Copy link
Contributor

cmonr commented Feb 13, 2019

@ARMmbed/mbed-os-maintainers Mergable at your disgression.

Adding MBEDTLS_ENTROPY_NV_SEED macro check in device key to allow key…
d8d91c3 
… derivation when there is no TRNG but there is DRBG
@NirSonnenschein
Copy link
Contributor

Hi @yanesca , please re-review the fix for your comments (this is rather urgent as it is critical for the client release).
re-starting CI in the meantime

yanesca
yanesca approved these changes Feb 13, 2019
View reviewed changes
Copy link
Contributor

@yanesca yanesca 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!

@mbed-ci
Copy link

mbed-ci commented Feb 13, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 2
Build artifacts

@cmonr cmonr merged commit 583980f into ARMmbed:master Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannybenor dannybenor dannybenor approved these changes

@cmonr cmonr cmonr approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@yanesca yanesca yanesca approved these changes

Assignees
No one assigned
Labels
release-version: 5.11.5-alpha
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@yossi2le @ciarmcom @adbridge @0xc0170 @yogpan01 @cmonr @mbed-ci @NirSonnenschein @yanesca @dannybenor