Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from
Aug 12, 2022

Conversation

lukidzi
Copy link
Contributor

@lukidzi lukidzi commented Aug 3, 2022

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 cluster 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 point to DataplaneIP

  1. Application that is binding to localhost won't be accessible anymore.
  2. User can expose application binding to localhost by dataplane.networking.inbound[].serviceAddress

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_LOCALHOST_INBOUND_CLUSTERS - not recommended, we are going to remove this flag in the future

Full changelog

  • [Changed the default inbound cluster IP from localhost to DataplaneIP]
  • [Setting upstream_bind_address for inbound clusters that don't set have localhost address]
  • [Added e2e tests that check behavior with different application bindings]
  • [Changed inbound clusters naming: localhost -> inbound]
  • [Made changes to applications metrics scraper to use DataplaneIP]

lukidzi added 3 commits August 3, 2022 08:32
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2022

Codecov Report

Merging #4750 (61329d7) into master (9e0f8c9) will increase coverage by 0.00%.
The diff coverage is 44.44%.

@@           Coverage Diff           @@
##           master    #4750   +/-   ##
=======================================
  Coverage   46.45%   46.45%           
=======================================
  Files         690      690           
  Lines       47115    47164   +49     
=======================================
+ Hits        21888    21912   +24     
- Misses      23300    23322   +22     
- Partials     1927     1930    +3     
Impacted Files Coverage Δ
pkg/core/bootstrap/bootstrap.go 0.00% <0.00%> (ø)
pkg/xds/bootstrap/components.go 0.00% <0.00%> (ø)
pkg/xds/bootstrap/handler.go 35.89% <0.00%> (-0.47%) ⬇️
pkg/xds/envoy/names/resource_names.go 22.72% <ø> (ø)
app/kuma-dp/pkg/dataplane/metrics/server.go 9.75% <8.69%> (-1.57%) ⬇️
pkg/util/net/ips.go 36.36% <42.85%> (+3.03%) ⬆️
app/kuma-dp/cmd/run.go 66.16% <50.00%> (-0.33%) ⬇️
api/mesh/v1alpha1/dataplane_helpers.go 52.30% <60.00%> (-0.04%) ⬇️
pkg/xds/generator/inbound_proxy_generator.go 74.77% <66.66%> (-0.47%) ⬇️
pkg/xds/bootstrap/generator.go 61.11% <84.21%> (-0.12%) ⬇️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lukidzi lukidzi changed the title fix(*):localhost exposed application shouldn't be reachable fix(*): localhost exposed application shouldn't be reachable Aug 3, 2022
lukidzi added 2 commits August 3, 2022 11:53
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
@lukidzi lukidzi marked this pull request as ready for review August 3, 2022 12:29
@lukidzi lukidzi requested a review from a team as a code owner August 3, 2022 12:29
lukidzi added 3 commits August 3, 2022 15:57
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for the commits, much easier to review IMO

pkg/xds/generator/inbound_proxy_generator.go Show resolved Hide resolved
test/e2e_env/kubernetes/gateway/cross-mesh.go Show resolved Hide resolved
test/server/cmd/echo.go Outdated Show resolved Hide resolved
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
@lobkovilya
Copy link
Contributor

@lukidzi what happens if application binds to 127.0.0.6 when acts as a client? Does it bypass the envoy in that case?

@lukidzi lukidzi added the ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully) label Aug 9, 2022
@slonka
Copy link
Contributor

slonka commented Aug 9, 2022

@lukidzi - you need to pull master for the ci/run-full-matrix label to work

@lukidzi
Copy link
Contributor Author

lukidzi commented Aug 9, 2022

@lukidzi what happens if application binds to 127.0.0.6 when acts as a client? Does it bypass the envoy in that case?

It seems so, but it looks like it's the same behavior now. We've already had that rule so from iptables there is no change.

lukidzi added 5 commits August 9, 2022 16:48
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
Signed-off-by: Łukasz Dziedziak <lukidzi@gmail.com>
@lukidzi lukidzi merged commit 74b0ee0 into kumahq:master Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Applications bound to localhost behind the Service are reachable when Kuma deployed
6 participants