[WIP]: POC of triggers in different namespace than broker

[Cali0707]

This is a very rough POC of just the RBAC aspect of #7439 , all the reconciler and/or data plane changes still need to be made

Proposed Changes

• use SubjectAccessReviews to check if the user creating a trigger has access to getting a broker in a different namespace than the trigger if the namespace is different than the trigger's namespace.

To test this, create a new user (I called mine cmurray), and given them the following role + rolebinding:

kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  namespace: ns-test 
  name: trigger-broker-manager
rules:
  - apiGroups: 
      - "eventing.knative.dev"
    resources:
      - "triggers"
      - "brokers"
    verbs:
      - "get"
      - "watch"
      - "list"
      - "create"
      - "delete"
---
kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: manager-trigger-broker
  namespace: ns-test 
subjects:
  - kind: User
    name: cmurray
    apiGroup: rbac.authorization.k8s.io
roleRef:
  kind: Role
  name: trigger-broker-manager
  apiGroup: rbac.authorization.k8s.io

Then, create a broker in default namespace, and try to create a trigger in namespace ns-test with a user which has appropriate permissions (for me it was minikube). It should succeed. If you try again with the user without proper permissions, you will see it fails. For example, if I have a trigger in tmp/trigger-other-namespace.yaml, here are my outputs when running this locally:

cat tmp/trigger-other-namespace.yaml
apiVersion: eventing.knative.dev/v1
kind: Trigger
metadata:
  name : test1
  namespace: ns-test
spec:
  broker: my-broker
  brokerNamespace: default 
  subscriber:
    uri: "http://google.com"

kubectl apply -f tmp/trigger-other-namespace.yaml 
trigger.eventing.knative.dev/test1 created

kubectl delete -f tmp/trigger-other-namespace.yaml 
trigger.eventing.knative.dev "test1" deleted

kubectl config use-context cmurray
Switched to context "cmurray".

kubectl apply -f tmp/trigger-other-namespace.yaml 
Error from server (BadRequest): error when creating "tmp/trigger-other-namespace.yaml": admission webhook "validation.webhook.eventing.knative.dev" denied the request: validation failed: user cmurray is not authorized to get brokers in namespace: default: .spec.BrokerNamespace

Still TODO: verify that this RBAC approach is good enough, and deal with writing the actual reconciliation for the trigger when the broker is in a different namespace

Pre-review Checklist

• At least 80% unit test coverage
• E2E tests for any new behavior
• Docs PR for any user-facing impact
• Spec PR for any new API feature
• Conformance test for any change to the spec

Release Note

Docs

[knative-prow]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Cali0707]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Cali0707]

I just realized that there is still one possible RBAC problem here: if a user (different to the user who created the trigger) does not have permission to get brokers in another namespace but has permissions to get triggers in the current namespace, they will be able to see that there is a broker in another namespace with a certain name. Not sure if that is a problem @pierDipi

[Cali0707]

Summarizing discussion with @pierDipi about a path forwards here: we should create a new RBAC verb (e.g. knsubscribe) which allows for the creation of a trigger for a broker, or of a subscription for a channel. Besides that, it will work as this POC with the exception of checking for knsubscribe instead of get on the broker, and checking for the broker name as well

[sadath-12]

Hi @Cali0707 . Was just thinkering around a bit here would be great if you provide the differences that our new verb would make ? . Since the non-creator of trigger policy without this verb enabled and with the "get" verb enabled on our trigger can still see the trigger yaml .

[Cali0707]

@sadath-12 yes they would still be able to see the trigger yaml. The idea of this new verb is to give finer-grained control over who can subscribe to brokers or channels. So I could allow user A to subscribe to broker 1, even if they can't get broker 1. In this scenario, yes people would know that there is a broker with a given name in a namespace, but they won't know anything else. So, I can let people subscribe to my broker without giving them anymore information than the fact that there is a broker with a name in a namespace. Hope this helps!

[sadath-12]

@Cali0707 I would think to merge this (currently implemented in the pr) as a temporary solution and guiding users some strict guide to follow on our documentation until we get up with the perfect solution. Since there are many of them waiting for this

[pierDipi]

Closing this PR for now as it is only meant to prove the ideas, we can keep referring to it as we continue discussing options on the original issue