Refactor mTLS & identity crates #1333

olix0r · 2021-10-26T03:49:25Z

The linkerd-proxy-identity crate, which is responsible for obtaining
certificates from the identity service, currently has a hard dependency
on our linkerd-tls-rustls crate.

This change introduces trait
so that the identity API client can be generic over the TLS
implementation that receives certificate updates.

To accomodate this, the identity/mtls crates are refactored as follows:

linkerd-proxy-identity gets a new Credentials trait so that the
certify module is responsible for refreshing certificates from the
API without hardcoding a TLS implementation.
The linkerd-tls-rustls crate is moved to
linkerd-proxy-identity-default and compltely encapsulates the
ring/rustls/webpki dependencies. This module provides an
implementation of Credentials that validates and publishes rustls
configurations to receivers--either NewClient or Server.
The app crates currently have a hard dependency on the
identity-default crate, but this will be made configurable in
follow-up changes.

There are no functional changes.

The `linkerd-proxy-identity` crate, which is responsible for obtaining certificates from the identity service, currently has a hard dependency on our rustls crate. This change introduces a `linkerd_proxy_identity::Credentials` trait which provides an API for publishing certificates obtained from the identity service.

The `linkerd-tls` crate has a test dependency on `linkerd-tls-rustls`, which itself depends on `linkerd-tls`. This is an avoidable circular dependency. This change moves all of the TLS testdata from the `linkerd-tls-rustls` crate to the new `linkerd-tls-test-util` crate. The `tls_accept` test is moved from `linkerd-tls` to `linkerd-tls-rustls` so that `linkerd-tls` no longer depends on `linkerd-tls-rustls`.

The `linkerd-proxy-identity` crate, which is responsible for obtaining certificates from the identity service, currently has a hard dependency on our `linkerd-tls-rustls` crate. This change introduces trait so that the identity API client can be generic over the TLS implementation that receives certificate updates. To accomodate this, the identity/mtls crates are refactored as follows: * `linkerd-proxy-identity` gets a new `Credentials` trait so that the `certify` module is responsible for refreshing certificates from the API without hardcoding a TLS implementation. * The `linkerd-tls-rustls` crate is moved to `linkerd-proxy-identity-default` and compltely encapsulates the ring/rustls/webpki dependencies. This module provides an implementation of `Credentials` that validates and publishes rustls configurations to receivers--either `NewClient` or `Server`. * The _app_ crates currently have a hard dependency on the `identity-default` crate, but this will be made configurable in follow-up changes. There are no functional changes.

hawkw

sorry it took so long to read through this one --- for the most part, this looks good to me. i noticed one thing that seems like a potential bug (the JoinHandle in Server) and it would be nice to know what's going on there. other than that, most of my comments were style and naming nits, or questions that would be nice to answer, not blockers.

linkerd/app/admin/src/stack.rs

hawkw · 2021-10-26T21:35:35Z

linkerd/app/src/identity.rs

+}
+
+#[derive(Clone)]
+pub struct Documents {


cute name :)

hawkw · 2021-10-26T22:41:07Z

linkerd/app/src/identity.rs

+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("Documents")
+            .field("id", &self.id)
+            .field("trust_anchors_pem", &self.trust_anchors_pem)


nit/tioli: maybe worth a comment on why some fields aren't included?

linkerd/proxy/identity/default/src/creds.rs

linkerd/proxy/identity/default/src/server.rs

linkerd/proxy/identity/src/lib.rs

hawkw · 2021-10-27T20:33:42Z

linkerd/proxy/identity/src/metrics.rs

+    expiry: Arc<Mutex<SystemTime>>,
+    refreshes: Arc<Counter>,


is there a reason we need two separate Arcs for this? can't there just be one Arc around the Metrics?

not a blocker.

no... but I'll probably do some more work on the metrics before this is done and will address it there.

hawkw · 2021-10-27T20:36:08Z

linkerd/proxy/tap/src/accept.rs

@@ -5,6 +5,7 @@ use linkerd_conditional::Conditional;
 use linkerd_error::Error;
 use linkerd_io as io;
 use linkerd_proxy_http::{trace, HyperServerSvc};
+use linkerd_proxy_identity_default as mtls;


nit/tioli: elsewhere we import this as identity, should we maybe be consistent about that? IMO mtls is maybe clearer at least when the IO types are imported...not a big deal though!

so, I didn't want to just use an Arc<Notify>, since then there's an ordering concern about whether the receiver watches before the first notification is sent. We could use an Arc with an AtomicBool, but it seems easier just to use the watch and as this is only used during startup, I'm not really worried about minimizing overhead absolutely. I'd rather be more confident we don't have subtle bugs.

(i think this was supposed to be a reply to a different comment) sure, that makes sense, i just wasn't entirely clear on why the bool was important, which is what i wanted to understand here...

linkerd/app/src/identity.rs

* stop storing the ALPN background task handle and instead handle task completion more gracefully. * more comments * avoid a clone

hawkw · 2021-10-27T21:24:55Z

linkerd/proxy/identity/default/src/server.rs

+        // The background task completes when the original sender is closed or when all receivers
+        // are dropped.


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)

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)

olix0r added 29 commits October 22, 2021 22:51

wip

726ce9e

wip

8bd76e9

touchups

1f074fe

wip

963c2c2

wip

85e7d68

wip

e927c86

wip

2175fd2

wip

679f37a

Test updates

4005eca

Test updates

d207f9a

Simplify server config with a default

6af5e19

Update TLS tests (moved into rustls)

bb7be22

Use rustls::creds::Receiver in runtime

3636265

fixup inbound tests

967dd74

fix outbound tests

b9e0f43

fix admin

87ae0df

Port config over

0aa9cb5

fixup

8163c26

compiles

809d1c0

fixup tests

46e87bb

Terminate => Server

4d15d05

-needless dep

3d9f8f1

fmt

6d489e7

Touchup Credentials API

d5a3e67

Cleanup rustls credential store

06b9bd7

fixup tls dev deps

3d1e9c9

move rustls rate to proxy/identity/rustls

8740f44

olix0r mentioned this pull request Oct 26, 2021

tls: Avoid circular dependency in tests #1334

Merged

olix0r added 2 commits October 26, 2021 17:04

Merge branch 'ver/tls-test-util' into ver/proxy-identity-split

0a82029

fix fuzzer

2337c8c

olix0r changed the base branch from main to ver/tls-test-util October 26, 2021 17:26

Base automatically changed from ver/tls-test-util to main October 26, 2021 18:10

olix0r added 6 commits October 26, 2021 18:26

Merge branch 'main' into ver/proxy-identity-split

bc2811d

-redundant dep

8f5b6b8

Merge branch 'main' into ver/proxy-identity-split

3038df4

Don't reexport rustls types outside of the identity-rustls crate

23d80e3

rename identity-rustls to identity-default

f8184ad

olix0r changed the title ~~Decouple identity renewal from the TLS implementation~~ Refactor mTLS & identity crates Oct 26, 2021

olix0r marked this pull request as ready for review October 26, 2021 19:53

olix0r requested a review from a team October 26, 2021 19:53

olix0r added 2 commits October 26, 2021 19:54

undo libc downgrade

08466da

Merge branch 'main' into ver/proxy-identity-split

f9e81c1

hawkw reviewed Oct 27, 2021

View reviewed changes

olix0r added 3 commits October 27, 2021 21:22

review feedback

baa4480

* stop storing the ALPN background task handle and instead handle task completion more gracefully. * more comments * avoid a clone

typo

63ee3ef

naming

bbb009b

hawkw approved these changes Oct 27, 2021

View reviewed changes

naming

1fd2d35

olix0r merged commit 2dd7bb8 into main Oct 27, 2021

olix0r deleted the ver/proxy-identity-split branch October 27, 2021 22:36

olix0r mentioned this pull request Nov 4, 2021

proxy: v2.165.1 linkerd/linkerd2#7224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor mTLS & identity crates #1333

Refactor mTLS & identity crates #1333

olix0r commented Oct 26, 2021 •

edited

Loading

hawkw left a comment

hawkw Oct 26, 2021

hawkw Oct 26, 2021

hawkw Oct 27, 2021

olix0r Oct 27, 2021

hawkw Oct 27, 2021

olix0r Oct 27, 2021

hawkw Oct 27, 2021

hawkw Oct 27, 2021

		// The background task completes when the original sender is closed or when all receivers
		// are dropped.

Refactor mTLS & identity crates #1333

Refactor mTLS & identity crates #1333

Conversation

olix0r commented Oct 26, 2021 • edited Loading

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

olix0r commented Oct 26, 2021 •

edited

Loading