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

Remove the secret from clusterrole #3668

Closed
kevinteng525 opened this issue Sep 14, 2022 · 12 comments
Closed

Remove the secret from clusterrole #3668

kevinteng525 opened this issue Sep 14, 2022 · 12 comments
Labels
feature-request All issues for new features that have not been committed to needs-discussion stale All issues that are marked as stale due to inactivity

Comments

@kevinteng525
Copy link
Contributor

Proposal

Greetings,

We want to leverage KEDA for autoscaling, however KEDA will require list/watch/get all secrets across all namespaces. This will introduce security risks and could not deploy on production env.

So we want to limit the secret permission to only list/watch/get on KEDA namespace, and we will only use ClusterTriggerAuthentication. Currently we have 2 proposals:

  1. Add secret to ClientDisableCacheFor when NewManager, however it will increase load on APIServer
  2. Add namespaced informer for secret to only list/watch on KEDA namespace

Both will require code changes.

So we just wondering is there any better options to approach this?

Thanks,
Kevin

Use-Case

Remove the secret from clusterrole, only need to get/list/watch secret on KEDA namespace

Anything else?

No response

@kevinteng525 kevinteng525 added feature-request All issues for new features that have not been committed to needs-discussion labels Sep 14, 2022
@tomkerkhove tomkerkhove moved this to Proposed in Roadmap - KEDA Core Sep 14, 2022
@JorTurFer
Copy link
Member

JorTurFer commented Sep 14, 2022

Hey,
I have reviewed the code and you are right. KEDA gets the secrets for each deployment during the scaler build.
I think that you requirement makes sense totally but I'd go with the second one because I'm not sure if the 1st solution works, with or without cache, you will request the secret.
Are you willing to contribute with it?

Maybe we could add a flag like strictSecretAccess or similar and, based on it, try to recover the variables (secrets & configMaps) only from KEDA namespace. I think that we could check the flag during get operations and if the flag is set, limit the access to KEDA namespace, returning empty map in case of other namespace. I guess this change could avoid the requirement of list/get/watch permissions on other namespaces.
Once this thing is defined, we have to update the chart and the docs to support/explain this feature.

WDYT @kedacore/keda-core-contributors ?

kevinteng525 added a commit to kevinteng525/keda that referenced this issue Sep 19, 2022
Support Restrict Secret Access, refer to kedacore#3668
@kevinteng525
Copy link
Contributor Author

Hey JorTurFer,

Sure, pls. help to review #3677

@JorTurFer
Copy link
Member

Sure, I'll review during the week, I'm a bit busy atm

kevinteng525 added a commit to kevinteng525/keda that referenced this issue Sep 20, 2022
Support Restrict Secret Access, refer to kedacore#3668

Signed-off-by: kevin <[email protected]>
kevinteng525 added a commit to kevinteng525/keda that referenced this issue Oct 8, 2022
Support Restrict Secret Access, refer to kedacore#3668

Signed-off-by: kevin <[email protected]>
@kevinteng525
Copy link
Contributor Author

Hey JorTurFer, I reviewed all your comments and also replied, pls. have a check on this PR again, #3677
BTW, I tried to pass all CI jobs, however there is still one job failed, CI / Static Checks (pull_request) , error as following:

controllers/keda/scaledobject_controller.go:36: File is not `gci`-ed with --skip-generated -s standard,default,prefix(github.com/kedacore/keda) (gci)
	"k8s.io/client-go/dynamic"
controllers/keda/scaledobject_controller.go:54: File is not `gci`-ed with --skip-generated -s standard,default,prefix(github.com/kedacore/keda) (gci)
	corev1listers "k8s.io/client-go/listers/core/v1"
controllers/keda/scaledobject_controller_test.go:34: File is not `gci`-ed with --skip-generated -s standard,default,prefix(github.com/kedacore/keda) (gci)
	"k8s.io/apimachinery/pkg/types"

Error: Process completed with exit code 1.

Actually I didn't touch line 34 and line 36, only added line 54, not sure why it will be failed, would you help me to understand this golangci? or can we just ignore this error?

Regarding configmap restriction, I will create a separate PR so as to be clear and fully tested before submit.
Also I will create PRs to docs and charts later once this PR merged

@JorTurFer
Copy link
Member

I have reviewed the PR 😄
WRTconfigMaps and docs, no worries doing configMaps in another PR, but the docs about secrets should be ready to be merged at the same time we merged the feature. We follow this approach to have synced both and also to have the capability to discuss the approach from both POVs. (I have seen that you already opened the PR to docs ❤️ )

@JorTurFer
Copy link
Member

The issue with the stacic checks is because we enforce the grouping of imports:

- golang
- deps
- keda

In case of scaledobjectcontroller for instance, you are adding a dep in keda group, move it from the 3rd group to the 2nd and the issue will be gone
image

kevinteng525 added a commit to kevinteng525/keda that referenced this issue Dec 4, 2022
Support Restrict Secret Access, refer to kedacore#3668

Signed-off-by: kevin <[email protected]>
kevinteng525 added a commit to kevinteng525/keda that referenced this issue Dec 4, 2022
Support Restrict Secret Access, refer to kedacore#3668

Signed-off-by: kevin <[email protected]>
kevinteng525 added a commit to kevinteng525/keda that referenced this issue Dec 4, 2022
Support Restrict Secret Access, refer to kedacore#3668

Signed-off-by: kevin <[email protected]>
kevinteng525 added a commit to kevinteng525/keda that referenced this issue Dec 5, 2022
Support Restrict Secret Access, refer to kedacore#3668

Signed-off-by: kevin <[email protected]>
kevinteng525 added a commit to kevinteng525/keda that referenced this issue Dec 6, 2022
Support Restrict Secret Access, refer to kedacore#3668

Signed-off-by: kevin <[email protected]>
@stale
Copy link

stale bot commented Dec 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Dec 9, 2022
Repository owner moved this from Proposed to Ready To Ship in Roadmap - KEDA Core Dec 9, 2022
@sdiamond2
Copy link

Posting here, cause I have been troubleshooting for while. Looking at RBACs rules and sharing secrets with the Keda Operator. I have some issues with the current implementation where I need restricted access to secrets in the ClusterRole. I want to give access to the Service account via Roles and RoleBindings in the specific namespaces. I don't want to store any secrets in the Keda Operator's namespace. Do you guys know how to do this or am I crazy for trying this? Thanks

@JorTurFer
Copy link
Member

Do you guys know how to do this or am I crazy for trying this?

You aren't crazy, but I'm not sure if this can be done right now. You could try to enable via RBAC the permission on the namespaces where you want and check if KEDA fails (in case of failures, you will see errors in KEDA logs).
Other option if you use Azure Key Vault or Hashicorp Vault for the secrets is to use the integration directly, it doesn't require any permission on RBAC

@sdiamond2
Copy link

So when I remove the KEDA_RESTRICT environment variable from the deployment, and trying to add a new cluster role (with only a specific secret) and binding, Keda is expecting being able to read ALL secrets, which is not what would want.

Can this be a feature request, we running this on premise, and we are not using Hashicorp vaults? Other projects such as Rancher does allow this to work. I am just wondering.

Thanks for you help! :)

@JorTurFer
Copy link
Member

Can this be a feature request, we running this on premise, and we are not using Hashicorp vaults? Other projects such as Rancher does allow this to work. I am just wondering.

Yeah, sure! Could you open an issue to request it?

@sdiamond2
Copy link

Sound good, I will do it momentarily. I appreciate your help!

I will also add some more details! Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request All issues for new features that have not been committed to needs-discussion stale All issues that are marked as stale due to inactivity
Projects
Archived in project
Development

No branches or pull requests

3 participants