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

Factorize nifs for OTP crypto module #931

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

pguyot
Copy link
Collaborator

@pguyot pguyot commented Nov 6, 2023

Move implementation from ESP32's platform_nifs.c to otp_crypto.c and make it available to all
platforms using mbedtls.

Alter Pico's mbedtls configuration to handle the ciphers we support

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

@pguyot pguyot force-pushed the w45/factorize-otp-crypto-mbedtls branch from 1ab57e3 to f3e47a7 Compare November 7, 2023 06:07
@pguyot pguyot marked this pull request as ready for review November 7, 2023 06:09
src/libAtomVM/otp_crypto.c Outdated Show resolved Hide resolved
@bettio
Copy link
Collaborator

bettio commented Nov 7, 2023

As a general preference for future changes, when a PR contains both refactoring and fixes I rather prefer dividing it in 2 different commits, so the fix is more evident compared to "background" refactoring changes. This is specially useful when doing a huge code block move, since it is not possible to tell what changed without comparing side by side different files.

@pguyot
Copy link
Collaborator Author

pguyot commented Nov 7, 2023

As a general preference for future changes, when a PR contains both refactoring and fixes I rather prefer dividing it in 2 different commits, so the fix is more evident compared to "background" refactoring changes. This is specially useful when doing a huge code block move, since it is not possible to tell what changed without comparing side by side different files.

Very true. Let's try to do it here. I'll open a separate PR for the iv-compatibilitty fix.

@pguyot pguyot force-pushed the w45/factorize-otp-crypto-mbedtls branch 2 times, most recently from f19c2c0 to 0e60cc2 Compare November 7, 2023 19:40
@bettio
Copy link
Collaborator

bettio commented Nov 7, 2023

CodeQL fails with: "Completed in 3s — 1 configuration not found".

Copy link
Collaborator

@bettio bettio left a comment

Choose a reason for hiding this comment

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

Everything good here, just few missing things.
Let's mention in our changelog:

  • that now hashing functions require mbedTLS instead of OpenSSL
  • that crypto functions are available also for generic_unix and rp2040 when using mbedTLS

src/libAtomVM/otp_crypto.c Show resolved Hide resolved
@pguyot
Copy link
Collaborator Author

pguyot commented Nov 9, 2023

CodeQL fails with: "Completed in 3s — 1 configuration not found".

Was it solved with a rerun? I can't find this error.

Move implementation from ESP32's platform_nifs.c to otp_crypto.c and make it
available to all platforms using mbedtls.

Alter Pico's mbedtls configuration to handle the ciphers we support.

Signed-off-by: Paul Guyot <pguyot@kallisys.net>
@pguyot pguyot force-pushed the w45/factorize-otp-crypto-mbedtls branch from 0e60cc2 to 17c1ce4 Compare November 9, 2023 20:53
@bettio
Copy link
Collaborator

bettio commented Nov 9, 2023

CodeQL fails with: "Completed in 3s — 1 configuration not found".

Was it solved with a rerun? I can't find this error.

Now it is gone.

@bettio bettio merged commit 8b39bd8 into atomvm:master Nov 9, 2023
83 of 84 checks passed
@pguyot pguyot deleted the w45/factorize-otp-crypto-mbedtls branch November 10, 2023 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants