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.

Sign up for GitHub

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

Jump to bottom

Reduce RBAC permissions for the various components #3040

Merged
merged 10 commits into from
May 6, 2024
Merged

Reduce RBAC permissions for the various components #3040

merged 10 commits into from
May 6, 2024

Conversation

tpantelis
Copy link
Contributor

@tpantelis tpantelis commented May 2, 2024 •
edited
Loading

A recent post to the submariner-security outlined potential vulnerabilities regarding submariner's RBAC permissions being too broad:

  • create/patch/update verb of the statefulsets/replicasets resource
    • A malicious user can create a privileged container with a malicious container image capable of container escape. This would allow him/she gaining ROOT privilege of the worker node the created container deployed. Since the malicious user can control the pod scheduling parameters (e.g., replicas number, node affinity, …), he/she should be able to deploy the malicious container to every (or almost every) worker node. This means the malicious user will be able to control every (or almost every) worker node in the cluster.
    • The other permissions of statefulsets/replicasets/deployments/daemonsets also share the same risks.

In addition, many permissions aren't even used/needed. Also specifying a wildcard for resources and verbs is not best practice - Sonar flags this.

The intent of this PR is to reduce the permissions to only what's needed. See individual commits for details.

Note that for these changes to actually be applied requires updating and rebuilding subctl. However, I mirrored the changes in submariner-io/submariner-charts#508 and the E2E in the submariner-charts repo directly tests the changes in the PR. So we can be pretty confident the changes are accurate.

Fixes #3041

Depends on submariner-io/admiral#905

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr3040/tpantelis/reduce_rbac
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@tpantelis tpantelis self-assigned this May 2, 2024
@tpantelis tpantelis mentioned this pull request May 2, 2024
Merged
skitt
skitt requested changes May 2, 2024
View reviewed changes
Copy link
Member

@skitt skitt left a comment

Choose a reason for hiding this comment

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

This needs some autosquashing ;-).

@tpantelis tpantelis added the release-note-needed Should be mentioned in the release notes label May 2, 2024
@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label May 3, 2024
@tpantelis
Copy link
Contributor Author

tpantelis commented May 4, 2024 •
edited
Loading

I added 2691550 to eliminate the operator namespaces permission.

Also added b268cd5 and 4b7e3e2.

@tpantelis tpantelis requested review from skitt and aswinsuryan May 4, 2024 14:54
tpantelis added 7 commits May 4, 2024 13:51
@tpantelis
Reduce submariner-operator RBAC permissions
3c1765b 
- Remove Role wildcard access to "pods" - only "get" and "list" access is
  needed which is provided by the ClusterRole.
- "services" are created for metrics so reduce Role access accordingly
- The operator does not access "endpoints", "persistentvolumeclaims",
  "events", "replicasets" or "statefulsets" so remove access.
- Reduce access to "deployments", "daemonsets" and "submariner.io" resources
  to only what's needed and remove wildcard access.

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis
Reduce submariner-gateway RBAC permissions
04e6068 
- The following resources are not accessed at all so remove permissions:
  - "services"
  - "endpoints"
  - "events"
  - "configmaps"
  - "deployments"
  - "daemonsets"
  - "replicasets"
  - "statefulsets"
  - "customresourcedefinitions",
  - "operator.openshift.io/dnses"
  - "config.openshift.io/networks"
  - "monitoring.coreos.com/servicemonitors"

- The gateway pod is annotated - it only needs "get" and "update" access

- Remove wildcard access to "submariner.io" resources and specify
  exactly what's needed.

- Reduce "configmaps" access to just "get".

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis
Reduce submariner-routeagent RBAC permissions
e8268a1 
- The following resources are not accessed so remove permissions:
  - "services"
  - "endpoints"
  - "events"
  - "deployments"
  - "daemonsets"
  - "replicasets"
  - "statefulsets"
  - "customresourcedefinitions",
  - "operator.openshift.io/dnses"
  - "monitoring.coreos.com/servicemonitors"

- Reduce access to "pods", "services", "secrets", "configmaps", and
  "endpoints" to "get" and "list"

- Remove wildcard access to "submariner.io" resources and specify exactly
  what's needed.

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis
Reduce submariner-globalnet RBAC permissions
7e0386a 
- The following resources are not accessed at all so remove permissions:
  - "services"
  - "endpoints"
  - "persistentvolumeclaims"
  - "events"
  - "configmaps"
  - "secrets"
  - "deployments"
  - "daemonsets"
  - "replicasets"
  - "statefulsets"
  - "namespaces"
  - "monitoring.coreos.com/servicemonitors"

- Reduce "pods" access to only "get", "list", "watch"

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis
Reduce lighthouse-agent RBAC permissions
56b7e1f 
- Remove "update" access to "services", "namespaces" and "endpoints".
- Remove wildcard access to "multicluster.x-k8s.io" resources and specify
  exactly what's needed.

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis
Reduce lighthouse-coredns RBAC permissions
3197136 
- The following resources are not accessed so remove permissions:
  - "services"
  - "endpoints"
  - "namespaces"

- Reduce "endpointslices" access to "get", "list", and "watch"

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis
Re-generate pkg/embeddedyamls/yamls.go
830adef 
Signed-off-by: Tom Pantelis <[email protected]>
skitt
skitt requested changes May 6, 2024
View reviewed changes
Copy link
Member

@skitt skitt left a comment

Choose a reason for hiding this comment

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

Requesting changes just to make sure the Admiral import is updated to match the merged commit after submariner-io/admiral#905 goes in.

go.mod Outdated
@@ -13,7 +13,7 @@ require (
github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring v0.73.2
github.com/prometheus-operator/prometheus-operator/pkg/client v0.73.2
github.com/prometheus/client_golang v1.19.0
github.com/submariner-io/admiral v0.18.0-m3
github.com/submariner-io/admiral v0.18.0-m3.0.20240504143439-22ee51817b45
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be updated once submariner-io/admiral#905 is merged.

This was referenced May 6, 2024
Closed
Closed
tpantelis added a commit to tpantelis/submariner-website that referenced this pull request May 6, 2024
@tpantelis
Add 0.18 release notes for submariner-operator PR #3040
09be1fc 
Release notes for submariner-io/submariner-operator#3040

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis tpantelis mentioned this pull request May 6, 2024
Merged
tpantelis added 3 commits May 6, 2024 10:08
@tpantelis
Adjust code in order to remove operator RBAC permissions for namespaces
fe709ca 
We grant namespace read permissions which is only needed to query for
the existence of the "openshift-monitoring" namespace to determine
where to create ServiceMonitors. However we can eliminate this query
and thus the permissions by trying to create ServiceMonitors in the
"openshift-monitoring" namespace and, if the error indicates the
namespace is missing, create in the provided namespace.

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis
Restrict config.openshift.io/networks RBAC permissions
c5e1ee7 
This resource type is only used for network settings discovery to query
the "cluster" Network so restrict the RBAC to only "get" access to the
"cluster" resource name.

This applies to both the submariner-operator:

https://github.com/submariner-io/submariner/blob/85fea596f30b0e84d6962c92bb129a6b8bce8028/pkg/routeagent_driver/handlers/ovn/connection.go#L358

and route-agent components:

https://github.com/submariner-io/submariner/blob/85fea596f30b0e84d6962c92bb129a6b8bce8028/pkg/routeagent_driver/handlers/ovn/connection.go#L358

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis
Restrict operator ClusterRole*, ServiceAccount permissions
737e5e6 
These were added for network-plugin syncer removal so retrict the
delete permissions to the networkplugin-syncer resource names.

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis
Copy link
Contributor Author

Requesting changes just to make sure the Admiral import is updated to match the merged commit after submariner-io/admiral#905 goes in.

Done

@github-actions github-actions bot removed the dependent label May 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 6, 2024

This PR/issue depends on:

@tpantelis tpantelis enabled auto-merge (rebase) May 6, 2024 14:24
@tpantelis tpantelis merged commit 94716b9 into submariner-io:devel May 6, 2024
35 checks passed
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr3040/tpantelis/reduce_rbac]

@tpantelis tpantelis mentioned this pull request May 6, 2024
Merged
@tpantelis tpantelis mentioned this pull request May 6, 2024
Merged
skitt pushed a commit to submariner-io/submariner-website that referenced this pull request May 6, 2024
@tpantelis @skitt
Add 0.18 release notes for submariner-operator PR #3040
76f420b 
Release notes for submariner-io/submariner-operator#3040

Signed-off-by: Tom Pantelis <[email protected]>
@tpantelis tpantelis mentioned this pull request May 6, 2024
Merged
@tpantelis tpantelis deleted the reduce_rbac branch May 21, 2024 18:30
tpantelis added a commit to tpantelis/submariner-website that referenced this pull request Jul 8, 2024
@tpantelis
Add 0.18 release notes for submariner-operator PR #3040
7702488 
Release notes for submariner-io/submariner-operator#3040

Signed-off-by: Tom Pantelis <[email protected]>
tpantelis added a commit to submariner-io/submariner-website that referenced this pull request Jul 9, 2024
@tpantelis
Add 0.18 release notes for submariner-operator PR #3040
8e30906 
Release notes for submariner-io/submariner-operator#3040

Signed-off-by: Tom Pantelis <[email protected]>
@skitt skitt mentioned this pull request Jan 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaaass kaaass kaaass approved these changes

@skitt skitt skitt approved these changes

@dfarrell07 dfarrell07 dfarrell07 approved these changes

@Oats87 Oats87 Awaiting requested review from Oats87 Oats87 is a code owner

@sridhargaddam sridhargaddam Awaiting requested review from sridhargaddam sridhargaddam is a code owner

@vthapar vthapar Awaiting requested review from vthapar vthapar is a code owner

@maayanf24 maayanf24 Awaiting requested review from maayanf24 maayanf24 is a code owner

@yboaron yboaron Awaiting requested review from yboaron yboaron is a code owner

@aswinsuryan aswinsuryan Awaiting requested review from aswinsuryan aswinsuryan is a code owner

Assignees

@tpantelis tpantelis

Labels
backport-handled ready-to-test When a PR is ready for full E2E testing release-note-handled release-note-needed Should be mentioned in the release notes
Projects
No open projects
Submariner 0.18
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove unnecessary RBAC permission in helm charts
6 participants
@tpantelis @submariner-bot @dfarrell07 @skitt @kaaass @aswinsuryan