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

boot: Add MCUBOOT_HW_KEY support for image encryption #1722

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

boot: Add MCUBOOT_HW_KEY support for image encryption #1722

wants to merge 1 commit into from
+86 −11

Conversation

DineshDK03
Copy link

Currently encryption supports only private key embed in mcuboot itself. To support MCUBOOT_HW_KEY for image encryption boot_retrieve_private_key() hook is added.

This hook helps retrieving private key from trusted sources like OTP, TPM.

d3zd3z
d3zd3z requested changes Jun 8, 2023
View reviewed changes
Copy link
Member

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

I like the idea of this change. I'd like to see if we can do this without introducing more ifdefs throughout the code. Perhaps we make the main code so that it always calls this function, and then, depending on the feature (ifdefs around whole functions are better than ones sprinkled through the code), we could just return a copy of the compiled in one. This could be conditionalized out if a given developer wishes to provide their own implementation.

boot/bootutil/src/encrypted.c Outdated Show resolved Hide resolved
@DineshDK03 DineshDK03 mentioned this pull request Jun 9, 2023
Closed
@de-nordic de-nordic added the crypto Encryption support label Jun 14, 2023
@DineshDK03 DineshDK03 requested a review from d3zd3z June 16, 2023 04:39
@DineshDK03
Copy link
Author

@d3zd3z please revisit.

@DineshDK03
Copy link
Author

@d3zd3z Ping

@jimmyzumthurm
Copy link

Any update on this ? We have similar solution currently in our software by patching MCUboot, but would highly appreciate if that would be officially available

@DineshDK03
Copy link
Author

Any update on this ? We have similar solution currently in our software by patching MCUboot, but would highly appreciate if that would be officially available

@jimmyzumthurm. I have already addressed the comments from @mingulov and @d3zd3z and waiting for their acceptance to merge this PR.

@mingulov
Copy link
Contributor

Any update on this ? We have similar solution currently in our software by patching MCUboot, but would highly appreciate if that would be officially available

@jimmyzumthurm. I have already addressed the comments from @mingulov and @d3zd3z and waiting for their acceptance to merge this PR.

I am not an official approver / maintainer for MCUboot, so unfortunately my acceptance would not help anyhow.

@DineshDK03
Copy link
Author

@d3zd3z @de-nordic @nordicjm please have a look.

@DineshDK03
Copy link
Author

@d3zd3z ping

@nordicjm
Copy link
Collaborator

@sigvartmh

@DineshDK03
Copy link
Author

@d3zd3z @sigvartmh @nordicjm @de-nordic please have a look, if all is good, kindly merge this PR.

d3zd3z
d3zd3z requested changes Dec 12, 2023
View reviewed changes
Copy link
Member

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

Other than the whitespace issue, this looks good now.

boot/bootutil/src/encrypted.c Outdated Show resolved Hide resolved
@DineshDK03
Copy link
Author

@d3zd3z Thanks for the review and fixed all the whitespace issues now.

@d3zd3z d3zd3z self-requested a review December 14, 2023 15:23
@DineshDK03
Copy link
Author

@d3zd3z pushed changes to fix the CI. please approve to run the workflow jobs.

d3zd3z
d3zd3z requested changes Feb 8, 2024
View reviewed changes
boot/mbed/app_enc_keys.c Outdated Show resolved Hide resolved
@DineshDK03
boot: Add MCUBOOT_HW_KEY support for image encryption
4e47530 
Currently encryption supports only private key embed
in mcuboot itself. To support MCUBOOT_HW_KEY for image
encryption boot_retrieve_private_key() hook is added.

This hook helps retrieving private key from trusted
sources like OTP, TPM.

Signed-off-by: Dinesh Kumar K <dinesh@linumiz.com>
@DineshDK03 DineshDK03 requested a review from d3zd3z March 3, 2024 13:02
@DineshDK03
Copy link
Author

@d3zd3z please have a look, all the comments addressed and waiting to be merged.

@davidvincze davidvincze requested review from davidvincze and removed request for mingulov May 23, 2024 11:39
davidvincze
davidvincze reviewed May 23, 2024
View reviewed changes
boot/bootutil/include/bootutil/enc_key.h
* @return 0 on success; nonzero on failure.
*
*/
int boot_retrieve_private_key(struct bootutil_key **private_key);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind renaming this function to boot_enc_retrieve_priv_key or similar?
It's for avoiding ambiguity - one might think that it's retrieving the private key used for image signing (there's already a function called boot_retrieve_public_key_hash).

boot/cypress/MCUBootApp/keys.c

return 0;
}
#endif /* !MCUBOOT_HW_KEY */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#endif /* !MCUBOOT_HW_KEY */
#endif /* !MCUBOOT_HW_KEY && MCUBOOT_ENC_IMAGES */

boot/mbed/app_enc_keys.c

return 0;
}
#endif /* !MCUBOOT_HW_KEY */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#endif /* !MCUBOOT_HW_KEY */
#endif /* !MCUBOOT_HW_KEY && MCUBOOT_ENC_IMAGES */

boot/zephyr/keys.c

return 0;
}
#endif /* !MCUBOOT_HW_KEY */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#endif /* !MCUBOOT_HW_KEY */
#endif /* !MCUBOOT_HW_KEY && MCUBOOT_ENC_IMAGES */

ci/mynewt_keys/enc_kw/src/keys.c

return 0;
}
#endif /* !MCUBOOT_HW_KEY */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#endif /* !MCUBOOT_HW_KEY */
#endif /* !MCUBOOT_HW_KEY && MCUBOOT_ENC_IMAGES */

ci/mynewt_keys/enc_rsa/src/keys.c

return 0;
}
#endif /* !MCUBOOT_HW_KEY */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#endif /* !MCUBOOT_HW_KEY */
#endif /* !MCUBOOT_HW_KEY && MCUBOOT_ENC_IMAGES */

sim/mcuboot-sys/csupport/keys.c

return 0;
}
#endif /* !MCUBOOT_HW_KEY */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#endif /* !MCUBOOT_HW_KEY */
#endif /* !MCUBOOT_HW_KEY && MCUBOOT_ENC_IMAGES */

@davidvincze
Copy link
Collaborator

davidvincze commented May 23, 2024 •
edited

@DineshDK03 please add a top commit that adds a release note snippet for this feature.
(https://github.com/mcu-tools/mcuboot/blob/main/docs/SubmittingPatches.md#release-notes)
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mingulov mingulov mingulov left review comments

@davidvincze davidvincze davidvincze left review comments

@d3zd3z d3zd3z Awaiting requested review from d3zd3z

Requested changes must be addressed to merge this pull request.

Assignees

@d3zd3z d3zd3z

Labels
crypto Encryption support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@DineshDK03 @jimmyzumthurm @mingulov @nordicjm @davidvincze @d3zd3z @de-nordic