Initial work for introducing openssl/boringssl as a TLS provider

[arnarpall]

In order be able to support running linkerd in fips compliant mode we need replace rustls and ring with openssl
since ring is not fips validated/compliant.

• Added support for openssl
• Testing out Boringssl
• Compilation features flags (openssl-tls, rustls-tls, boring-tls)

[olix0r]

I'll continue looking at the feature flagging scheme to see what changes we can make to our APIs to support this better. In the meantime it would be great to minimize the dependencies for boring-sys.

[olix0r]

Do we really need golang here?

[arnarpall]

Ah yeah that's crazy, the boring-sys crate uses go as part of the whole build process.

We probably have to figure out a better way to handle this docker file.

[olix0r]

We shouldn't need to pull in env-logger... Is this coming from one of the new dependencies?

env-logger should only be needed by binary entry points and not by libraries. And for libraries, the tracing crate is definitely preferable.

[olix0r]

Similarly, this pulls in a second (older) version of ansi_term. It would be best if we could avoid these duplicate deps.

[olix0r]

This is a command-line parsing crate. Why is this getting pulled in?

[olix0r]

It looks like boring-sys should use bindgen without default features (https://github.com/rust-lang/rust-bindgen/blob/b60339ece3994868a55889b7f1f6043ab29ba30e/Cargo.toml#L74-L80) to avoid pulling in unnecessary dependencies.

[olix0r]

Specifically, pulling in log is okay; but pulling in env_logger is undesirable. It seems, though, that bindgen doesn't expose distinct dependencies.

Is bindgen actually a runtime dependency or is it only a build-time dependency? If the latter, it should probably be in dev-dependencies and not dependencies.

[olix0r]

Ah yeah, I see now that it is indeed a build-only dependency in boring-sys. It's probably less of an issue if these aren't actually pulled into the runtime, then.

[olix0r]

Also note that we'll need some DCO sign-off for this change. See https://github.com/linkerd/linkerd2/blob/main/CONTRIBUTING.md#developer-certificate-of-origin. Note that it's not necessary to squash/amend all commits with the sign-off if long as you make a public statement in the PR.

[arnarpall]

I agree to the DCO for all the commits in this PR.

[olix0r]

It looks like the linkerd/identity crate's dependencies need some fixing:

:; (cd linkerd/identity && cargo check)
...
error: aborting due to 25 previous errors

Some errors have detailed explanations: E0412, E0433.
For more information about an error, try `rustc --explain E0412`.
error: could not compile `linkerd-identity`

I suspect related to feature-flagging and conditional compilation.

I don't think the build dependencies are actually a blocker, though we'll probably need to update deny.toml

[hawkw]

i gave this a quick review focusing mostly on style and idiomatic Rust --- I'd like to look closer at the feature-flagging strategy here in the future. the overall strategy --- incapsulating rustls/openssl/boringssl-specific implementation details in private modules that are wrapped by backend-agnostic types to expose them to the rest of the codebase --- definitely looks correct!

some general notes:

• i noticed there's a very large number of unwrap()s in this PR. unwrap will cause the process to panic (crash instantly).

  in general, we should try to avoid using unwrap(), even for errors that are unrecoverable and should cause the proxy to panic --- we should prefer expect for fatal errors, because expect includes a message. in general, we should try to include messages for these errors explaining why the error is fatal and/or why we expect it to not occur in normal operation.

• also, i noticed that there are a lot of println!s, presumably for locally debugging while working on this change. before we merge this, we should try to remove those, or replace them with logging if that information is valuable to record in production.

• some error-handling boilerplate could probably be simplified a lot by using the ? operator in more places

[hawkw]

nit: it looks like the intention of this module is to hide the openssl/boringssl-specific implementation details behind this module's abstraction boundary, so that the rest of the project can be agnostic over whether we are using openssl or rustls?

if so, i think this field should not be pub, to avoid leaking implementation details:

Suggested change

	pub struct Key(pub Arc<PKey<Private>>);
	pub struct Key(Arc<PKey<Private>>);

[arnarpall]

Yeah I totally agree.
I'm looking into what is the best solution here. The reason why this is public at this time is because of the identity vs tls modules.

When constructing a TlsConnector I need access to the primitive type Pkey from within the tls module.
I'm pretty sure this is something that can be solved by implementing From and Into traits but I'm not seeing the solution at this time.

[arnarpall]

@hawkw @olix0r could you guys give the pull request another look over ?

I believe I've fixed most of your concerns @hawkw aside from the public fields in a few places. It would be awesome if we could perhaps try to sync up at some point and figure out a good way to hide these fields.

I have not fixed the build/dependency issues that you mentioned @olix0r I will circle back to that.

[codecov-commenter]

Codecov Report

Merging #956 (f416e8b) into main (8fa04f5) will decrease coverage by 1.07%.
The diff coverage is 37.28%.

@@            Coverage Diff             @@
##             main     #956      +/-   ##
==========================================
- Coverage   74.45%   73.38%   -1.08%     
==========================================
  Files         233      238       +5     
  Lines       14671    15041     +370     
==========================================
+ Hits        10924    11038     +114     
- Misses       3747     4003     +256     

Impacted Files	Coverage Δ
linkerd/app/inbound/src/direct.rs	23.17% <0.00%> (+0.55%)	⬆️
linkerd/identity/src/imp/openssl.rs	0.00% <0.00%> (ø)
linkerd/io/src/lib.rs	77.77% <ø> (+8.54%)	⬆️
linkerd/proxy/identity/src/certify.rs	82.22% <ø> (ø)
linkerd/tls/src/imp/openssl.rs	0.00% <0.00%> (ø)
linkerd/tls/src/protocol.rs	0.00% <0.00%> (ø)
linkerd/identity/src/lib.rs	69.47% <52.08%> (-2.83%)	⬇️
linkerd/identity/src/imp/rustls.rs	62.09% <62.09%> (ø)
linkerd/tls/src/client.rs	56.33% <63.15%> (+3.70%)	⬆️
linkerd/tls/src/imp/rustls.rs	78.78% <78.78%> (ø)
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fa04f5...f416e8b. Read the comment docs.

[olix0r]

Last week I spent a few days trying to rework the TLS and identity modules. That work is incomplete but shared here. The summary is:

• The TLS module is generic over the actual TLS implementation (which holds the config types -- these should not be exposed generally).
• The identity module holds only the core structs & traits needed to implement identity. These traits model clients and servers for TLS initiation and termination.
• the proxy-identity module communicates with the API but delegates validation to the underlying identity implementation.
• at startup, the proxy is configured with an identity mode that determines which implementation is used from the implementations compiled with the proxy. This lets us play well with the Rust feature system so that features are purely additive and do not conflict.

I had to put this down to focus on inbound policy changes, but I should be able to revive this work once our access policy work lands.

[olix0r]

Closing this out in favor of #1351, which takes advantage of recent API changes to support compilation with --all-features (i.e. rustls and boring are not mutually exclusive).

There are some issues we'll need to resolve before merging that. Notably: ALPN support & trust anchor parsing