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

Enable linking against BoringSSL #349

Closed
wants to merge 12 commits into from
Closed

Enable linking against BoringSSL #349

wants to merge 12 commits into from

Conversation

bifurcation
Copy link
Contributor

This is a very drafty draft PR looking at the possibility of enabling linkage against BoringSSL as well as OpenSSL. I have included only the changes required to the CMake files to find the appropriate parts of BoringSSL (and even then, manually pointed to my local install). To get this merged, we would need to:

  • Clean up the build system changes (e.g., make them parallel to the 1.1/3.0 switch, and remove the manual OPENSSL_* variable assignments)
  • Add some conditional switches in the hpke module to accommodate API differences, as in Updated to support OpenSSL 3 #295

@bifurcation
Copy link
Contributor Author

Latest commit updates to current main and makes the required changes to get the library building and passing tests. Notable changes:

  • AEAD operations use the BoringSSL EVP_AEAD scheme
  • Support for X448 / Ed448 is removed because BoringSSL doesn't these curves
  • One test is disabled due to malformed certificates (which should be regenerated before merging)

The CMake magic for telling the build system to link BoringSSL is still not great, but other than that, I think this is approaching a plausible state. We should add a CI job for it before merging.

@birarda
Copy link
Contributor

birarda commented Oct 4, 2023

Is a merge for this one imminent? We're integrating mlspp and currently leveraging the boring branch for BoringSSL support but would like to get back to main.

@bifurcation
Copy link
Contributor Author

@birarda - To be honest, some help here would be welcome. I think the code changes are pretty much done, and just need some review. The main outstanding tasks are (1) updating the CMake build structure to be more elegant, (2) adding a CI job that builds against BoringSSL (in parallel with OpenSSL 1.1 / 3.0). And maybe (3) adding a section to the README about how to select which of the three libraries to use to use.

@birarda
Copy link
Contributor

birarda commented Oct 7, 2023

@birarda - To be honest, some help here would be welcome. I think the code changes are pretty much done, and just need some review. The main outstanding tasks are (1) updating the CMake build structure to be more elegant, (2) adding a CI job that builds against BoringSSL (in parallel with OpenSSL 1.1 / 3.0). And maybe (3) adding a section to the README about how to select which of the three libraries to use to use.

Would be happy to assist here. I’m guessing you’ll need to do (2)? Could you provide any further guidance on what you’d like for (1), if necessary outside of what is described in the PR description? I can PR to this branch for (1) and (3).

@bifurcation
Copy link
Contributor Author

I think you should be able to do (2) just by making changes in the .github/workflows/ directory. I may have to approve them running in CI on the PR, but obviously happy to do that. Maybe the way to start is to make a separate YAML file to get the steps right, and then we can fold it into the main CI build file.

For (1) -- If you look at the CMakeLists in this PR, it is hard-coded to always use BoringSSL, and always find it in ../boringssl. Clearly not a generic solution! Assuming that (a) it's possible to guide find_package(OpenSSL) to a BoringSSL install using environment variables or something and (b) it's possible for CMake to detect that it is using BoringSSL, then we could extend the selection logic. Otherwise, we might need a more explicit override. The critical things are that the builder can specify which library is used, and that the WITH_OPENSSL3 / WITH_BORINGSSL variables get set accurately.

add info to readme for BoringSSL/OpenSSL

undo spacing changes

prepare for vcpkg enabled boringssl

add test workflow for boringssl

remove the cmake module path

remove an extra newline

fix a typo

remove an extra newline
@bifurcation bifurcation marked this pull request as ready for review October 12, 2023 13:40
@bifurcation
Copy link
Contributor Author

Replaced by #376

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