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

Enables the use of wolfSSL for crypto primitives #692

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Enables the use of wolfSSL for crypto primitives #692

merged 1 commit into from
Apr 26, 2024

Conversation

SparkiDev
Copy link
Contributor

To configure:
./configure --enable-wolfssl --with-wolfssl-dir=

Add implementations of SRTP KDF, HMAC, AES-GCM and AES-CTR using wolfSSL.

@SparkiDev
Copy link
Contributor Author

Can I get a review of this code?

@pabuhler
Copy link
Member

Can I get a review of this code?

Yes it is on my list of things todo in the next week, sorry if that seams like a long time.
If possible can you add support for building with cmake as it is our main builld system these days and having a CI build for at least one build system and on at least one platform would be very helpful. see .github/workflows/cmake.yml or .github/workflows/autotools.yml

@JonathanLennox
Copy link

Wrong things will happen if you enable two or more of --enable-openssl, --enable-wolfssl, and --enable-nss, and I think we have nothing that guards against this. This isn't a new problem with wolfssl (it was already a problem with just openssl and nss), but should we have something to protect against this?

Or possibly even change the configure option syntax to --enable-crypto=[openssl,wolfssl,nss] with the 3.0 release, since that's where we're putting other breaking changes.

@pabuhler pabuhler mentioned this pull request Apr 1, 2024
Open
@pabuhler
Copy link
Member

pabuhler commented Apr 1, 2024

Wrong things will happen if you enable two or more of --enable-openssl, --enable-wolfssl, and --enable-nss, and I think we have nothing that guards against this. This isn't a new problem with wolfssl (it was already a problem with just openssl and nss), but should we have something to protect against this?

Or possibly even change the configure option syntax to --enable-crypto=[openssl,wolfssl,nss] with the 3.0 release, since that's where we're putting other breaking changes.

It think there is some protection in the cmake build files at least, but I agree at this point when there are now ~5 options the config should be changed to a switch as you suggested

pabuhler
pabuhler reviewed Apr 1, 2024
View reviewed changes
srtp/srtp.c Show resolved Hide resolved
@SparkiDev SparkiDev force-pushed the wolfssl branch 2 times, most recently from 4ac1e46 to e65b27f Compare April 11, 2024 22:56
@SparkiDev
Copy link
Contributor Author

SparkiDev commented Apr 11, 2024 •
edited
Loading

Changed CI testing to always use GitHub to clone wolfssl using https.

@pabuhler
Copy link
Member

@SparkiDev ,
Hi I will look in to the failing Meson build, seams to also fail in the main branch at the moment. As to the other failures, maybe try to install "brew install automake" on macos and building wolfssl as a static library .... They are are just quick thoughts I have looked to deeply into the failures..

@SparkiDev
Copy link
Contributor Author

Changes:
Installing autoconf,, automake and libtool on make to build wolfSSL from source.
Invloking ldconfig on Ubuntu so that wolfSSL library is seen.

@SparkiDev
Copy link
Contributor Author

Attempted to fix cmake on macOS by exporting include path /usr/local/include

pabuhler
pabuhler reviewed Apr 18, 2024
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@SparkiDev SparkiDev force-pushed the wolfssl branch 2 times, most recently from f3bdf13 to 74c4a97 Compare April 18, 2024 21:24
@SparkiDev
Copy link
Contributor Author

Changed WOLFSSL_INCLUDE_DIRS -> WOLFSSL_INCLUDE_DIR in FindwolfSSL.cmake to match CMakeLists.txt.

@SparkiDev
Copy link
Contributor Author

Changed when the Aes object is allocated to minimize memory usage.

@SparkiDev SparkiDev force-pushed the wolfssl branch 2 times, most recently from f33986c to 79e6346 Compare April 24, 2024 06:23
@SparkiDev
Copy link
Contributor Author

Improved memory usage around AES-GCM.

pabuhler
pabuhler reviewed Apr 25, 2024
View reviewed changes
Copy link
Member

@pabuhler pabuhler left a comment

Choose a reason for hiding this comment

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

In general it is fine, will approve once you have responded to the comments.

CMakeLists.txt Show resolved Hide resolved
config_in_cmake.h Show resolved Hide resolved
crypto/cipher/aes_icm_wssl.c Outdated Show resolved Hide resolved
crypto/hash/hmac_wssl.c Show resolved Hide resolved
@SparkiDev
Enables the use of wolfSSL for crypto primitives
35454c0 
To configure:
    ./configure --enable-wolfssl --with-wolfssl-dir=<wolfssl dir>

Add implementations of SRTP KDF, HMAC, AES-GCM and AES-CTR using
wolfSSL.
@pabuhler pabuhler merged commit 5fb0759 into cisco:main Apr 26, 2024
37 checks passed
@pabuhler
Copy link
Member

pabuhler commented Apr 27, 2024 •
edited
Loading

@SparkiDev , it is merged but now the ci builds fail to build wolfssl ... https://github.com/cisco/libsrtp/actions/runs/8861716469/job/24333886680
Can you take a look?
As a side note should probably not be build from tip of main branch of wolfssl, mayne use v5.7.0-stable tag ?
Thanks

@SparkiDev
Copy link
Contributor Author

#704 should fix this.

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

@pabuhler pabuhler pabuhler left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SparkiDev @pabuhler @JonathanLennox