Skip to content

Exclude unused resources from rbac#4572

Merged
gusfcarvalho merged 2 commits intoexternal-secrets:mainfrom
bb-Ricardo:exclude-unused-resources-from-rbac
Mar 20, 2025
Merged

Exclude unused resources from rbac#4572
gusfcarvalho merged 2 commits intoexternal-secrets:mainfrom
bb-Ricardo:exclude-unused-resources-from-rbac

Conversation

@bb-Ricardo
Copy link
Copy Markdown
Contributor

@bb-Ricardo bb-Ricardo commented Mar 20, 2025

Warning

This PR is based on #4570

Problem Statement

The current Helm chart assigns resources in the RBAC definition even if they are not used by the controller

Proposed Changes

Based on the configuration, even if (for example) processClusterExternalSecret is set to false the controller is assigned permissions to manage ClusterExternalSecret resources. This is highly undesired in clusters with multiple installed ES instances.

Checklist

  • I have read the contribution guidelines
  • All commits are signed with git commit --signoff
  • My changes have reasonable test coverage
  • All tests pass with make test
  • I ensured my PR is ready for review with make reviewable

@bb-Ricardo
adds Helm value option to disable OpenShift finalizers in RBAC
8f69b12 
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
@bb-Ricardo
excludes unused resources from RBAC definitions
72f2127 
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
@bb-Ricardo bb-Ricardo requested a review from a team as a code owner March 20, 2025 11:35
@bb-Ricardo bb-Ricardo requested a review from gusfcarvalho March 20, 2025 11:35
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Mar 20, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Mar 20, 2025

Hi @bb-Ricardo ! thanks for your contribution!

Just to see if I understood - you have cluster scoped resources installed, but you have some external-secrets deployments there in which you do not wish for them to handle those cluster scoped resources?

@bb-Ricardo
Copy link
Copy Markdown
Contributor Author

bb-Ricardo commented Mar 20, 2025

Hi @gusfcarvalho,

Szenario:

  • operator team provides external-secrets instances namespace scoped in tenant namespace.
  • cluster scoped resources have been disable in Helm chart deployment
  • tenant can use the service account token to edit cluster scoped resources (even if the tenant should not)

@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Mar 20, 2025

Thanks for the clarification! While this PR is useful, I don't personally think it enhances the security posture that much in your use case, since cluster scoped resources are already disabled. In any case, for sure, least privileges are always good to have :)

(also worth to say: you should block tenants from serviceaccounts/token:create for controllers/operators they do not own!)

@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Mar 20, 2025 •
edited
Loading

can you please just run make reviewable? there are a couple of updates to helm tests that are missing here.

Or, individually:
make helm.generate
make helm.docs
make helm.test.update
make test.crds.update
make helm.schema.update

@bb-Ricardo
Copy link
Copy Markdown
Contributor Author

bb-Ricardo commented Mar 20, 2025 •
edited
Loading

I ran make reviewable but there are no further changes to the source code.

/Volumes/github/bb-external-secrets14:19 $ make helm.generate
make helm.docs
make helm.test.update
make test.crds.update
make helm.schema.update
./hack/helm.generate.sh deploy/crds deploy/charts/external-secrets
14:19:09 [ OK ] Finished generating helm chart files
time="2025-03-20T13:19:10Z" level=info msg="Found Chart directories [.]"
time="2025-03-20T13:19:10Z" level=info msg="Generating README Documentation for chart /helm-docs"
./hack/helm.generate.sh deploy/crds deploy/charts/external-secrets
14:19:12 [ OK ] Finished generating helm chart files
Error: unknown command "unittest" for "helm"
Run 'helm --help' for usage.
make: *** [helm.test.update] Error 1
test -s /Volumes/github/bb-external-secrets/bin/cty || curl -fsSL https://github.com/Skarlso/crd-to-sample-yaml/releases/download/v1.1.3/cty_darwin_amd64.tar.gz | tar -xz -C /Volumes/github/bb-external-secrets/bin cty
./hack/test.crds.generate.sh deploy/crds tests/crds
14:19:15 [ OK ] Finished generating crds for testing
14:19:15 [ .. ] /Volumes/github/bb-external-secrets/bin/cty test tests -u
/Volumes/github/bb-external-secrets/bin/cty test tests -u
+--------+------------------------------------------------+---------------+-------+--------------------------------------+
| STATUS | IT                                             | MATCHER       | ERROR | TEMPLATE                             |
+--------+------------------------------------------------+---------------+-------+--------------------------------------+
| PASS   | matches ExternalSecret correctly               | matchSnapshot |       | tests/crds/externalsecret.yml        |
| PASS   | matches GithubAccessToken correctly            | matchSnapshot |       | tests/crds/githubaccesstoken.yml     |
| PASS   | matches Grafana correctly                      | matchSnapshot |       | tests/crds/grafana.yml               |
| PASS   | matches Password correctly                     | matchSnapshot |       | tests/crds/password.yml              |
| PASS   | matches STSSessionToken correctly              | matchSnapshot |       | tests/crds/stssessiontoken.yml       |
| PASS   | matches VaultDynamicSecret generator correctly | matchSnapshot |       | tests/crds/vaultdynamicsecret.yml    |
| PASS   | matches ClusterExternalSecret correctly        | matchSnapshot |       | tests/crds/clusterexternalsecret.yml |
| PASS   | matches ECRAuthorizationToken correctly        | matchSnapshot |       | tests/crds/ecrauthorizationtoken.yml |
| PASS   | matches ACRAccessToken correctly               | matchSnapshot |       | tests/crds/acraccesstoken.yml        |
| PASS   | matches SecretStore correctly                  | matchSnapshot |       | tests/crds/secretstore.yml           |
| PASS   | matches QuayAccessToken correctly              | matchSnapshot |       | tests/crds/quayaccesstoken.yml       |
| PASS   | matches UUID generator correctly               | matchSnapshot |       | tests/crds/uuid.yml                  |
| PASS   | matches GCRAccessToken generator correctly     | matchSnapshot |       | tests/crds/gcraccesstoken.yml        |
| PASS   | matches GeneratorState correctly               | matchSnapshot |       | tests/crds/generatorstate.yml        |
| PASS   | matches Fake generator correctly               | matchSnapshot |       | tests/crds/fake.yml                  |
| PASS   | matches PushSecret correctly                   | matchSnapshot |       | tests/crds/pushsecret.yml            |
| PASS   | matches Webhook generator correctly            | matchSnapshot |       | tests/crds/webhook.yml               |
| PASS   | matches ClusterGenerator correctly             | matchSnapshot |       | tests/crds/clustergenerator.yml      |
| PASS   | matches ClusterSecretStore correctly           | matchSnapshot |       | tests/crds/clustersecretstore.yml    |
+--------+------------------------------------------------+---------------+-------+--------------------------------------+

Tests total: 19, failed: 0, passed: 19
14:19:16 [ OK ] Successfully updated all test snapshots
14:19:16 [ .. ] Installing helm-values-schema-json plugin
Error: plugin already exists
14:19:19 [ OK ] Installed helm-values-schema-json plugin
14:19:19 [ .. ] Generating values.schema.json
JSON schema successfully generated
14:19:19 [ OK ] Generated values.schema.json
✔ /Volumes/github/bb-external-secrets [exclude-unused-resources-from-rbac|✔]
14:19 $ git status
On branch exclude-unused-resources-from-rbac
Your branch is up to date with 'origin/exclude-unused-resources-from-rbac'.

nothing to commit, working tree clean

@bb-Ricardo
Copy link
Copy Markdown
Contributor Author

bb-Ricardo commented Mar 20, 2025

Thanks for the clarification! While this PR is useful, I don't personally think it enhances the security posture that much in your use case, since cluster scoped resources are already disabled. In any case, for sure, least privileges are always good to have :)

(also worth to say: you should block tenants from serviceaccounts/token:create for controllers/operators they do not own!)

Consider an additional cluster scoped instance installed as well. And if the user can shell into the tenant scoped external-secrets container it is easy to extract the projected service account token.

@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Mar 20, 2025

Thanks for the clarification! While this PR is useful, I don't personally think it enhances the security posture that much in your use case, since cluster scoped resources are already disabled. In any case, for sure, least privileges are always good to have :)
(also worth to say: you should block tenants from serviceaccounts/token:create for controllers/operators they do not own!)

Consider an additional cluster scoped instance installed as well. And if the user can shell into the tenant scoped external-secrets container it is easy to extract the projected service account token.

Yeah, that was my concern.

Please beware this setup you have (cluster scoped and namespace scoped instances installed within the same cluster) is neither supported nor tested upstream. This is just a heads up as things can go wrong very fast in many ways 😄

re: the PR itself, it looks good to me. Thanks for your contribution 🙌🏾🙌🏾

@gusfcarvalho gusfcarvalho merged commit 82cad90 into external-secrets:main Mar 20, 2025
21 checks passed
@gusfcarvalho gusfcarvalho mentioned this pull request Mar 20, 2025
Closed
5 tasks
@bb-Ricardo
Copy link
Copy Markdown
Contributor Author

bb-Ricardo commented Mar 20, 2025

Awesome, thank you very much

ivankatliarchuk added a commit to gofogo/external-secrets-fork that referenced this pull request Mar 20, 2025
@ivankatliarchuk
Merge branch 'main' into feat-aws-tags-1821
472ce24 
* main:
  Clarify that setting `spec.refreshInterval` to 0 disables all update behaviour (external-secrets#4567)
  Helm: disable ClusterPushSecret reconciler when using scoped RBAC (external-secrets#4571)
  Exclude unused resources from rbac (external-secrets#4572)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gusfcarvalho gusfcarvalho gusfcarvalho approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bb-Ricardo @gusfcarvalho