Require identity on tap requests by kleimkuhler · Pull Request #295 · linkerd/linkerd2-proxy

kleimkuhler · 2019-08-01T16:35:45Z

Motivation

With linkerd/linkerd2#3155 merging, proxy containers will now always have the LINKERD2_PROXY_TAP_SVC_NAME variable in their environment. This variable can now be reliably used to check that the client names of incoming tap requests match this value.

If the values do not match, we will always return the gRPC Unauthenticated status code. If the values do match, we will accept the connection.

If there is no client identity, we will similarly return the gRPC Unauthenticated status code.

Closes linkerd/linkerd2#3157
Closes linkerd/linkerd2#3163

Signed-off by: Kevin Leimkuhler kleimkuhler@icloud.com

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

adleong

If the environment variable is not provided, this will return Unauthenticated on every request? Wouldn't it be better to simply error out at startup time if the variable is missing?

Or, rather, how does this interact with the setting to disable the tap server? I'd expect this behavior:

Tap disabled + No tap svc name => Don't listen on the tap port
Tap disabled + tap svc name => Don't listen on the tap port
Tap enabled + No tap svc name => config error on startup
Tap enabled + tap svc name => listen on tap port and require client name to match

If identity is disabled, then tap must be explicitly disabled. If ideneity is enabled, then tap can either be enabled or disabled. Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler · 2019-08-02T21:28:36Z

@adleong @hawkw Tap has now been tied to identity in the proxy config. This prevents a user from accidentally setting up a proxy environment where there is an error in expecting tap to work if identity has been explicitly disabled.

The behavior now ensures that if identity has been disabled, tap must also be explicitly disabled. If identity is enabled, then tap can either be enabled or disabled.

Once the above configuration has been verified, if tap is disabled then the tap service name is set to None. If tap is enabled, then LINKERD2_PROXY_TAP_SVC_NAME must be set.

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

adleong

What do you think about changing the type of Config.control_listener from Option<Listener> to Option<(Listener, identity::Name)> and getting rid of the config.tap_svc_name field? This structurally enforces that a tap server MUST have a name.

This lets us consolidate parse_control_listener and parse_tap_svc_name into one function and brings all the tap config error handling into one place.

It also simplifies serve_tap since we can be guaranteed that there will be tap_svc_name.

src/app/tap.rs

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler · 2019-08-03T00:02:52Z

@adleong serve_tap now assumes that LINKERD2_PROXY_TAP_SVC_NAME is set in the environment if it is spawning the tap server!

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler · 2019-08-05T21:07:51Z

src/app/main.rs


    admin_listener: Listen<identity::Local, ()>,
-    control_listener: Option<Listen<identity::Local, ()>>,
+    control_listener: Option<(Listen<identity::Local, ()>, identity::Name)>,


I kept this as a tuple because we destructure it right away here. I could be convinced to add a similar TapSettings type in tap.rs, but after trying that out it seemed like a lot of boilerplate for not much of an abstraction.

@kleimkuhler FWIW, it looks like we also access a field of the tuple here, which is a little unclear out of context (though not a big deal).

Yep I should have pointed that out too. For the clarity we gain in that access though, it still seemed a little unnecessary to add a new struct for it.

hawkw

I noticed some potential test improvements + some other nits that are not really blocking.

hawkw · 2019-08-05T22:43:14Z

src/app/config.rs

-    /// Where to listen for connections initiated by the control plane.
-    pub control_listener: Option<Listener>,
+    /// Where to listen for connections initiated by the tap service.
+    pub control_listener: Option<TapSettings>,


Nit/TIOLI: If this is tap-specific, and contains more than just a listener configuration, what do you think of renaming the field to tap_settings or something?

Yep sounds good!

@hawkw A naming change like this touches a few places in main.rs that were not before. I may default to a separate PR to keep the diff down. I feel like we usually default to this so I'm going to as well in this case.

Especially since @adleong has already approved. I don't want to introduce additional changes that were not there before

tests/tap.rs

hawkw · 2019-08-05T22:53:52Z

src/app/main.rs


    admin_listener: Listen<identity::Local, ()>,
-    control_listener: Option<Listen<identity::Local, ()>>,
+    control_listener: Option<(Listen<identity::Local, ()>, identity::Name)>,


@kleimkuhler FWIW, it looks like we also access a field of the tuple here, which is a little unclear out of context (though not a big deal).

adleong

⭐ 🗜

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler · 2019-08-06T00:29:08Z

I opened linkerd/linkerd2#3197 to track renaming control_listener and possible LINKERD2_PROXY_CONTROL_LISTEN_ADDR

* update-rust-version: Check usage (linkerd/linkerd2-proxy#298) * tap: fix tap response streams never ending (linkerd/linkerd2-proxy#299) * Require identity on tap requests (linkerd/linkerd2-proxy#295) * Authority label should reflect logical dst (linkerd/linkerd2-proxy#300) * Replace futures_watch with tokio::sync::watch (linkerd/linkerd2-proxy#301)

* Update h2 to v0.1.26 * Properly fall back in the dst_router (linkerd/linkerd2-proxy#291) * Tap server authorizes clients when identity is expected (linkerd/linkerd2-proxy#290) * update-rust-version: Check usage (linkerd/linkerd2-proxy#298) * tap: fix tap response streams never ending (linkerd/linkerd2-proxy#299) * Require identity on tap requests (linkerd/linkerd2-proxy#295) * Authority label should reflect logical dst (linkerd/linkerd2-proxy#300) * Replace futures_watch with tokio::sync::watch (linkerd/linkerd2-proxy#301) * metrics: add `request_handle_us` histogram (linkerd/linkerd2-proxy#294) * linkerd2-proxy: Adopt Rust 2018 (linkerd/linkerd2-proxy#302) * Remove futures-mpsc-lossy (linkerd/linkerd2-proxy#305) * Adopt std::convert::TryFrom (linkerd/linkerd2-proxy#304) * lib: Rename directories to match crate names (linkerd/linkerd2-proxy#303)

* update-rust-version: Check usage (linkerd/linkerd2-proxy#298) * tap: fix tap response streams never ending (linkerd/linkerd2-proxy#299) * Require identity on tap requests (linkerd/linkerd2-proxy#295) * Authority label should reflect logical dst (linkerd/linkerd2-proxy#300) * Replace futures_watch with tokio::sync::watch (linkerd/linkerd2-proxy#301) * metrics: add `request_handle_us` histogram (linkerd/linkerd2-proxy#294) * linkerd2-proxy: Adopt Rust 2018 (linkerd/linkerd2-proxy#302) * Remove futures-mpsc-lossy (linkerd/linkerd2-proxy#305) * Adopt std::convert::TryFrom (linkerd/linkerd2-proxy#304) * lib: Rename directories to match crate names (linkerd/linkerd2-proxy#303)

kleimkuhler added 2 commits July 31, 2019 10:39

Reject all tap requests that do not match the expected

c8475e3

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

Fix tests for always requiring an identity on tap requests

867cb0e

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler requested review from adleong, hawkw and seanmonstar August 1, 2019 16:35

kleimkuhler self-assigned this Aug 1, 2019

adleong reviewed Aug 1, 2019

View reviewed changes

Assert proper configuration between tap and identity

221c408

If identity is disabled, then tap must be explicitly disabled. If ideneity is enabled, then tap can either be enabled or disabled. Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler added 3 commits August 2, 2019 14:30

Remove old comment

8cfb161

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

Add clarifying comment

0463e85

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

Simplify tap disabled checks

9fde32e

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

adleong reviewed Aug 2, 2019

View reviewed changes

src/app/tap.rs Outdated Show resolved Hide resolved

Assume tap service name is set in serve_tap

fe7e4c8

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

Combine tap listener and tap_svc_name

d5fd05a

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler requested a review from adleong August 5, 2019 21:03

kleimkuhler commented Aug 5, 2019

View reviewed changes

hawkw reviewed Aug 5, 2019

View reviewed changes

adleong approved these changes Aug 5, 2019

View reviewed changes

Address review comments

d5d7686

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler requested a review from hawkw August 6, 2019 00:23

kleimkuhler mentioned this pull request Aug 6, 2019

proxy: Consider renaming control_listener to tap_settings linkerd/linkerd2#3197

Closed

hawkw approved these changes Aug 7, 2019

View reviewed changes

kleimkuhler merged commit 3acf03a into master Aug 7, 2019

kleimkuhler deleted the kleimkuhler/require-tap-svc-name branch August 7, 2019 22:18

scottcarol mentioned this pull request Aug 8, 2019

proxy: Update proxy to 3c3cda7 linkerd/linkerd2#3219

Closed

olix0r mentioned this pull request Aug 9, 2019

proxy: Update proxy to master linkerd/linkerd2#3225

Merged

kleimkuhler mentioned this pull request Oct 4, 2019

Tap controller should check that identity is not disabled on a resource linkerd/linkerd2#3164

Closed

Conversation

kleimkuhler commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Uh oh!

adleong left a comment

Choose a reason for hiding this comment

Uh oh!

kleimkuhler commented Aug 2, 2019

Uh oh!

adleong left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kleimkuhler commented Aug 3, 2019

Uh oh!

kleimkuhler Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

hawkw Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

kleimkuhler Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleimkuhler Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

kleimkuhler Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

kleimkuhler Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

adleong left a comment

Choose a reason for hiding this comment

Uh oh!

kleimkuhler commented Aug 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kleimkuhler commented Aug 1, 2019 •

edited

Loading

adleong left a comment •

edited

Loading

hawkw Aug 5, 2019 •

edited

Loading