Tap server authorizes clients when identity is expected by kleimkuhler · Pull Request #290 · linkerd/linkerd2-proxy

kleimkuhler · 2019-07-24T03:10:43Z

Motivation

As part of the tap hardening project the tap server must serve tap requests
exclusively to the tap service. Clients must be both authenticated and
authorized. Currently, the tap server still can accept clients over plaintext,
and does not authorize clients over private connections.

Details

In order for authenticated clients to be authorized, the tap_identity config
variable has been introduced. This value is used as the expected client identity
of incoming tap requests. When tap_identity is set, the tap server now checks
the peer identity of the client connection against this value. If they are equal
the connection is accept; otherwise is is rejected.

The tap server still accepts plaintext connections. This will be removed in a
future PR once the tap controller is sending tap requests with the required
l5d-require-id that is needed to set server identity on outbound requests to
IP addresses.

Tests

Tests have been added to tap::expected_identity that assert:

The tap server accepts plaintext connections when tap_identity is not set in
the config
The tap server rejects plaintext connections when tap_identity is set in the
config
The tap server rejects private connections when the client identity does not
match the expected tap_identity
The tap server accepts private connections when the client identity does match
the expected tap_identity

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

src/app/config.rs

src/app/tap.rs

adleong · 2019-07-24T23:53:25Z

src/proxy/grpc/service.rs

+    #[derive(Clone)]
+    pub struct Unauthenticated;
+
+    impl api::server::Tap for Unauthenticated {


When the client is not authenticated or authorized, is it better to accept the connection and serve back Unauthenticated errors like this? Or simply reject the connection?

So we could just reject the connection, and it would remove a majority of the changes this PR introduces. However, mainly in testing and especially for debugging tap, I found it useful to be able to see that a client is unauthenticated from tapping a certain pod. I would prefer to keep this, but I'm open to reason on why it should be removed.

src/app/tap.rs

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

Currently test passes with outbound socket, but this should be setup to use inbound socket Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

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

hawkw

This seems good to me, at a glance. Most of my comments were pretty nitpicky.

src/app/config.rs

tests/support/proxy.rs

src/app/tap.rs

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

kleimkuhler · 2019-07-29T22:19:33Z

@adleong @hawkw The comments I've left unresolved are there if you have any more questions/feedback, otherwise reviews have been addressed!

adleong

⭐ 👞

@siggy

### Summary In order for Pods' tap servers to start authorizing tap clients, the tap controller must open TLS connections so that it can identity itself to the server. This change introduces the use of `l5d-require-id` header on outbound tap requests. ### Details When tap requests are made by the tap controller, the `Authority` header is an IP address. The proxy does not attempt to do service discovery on such requests and therefore the connection is over plaintext. By introducing the `l5d-require-id` header the proxy can require a server name on the connection. This allows the tap controller to identity itself as the client making tap requests. The name value for the header can be made from the Pod Spec and tap request, so the change is rather minimal. #### Proxy Changes * Update h2 to v0.1.26 * Properly fall back in the dst_router (linkerd/linkerd2-proxy#291) ### Testing Unit tests for the header have not been added mainly because [no test infrastructure currently exists](https://github.com/linkerd/linkerd2/blob/065c2218588093831dc2f8f49516e85e4a71b896/controller/tap/server_test.go#L241) to mock proxy requests. After talking with @siggy a little about this, it makes to do in a separate change at some point when behavior like this cannot be reliably tested through integration tests either. Integration tests do test this well, and will continue to do once linkerd/linkerd2-proxy#290 lands. Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

Replace macros with a function Signed-off-by: Eliza Weisman <eliza@buoyant.io>

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

* 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)

* Tap server authorizes clients when identity is expected (linkerd#290) Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler requested review from adleong, hawkw and seanmonstar July 24, 2019 03:10

kleimkuhler self-assigned this Jul 24, 2019

adleong reviewed Jul 24, 2019

View reviewed changes

Kevin Leimkuhler and others added 17 commits July 26, 2019 13:50

Require expected client identity when serving tap

964de3c

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

WIP: Returning Unauthorized in serve_tap()

100489c

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

WIP: Trying to use svc::mk()

6050aec

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

Tap server returns grpc status code

34eb9ca

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

Tap test expects Unauthenticated GRPC code

a55451f

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

Simplify serve_tap()

48d96be

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

WIP: Adding Tap tls client feature

c728763

Currently test passes with outbound socket, but this should be setup to use inbound socket Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

WIP: Working towards tap rejecting identity

9169b5e

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

WIP: Try adding dst for proxy.control

1ef4103

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

Finish adding tap identity support in tests

4c7bf54

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

Tap client identity not authorized

bc29e3a

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

Set outbound_ip to the outbound that is mocked

03e25b1

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

Needs l5d-force-id header now

c389b12

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

Add l5d-require-id header to tests

cf96cb8

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

Remove TLS client refactor

369cc34

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

Fix tests for outbound server support

0fb6eb3

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

Cleanup various commits

3366ea3

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

kleimkuhler force-pushed the kleimkuhler/tap-identity-chk branch from 16d05ed to 3366ea3 Compare July 26, 2019 20:50

Change naming

7dcc201

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

kleimkuhler mentioned this pull request Jul 28, 2019

Add l5d-require-id header to Tap requests linkerd/linkerd2#3154

Merged

hawkw reviewed Jul 29, 2019

View reviewed changes

src/app/config.rs Show resolved Hide resolved

tests/support/proxy.rs Show resolved Hide resolved

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

src/app/tap.rs Show resolved Hide resolved

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

kleimkuhler mentioned this pull request Jul 29, 2019

Tap server only accepts TLS connections linkerd/linkerd2#3157

Closed

Improve comments and logging

36606df

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

kleimkuhler requested a review from hawkw July 30, 2019 16:55

kleimkuhler requested a review from adleong July 30, 2019 16:55

adleong approved these changes Jul 30, 2019

View reviewed changes

kleimkuhler added 2 commits July 30, 2019 13:18

replace macros with a function (#293)

9650a29

Replace macros with a function Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Format changes

5a64885

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

hawkw approved these changes Jul 30, 2019

View reviewed changes

kleimkuhler merged commit d315b9f into master Jul 31, 2019

kleimkuhler deleted the kleimkuhler/tap-identity-chk branch July 31, 2019 00:04

olix0r mentioned this pull request Aug 9, 2019

proxy: Update proxy to master linkerd/linkerd2#3225

Merged

sprt pushed a commit to sprt/linkerd2-proxy that referenced this pull request Aug 30, 2019

proxy: Update proxy to d315b9f (linkerd#3185)

f146041

* Tap server authorizes clients when identity is expected (linkerd#290) Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tap server authorizes clients when identity is expected#290

Tap server authorizes clients when identity is expected#290
kleimkuhler merged 21 commits intomasterfrom
kleimkuhler/tap-identity-chk

kleimkuhler commented Jul 24, 2019

Uh oh!

Uh oh!

Uh oh!

adleong Jul 24, 2019

Uh oh!

kleimkuhler Jul 25, 2019

Uh oh!

Uh oh!

hawkw left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kleimkuhler commented Jul 29, 2019

Uh oh!

adleong left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kleimkuhler commented Jul 24, 2019

Motivation

Details

Tests

Uh oh!

Uh oh!

Uh oh!

adleong Jul 24, 2019

Choose a reason for hiding this comment

Uh oh!

kleimkuhler Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kleimkuhler commented Jul 29, 2019

Uh oh!

adleong left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants