inbound: Discover policies from the control plane #1205

olix0r · 2021-08-18T18:58:08Z

We've recently introduced support for authorization policies. These
policies are configured statically from the proxy's environment. This
change adds an optional new mode to support dynamic configuration from
the control plane.

The proxy now supports new environment variables,
LINKERD2_PROXY_POLICY_SVC_{ADDR,NAME} that may be set with the address
and identity of the policy controller. When this is set, other static
inbound port configurations (requiring identity, marking ports as
opaque, etc) are ignored in favor of the results returned by the API.
Instead, the proxy uses the LINKER2_PROXY_INBOUND_PORTS environment
variable to discover the list of ports configured in the pod's spec and
discovers the policies for each of these ports before admitting inbound
connections. When a connection is received for a port not in this list,
the LINKERD2_PROXY_INBOUND_DEFAULT_POLICY configuration is used to
determine whether the connection may be permitted or whether it should
be refused.

This change modifies the set of default policies to support only deny,
all-authenticated and all-unauthenticated (removing
all-mtls-unauthenticated). Instead a
LINKERD2_PROXY_INBOUND_PORTS_REQUIRE_TLS configuration is introduced
to enable tls requirements for individual pods. This static
configuration should only be necessary for the identity controller, in
which case we need to use an unauthenticated default policy to permit
kubernetes healtchecking probes; and we can use this new configuration
to enforce a default policy on the gRPC API port.

This change makes the inbound accept stack generic over an implementation of `CheckPolicy`. This lets us start modifying the concrete PortPolicies type without changing the accept stack.

Supports policies on opaque transported connections.

Move the store into its own module to help hide/isolate implementation details. Move discover/initialization into Store::discover.

We've recently introduced support for authorization policies. These policies are configured statically from the proxy's environment. This change adds an optional new mode to support dynamic configuration from the control plane. The proxy now supports new environment variables, `LINKERD2_PROXY_POLICY_SVC_{ADDR,NAME}` that may be set with the address and identity of the policy controller. When this is set, other static inbound port configurations (requiring identity, marking ports as opaque, etc) are ignored in favor of the results returned by the API. Instead, the proxy uses the `LINKER2_PROXY_INBOUND_PORTS` environment variable to discover the list of ports configured in the pod's spec and discovers the policies for each of these ports before admitting inbound connections. When a connection is received for a port not in this list, the `LINKERD2_PROXY_INBOUND_DEFAULT_POLICY` configuration is used to determine whether the connection may be permitted or whether it should be refused. This change modifies the set of default policies to support only `deny`, `all-authenticated` and `all-unauthenticated` (removing `all-mtls-unauthenticated`). Instead a `LINKERD2_PROXY_INBOUND_PORTS_REQUIRE_TLS` configuration is introduced to enable tls requirements for individual pods. This static configuration should only be necessary for the identity controller, in which case we need to use an unauthenticated default policy to permit kubernetes healtchecking probes; and we can use this new configuration to enforce a default policy on the gRPC API port.

linkerd/app/inbound/src/detect.rs

linkerd/app/inbound/src/policy/discover.rs

linkerd/app/inbound/src/policy/mod.rs

linkerd/app/inbound/src/policy/store.rs

linkerd/app/inbound/src/policy/tests.rs

linkerd/app/src/env.rs

olix0r · 2021-08-19T00:00:58Z

I've tested this end-to-end by manually configuring the proxy for discovery. There's more we need to do testing-wise but for now I think we should try to get this merged and then focus on integration testing in the linkerd2 repo.

olix0r · 2021-08-19T00:02:01Z

linkerd/app/core/src/serve.rs

@@ -75,7 +75,8 @@ pub async fn serve<M, S, I, A>(
                }
            }
        }
-    };
+    }
+    .in_current_span();


I'm surprised this is necessary but without it we don't see the inbound/outbound span on log messages from this future

oh, huh...it really seems like this shouldn't be necessary, since the future isn't being spawned, so it should all be in the current span of whatever context serve is awaited in. weird!

olix0r · 2021-08-19T00:03:04Z

linkerd/app/inbound/src/detect.rs

+                            Protocol::Http1 => {
+                                // HTTP/1 services may actually be transported over HTTP/2
+                                // connections between proxies, so we have to do detection.
+                                return Ok(svc::Either::B(Detect {
+                                    timeout: detect_timeout,
+                                    tls,
+                                }));
+                            }


This was an oversight before: we need to do detection in this case to support protocol upgrade (found during testing)

olix0r · 2021-08-19T16:28:24Z

The release build is right at the 14-15 minute mark (even with boxing). Not much we can do, I think, but wait for the fixes for rust-lang/rust#84873 to hit stable. Going to bump the timeout in the meantime

kleimkuhler

Code-wise looks good. Going to walk through the logic a little more with the changes actually in use in an installation, but nothing major standing out to me right now.

hawkw

noticed one thing that seemed like a potential concern, plus a couple small nits (not blockers). once that question's addressed, lgtm!

hawkw · 2021-08-19T18:57:29Z

linkerd/app/core/src/serve.rs

@@ -75,7 +75,8 @@ pub async fn serve<M, S, I, A>(
                }
            }
        }
-    };
+    }
+    .in_current_span();


oh, huh...it really seems like this shouldn't be necessary, since the future isn't being spawned, so it should all be in the current span of whatever context serve is awaited in. weird!

linkerd/app/inbound/src/detect.rs

hawkw · 2021-08-19T19:01:26Z

linkerd/app/inbound/src/direct.rs

+                                    name: Some(name),
+                                    protocol,
+                                } => {
+                                    let allow = policies.check_policy(client.local_addr)?;


why does the case where the transport header includes a name and a port check the client's local addr, while the case where there's no name check the client's local addr IP and the port from the transport header?

is this an oversight, or is it on purpose? and if it's on purpose, can we have a comment here explaining why it's different?

when it includes a name/port, it's a gateway configuration targeting some other service; so the policy check determines whether the client can contact the gateway itself. will add a comment

hawkw

thanks for clarifying the gateway behavior, lgtm!

This release adds support for dynamic inbound policies. The proxy now discovers policies from Linkerd'2 policy-controller API for all application ports documented in a pod spec. Rejected connections are logged. Policies are not yet reflected in the proxy's metrics. These policies also allow the proxy to skip protocol detection when a server is explicitly annotated as HTTP/2 or when the server is documented to be opaque or application-terminated TLS. --- * inbound: Use policy protocol configurations (linkerd/linkerd2-proxy#1203) * build(deps): bump tokio from 1.9.0 to 1.10.0 (linkerd/linkerd2-proxy#1204) * build(deps): bump tracing-subscriber from 0.2.19 to 0.2.20 (linkerd/linkerd2-proxy#1207) * inbound: Discover policies from the control plane (linkerd/linkerd2-proxy#1205) * build(deps): bump httparse from 1.4.1 to 1.5.1 (linkerd/linkerd2-proxy#1208)

olix0r added 30 commits August 16, 2021 22:43

Introduce the CheckPolicy trait

18f8c05

This change makes the inbound accept stack generic over an implementation of `CheckPolicy`. This lets us start modifying the concrete PortPolicies type without changing the accept stack.

Make detect internals as private as possible

fa41974

rename port_policies module policy

b925ab1

Move default policies into a module

e2485ba

Expose a policy::Config type for env-based configuration

023ea69

Fix identity-client loopback config

5dd5a5a

touchup inbound port config

7bfc1d4

simplify control address configuration

42ee5f8

support configuration for inbound policy discovery

da8d20c

Implement a discovery client

b20e7a4

wtfwtfwtfwtfwtfwtfwtf

ec3a3d5

wip: compiles

1015bbc

compiles

2bbdb75

uh... actual compile... maybe

5b1a6eb

lessen type constraints

ca1ab19

wip: Convert protobuf responses to ServerPolicy instances

d95ec3d

compiles

623febf

-rustfmt

3bb5f46

Add a LINKERD2_PROXY_INBOUND_PORTS_REQUIRE_TLS configuration

5990fb3

build the discovery-backed PortPolicy

beb8de4

Store watches directly, avoiding locking/indirection

d0e5195

cleanup/comment env configs

29d98a8

Implement policies in the direct stack

6973200

Supports policies on opaque transported connections.

remove unused CheckPolicy impl

d178eb5

- uneeded temp val

db24280

split policy tests into a file

5626f84

Split policy::Config into its own module

62a2c83

Rename policy::PortPolicies to policy::Store

0739851

Move the store into its own module to help hide/isolate implementation details. Move discover/initialization into Store::discover.

touchup

99864c2

hawkw reviewed Aug 18, 2021

View reviewed changes

olix0r added 7 commits August 18, 2021 23:18

We can't short-circuit HTTP/1 detection

08be8bc

fixup tests; no need for tokio::test in policy tests

8b996e0

avoid needless arc bump

670628d

fixup comment

dc270b8

avoid needless match

69fac9f

We can't support default-deny without discovery

ba0a901

fix comment

d555c85

olix0r marked this pull request as ready for review August 19, 2021 00:00

olix0r commented Aug 19, 2021

View reviewed changes

olix0r added 5 commits August 19, 2021 00:06

fixup

8c5d816

back out unneeded changes

853fb29

box detect stacks for now -- 7% faster

7f0e670

Add policy checking on gateway connections

00305bb

fix comment

c598b6c

olix0r added 2 commits August 19, 2021 16:28

bump release build timeout

533d0bb

Add the inbound port for policy discovery on gateways

6f19de0

kleimkuhler approved these changes Aug 19, 2021

View reviewed changes

hawkw reviewed Aug 19, 2021

View reviewed changes

olix0r added 2 commits August 19, 2021 19:24

gateway commentary

0576013

Add more detect tests

2834cf2

hawkw approved these changes Aug 19, 2021

View reviewed changes

olix0r merged commit 822cd15 into main Aug 19, 2021

olix0r deleted the ver/policy-discover branch August 19, 2021 19:39

olix0r mentioned this pull request Aug 19, 2021

proxy: v2.153.0 linkerd/linkerd2#6707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inbound: Discover policies from the control plane #1205

inbound: Discover policies from the control plane #1205

olix0r commented Aug 18, 2021

olix0r commented Aug 19, 2021

olix0r Aug 19, 2021

hawkw Aug 19, 2021

olix0r Aug 19, 2021

olix0r commented Aug 19, 2021

kleimkuhler left a comment

hawkw left a comment

hawkw Aug 19, 2021

hawkw Aug 19, 2021

olix0r Aug 19, 2021

hawkw left a comment

inbound: Discover policies from the control plane #1205

inbound: Discover policies from the control plane #1205

Conversation

olix0r commented Aug 18, 2021

olix0r commented Aug 19, 2021

olix0r Aug 19, 2021

Choose a reason for hiding this comment

hawkw Aug 19, 2021

Choose a reason for hiding this comment

olix0r Aug 19, 2021

Choose a reason for hiding this comment

olix0r commented Aug 19, 2021

kleimkuhler left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Aug 19, 2021

Choose a reason for hiding this comment

hawkw Aug 19, 2021

Choose a reason for hiding this comment

olix0r Aug 19, 2021

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment