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

boot: Rename boot_enc_decrypt to boot_decrypt_key #2003

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

de-nordic
Copy link
Collaborator

All of boot_enc_ function follow the same pattern where they take encryption context as the first parameter, and the boot_enc_decrypt stands out here as it does not work around the encryption context, but is rather single-part decryption function only used for decrypting of the image encryption key.

All of boot_enc_ function follow the same pattern where
they take encryption context as the first parameter, and the
boot_enc_decrypt stands out here as it does not work around
the encryption context, but is rather single-part decryption
function only used for decrypting of the image encryption
key.

Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>
@utzig
Copy link
Member

utzig commented Jul 15, 2024

I think this is arguably worse; as part of the encrypted images "api" all functions have names that start with boot_enc_, and you are breaking this "standard" naming. Either it should be called boot_enc_decrypt_key, which I don't see a reason for either, or you want to reuse it for something else, in which case it's better to just rename as part of the new use PR.

@de-nordic
Copy link
Collaborator Author

I think this is arguably worse; as part of the encrypted images "api" all functions have names that start with boot_enc_, and you are breaking this "standard" naming. Either it should be called boot_enc_decrypt_key, which I don't see a reason for either, or you want to reuse it for something else, in which case it's better to just rename as part of the new use PR.

Putting api and standard in quotes seems to mock them rather then making them an argument in discussion.

Worse? Nah. All except this one function take enc_key_data objects. That one function is completely independent from encryption context setup by all the other functions and completely setups, decrypts, and drops encryption context within its body.

And the standard somehow names the encryption function boot_encrypt, even though it is one taking the context, and yet stands out in naming.

For me pattern was clear: everything boot_enc_ operates around enc_key.

I do not want to make everything in one PR and I am not going to reuse boot_decrypt, actually I have to implement boot_enc_decrypt and boot_enc_encrypt , and various others encryption/decryption only functions, as the PSA, which I am using, uses different set of functions for decryption/encryption, which also includes the process setup.

Copy link
Collaborator

@nvlsianpu nvlsianpu left a comment

Choose a reason for hiding this comment

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

I'm fine with the change which makes the function name descriptive.

I do not want to make everything in one PR and I am not going to reuse boot_decrypt, actually I have to implement boot_enc_decrypt and boot_enc_encrypt...

Yes - this will fit to the pattern.

@de-nordic de-nordic added the [DNM] Do Not Merge label Jul 16, 2024
@de-nordic
Copy link
Collaborator Author

I have marked it as DNM, because while reworking boot_enc_load I am now considering making both boot_enc_load and boot_decrypt_key part of boot_status API.

@de-nordic
Copy link
Collaborator Author

I have marked it as DNM, because while reworking boot_enc_load I am now considering making both boot_enc_load and boot_decrypt_key part of boot_status API.

Nope, not the good idea. But the boot_enc_load is gone.

@de-nordic de-nordic removed the [DNM] Do Not Merge label Jul 16, 2024
@nvlsianpu nvlsianpu added the crypto Encryption support label Jul 23, 2024
@nvlsianpu
Copy link
Collaborator

@d3zd3z, @utzig last call. This is just mcuboot internal function rename l I'm going to merge this PR.

@utzig
Copy link
Member

utzig commented Jul 23, 2024

Putting api and standard in quotes seems to mock them rather then making them an argument in discussion.

If it's an API to you, then it's an API. I wrote it so there would be no point in mocking! You need to write "boot_enc_encrypt", that's all that you had to say, it's fine.

@d3zd3z, @utzig last call. This is just mcuboot internal function rename l I'm going to merge this PR.

YOU THE CAPTAIN NOW!

@nvlsianpu nvlsianpu merged commit 2371c0a into mcu-tools:main Jul 24, 2024
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Encryption support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants