fix(*): localhost exposed application shouldn't be reachable #4654

lukidzi · 2022-07-22T12:31:03Z

Summary

When a user deploys Kuma the behavior of communication with services changes.
When there is no Kuma and the application running within the pod is binding to PodIP or Wildcard(0.0.0.0) other pods are able to reach the application. After Kuma is deployed it's not possible anymore. Instead, applications that bind to localhost or wildcard are exposed outside. That's a big security threat. In this PR we are introducing a different way how the inbound listener is configured by default.

Previously we had an inbound listener that had a static cluster that pointed to the address localhost:PORT. Now it's going to work a bit differently and might be breaking change for applications that were listening on localhost.

if dataplane.networking.inbound[].serviceAddress is not defined and transparent proxy is enabled:

We are creating an inbound cluster that is ORIGINAL_DST so depends on the real Envoy is going to route traffic to the application:
Flow:
POD1(curl 10.2.0.5:8080) -> kuma-dp(POD1)/or no kuma-dp -> kuma-dp (POD2) -> listener (inbound:8080) -> cluster (inbound:8080) which is original dst and is going to request 10.2.0.5:8080 but with source IP 127.0.0.6 -> iptables (MESH_OUTPUT) 1 rules -> application listening on 0.0.0.0 or 10.2.0.5.

if dataplane.networking.inbound[].serviceAddress is not defined and transparent proxy is disabled:

We are creating a static cluster with dataplaneIP because we are not able to get the IP address and we don't want to expose localhost. In this case, cluster is using upstream_bind_config to make a faster jump to the application in the first rule of iptables (MESH_OUTPUT) rule 1. This case is tricky and without this we could break health checking on universal.

if dataplane.networking.inbound[].serviceAddress is set, we are just using it and creating a static cluster, with the same flow as above to return fast from IPTABLES

What can I do to make the smooth upgrade?

check if your applications are listening on localhost and should be exposed outside - if no change to 0.0.0.0
if you want to expose application binding to localhost set dataplane.networking.inbound[].serviceAddress to 127.0.0.1
you can disable this behavior with kuma-cp configuration or env to kuma-cp KUMA_DEFAULTS_ENABLE_INBOUND_PASSTHROUGH - not recommended, we are going to remove this flag in the future

Full changelog

[Changed the default inbound cluster from STATIC to ORIGINAL_DST]
[Setting upstream_bind_address for inbound clusters that don't set have localhost address]
[Added e2e tests that check behavior with different application bindings]
[On universal we are using dataplane Ip as fallback when address is not defined and there is no transparent proxy]
[Changed inbound clusters naming: localhost -> inbound]
[Made changes to applications metrics scraper to use different address and allows to configure it]

Issues resolved

Fix #4630

Documentation

Link to the website documentation PR

Testing

Unit tests
E2E tests
Manual testing on Universal
Manual testing on Kubernetes

Backwards compatibility

Update UPGRADE.md with any steps users will need to take when upgrading.
~~[ ] Add backport-to-stable label if the code follows our backporting policy~~

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

…ix/localhost-expose

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

codecov-commenter · 2022-07-22T13:07:14Z

Codecov Report

Merging #4654 (ca96fab) into master (fd3c678) will increase coverage by 0.00%.
The diff coverage is 55.28%.

@@           Coverage Diff            @@
##           master    #4654    +/-   ##
========================================
  Coverage   46.43%   46.44%            
========================================
  Files         687      688     +1     
  Lines       46710    46847   +137     
========================================
+ Hits        21692    21758    +66     
- Misses      23097    23161    +64     
- Partials     1921     1928     +7

Impacted Files	Coverage Δ
api/mesh/v1alpha1/metrics.pb.go	`12.24% <0.00%> (-0.21%)`	⬇️
pkg/api-server/server.go	`74.38% <0.00%> (-0.31%)`	⬇️
pkg/core/bootstrap/bootstrap.go	`0.00% <0.00%> (ø)`
pkg/hds/tracker/callbacks.go	`0.00% <0.00%> (ø)`
pkg/plugins/runtime/k8s/metadata/annotations.go	`68.88% <ø> (ø)`
pkg/xds/bootstrap/components.go	`0.00% <0.00%> (ø)`
pkg/xds/bootstrap/handler.go	`35.89% <0.00%> (-0.47%)`	⬇️
...s/envoy/clusters/v3/cleanup_interval_configurer.go	`0.00% <0.00%> (ø)`
pkg/xds/envoy/names/resource_names.go	`21.73% <0.00%> (-0.99%)`	⬇️
pkg/xds/sync/dataplane_proxy_builder.go	`0.00% <0.00%> (ø)`
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd3c678...ca96fab. Read the comment docs.

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

…ix/localhost-expose

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

…ix/localhost-expose

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

lukidzi · 2022-07-28T21:59:31Z

I need to add some tests but the basic logic should be ready.

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

michaelbeaumont

@lukidzi would it be possible to break this into logical commits? IMO it's hard to review a PR this big otherwise

michaelbeaumont · 2022-08-01T09:33:14Z

mk/e2e.new.mk

@@ -129,7 +129,7 @@ test/e2e-kubernetes: $(E2E_DEPS_TARGETS)
 	$(MAKE) test/e2e/k8s/start/cluster/kuma-1
 	$(MAKE) test/e2e/k8s/wait/kuma-1
 	$(MAKE) test/e2e/k8s/load/images/kuma-1
-	$(E2E_ENV_VARS) $(GINKGO_TEST_E2E) $(KUBE_E2E_PKG_LIST) || (ret=$$?; $(MAKE) test/e2e/k8s/stop/cluster/kuma-1 && exit $$ret)
+	$(E2E_ENV_VARS) $(GINKGO_TEST_E2E) $(KUBE_E2E_PKG_LIST)	|| (ret=$$?; $(MAKE) test/e2e/k8s/stop/cluster/kuma-1 && exit $$ret)


Does this change need to be here?

jakubdyszkiewicz · 2022-08-01T06:47:04Z

mk/e2e.new.mk

@@ -129,7 +129,7 @@ test/e2e-kubernetes: $(E2E_DEPS_TARGETS)
 	$(MAKE) test/e2e/k8s/start/cluster/kuma-1
 	$(MAKE) test/e2e/k8s/wait/kuma-1
 	$(MAKE) test/e2e/k8s/load/images/kuma-1
-	$(E2E_ENV_VARS) $(GINKGO_TEST_E2E) $(KUBE_E2E_PKG_LIST) || (ret=$$?; $(MAKE) test/e2e/k8s/stop/cluster/kuma-1 && exit $$ret)
+	$(E2E_ENV_VARS) $(GINKGO_TEST_E2E) $(KUBE_E2E_PKG_LIST)	|| (ret=$$?; $(MAKE) test/e2e/k8s/stop/cluster/kuma-1 && exit $$ret)


jakubdyszkiewicz · 2022-08-01T06:49:34Z

pkg/config/app/kuma-cp/config.go

@@ -27,6 +27,8 @@ var _ config.Config = &Defaults{}

 type Defaults struct {
 	SkipMeshCreation bool `yaml:"skipMeshCreation" envconfig:"kuma_defaults_skip_mesh_creation"`
+	// If true, instead of providing inbound clusters with address of localhost, generates cluster with ORIGINAL_DST


also add a security threat if you disable it, and the fact that this will be removed.

jakubdyszkiewicz · 2022-08-01T07:08:31Z

pkg/plugins/runtime/k8s/metadata/annotations.go

@@ -97,6 +97,8 @@ const (
 	// Available values are: [trace][debug][info][warning|warn][error][critical][off]
 	KumaEnvoyLogLevel = "kuma.io/envoy-log-level"

+	// KumaMetricsPrometheusAggregateAddress allows to specify which address for specific app should request for metrics
+	KumaMetricsPrometheusAggregateAddress = "prometheus.metrics.kuma.io/aggregate-%s-address"


why would anyone need to set this to anything other than Pod IP?

jakubdyszkiewicz · 2022-08-01T07:15:02Z

pkg/xds/envoy/clusters/v3/cleanup_interval_configurer.go

+var _ ClusterConfigurer = &CleanupIntervalConfigurer{}
+
+func (config *CleanupIntervalConfigurer) Configure(c *envoy_cluster.Cluster) error {
+	c.CleanupInterval = &durationpb.Duration{Seconds: config.Interval}


shouldn't it just be added to PassThroughClusterConfigurer?

jakubdyszkiewicz · 2022-08-01T07:16:53Z

pkg/xds/generator/inbound_proxy_generator.go

+			clusterBuilder = envoy_clusters.NewClusterBuilder(proxy.APIVersion).
+				Configure(envoy_clusters.ProvidedEndpointCluster(localClusterName, false, core_xds.Endpoint{Target: endpoint.WorkloadIP, Port: endpoint.WorkloadPort})).
+				Configure(envoy_clusters.Timeout(defaults_mesh.DefaultInboundTimeout(), protocol))
+			if endpoint.WorkloadIP != core_mesh.IPv4Loopback.String() {


I could use some explanation what is going on here. Why are we setting upstream bind config on this condition?

jakubdyszkiewicz · 2022-08-01T07:48:36Z

test/e2e_env/multizone/inbound_communication/inbound_passthrough.go

+	})
+	E2EAfterAll(func() {
+		Expect(env.KubeZone1.TriggerDeleteNamespace(namespace)).To(Succeed())
+		Expect(env.KubeZone1.DeleteMesh(mesh))


how did it work... we should only be able to delete mesh from global cp

jakubdyszkiewicz · 2022-08-01T09:32:14Z

test/e2e_env/multizone/inbound_communication/inbound_passthrough.go

+		Expect(env.KubeZone1.TriggerDeleteNamespace(namespace)).To(Succeed())
+		Expect(env.KubeZone1.DeleteMesh(mesh))
+		Expect(env.UniZone1.DeleteMeshApps(mesh)).To(Succeed())
+		Expect(env.Global.DeleteMeshApps(mesh)).To(Succeed())


global does not have mesh apps

jakubdyszkiewicz · 2022-08-01T09:37:08Z

test/e2e_env/multizone/inbound_communication/inbound_passthrough_disabled.go

+		Expect(env.KubeZone2.TriggerDeleteNamespace(namespace)).To(Succeed())
+		Expect(env.KubeZone2.DeleteMesh(mesh))
+		Expect(env.UniZone2.DeleteMeshApps(mesh)).To(Succeed())
+		Expect(env.Global.DeleteMeshApps(mesh)).To(Succeed())


jakubdyszkiewicz · 2022-08-01T09:48:43Z

test/e2e_env/multizone/inbound_communication/inbound_passthrough_disabled.go

+		Expect(err).To(HaveOccurred())
+	})
+
+	It("should check communication k8s to universal", func() {


I'd consider splitting this to Describe("k8s to universal communication") and many Its

Same for other its and second file with e2e tests

jakubdyszkiewicz · 2022-08-01T09:55:04Z

api/mesh/v1alpha1/dataplane_helpers_test.go

@@ -8,7 +8,7 @@ import (
 )

 var _ = Describe("MultiValueTagSet", func() {
-
+	EnableInboundPassthrough = true


I'd rather see this in BeforeSuit / BeforeAll / It section. Right now, the scope of this change is not really clear to me.

lobkovilya

Great job! I think I covered just half of the changes, I'll need another round

lobkovilya · 2022-08-03T08:44:34Z

api/mesh/v1alpha1/dataplane_helpers.go

@@ -38,6 +38,9 @@ const (
 	TCPPortReserved = 49151 // IANA Reserved
 )

+// We should remove it in the future version


maybe it should be formatted as todo?

and a linked issue to get rid of this

lobkovilya · 2022-08-03T08:47:37Z

api/mesh/v1alpha1/dataplane_helpers.go

-		iface.WorkloadIP = "127.0.0.1"
+		switch EnableInboundPassthrough {
+		case true:
+			if n.TransparentProxying != nil &&


can we use IsUsingInboundTransparentProxy here?

lobkovilya · 2022-08-03T08:50:28Z

api/mesh/v1alpha1/dataplane_helpers.go

@@ -431,6 +444,18 @@ func (d *Dataplane) GetIdentifyingService() string {
 	return ServiceUnknown
 }

+func (d *Dataplane) IsUsingInboundTransparentProxy() bool {
+	return d.GetNetworking() != nil &&


probably just

return d.GetNetworking().GetTransparentProxying().GetRedirectPortInbound() != 0

is enough

lobkovilya · 2022-08-03T08:52:17Z

api/mesh/v1alpha1/metrics.proto

@@ -73,6 +73,9 @@ message PrometheusAggregateMetricsConfig {
  // If false then the application won't be scrapped. If nil, then it is treated
  // as true and kuma-dp scrapes metrics from the service.
  google.protobuf.BoolValue enabled = 4;
+
+  // Address on which a service listen for incoming requests.


Suggested change

// Address on which a service listen for incoming requests.

// Address on which a service listens for incoming requests.

lobkovilya · 2022-08-03T09:10:53Z

pkg/config/app/kuma-cp/config.go

@@ -27,6 +27,8 @@ var _ config.Config = &Defaults{}

 type Defaults struct {
 	SkipMeshCreation bool `yaml:"skipMeshCreation" envconfig:"kuma_defaults_skip_mesh_creation"`
+	// If true, instead of providing inbound clusters with address of localhost, generates cluster with ORIGINAL_DST
+	EnableInboundPassthrough bool `yaml:"enableInboundPassthrough" envconfig:"KUMA_DEFAULTS_ENABLE_INBOUND_PASSTHROUGH"`


I can imagine users just deleting enableInboundPassthrough if they want to disable this behavior. So I'd rather call it DisableInboundPassthrough and in that case deleting it in the future won't be a breaking change if you don't have this field in the config. Also DisableInboundPassthrough gives the user a hint that this is a less desired behavior. WDYT?

lobkovilya · 2022-08-03T09:21:48Z

pkg/core/bootstrap/bootstrap.go

@@ -125,6 +126,9 @@ func buildRuntime(appCtx context.Context, cfg kuma_cp.Config) (core_runtime.Runt
 		))
 	}

+	// The setting should be removed, and there is no easy way to set it without breaking most of the code
+	mesh_proto.EnableInboundPassthrough = builder.Config().Defaults.EnableInboundPassthrough


can we be sure there are no concurrent reads/writes to this variable?

lobkovilya · 2022-08-03T11:36:56Z

pkg/xds/generator/inbound_proxy_generator.go

+		} else {
+			listenerPort = endpoint.DataplanePort
+			inboundListenerName = envoy_names.GetInboundListenerName(endpoint.DataplaneIP, endpoint.DataplanePort)
+			localClusterName = envoy_names.GetInboundClusterName(endpoint.WorkloadPort)


should this be:

localClusterName = envoy_names.GetInboundClusterName(endpoint. DataplanePort)

because otherwise localClusterName is the same regardless EnableInboundPassthrough

lobkovilya · 2022-08-03T11:38:28Z

pkg/xds/generator/inbound_proxy_generator.go

+			localClusterName = envoy_names.GetInboundClusterName(endpoint.WorkloadPort)
+			clusterBuilder = envoy_clusters.NewClusterBuilder(proxy.APIVersion).
+				Configure(envoy_clusters.ProvidedEndpointCluster(localClusterName, false, core_xds.Endpoint{Target: endpoint.WorkloadIP, Port: endpoint.WorkloadPort})).
+				Configure(envoy_clusters.Timeout(defaults_mesh.DefaultInboundTimeout(), protocol))


why timeouts are different depending on EnableInboundPassthrough?

lobkovilya · 2022-08-03T11:41:18Z

pkg/xds/generator/probe_generator.go

@@ -30,11 +42,18 @@ func (g ProbeProxyGenerator) Generate(ctx xds_context.Context, proxy *model.Prox
 	virtualHostBuilder := envoy_routes.NewVirtualHostBuilder(proxy.APIVersion).
 		Configure(envoy_routes.CommonVirtualHost("probe"))

-	portSet := map[uint32]bool{}
+	inbounds := map[uint32]ProtocolInboundInfo{}


I'd call it inboundsByPort to be more obvious what's the key

lukidzi · 2022-08-03T12:29:12Z

I've redesigned the solution and I will create a new PR which is going to be easier to review. This solution might break the current matching of rules that's why I've implemented this a bit differently. Didn't want to push them here because a number of commits would make it unreadable. Here is the new PR #4750

lukidzi added 7 commits July 19, 2022 21:07

init

0f9796f

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

test

c103eda

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

fix(*): changed how we deliver cluster port

181eb46

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

Merge branch 'master' into fix/localhost-expose

2b358c1

fix(*): fixed test

7eb39fc

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

Merge branch 'fix/localhost-expose' of github.com:lukidzi/kuma into f…

8b678ba

…ix/localhost-expose

test

59e7559

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

lukidzi added 22 commits July 26, 2022 16:00

pass flag

d831372

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

added some tests

e9727be

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

added e2e multizone test

79a6181

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

added more test

559e8e6

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

pass arg

f2e48a6

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

check fix

c639489

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

cleanup

0f57652

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

adding tests

dda1d0c

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

fix

8675e2a

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

Merge branch 'master' into fix/localhost-expose

79e1f54

split test

a13f7b8

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

Merge branch 'fix/localhost-expose' of github.com:lukidzi/kuma into f…

8170f5d

…ix/localhost-expose

fix check

0e9451d

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

fix tests

65565ea

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

Merge branch 'master' into fix/localhost-expose

0007f14

moved to global

21f81fe

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

Merge branch 'fix/localhost-expose' of github.com:lukidzi/kuma into f…

22f99e0

…ix/localhost-expose

test

f0c9e25

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

changed the way of generation clusters

1491b4d

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

Merge branch 'master' into fix/localhost-expose

5e184e3

changed to switch

c53f44d

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

make check

292049e

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

lukidzi added 2 commits July 28, 2022 22:22

reverted cleanup

3be2fe7

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

changed naming

04b0143

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

lukidzi marked this pull request as ready for review July 28, 2022 21:58

lukidzi requested a review from a team as a code owner July 28, 2022 21:58

lukidzi added 7 commits July 29, 2022 11:05

fix(*): added e2e test universal with containerIP and fixed outbound

0c383fa

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

fix(*): added metrics test

7804f76

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

added service address test

9390c65

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

added upgrade.md

138473f

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

Merge branch 'master' into fix/localhost-expose

d547fe1

Merge branch 'master' into fix/localhost-expose

ccfb8c6

fix(*): fixed tests after logic change

ca96fab

Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>

michaelbeaumont reviewed Aug 1, 2022

View reviewed changes

jakubdyszkiewicz reviewed Aug 1, 2022

View reviewed changes

lobkovilya reviewed Aug 3, 2022

View reviewed changes

lukidzi closed this Aug 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(*): localhost exposed application shouldn't be reachable #4654

fix(*): localhost exposed application shouldn't be reachable #4654

lukidzi commented Jul 22, 2022 •

edited

Loading

codecov-commenter commented Jul 22, 2022 •

edited

Loading

lukidzi commented Jul 28, 2022

michaelbeaumont left a comment

michaelbeaumont Aug 1, 2022

jakubdyszkiewicz Aug 1, 2022

jakubdyszkiewicz Aug 1, 2022

jakubdyszkiewicz Aug 1, 2022

jakubdyszkiewicz Aug 1, 2022

jakubdyszkiewicz Aug 1, 2022

jakubdyszkiewicz Aug 1, 2022

jakubdyszkiewicz Aug 1, 2022

jakubdyszkiewicz Aug 1, 2022

jakubdyszkiewicz Aug 1, 2022

jakubdyszkiewicz Aug 1, 2022

lobkovilya left a comment

lobkovilya Aug 3, 2022

lobkovilya Aug 3, 2022

lobkovilya Aug 3, 2022

lobkovilya Aug 3, 2022

lobkovilya Aug 3, 2022

lobkovilya Aug 3, 2022

lobkovilya Aug 3, 2022

lobkovilya Aug 3, 2022

lobkovilya Aug 3, 2022

lobkovilya Aug 3, 2022

lukidzi commented Aug 3, 2022

	// Address on which a service listen for incoming requests.
	// Address on which a service listens for incoming requests.

fix(*): localhost exposed application shouldn't be reachable #4654

fix(*): localhost exposed application shouldn't be reachable #4654

Conversation

lukidzi commented Jul 22, 2022 • edited Loading

Summary

What can I do to make the smooth upgrade?

Full changelog

Issues resolved

Documentation

Testing

Backwards compatibility

codecov-commenter commented Jul 22, 2022 • edited Loading

Codecov Report

lukidzi commented Jul 28, 2022

michaelbeaumont 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lobkovilya 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukidzi commented Aug 3, 2022

lukidzi commented Jul 22, 2022 •

edited

Loading

codecov-commenter commented Jul 22, 2022 •

edited

Loading