meshtls: Add a `boring` backend #1351

olix0r · 2021-11-02T16:25:03Z

This change adds a meshtls-boring proxy feature that can be used to
compile the proxy with an alternate TLS implementation. The
meshtls-rustls feature should be disabled to take advantage of this
alternate backend.

In its current mode, the boring backend is compatible with the existing
identity credentials and algorithms (specifically TLSv1.3 and
ECDSA-P256-SHA256 with CHACHA20-POLY1305-SHA256).

In future changes--once boring has been updated--we can:

Improve error handling, especially for SSL errors
Relax deny.toml changes needed by bindgen features
Add a FIPS mode

Co-authored-by: @arnarpall

olix0r · 2021-11-02T17:05:12Z

Related cloudflare/boring#55

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

This branch adds a simple nix-env configuration for building the proxy locally on NixOS. This does *not* yet introduce a derivation for *packaging* the proxy as a Nix package, just a nix-env for local development for Nix users. Actually packaging the proxy for Nix could be fun, but it's not really necessary, since the proxy is not currently distributed as a package for other package managers --- it's distributed as a docker image. If we were going to actually distribute something as a Nix package, it would be the Linkerd CLI (which could be worth doing!). This branch *does* include the necessary configuration to build `cloudflare/boring`, which is added as a dependency by @olix0r's PR #1351. I've confirmed that it is possible to build that branch with these configs, but I opened this as a separate PR against `main` so that we can merge it separately. Currently, we do have to build `boringssl` from source, rather than depending on it from nixpkgs, which is kind of a bummer, but this can be fixed later.

This change adds a `meshtls-boring` proxy feature that can be used to compile the proxy with an alternate TLS implementation. The `meshtls-rustls` feature should be disabled to take advantage of this alternate backend. In its current mode, the boring backend is compatible with the existing identity credentials and algorithms (specifically TLSv1.3 and ECDSA-P256-SHA256 with CHACHA20-POLY1305-SHA256). In future changes--once `boring` has been updated--we can: - Improve error handling, especially for SSL errors - Relax deny.toml changes needed by bindgen features - Add a FIPS mode Co-authored-by: Arnar Páll <[email protected]>

olix0r · 2021-11-08T18:42:30Z

@hawkw @arnarpall This change should be mergeable now (once CI clears). We run our normal end-to-end tests against the boring module and I've tested this branch manually as well.

This change does not include the FIPS functionality being discussed in cloudflare/boring#52; and it sounds like even once that change merges (if it lands in its current state), it will be difficult to build in that mode (as a FIPS-compliant boring library will have to be provided externally).

I haven't taken advantage of any of the recently changed error functionality either. We'll pick that up once boring does a proper crate release.

arnarpall · 2021-11-08T18:55:11Z

@hawkw @arnarpall This change should be mergeable now (once CI clears). We run our normal end-to-end tests against the boring module and I've tested this branch manually as well.

This change does not include the FIPS functionality being discussed in cloudflare/boring#52; and it sounds like even once that change merges (if it lands in its current state), it will be difficult to build in that mode (as a FIPS-compliant boring library will have to be provided externally).

I haven't taken advantage of any of the recently changed error functionality either. We'll pick that up once boring does a proper crate release.

Truly awesome work.

I believe I could rework my original boring pull request once this one is merged where we hook into the boring build process to enable fips mode without having to to provide it externally. I'd like to get to a point where this is as transparent as possible

hawkw

overall, this looks good to me. i had a couple questions and minor nits but nothing stood out as a blocker!

Dockerfile

hawkw · 2021-11-08T19:29:32Z

deny.toml

+    # boring-sys pulls in an old version via bindgen. See
+    # https://github.com/cloudflare/boring/pull/55.
+    { name = "ansi_term" },


nit/tioli: it seems like this might, potentially, be better expressed as a skip-tree entry for bindgen? that way, we still get an error on duplicate deps if they're from another crate.

not actually important, of course.

this would also ignore all other bindgen deps, though. I think the skip is narrower.

linkerd/meshtls/boring/src/client.rs

hawkw · 2021-11-08T20:11:08Z

linkerd/meshtls/boring/src/creds.rs

+        // XXX(ver) This function reads from the environment and/or the filesystem. This likely is
+        // at best wasteful and at worst unsafe (if another thread were to mutate these environment
+        // variables simultaneously, for instance). Unfortunately, the boring APIs don't really give
+        // us an alternative AFAICT.


do you know if this is something that can be fixed upstream in the boring crate, or is it a boringssl behavior that we can't avoid?

boring could probably have other constructors that don't load default trust anchors. I don't think it actually matters for us, in practice, though.

that's fair; it seems like something that could be worth opening an issue for, but maybe not worth putting effort into implementing?

hawkw · 2021-11-08T20:11:23Z

linkerd/meshtls/boring/src/creds.rs

+        // XXX(ver) if we disable use of TLSv1.2, connections just hang.
+        //conn.set_options(ssl::SslOptions::NO_TLSV1_2);


should we report a bug for this upstream?

maybe? this is probably a bug in boringssl and not the rust bindings, if it's a bug.

hawkw · 2021-11-08T20:38:18Z

linkerd/meshtls/boring/src/lib.rs

+//! A new SSL context is created for each connection. This is probably
+//! unnecessary, but it's simpler for now. We can revisit this if needed.


not a blocker at all, just curious: how much do we know about what this actually means? e.g. do we know what a "SSL context" in boring consists of, and what are the implications of creating them?

Some allocations + everything in the connector/acceptor constructors (i.e. trust anchors, private key, certs).

linkerd/meshtls/boring/src/server.rs

linkerd/meshtls/src/server.rs

This release updates the proxy's `rustls`, `ring`, and `webpki` dependencies. Additionally, the proxy can now be built to use a `boringssl` backend instead of the default `rustls` backend, but this functionality is disabled in default builds. --- * meshtls: replace build script with `compile_error!` macro (linkerd/linkerd2-proxy#1357) * ci: Split actions into several workflows (linkerd/linkerd2-proxy#1356) * ci: Make job names uniform (linkerd/linkerd2-proxy#1358) * meshtls: allow building without any TLS impls enabled (linkerd/linkerd2-proxy#1359) * `app-core` should not enable `meshtls-rustls` (linkerd/linkerd2-proxy#1360) * Restore rustls credential tests (linkerd/linkerd2-proxy#1363) * build(deps): bump hex from 0.3 to 0.4 (linkerd/linkerd2-proxy#1364) * ci: Split jobs into 'fast' and 'slow' workflows (linkerd/linkerd2-proxy#1365) * meshtls: Move TLS e2e tests into the meshtls crate (linkerd/linkerd2-proxy#1366) * rustls: Tidy std::task imports (linkerd/linkerd2-proxy#1367) * build(deps): bump serde_json from 1.0.68 to 1.0.69 (linkerd/linkerd2-proxy#1368) * build(deps): bump libc from 0.2.106 to 0.2.107 (linkerd/linkerd2-proxy#1369) * meshtls: Add a `boring` backend (linkerd/linkerd2-proxy#1351) * meshtls-rustls: update to `rustls` 0.20 and `tokio-rustls` 0.23 (linkerd/linkerd2-proxy#1362)

olix0r added 6 commits November 2, 2021 16:19

wip: layout

e188d6a

wip

05a4f53

wip

00c2dba

wip

4fa0cc5

sketch out boring initialization

8e24034

rename crates: identity-default => identity-rustls-meshtls

f2d9ab8

olix0r changed the title ~~identity: Add a boring-backed~~ identity: Add a boring-backed mTLS implementation Nov 2, 2021

olix0r added 15 commits November 2, 2021 17:15

add cmake to actions

b9b41c8

more deps. yay.

c31ca52

+deny

ead49f8

wip: client compiles

fe0ed29

+ServerIo

6742b52

Address warnings

4bbbeb7

Add ALPN todo

fc452e4

Get the facade compiling

c6721ea

+ServerIo

08b44ae

Move union types into the meshtls crate

e4d2f2a

Move rustls and boring modules under mtls

8bb531f

Split meshtls crate into multiple modules

bc38ee3

fixup tokio features

0bd1104

Merge branch 'main' into ver/boring

ed875aa

fixup fuzzer

56da589

olix0r mentioned this pull request Nov 3, 2021

Initial work for introducing openssl/boringssl as a TLS provider #956

Closed

olix0r mentioned this pull request Nov 3, 2021

Introduce meshtls facade to hide rustls crate #1353

Merged

hawkw mentioned this pull request Nov 3, 2021

build: add nix-env for building the proxy #1354

Closed

Merge branch 'ver/boring-minus-boring' into ver/boring

9a35243

olix0r and others added 17 commits November 8, 2021 15:28

split docs check

e505803

build docs with tests

1c481c4

avoid building app-test in fast build

c44790b

go back to separate docs job

db9210d

release build should actually set --release

5028ebb

Merge branch 'main' into ver/boring

d6fdd85

restore deps tests

1445c0c

ws

fdad3e7

test timeouts

2ecfdf9

undo needless changes

9b5e53b

fixup boring dev deps

3dbc316

back-out needless change to slow.yml

cc50fa1

back-out needless change to fast.yml

53dfd06

wrap comments more tightly

92d4eb9

back-out rustls changes in favor of #1367

eb77991

-dead_code

5716f5c

olix0r marked this pull request as ready for review November 8, 2021 18:36

olix0r requested a review from a team November 8, 2021 18:36

olix0r changed the title ~~identity: Add a boring-backed mTLS implementation~~ meshtls: Add a boring backend Nov 8, 2021

hawkw approved these changes Nov 8, 2021

View reviewed changes

olix0r added 2 commits November 8, 2021 21:19

needless match

845bd7f

let with_alpn clear alpn protocols as well

1d3227f

olix0r merged commit 50003a4 into main Nov 8, 2021

olix0r deleted the ver/boring branch November 8, 2021 21:44

olix0r mentioned this pull request Nov 9, 2021

proxy: v2.166.0 linkerd/linkerd2#7244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

meshtls: Add a `boring` backend #1351

meshtls: Add a `boring` backend #1351

olix0r commented Nov 2, 2021 •

edited

Loading

olix0r commented Nov 2, 2021

olix0r commented Nov 8, 2021 •

edited

Loading

arnarpall commented Nov 8, 2021

hawkw left a comment

hawkw Nov 8, 2021

olix0r Nov 8, 2021

hawkw Nov 8, 2021

olix0r Nov 8, 2021

hawkw Nov 8, 2021

hawkw Nov 8, 2021

olix0r Nov 8, 2021

hawkw Nov 8, 2021

olix0r Nov 8, 2021

		// XXX(ver) if we disable use of TLSv1.2, connections just hang.
		//conn.set_options(ssl::SslOptions::NO_TLSV1_2);

		//! A new SSL context is created for each connection. This is probably
		//! unnecessary, but it's simpler for now. We can revisit this if needed.

meshtls: Add a boring backend #1351

meshtls: Add a boring backend #1351

Conversation

olix0r commented Nov 2, 2021 • edited Loading

olix0r commented Nov 2, 2021

olix0r commented Nov 8, 2021 • edited Loading

arnarpall commented Nov 8, 2021

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meshtls: Add a `boring` backend #1351

meshtls: Add a `boring` backend #1351

olix0r commented Nov 2, 2021 •

edited

Loading

olix0r commented Nov 8, 2021 •

edited

Loading