Add l5d-client-id on inbound requests if meshed TLS #184

seanmonstar · 2019-01-31T23:28:23Z

If meshed TLS is used on an inbound connection, the TLS Identity of client is added as a header l5d-client-id to the request, so that the internal service can see it was encrypted.

Client certificates were already automatically used if a proxy was configured with TLS.
The server verifier was changed to require a client certificates.
We assume the first DNS name in the certificate is the TLS identity assigned by the controller's CA.
- In order to get the DNS names from the certificate, a fork of webpki is used, while waiting for the pull request to land.
The l5d-client-id header is stripped on both inbound and outbound requests at the beginning, so only the inbound proxy will expose this header.

Closes linkerd/linkerd2#2125
Closes linkerd/linkerd2#2126

seanmonstar · 2019-01-31T23:29:37Z

Hm, maybe this doesn't completely do all of linkerd/linkerd2#2126, as the ID isn't yet included in tap or metrics...

Signed-off-by: Sean McArthur <[email protected]>

seanmonstar · 2019-02-01T01:21:49Z

I wasn't sure about format or labels, so I just arbitrarily decided on this:

In metrics, tls="true" is still present, and client_id="..." or server_id="..." is also included.
In tap events, the {"tls": "true"} is still set, and "client_id": "..." or "server_id": "..." are also included.
In both cases, if TLS is not enabled, the reason why is still included, and the server_id/client_id is left off, so to not state the reason twice.

olix0r · 2019-02-01T01:44:23Z

@seanmonstar those sound like some good arbitrary decisions to me ;) i'll review & test in the AM

hawkw

This seems correct to me. I commented mainly on some style nits and on potential future refactoring.

src/transport/connection.rs

hawkw · 2019-02-01T19:48:40Z

src/transport/tls/mod.rs

@@ -44,6 +44,8 @@ pub use self::{
 #[cfg(test)]
 pub use self::config::test_util as config_test_util;

+pub type ConditionalIdentity = Conditional<Identity, ReasonForNoTls>;


Nit/TIOLI: It seems like

Conditional<&Identity, ReasonForNoTls>

is at least as common as

Conditional<Identity, ...>

Maybe the best way to reduce verbosity is just to have the TLS module define a

pub type Conditional<T> = conditional::Conditional<T, ReasonForNoTls>;

so that code elsewhere can just talk about

tls::Conditional<Identity>

or

tls::Conditional<&Identity>

On the other hand, this seems kind of similar to the result type alias pattern that's generally frowned upon, so 🤷‍♀️

hawkw · 2019-02-01T19:50:18Z

tests/discovery.rs

@@ -638,6 +638,57 @@ mod proxy_to_proxy {
        assert_eq!(res.version(), http::Version::HTTP_2);
    }

+    #[test]
+    fn inbound_should_strip_l5d_client_id() {


Nit: these seem like they may as well be generated for both protocols?

src/transport/tls/mod.rs

hawkw · 2019-02-01T19:55:33Z

src/tap/grpc/server.rs

            Some(m)
        },
        destination: inspect.dst_addr(req).as_ref().map(|a| a.into()),
        destination_meta: inspect.dst_labels(req).map(|labels| {
            let mut m = api::tap_event::EndpointMeta::default();
            m.labels.extend(labels.iter().map(|(k, v)| (k.clone(), v.clone())));
-            let tls = format!("{}", inspect.dst_tls(req));
-            m.labels.insert("tls".to_owned(), tls);
+            let tls = inspect.dst_tls(req);


It would be nice if this logic didn't have to be repeated for src/dst, but it's not a huge deal...

src/app/main.rs

src/app/inbound.rs

Co-Authored-By: seanmonstar <[email protected]>

olix0r · 2019-02-03T22:34:06Z

After some testing, it seems that the introduction of client authentication breaks our TLS detection logic:

linkerd-controller-856bc5d794-lmvjj linkerd-proxy TRCE proxy={server=in listen=0.0.0.0:4143} linkerd2_proxy::transport::tls::conditional_accept match_client_hello: failed to parse up to SNI
linkerd-controller-856bc5d794-lmvjj linkerd-proxy TRCE proxy={server=in listen=0.0.0.0:4143} linkerd2_proxy::transport::connection passing through accepted connection without TLS

the buffering/parsing logic wlil have to be made more robust.

olix0r · 2019-02-03T23:06:34Z

This issue may not actually be caused by buffering. It appears that SNI is failing to match due to an injection bug.

olix0r · 2019-02-05T00:33:51Z

The issue i saw is fixed by linkerd/linkerd2@4798ad3. I'm going to run a few more tests off of this branch (after merging master) and prepare it to merge.

olix0r · 2019-02-06T17:42:37Z

src/proxy/server.rs

@@ -70,7 +70,7 @@ pub struct Source {
    pub remote: SocketAddr,
    pub local: SocketAddr,
    pub orig_dst: Option<SocketAddr>,
-    pub tls_status: tls::Status,
+    pub tls_peer: tls::ConditionalIdentity,


If I understand correctly, this means that we can't describe the situation where TLS is established and a client ID is not provided. This case is important, especially for the service that has to bootstrap certificates -- proxies will have to be able to connect to this service before they have an identity.

I think we'll need to make ConditionalIdentity more like:

type ClientIdentity = Conditional<Identity, ReasonForNoClientIdentity>; type ServerStatus = Conditional<ClientIdentity, ReasonForNoTls>;

After taking a stab at this, it's a little complex; and, since much of the internals of this are going to have to change in an upcoming release, it's probably best to merge this as-is and address these concerns later.

I may be missing some context, but as I was reading through the TLS code, it appeared to me that TLS couldn't be established without an identity... 🤷‍♂️

seanmonstar requested a review from olix0r January 31, 2019 23:29

Add l5d-client-id on inbound requests if meshed TLS

1e0787a

Signed-off-by: Sean McArthur <[email protected]>

seanmonstar force-pushed the tls-client-auth branch from 5bf02b0 to 1e0787a Compare January 31, 2019 23:45

seanmonstar added 2 commits January 31, 2019 16:22

add client_id and server_id to metrics labels

d1bb52f

Signed-off-by: Sean McArthur <[email protected]>

include client_id and server_id in Tap events

4890f4b

Signed-off-by: Sean McArthur <[email protected]>

hawkw reviewed Feb 1, 2019

View reviewed changes

hawkw and others added 2 commits February 1, 2019 12:26

Update src/transport/tls/mod.rs

1c1d077

Co-Authored-By: seanmonstar <[email protected]>

Update src/transport/connection.rs

941e1c2

Co-Authored-By: seanmonstar <[email protected]>

olix0r self-assigned this Feb 5, 2019

Merge branch 'master' into ver/tls-client-auth

cd8a04d

olix0r reviewed Feb 6, 2019

View reviewed changes

olix0r approved these changes Feb 7, 2019

View reviewed changes

olix0r merged commit 7add4fc into master Feb 7, 2019

olix0r deleted the tls-client-auth branch February 7, 2019 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add l5d-client-id on inbound requests if meshed TLS #184

Add l5d-client-id on inbound requests if meshed TLS #184

seanmonstar commented Jan 31, 2019

seanmonstar commented Jan 31, 2019

seanmonstar commented Feb 1, 2019

olix0r commented Feb 1, 2019

hawkw left a comment

hawkw Feb 1, 2019

hawkw Feb 1, 2019

hawkw Feb 1, 2019

olix0r commented Feb 3, 2019

olix0r commented Feb 3, 2019

olix0r commented Feb 5, 2019

olix0r Feb 6, 2019 •

edited

Loading

olix0r Feb 7, 2019

seanmonstar Feb 11, 2019

Add l5d-client-id on inbound requests if meshed TLS #184

Add l5d-client-id on inbound requests if meshed TLS #184

Conversation

seanmonstar commented Jan 31, 2019

seanmonstar commented Jan 31, 2019

seanmonstar commented Feb 1, 2019

olix0r commented Feb 1, 2019

hawkw left a comment

Choose a reason for hiding this comment

hawkw Feb 1, 2019

Choose a reason for hiding this comment

hawkw Feb 1, 2019

Choose a reason for hiding this comment

hawkw Feb 1, 2019

Choose a reason for hiding this comment

olix0r commented Feb 3, 2019

olix0r commented Feb 3, 2019

olix0r commented Feb 5, 2019

olix0r Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

olix0r Feb 7, 2019

Choose a reason for hiding this comment

seanmonstar Feb 11, 2019

Choose a reason for hiding this comment

olix0r Feb 6, 2019 •

edited

Loading