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

ssl: modify ssl packages to allow for pluggable openssl to replace boringssl #5161

Closed
wants to merge 1 commit into from
Closed

Conversation

bdecoste
Copy link
Contributor

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Creates an abstraction layer in ssl packages (e.g. common/ssl and extensions/filters/listener/tls_inspector) to separate the code sections that differ between boringssl and openssl. Includes corresponding changes to bazel configuration. There is an external repo (https://github.com/bdecoste/sslimpl) that contains a script (openssl.sh) that will replace the boringssl-specific libraries with external openssl-specific libraries via the bazel config.
Risk Level: Medium
Testing: All common/ssl and extensions/filters/listener/tls_inspector tests passing for both boringssl changes in this PR as well as the plugged-in boringssl external libraries.
Docs Changes: No docs changes
Release Notes: No release notes
[Optional Fixes #Issue]
[Optional Deprecated:]

@htuch
Copy link
Member

htuch commented Nov 29, 2018

Thanks @bdecoste. @PiotrSikora @lizan can you take a look?

As a heads up, while this review happens, PRs for #1319 land will be landing over the coming week, which will probably require some changes here. I'm guessing we'll finish multi-cert SSL support before this one lands.

@bdecoste
Copy link
Contributor Author

Thanks @htuch. I'm expecting to have to redo this PR when and if it's approved as I'm also expecting a fairly lengthy discussion and review.

@bdecoste
Copy link
Contributor Author

bdecoste commented Dec 1, 2018

Adding text from email conversation...

We've created an abstraction layer that separates the areas of code in the ssl packages (e.g. common/ssl and extensions/filters/listener/tls_inspector) where behavior and/or SPI differ between the ssl libraries. This allows us to run a script (openssl.sh in https://github.com/bdecoste/sslimpl) that will reconfigure the upstream bazel config to replace the upstream openssl-specific implementation of the new abstraction layer with external openssl-specific libraries. There are no functional changes or anything related to openssl in the PR - just a separation of the areas where there are differences between the two ssl libraries.

Our main goal of the PR is to serve as a starting point for a discussion on the work we have done to date and an initial implementation proposal. Some outstanding questions, beyond a general review, are:

  1. The PR only contains those changes required for pluggable openssl so the two ssl packages contain a mix of calls to the abstraction layer (required by differences between boringssl and openssl) and direct calls to the ssl library. This could cause confusion for future development as to which direction to use as it's not obvious to boringssl developers where abstraction is required for openssl. One option would be to abstract all calls to the ssl library but we wanted to at least get the conversation started before heading down this road.

  2. openssl does not allow for direct access to the internal structures like boringssl allows so we'd need a mechanism (build tooling?) to identify PRs that include code where internal structures directly accessed which would break openssl. Similarly for SPI calls that do not exist or have different signatures in openssl.

@bdecoste
Copy link
Contributor Author

bdecoste commented Dec 1, 2018

From @mattklein123: I think we agreed before that we would not have any CI related to openssl? Obviously we should do our best to not introduce problematic code, but I think the work would be on you all to come back and fix stuff if things slip in?

From @bdecoste: Wanted to raise the possibility of having CI flag anything that would obviously break openssl integration. I'm not suggesting that we do any CI openssl integration or testing. Or perhaps document some ssl best practices on what to avoid.

From @mattklein123: Yes I think we can do check-format checks if you are willing to code them. With that being said, I think we need to have a discussion about what happens if/when one of those checks reduces developer velocity because of some divergence. Hopefully this will never happen but I would like to plan for the worst.

@bdecoste bdecoste closed this Dec 11, 2018
@bdecoste
Copy link
Contributor Author

Closing per our discussion at KubeCon

@ggreenway
Copy link
Contributor

The plan going forward is to support alternative TLS libraries such as OpenSSL. Envoy will provide the ability to compile out BoringSSL completely, and the ability to provide an alternate implementation of code which handles TLS. Specifically, there will be a new TransportSocket for OpenSSL, and whatever code in source/common/ssl needs to be replaced.

Despite the fact that the OpenSSL and BoringSSL APIs are similar right now, we will treat them as completely separate APIs, because the two are expected to drift more and more over time.

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.

None yet

5 participants