tls: Disambiguate client and server identities #855

olix0r · 2021-01-18T20:46:15Z

The tls::PeerIdentity type is used to describe both remote clients and
servers. This can easily lead to confusion, as it can be ambiguous as to
whether an identity is a client's identity or a target server's
identity.

This change introduces new marker types:

identity::LocalId: The local proxy's ID;
tls::server::ClientId: A remote client ID; and
tls::client::ServerId: A target server ID.

Furthermore, the tls::ReasonForNoPeerName has been split into distinct
tls::server::NoClientId and tls::client::NoServerId types. This
change eliminates the tls::HasPeerIdentity and
tls::{client, server}::HasConfig types, in favor of simple Into coercions.

This change requires changes to the metric labeling, though no functional
changes have been introduced.

`tls::accept` has a `Detectable` trait which allows TLS detection to use `TcpStream::peek`; but this is inflexible if we want to wrap `TcpStream` with any additional behavior and it limits our ability to write tests for this module. This change introduces a new `io::Peek` trait to model `TcpStream::peek` and removes the `tls::accept::Detectable` trait.

The `tls::PeerIdentity` type is used to describe both remote clients and servers. This can easily lead to confusion, as it can be ambiguous as to whether an identity is a client's identity or a target server's identity. This change introduces new marker types: - `identity::LocalId`: The local proxy's ID; - `tls::server::ClientId`: A remote client ID; and - `tls::client::ServerId`: A target server ID. Furthermore, the `tls::ReasonForNoPeerName` has been split into distinct `tls::server::NoClientId` and `tls::client::NoServerId` types. This change eliminates the `tls::HasPeerIdentity` and `tls::{client, server}::HasConfig` types, in favor of simple `Into` coercions. This change requires changes to the metric labeling.

hawkw

this is a big change, but most of it is just renaming. overall, I think this is definitely much clearer! i picked a few nits but didn't see any serious blockers.

hawkw · 2021-01-20T18:29:51Z

linkerd/app/core/src/metrics.rs

+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+pub struct InboundEndpointLabels {
+    pub client_id: tls::server::ConditionalTls,
+    pub authority: Option<http::uri::Authority>,


was kind of wondering if this should be on the EndpointLabels type, since both directions have an authority, but i realize that just complicates the Into impls a bunch so nvm.

hawkw · 2021-01-20T18:34:26Z

linkerd/app/inbound/src/lib.rs

+mod allow_discovery;
+mod prevent_loop;
+mod require_identity_for_ports;
+pub mod target;


nit: any reason for moving this?

It's kind of become the idiom to put modules before imports... From a discoverability point of view: easier to see all the modules as soon as you open the file, and then imports are closer uses (and reference modules that have already been defined). Just a nit really

hawkw · 2021-01-20T18:34:34Z

linkerd/app/inbound/src/lib.rs

@@ -171,7 +169,7 @@ impl Config {
                svc::stack(tcp_forward)
                    .push_map_target(TcpEndpoint::from)
                    .push(metrics.transport.layer_accept())
-                    .push_map_target(TcpAccept::from)
+                    .push_map_target(TcpAccept::port_skipped)


hawkw · 2021-01-20T18:37:31Z

linkerd/app/test/src/http_util.rs

@@ -13,7 +8,7 @@ use tracing::Instrument;

 pub struct Server {
    settings: hyper::server::conn::Http,
-    identity: Option<PeerIdentity>,
+    expected_id: Option<tls::ConditionalServerId>,


👍 for renaming this

linkerd/identity/src/lib.rs

hawkw · 2021-01-20T18:43:23Z

linkerd/proxy/tap/src/grpc/server.rs

-                        .insert("client_id".to_owned(), id.as_ref().to_owned());
+                    m.labels.insert(
+                        "client_id".to_owned(),
+                        id.map(|id| id.to_string()).unwrap_or_default(),


nit/TIOLI:

Suggested change

id.map(|id| id.to_string()).unwrap_or_default(),

id.map(ToString::to_string).unwrap_or_default(),

linkerd/tls/src/client.rs

hawkw · 2021-01-20T18:45:12Z

linkerd/tls/src/client.rs

-    fn tls_client_config(&self) -> Arc<Config>;
+/// A marker type for target server identities.
+#[derive(Clone, Debug, Eq, PartialEq, Hash)]
+pub struct ServerId(pub id::Name);


it feels weird elsewhere to see a type named ServerId imported from the client module, but after actually thinking about it, it does make sense...and I don't think we should give this a wordier name to clarify that. so 👍

linkerd/tls/src/server.rs

hawkw · 2021-01-20T18:48:50Z

linkerd/tls/src/server.rs

 }

+pub type ConditionalTls = Conditional<Option<ClientId>, NoTls>;


nit/TIOLI: it's a little bit weird that the corresponding type in the client module is ConditionalServerId, but this is ConditionalTls...but, it makes sense because here, on the accept side, we may be doing TLS without a client identity if it's not mutual TLS. I think the names should stay as they are, but it's maybe worth a comment explaining this, though?

This release improves diagnostics about the proxy's failfast state: * Warnings are now emitted when the failfast state is entered; * The "max concurrency exhausted" gRPC message has been changed to more-clearly indicate a failfast state error; and * Failfast recovery has been made more robust, ensuring that a service can recover indepenently of new requests being received. Furthermore, metric labeling has been improved: * TCP server metrics are now annotated with the original `target_addr`; * The `tls` label is now set to true for inbound TLS connections that lack a client ID. This is mostly helpful to clarify inbound metrics on the `identity` controller; * Outbound `tls` metrics could be reported incorrectly when a proxy was configured to not use identity. This has been corrected. Finally, socket-level errors now include a _client_ or _server_ prefix to indicate which side of the proxy encountered the error. --- * stack: remove `map_response` (linkerd/linkerd2-proxy#835) * replace `RequestFilter` with Tower's upstream impl (linkerd/linkerd2-proxy#842) * tracing: fix incorrect field format when logging in JSON (linkerd/linkerd2-proxy#845) * replace `FutureService` with Tower's upstream impl (linkerd/linkerd2-proxy#839) * integration: improve tracing in tests (linkerd/linkerd2-proxy#846) * service-profiles: Prevent Duration coercion panics (linkerd/linkerd2-proxy#844) * inbound: Separate HTTP server logic from protocol detection (linkerd/linkerd2-proxy#843) * Correct gRPC 'max-concurrency exhausted' error messages (linkerd/linkerd2-proxy#847) * Update tonic to v0.4 (linkerd/linkerd2-proxy#849) * failfast: Improve diagnostic logging (linkerd/linkerd2-proxy#848) * Update the base docker image (linkerd/linkerd2-proxy#850) * stack: Implement Clone for ResultService (linkerd/linkerd2-proxy#851) * Ensure services in failfast can become ready (linkerd/linkerd2-proxy#858) * tests: replace string matching on metrics with parsing (linkerd/linkerd2-proxy#859) * Decouple tls::accept from TcpStream (linkerd/linkerd2-proxy#853) * metrics: Handle NoPeerIdFromRemote properly (linkerd/linkerd2-proxy#857) * metrics: Reorder metrics labels (linkerd/linkerd2-proxy#856) * Rename tls::accept to tls::server (linkerd/linkerd2-proxy#854) * Annotate socket-level errors with a scope (linkerd/linkerd2-proxy#852) * test: reduce repetition in metrics tests (linkerd/linkerd2-proxy#860) * tls: Disambiguate client and server identities (linkerd/linkerd2-proxy#855) * Update to tower v0.4.4 (linkerd/linkerd2-proxy#864) * Update cargo dependencies (linkerd/linkerd2-proxy#865) * metrics: add `target_addr` label for accepted transport metrics (linkerd/linkerd2-proxy#861) * outbound: Strip endpoint identity when disabled (linkerd/linkerd2-proxy#862) --- The opaque-ports test has been updated to reflect proxy metrics changes.

This release improves diagnostics about the proxy's failfast state: * Warnings are now emitted when the failfast state is entered; * The "max concurrency exhausted" gRPC message has been changed to more-clearly indicate a failfast state error; and * Failfast recovery has been made more robust, ensuring that a service can recover indepenently of new requests being received. Furthermore, metric labeling has been improved: * TCP server metrics are now annotated with the original `target_addr`; * The `tls` label is now set to true for inbound TLS connections that lack a client ID. This is mostly helpful to clarify inbound metrics on the `identity` controller; * Outbound `tls` metrics could be reported incorrectly when a proxy was configured to not use identity. This has been corrected. Finally, socket-level errors now include a _client_ or _server_ prefix to indicate which side of the proxy encountered the error. --- * stack: remove `map_response` (linkerd/linkerd2-proxy#835) * replace `RequestFilter` with Tower's upstream impl (linkerd/linkerd2-proxy#842) * tracing: fix incorrect field format when logging in JSON (linkerd/linkerd2-proxy#845) * replace `FutureService` with Tower's upstream impl (linkerd/linkerd2-proxy#839) * integration: improve tracing in tests (linkerd/linkerd2-proxy#846) * service-profiles: Prevent Duration coercion panics (linkerd/linkerd2-proxy#844) * inbound: Separate HTTP server logic from protocol detection (linkerd/linkerd2-proxy#843) * Correct gRPC 'max-concurrency exhausted' error messages (linkerd/linkerd2-proxy#847) * Update tonic to v0.4 (linkerd/linkerd2-proxy#849) * failfast: Improve diagnostic logging (linkerd/linkerd2-proxy#848) * Update the base docker image (linkerd/linkerd2-proxy#850) * stack: Implement Clone for ResultService (linkerd/linkerd2-proxy#851) * Ensure services in failfast can become ready (linkerd/linkerd2-proxy#858) * tests: replace string matching on metrics with parsing (linkerd/linkerd2-proxy#859) * Decouple tls::accept from TcpStream (linkerd/linkerd2-proxy#853) * metrics: Handle NoPeerIdFromRemote properly (linkerd/linkerd2-proxy#857) * metrics: Reorder metrics labels (linkerd/linkerd2-proxy#856) * Rename tls::accept to tls::server (linkerd/linkerd2-proxy#854) * Annotate socket-level errors with a scope (linkerd/linkerd2-proxy#852) * test: reduce repetition in metrics tests (linkerd/linkerd2-proxy#860) * tls: Disambiguate client and server identities (linkerd/linkerd2-proxy#855) * Update to tower v0.4.4 (linkerd/linkerd2-proxy#864) * Update cargo dependencies (linkerd/linkerd2-proxy#865) * metrics: add `target_addr` label for accepted transport metrics (linkerd/linkerd2-proxy#861) * outbound: Strip endpoint identity when disabled (linkerd/linkerd2-proxy#862) --- The opaque-ports test has been updated to reflect proxy metrics changes. Signed-off-by: Jijeesh <jijeesh.ka@gmail.com>

olix0r added 12 commits January 17, 2021 18:07

touchups

0436283

touchup

b8e63df

Merge branch 'main' into ver/io-peek

cf2c854

wip

b47d0f0

Rename tls::accept to tls::server

2ccf9ad

cleanup labels

eee83d9

fixup tests

d0f960c

Rename tls::accept to tls::server

5e808a4

Merge branch 'ver/tls-server' into ver/tls-types

12023c5

tls::server: Make client ids explicitly optional

ecc139b

Base automatically changed from ver/tls-server to main January 19, 2021 22:01

olix0r added 4 commits January 19, 2021 22:22

Merge branch 'main' into ver/tls-types

dd59686

Merge branch 'main' into ver/tls-types

46ef1fe

Restore previous metrics behavior

b5a6d58

Extract direction label into EndpointLabels

5c16503

olix0r marked this pull request as ready for review January 19, 2021 23:01

olix0r requested a review from a team January 19, 2021 23:01

olix0r added 2 commits January 19, 2021 23:17

fix double comma

124a98e

inbound: Simplify target types

ba1ece6

hawkw approved these changes Jan 20, 2021

View reviewed changes

olix0r added 4 commits January 20, 2021 18:57

"marker type" => "newtype"

0da822d

+comment

b7e860a

Remove commented imports

07a6f44

Rename identity::Local to LocalCrtKey

dfcd7c9

olix0r merged commit 0d6471c into main Jan 20, 2021

olix0r deleted the ver/tls-types branch January 20, 2021 19:22

olix0r mentioned this pull request Jan 21, 2021

proxy: v2.129.0 linkerd/linkerd2#5581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: Disambiguate client and server identities #855

tls: Disambiguate client and server identities #855

olix0r commented Jan 18, 2021 •

edited

Loading

hawkw left a comment

hawkw Jan 20, 2021

hawkw Jan 20, 2021

olix0r Jan 20, 2021

hawkw Jan 20, 2021

hawkw Jan 20, 2021

hawkw Jan 20, 2021

hawkw Jan 20, 2021

hawkw Jan 20, 2021

	id.map(\|id\| id.to_string()).unwrap_or_default(),
	id.map(ToString::to_string).unwrap_or_default(),

		}

		pub type ConditionalTls = Conditional<Option<ClientId>, NoTls>;

tls: Disambiguate client and server identities #855

tls: Disambiguate client and server identities #855

Conversation

olix0r commented Jan 18, 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 Jan 18, 2021 •

edited

Loading