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

Request for change of behavior introduced in 1.2.0 which breaks prior use cases #28

Closed
dkowsley opened this issue Sep 26, 2022 · 4 comments · Fixed by #46
Closed

Request for change of behavior introduced in 1.2.0 which breaks prior use cases #28

dkowsley opened this issue Sep 26, 2022 · 4 comments · Fixed by #46

Comments

@dkowsley
Copy link

dkowsley commented Sep 26, 2022

In c55ad5e, a change was introduced which only allows DopplerSecret objects in the operator namespace. This breaks self-service within multi-tenancy.

Instead, I would like to propose a change that takes into account existing RBAC in the cluster while providing the original flexibility.

  1. Add a ValidatingWebhookConfiguration that registers a webhook for all DopplerSecret objects.
  2. Reject any DopplerSecret objects that attempt to overwrite any existing managedSecrets
  3. Reject any Doppler Secret objects that violate the user's RBAC. Combining SelfSubjectAccessReview and impersonation with the userInfo data will allow the operator to determine authorization of the action at time of creation of the DopplerSecret

For those looking for a solution to maintain the previous behavior

You should be able to specify the previous chart version of 1.1.1 with helm. That is our temporary solution.

@dkowsley
Copy link
Author

dkowsley commented Sep 27, 2022

Here are the two flows that this contains. Please note that the red and green rectangles contain a SelfSubjectAccessReview request nested within the initial ValidatingWebhook callback from the API.

User does NOT have RBAC access to source/dest secrets

sequenceDiagram
User ->> k8s: PUT/PATCH/POST DopplerSecret
rect rgb(255, 0, 0)
k8s ->> Operator: ValidatingWebhook POST: payload (contains userInfo from User)
Operator ->>k8s: SelfSubjectAccessReview POST: with payload.userInfo impersonation
k8s -x Operator: SelfSubjectAccessReview response: NO either source and/or dest secrets are not manageable by User
Operator -x k8s: ValidatingWebhook response: Resource is not valid. Deny it.
end
k8s -x User: DopplerSecret rejected.
Loading

User has RBAC access to source/dest secrets

sequenceDiagram
User ->> k8s: PUT/PATCH/POST DopplerSecret
rect rgb(0, 127, 0)
k8s ->> Operator: ValidatingWebhook POST: payload (contains userInfo from User)
Operator ->> k8s: SelfSubjectAccessReview POST: with payload.userInfo impersonation
k8s ->> Operator: SelfSubjectAccessReview response: YES source/dest secrets are manageable by User.
Operator ->> k8s: ValidatingWebhook response: Resource is valid. Accept it.
end
k8s ->> User: DopplerSecret accepted
k8s --) Operator: DopplerSecret event
Operator ->> Doppler API: Requests config with token
Doppler API ->> Operator: Provides config
Operator ->> k8s: PUT/PATCH/POST config to managedSecret
Loading

Additional notes: a ValidatingAdmissionWebhook, using best practices, will be able to accept or reject any version of a DopplerSecret prior to it being presented as a new/updated object to the Doppler operator event listener. This effectively implements user level authorization via k8s RBAC to the doppler operator as if it was a service itself with the same RBAC policies.

@draganm
Copy link

draganm commented Nov 8, 2022

+1
I've stumbled over this issue too. Using ArgoCD to deploy applications in different namespaces. It would be great if I could keep DopplerSecret resources together with the rest of the application resources so that they would be removed when the namespace is removed.

@draganm
Copy link

draganm commented Nov 17, 2022

Are there any plans for a new release of the operator without restriction of the namespace?
The introduced limitation makes the operator pretty much unusable for my projects, so if there where this is heading I'll have to abandon it.

@nmanoogian
Copy link
Member

Thanks for all of the feedback! We're reviewing options internally for how to meet these use cases. I'll post here with any updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants