Introduce meshtls facade to hide rustls crate#1353
Merged
Conversation
In #1351, we add an alternate identity/mtls implementation that uses `boring`. To setup for that, this change introduces a new `meshtls` crate that serves as a facade for application crates to depend on, independently of the actual crypto implementation. This change does not change any runtime logic and sets up for #1351 to enable an alternate TLS implementation as a build-time configuration.
olix0r
force-pushed
the
ver/boring-minus-boring
branch
from
a84590d to
5ffc10b
Compare
hawkw
approved these changes
Nov 3, 2021
Contributor
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
overall, this seems like the best possible way i could think of to structure the facade crate; it feels a big ugly, but i feel like abstracting over the different implementations with feature flags is always going to be ugly, so this looks like the neatest way of doing it.
i think it could be worth having the various unreachable!() panics include messages, just in case someone compiles a build where all the TLS implementations are disabled? but, that's not really a blocker.
also, i noticed a couple places where it looks like we forgot to forward an implementation through a wrapper; we should probably fix that before merging this?
other than that, lgtm!
linkerd/meshtls/src/client.rs
Outdated
| return Connect::Rustls(new_client.new_service(target)); | ||
| } | ||
|
|
||
| unreachable!() |
Contributor
There was a problem hiding this comment.
nit/TIOLI: not a big deal, but maybe this should say something like
Suggested change
| unreachable!() | |
| unreachable!("attempted o initiate a mTLS connection, but no TLS backends were enabled at compile time!") |
or something, just in case someone makes a build of the proxy where no backends were enabled?
Contributor
There was a problem hiding this comment.
alternatively, we could probably make this unreachable at compile-time by putting #[cfg(any(feature = "rustls", feature = "boring", ...etc))] on the Service impl (and other stuff in this crate), but i'm not sure if that would make other stuff more complicated...
Member
Author
There was a problem hiding this comment.
I changed this to use a match for all of these so there's no longer an unreachable case. I've added a build.rs check that ensures at least one feature is enabled.
hawkw
reviewed
Nov 3, 2021
Comment on lines
+2
to
+15
| // Ensure that at least one TLS implementation feature is enabled. | ||
| static TLS_FEATURES: &[&str] = &["rustls"]; | ||
| if !TLS_FEATURES | ||
| .iter() | ||
| .any(|f| std::env::var_os(&*format!("CARGO_FEATURE_{}", f.to_ascii_uppercase())).is_some()) | ||
| { | ||
| return Err(format!( | ||
| "at least one of the following TLS implementations must be enabled: '{}'", | ||
| TLS_FEATURES.join("', '"), | ||
| ) | ||
| .into()); | ||
| } | ||
|
|
||
| Ok(()) |
Contributor
There was a problem hiding this comment.
FWIW, I think this could be achieved without a build script by just sticking
#![cfg(not(any(feature = "rustls"))]
compile_error!("at least one of the following TLS implementations must be enabled: 'rustls}''")in lib.rs (and adding the other implementation feature flags as needed)
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Nov 4, 2021
This release improves retries so that requests without a `content-length` can be retried. This should permit requests emitted by grpc-go to be retried. Discovery diagnostics have also been improved by ensuring that service discovery updates are logged at DEBUG. Previously these messages were only emitted at the TRACE level. --- * build(deps): bump hdrhistogram from 7.3.0 to 7.4.0 (linkerd/linkerd2-proxy#1330) * build(deps): bump libc from 0.2.104 to 0.2.105 (linkerd/linkerd2-proxy#1332) * tracing: update `tracing-subscriber` to v0.3.x (linkerd/linkerd2-proxy#1327) * tls: Avoid circular dependencies (linkerd/linkerd2-proxy#1334) * Fix misspecified dependencies (linkerd/linkerd2-proxy#1335) * build(deps): bump tracing-subscriber from 0.2.25 to 0.3.1 (linkerd/linkerd2-proxy#1328) * update `tonic`, `prost`, and `linkerd2-proxy-api` (linkerd/linkerd2-proxy#1339) * Refactor mTLS & identity crates (linkerd/linkerd2-proxy#1333) * Log discovery changes at DEBUG (linkerd/linkerd2-proxy#1340) * build(deps): bump tokio-util from 0.6.8 to 0.6.9 (linkerd/linkerd2-proxy#1342) * build(deps): bump tokio from 1.12.0 to 1.13.0 (linkerd/linkerd2-proxy#1343) * build(deps): bump tokio-stream from 0.1.7 to 0.1.8 (linkerd/linkerd2-proxy#1344) * retry: allow retrying requests without content-length headers (linkerd/linkerd2-proxy#1341) * retry: Simplify ReplayBody::poll_data for readability (linkerd/linkerd2-proxy#1346) * build(deps): bump libc from 0.2.105 to 0.2.106 (linkerd/linkerd2-proxy#1348) * reorg: Decouple TLS implementation from proxy client (linkerd/linkerd2-proxy#1349) * build(deps): bump actions/checkout from 2.3.5 to 2.4.0 (linkerd/linkerd2-proxy#1352) * Introduce `meshtls` facade to hide rustls crate (linkerd/linkerd2-proxy#1353)
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Nov 5, 2021
This release improves retries so that requests without a `content-length` can be retried. This should permit requests emitted by grpc-go to be retried. Discovery diagnostics have also been improved by ensuring that service discovery updates are logged at DEBUG. Previously these messages were only emitted at the TRACE level. --- * build(deps): bump hdrhistogram from 7.3.0 to 7.4.0 (linkerd/linkerd2-proxy#1330) * build(deps): bump libc from 0.2.104 to 0.2.105 (linkerd/linkerd2-proxy#1332) * tracing: update `tracing-subscriber` to v0.3.x (linkerd/linkerd2-proxy#1327) * tls: Avoid circular dependencies (linkerd/linkerd2-proxy#1334) * Fix misspecified dependencies (linkerd/linkerd2-proxy#1335) * build(deps): bump tracing-subscriber from 0.2.25 to 0.3.1 (linkerd/linkerd2-proxy#1328) * update `tonic`, `prost`, and `linkerd2-proxy-api` (linkerd/linkerd2-proxy#1339) * Refactor mTLS & identity crates (linkerd/linkerd2-proxy#1333) * Log discovery changes at DEBUG (linkerd/linkerd2-proxy#1340) * build(deps): bump tokio-util from 0.6.8 to 0.6.9 (linkerd/linkerd2-proxy#1342) * build(deps): bump tokio from 1.12.0 to 1.13.0 (linkerd/linkerd2-proxy#1343) * build(deps): bump tokio-stream from 0.1.7 to 0.1.8 (linkerd/linkerd2-proxy#1344) * retry: allow retrying requests without content-length headers (linkerd/linkerd2-proxy#1341) * retry: Simplify ReplayBody::poll_data for readability (linkerd/linkerd2-proxy#1346) * build(deps): bump libc from 0.2.105 to 0.2.106 (linkerd/linkerd2-proxy#1348) * reorg: Decouple TLS implementation from proxy client (linkerd/linkerd2-proxy#1349) * build(deps): bump actions/checkout from 2.3.5 to 2.4.0 (linkerd/linkerd2-proxy#1352) * Introduce `meshtls` facade to hide rustls crate (linkerd/linkerd2-proxy#1353) * rustls: Configure the initial TLS client with trust roots (linkerd/linkerd2-proxy#1355)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #1351, we add an alternate identity/mtls implementation that uses
boring. To setup for that, this change introduces a newmeshtlscrate that serves as a facade for application crates to depend on,
independently of the actual crypto implementation.
This change does not change any runtime logic and sets up for #1351 to
enable an alternate TLS implementation as a build-time configuration.