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

Migrate to rustls #820

Merged
merged 4 commits into from
Feb 28, 2024
Merged

Migrate to rustls #820

merged 4 commits into from
Feb 28, 2024

Conversation

howardjohn
Copy link
Member

This PR moves from usage of boring as a crypto library and TLS library, to using rustls. Rustls is the standard TLS library in the rust ecosystem.

Rustls offers a pluggable crypto interface. There are ~3 providers (2 in core, 1 external): ring, aws_lc_rs, and boring. The latter two provider FIPS certified builds (though I think aws_lc only has in progress builds); ring has no FIPS certified builds though I think they are working towards one. In this model, the TLS aspects (which are a large portion of the code involved) are handled purely by rustls's code, written in safe rust. Just the crypto primitives are offloaded to these various libraries (which are all ultimately varying degrees of openssl forks, as I understand it).

This offers a few benefits:

  • For vendors, the ability to plug in alternative crypto modules
  • Better performance (~10% improvement)
  • Standardize with rest of rust ecosystem. Ever library integrates with rustls; we need to make our own shims for everything today like tonic, etc

Fixes #797
Fixes #641
Helps with #149
Fixes #110

@istio-testing
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 21, 2024
@@ -1,5 +1,5 @@
[build]
target-dir = "out/rust"
[env]
BORING_BSSL_PATH = { value = "vendor/boringssl-fips/linux_x86_64", force = true, relative = true }
BORING_BSSL_INCLUDE_PATH = { value = "vendor/boringssl-fips/include/", force = true, relative = true }
BORING_BSSL_FIPS_PATH = { value = "vendor/boringssl-fips/linux_x86_64", force = true, relative = true }
Copy link
Contributor

@bleggett bleggett Feb 21, 2024

Choose a reason for hiding this comment

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

Suggested change
BORING_BSSL_FIPS_PATH = { value = "vendor/boringssl-fips/linux_x86_64", force = true, relative = true }
BORING_BSSL_FIPS_PATH = { value = "vendor/boringssl-fips/linux_x86_64", force = true, relative = true }

Does this also fix: #399 ?

I haven't checked, but one of the minor annoyances before was we couldn't conditionally set this variable in build.rs per-arch/platform, because boring-sys was doing stuff in its own build.rs that both

A) didn't work with vendoring
B) defeated cargo's "recompute" logic making it impossible to override in our build.rs

which means the build has to sed this for multiarch builds, and those of us on linux_arm64 have to locally munge this file constantly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so unless you buid with ring instead of boring which sidesteps the issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I forgot a bit part of this - I am proposing we default Istio builds to ring as well.. that decision can be decoupled from the broader PR though

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 22, 2024
@ilrudie
Copy link
Contributor

ilrudie commented Feb 23, 2024

Big plus 1 for moving to rustls!

@howardjohn howardjohn marked this pull request as ready for review February 23, 2024 22:54
@howardjohn howardjohn requested review from a team as code owners February 23, 2024 22:54
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 23, 2024
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 23, 2024
@howardjohn howardjohn force-pushed the tls/rustls branch 2 times, most recently from 30dfb3d to d3794a0 Compare February 23, 2024 23:34
Copy link
Contributor

@bleggett bleggett left a comment

Choose a reason for hiding this comment

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

Seems happy building locally with both FIPS and non-FIPS variants on arm64 (with the vendored lib move mentioned in comments).

vendor/boringssl-fips/linux_x86_64/lib/libcrypto.a Outdated Show resolved Hide resolved
src/tls/csr.rs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@@ -25,70 +26,78 @@ name = "throughput"
harness = false

[dependencies]
http-02 = { package = "http", version = "0.2.9" }
# Enabled with 'tls-boring'
boring-rustls-provider = { git = "https://github.com/janrueth/boring-rustls-provider", optional = true } #
Copy link
Member

Choose a reason for hiding this comment

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

Will this repo actively maintained, and who takes charge

Copy link
Member Author

Choose a reason for hiding this comment

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

I am working to get this actively maintained (TBD by who). Note this is an off by default feature; the default Istio builds do not use it.

Copy link
Member

Choose a reason for hiding this comment

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

It has nothing todo with default or not. Every dependency should be actively maintained. So I get it, you own this crate

Copy link
Contributor

@bleggett bleggett 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 happy with this approach, LGTM.

@@ -25,70 +26,78 @@ name = "throughput"
harness = false

[dependencies]
http-02 = { package = "http", version = "0.2.9" }
# Enabled with 'tls-boring'
boring-rustls-provider = { git = "https://github.com/janrueth/boring-rustls-provider", optional = true } #
Copy link
Member

Choose a reason for hiding this comment

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

It has nothing todo with default or not. Every dependency should be actively maintained. So I get it, you own this crate

@istio-testing istio-testing merged commit 9f3e0ac into istio:master Feb 28, 2024
3 checks passed
stevenctl pushed a commit to stevenctl/ztunnel that referenced this pull request Mar 11, 2024
* Migrate TLS to rustls

* Makefile improvements

* fix arm64

* copywrite
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long term: Migrate from boring to rustls Boring build broken for non-FIPS Optimize Read/Write syscalls
7 participants