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

install/kubernetes: restrict k8s access for relay #16937

Merged
merged 1 commit into from Jul 22, 2021

Conversation

jonkerj
Copy link
Contributor

@jonkerj jonkerj commented Jul 20, 2021

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

It seems relay has no interaction with Kubernetes, and as such, it does not need (cluster)roles/-bindings or a mounted SA token. This prevents attackers from doing nasty stuff on behalf of relay.

It seems this clusterrole/-binding was introduced in e9cb43c as part of a large refactor of the chart.

I believe this is a (security) fix

Restrict Kubernetes access for hubble-relay

It seems relay has no interaction with Kubernetes, and as such, it does
not need (cluster)roles/-bindings or a mounted SA token. This prevents
attackers from doing nasty stuff on behalf of relay.

Fixes: e9cb43c ("Helm: full refactor of helm charts..")

Signed-off-by: Jorik Jonker <jorik.jonker@eu.equinix.com>
@jonkerj jonkerj requested a review from a team as a code owner July 20, 2021 09:15
@jonkerj jonkerj requested review from a team and errordeveloper July 20, 2021 09:15
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 20, 2021
@rolinh rolinh added the release-note/misc This PR makes changes that have no direct user impact. label Jul 20, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 20, 2021
@rolinh rolinh added sig/hubble Impacts hubble server or relay needs-backport/1.10 labels Jul 20, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.4 Jul 20, 2021
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Nice catch! Indeed, Hubble Relay doesn't need any of these permissions as parts of its regular operation. It seems that they were introduced by mistake as part of the big Helm refactoring (#13259). I've thus marked this PR for backport.

@rolinh
Copy link
Member

rolinh commented Jul 20, 2021

test-me-please

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 21, 2021
@brb brb merged commit 2857b3e into cilium:master Jul 22, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.4 Jul 26, 2021
@jibi jibi mentioned this pull request Jul 26, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.4 Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay
Projects
No open projects
1.10.4
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

8 participants