Triggers RBAC

[vtereso]

Expected Behavior

Users should specify a service account (or a role binding?) in the EventListener to be used for resources created as per specific TriggerTemplate.

Actual Behavior

Currently, there is no implementation.

Additional Info

Currently, the EventListener creates a Deployment and a load balancer Service (to reference the deployment), which enables the pods to be addressable/receive external traffic. The service account tied to the EventListener listener pod has permissions necessary to read Triggers resources, but how can it get the permissions necessary for creating resources defined in a particular TriggerTemplate, which are arbitrary?

[vtereso]

We could simply add the permissions necessary to create Tekton resources as a default. If roles could be added onto a service account for the life of the EventListener, this might work well too.

[ncskier]

Aside: we currently don't have a good name for the pod/deployment that runs when an EventListener is created; I don't like calling the pod the EventListener, because that's already the name of the resource. For now, I'll call that pod/deployment the EventListener "sink."

---

So, the design challenge I see at the moment is that the EventListener "sink" pod needs to get/create two different sets of resources:

• get: Triggers resources (EventListener, TriggerBinding, and TriggerTemplate)
• create: Arbitrary resource types specified in the TriggerTemplate spec

The EventListener "sink" must be authorized by RBAC to handle both of these resource groups. I think that Triggers should probably handle configuring the "Triggers resources" authorization, and the user should have to configure the "Arbitrary resource types" authorization.

Please let me know if I'm missing something; is this way simpler than I'm making it out to be? Here are a couple of implementations that I have been thinking about to address this design challenge (feedback is very welcome, and sorry for the long comment 😅 ):

Idea 1: EventListener spec specifies a ServiceAccount

When creating an EventListener, the user specifies a ServiceAccount in the EventListener spec, and this ServiceAccount gets attached to the EventListener "sink," authorizing its k8s requests. The user must set up this ServiceAccount so that it has access to create all "Arbitrary resources" defined in the associated TriggerTemplates.

There will exist a ClusterRole called event-listener-sink (or something) which has access to get the "Triggers resources," and this ClusterRole will have been created here when the user installed the TriggersProject. When the user creates an EventListener, our Triggers controller will create a ClusterRoleBinding to bind the event-listener-sink to the ServiceAccount specified in the EventListener spec. The EventListener resource will be one of the OwnerReferences on the ClusterRoleBinding, so when the EventListener is deleted, the ClusterRoleBinding will also be deleted.

✅ After the user has created their EventListener, the "sink" will be authorized by the user's ServiceAccount to create the "Arbitrary resource types specified in the TriggerTemplate spec," and the "sink" will be authorized by the event-listener-sink ClusterRole attached to the user's ServiceAccount to get "Triggers resources."

Idea 2: TriggerTemplate spec specifies a Role

This idea is motivated by the notion that since the TriggerTemplate defines the resources that it creates, it makes sense that it would also specify the authorization mechanism to create these resources.

When creating a TriggerTemplate, the user specifies a Role (or ClusterRole?) in the TriggerTemplate spec, which has authorization to create the "Arbitrary resource types specified in the TriggerTemplate."

There will exist the event-listener-sink ClusterRole from Idea 1 which has access to get the "Triggers resources." When an EventListener is created, the Triggers controller will create:

• a ServiceAccount
• the Role specified in the TriggerTemplate
• a RoleBinding that attaches this Role to the ServiceAccount
• a ClusterRoleBinding attaching the event-listener-sink ClusterRole to the ServiceAccount

Similarly to Idea 1, the ServiceAccount, Role, RoleBinding, and ClusterRoleBinding would all have the EventListener as one of their OwnerReferences. So, when the EventListener is deleted, all of these generated resources will also be deleted.

✅ After the user has created their EventListener, the "sink" will be authorized by the TriggerTemplate's Role bound to the generated ServiceAccount to create the "Arbitrary resource types specified in the TriggerTemplate spec," and the "sink" will be authorized by the event-listener-sink ClusterRole bound to the generated ServiceAccount to get "Triggers resources."

[vtereso]

Idea one 🙏

[vincent-pli]

Could we add permission for all resource of group tekton.dev (Pipeline) to ClusterRole: event-listener-sink.
I think most of case are in the scope of tekton pipeline.

Base on that, I'm a little prefer idea two. With idea two,

• In most of case (in tekton-pipeline), user need do nothing more than create EventListener, TriggerBinding and TriggerTemplate.
• If user want to handle resource not in tekton-pipeline, they should define resource in TriggerTemplate and of course should define the role in same place.

[iancoffey]

Another vote for idea one 👍

[vtereso]

Creating service accounts automatically seem questionable. Adding access to the Triggers resources shouldn't be an issue. I will take this approach.
/assign

[ncskier]

So that we only give access to read the Triggers resources in the current namespace, we can create a Role in that namespace with only get 👍

[dibyom]

Given that the user already has to ensure that the service account has access to other resources that get created from the templates, would it be too much work for the user to add the roles/bindings for the trigger resources?

I definitely think that we should make this easy for the user but perhaps docs, examples, and some validation (e.g. if the service account does not have access to trigger bindings and templates, we return an explicit error stating so) is sufficient?

Besides being more explicit, it reduces the complexity and scope of the controller (e.g. does not need access to create/update roles and bindings) , prevents some issues (like two event listeners using the same service account as @vtereso mentioned in #73)

Also, given #83 where we restrict templates to be able to create only Tekton resources, I think it might make even more sense to do this statically instead of dynamically generating roles/bindings from the controller?

[bobcatfish]

Strong +1 to what @dibyom said! I also want to add:

a) sorry for coming into this late!!! i think its very frustrating to implement something only to have someone come along and challenge the entire idea, i apologize for not voicing my opinion sooner 🙏
b) I think it comes down to what the purpose of the whole role / service account / role binding system is. afaik the idea is to give the operator of the system control over who can do what. Assuming the user intended for the provided service account to have the permission it needs to interact with our CRDs and granting that permission automatically seems like it defeats the purpose of putting the user/operator in control at all.

But the best part about leaving this up to the user for now is that we can easily add this later if we decide it's a good idea! So I vote for not adding this functionality right now and only adding it once we're sure we need it. (Maybe I'm missing a reason we need this right away?)

(Related: @pmorie raises an interesting point in general in #77 about users being able to escalate privileges - adding permissions to an existing service account feels like we would intentionally be literally escalating privileges?)

[vtereso]

I made a note in #73 that users could provide a field to do it automatically, which would default to not doing so. To the case of this being an escalation of RBAC, the SA will need the create permissions on Triggers resources (maybe just the EventListener) to generate the get privilege where this seems somewhat reasonable (in comparison to the reverse). I agree it's not necessary.

[bobcatfish]

@vtereso if we agree that it isn't necessary, what do you think about moving this out of the 0.1 milestone and revisiting it later?

Alternatively I could settle for making it configurable and off by default like you said, but my preference is (always XD) to avoid adding functionality as long as we can until we're sure we need it.

[vtereso]

I think it removing it from the milestone makes sense. To your point, we can revisit it if necessary.

[bobcatfish]

kk sounds good - thanks @vtereso !

[vtereso]

As per WG, this doesn't seem necessary so I am closing.