DopplerHQ / kubernetes-operator

Apache License 2.0
44 stars 18 forks source link

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

Closed dkowsley closed 1 year ago

dkowsley commented 1 year ago

In https://github.com/DopplerHQ/kubernetes-operator/commit/c55ad5e5e98a4fc459825ac49b57125be681d839, 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 commented 1 year ago

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.

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

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 commented 1 year ago

+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 commented 1 year ago

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 commented 1 year ago

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.