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

TaskRef: refer to a Task from another namespace #1409

Closed
vdemeester opened this issue Oct 10, 2019 · 24 comments
Closed

TaskRef: refer to a Task from another namespace #1409

vdemeester opened this issue Oct 10, 2019 · 24 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done.

Comments

@vdemeester
Copy link
Member

Expected Behavior

It would be nice to be able to refer a Task from another namespace. Something like the following.

apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: standard-go-pipeline
spec:
  params:
  - name: foo
    description: repository to clone
  tasks:
  - name: task-a
    taskRef: 
      name: foo
      from:
        namespace: another-namespace

or

apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: standard-go-pipeline
spec:
  params:
  - name: foo
    description: repository to clone
  tasks:
  - name: task-a
    taskRef: 
      name: foo
      namespace: another-namespace

The security aspect of this should be simple, we let kubernetes RBAC handle this (aka if the user can't access Task from the other namespace, then we fail and report an error 👼

Actual Behavior

To refer a Task in taskRef, today you need to have this Task in the same namespace.

Additional Info

Related to #964 (and https://docs.google.com/document/d/1O8VHZ-7tNuuRjPNjPfdo8bD--WDrkcz-lbtJ3P8Wugs/edit#)

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 10, 2019
@hrishin
Copy link
Member

hrishin commented Oct 14, 2019

/assign

hrishin added a commit to hrishin/tekton-pipeline that referenced this issue Oct 14, 2019
…skRun spec

Sometimes users don't want to copy-paste Task from one namespace to other.
One such use case could be enabling Task from Task catalog. Every time
duplicating the same Task in more than one namespace could become
maintenance nightmare.

This patch allows referencing a Task from a different namespace in TaskRun's
and PipelineRun's spec.TaskRef`. Hence this would allow the user to install the
Task once in the namespace. (Now not sure about its overlap with ClusterTask)

Fixes tektoncd#1409
@dibyom
Copy link
Member

dibyom commented Oct 15, 2019

Should we allow this for Pipelines/Conditions as well? Related: we have a feature request for Cluster scoped Conditions and Pipelines: #1266

@vdemeester
Copy link
Member Author

@dibyom right, this is something we need to discuss then. My gut feeling is yes, but maybe this means we don't need ClusterTask and other cluster scoped objects then 👼

@skaegi
Copy link
Contributor

skaegi commented Oct 16, 2019

One concern I have with this is that at least currently the Tekton Controller is doing the taskref lookup with elevated privilege. This might be a real problem in a multi-tenant environment where we want to prevent lookup of private Tasks that might inadvertently contain secret-ish materials.

@josephlewis42
Copy link
Contributor

I'm concerned about the statement in the proposal:

The security aspect of this should be simple, we let kubernetes RBAC handle this (aka if the user can't access Task from the other namespace, then we fail and report an error 👼

I don't think it's clear what would happen later on if the permissions changed. If any garbage collection scenarios would be changed with this (I don't believe owner references work across namespaces). Or what it would mean to have an instance of this TaskRun by someone who didn't have permission to see the other namespace (should it fail? should it succeed?); if it succeeds is there possibility of privilege escalation? Tasks can also reference things like volumes, service accounts, secrets and config maps. How do you reference those across namespaces ensuring permissions are maintained?

Also, I think there are side-channel attacks that open up if you start referencing things across namespaces. Unless you're very careful about error messages, you could probe for Tasks in other spaces by submitting pipelines and looking at the results.

@hrishin
Copy link
Member

hrishin commented Oct 17, 2019

We need to consider the overlap with ClusterTask and we are approaching the beta release soon. If we support both it and ClusterTask if goes into beta then do we need to support both in the long term?

Yes as @skaegi mentioned on other hand we need to handle the permission or authorization issue.

@vdemeester
Copy link
Member Author

One concern I have with this is that at least currently the Tekton Controller is doing the taskref lookup with elevated privilege. This might be a real problem in a multi-tenant environment where we want to prevent lookup of private Tasks that might inadvertently contain secret-ish materials.

That's a very good point that I didn't take into account initially 😅

@hrishin
Copy link
Member

hrishin commented Oct 23, 2019

One concern I have with this is that at least currently the Tekton Controller is doing the taskref lookup with elevated privilege. This might be a real problem in a multi-tenant environment where we want to prevent lookup of private Tasks that might inadvertently contain secret-ish materials.

One way to address the permission issue by either placing the authorization checks in the tekton pipeline webhook based on the user's token/ServiceAccount provided PipelineRun/TaskRun or impersonate the request to resolve the TaskRef with ServiceAccount mentioned in PipelineRun(Pipeline)/TaskRun definition.

@ahpook
Copy link
Contributor

ahpook commented Oct 23, 2019

FWIW I'm neutral to -1 on this, but our use case (this was echoed by @rakhbari on the WG call) is that we use namespaces as a mechanism for isolation and some security guarantees, so if it were implemented we couldn't use it.

@bobcatfish
Copy link
Collaborator

Slight preference for design 2:

  - name: task-a
    taskRef: 
      name: foo
      namespace: another-namespace

Re. service account issues - could we initially assume the controller service account has the permissions it needs, and ask users to add more permissions to that account if needed?

@vdemeester
Copy link
Member Author

Re. service account issues - could we initially assume the controller service account has the permissions it needs, and ask users to add more permissions to that account if needed?

The problem is opposite, if the user has less permission than the controller (which might easily be the case), then we are in a situation of privileged escalation (kinda).

@poy
Copy link
Contributor

poy commented Oct 23, 2019

I'm curious, can you not get the same/similar result by using a ClusterPipeline instead?

The nice thing about using a ClusterXXX is, if you create it, you go in knowing that your pipeline is not scoped to a namespace. This should resolve the many security concerns.

@bobcatfish
Copy link
Collaborator

The problem is opposite, if the user has less permission than the controller (which might easily be the case), then we are in a situation of privileged escalation (kinda).

Ah interesting!! Is this already the case then, even with Tasks in the same namespace? Like a user only needs to be able to create a TaskRun or a PipelineRun, and the controller will fetch any Tasks etc. that are needed, and even create pods, even if the user doesn't have permission to?

(In some ways this reminds me of tektoncd/triggers#77)

@vdemeester
Copy link
Member Author

@bobcatfish yeah that's the case already 👼 You can refer a Task in the same namespace even if you don't have the right to get/list/update/… Tasks 👼

@sthaha
Copy link
Member

sthaha commented Oct 31, 2019

Continuing discussion on #1420 (comment)

Another worry was on updates on the Task definition — if the refered task gets updated, it affect all the TaskRun/Pipeline/… that depends on it, and might break something, where a copy approach does not.

But a namespaced task or a cluster-task does not solve this issue that the task definition can change and the next pipeline-run can fail.

So what is the recommended approach if a task definition need to be shared amongst only few namespaces/projects but not all?

@rakhbari
Copy link

@sthaha IMHO we're getting into the area of artifact versioning and that's already covered by your SCM tool, in most cases github. If you organize your shared tasks properly in a github repo, that repo can be attached to other repos as a git submodule. That submodule is defined at a particular SHA of the shared tasks repo giving you automatic version control. You then apply those shared tasks to your cluster in your own namespace and if/when the shared tasks are updated, your repo's submodule still remains at the original version. You then have the choice to examine the shared tasks updates (at the HEAD of that repo), even set up a pipeline to test out the HEAD of that repo and if all is fine, update your submodule SHA to the HEAD and re-apply into your cluster again.

Lots of common script repos are shared this way here inhouse. The owners of the repos that have a dependency on the shared script repo decide when they're ready to update their repo to the HEAD of the shared repo.

@sthaha
Copy link
Member

sthaha commented Nov 1, 2019

@rakhbari thanks, that is a good enough approach albeit it does feel like working around the limitation that a task can't be shared outside a namespace.

@vdemeester vdemeester added the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label Nov 5, 2019
@csantanapr
Copy link

Trying to understand how is this different from using a ClusterTask?

For example in a pipeline using:

taskRef: 
      name: foo
      namespace: another-namespace

vs

taskRef: 
      name: foo
      kind: ClusterTask

@sthaha
Copy link
Member

sthaha commented Nov 8, 2019

ClusterTask available / viewable by everyone in the cluster vs if namespaced, the access to the task can be controlled by RBAC if we choose to implement this feature request.

@rakhbari
Copy link

rakhbari commented Nov 8, 2019

@sthaha I actually don't see it as a "limitation" at all. I see it as proper use of existing tools features (rather combining features of different tools) to achieve an end. Use the right tool for the job. TektonCD shouldn't include any features that creep into version control. We already have a tool for that (Github). Hopefully that makes more sense. 😃

@hrishin
Copy link
Member

hrishin commented Dec 11, 2019

What should be our final take on this issue?

@vdemeester @bobcatfish @dibyom @sthaha @rakhbari @poy @ahpook @skaegi
(sorry for wide ping 🙏)

@sthaha
Copy link
Member

sthaha commented Dec 11, 2019

My take on this is that time will tell. Right now the overwhelming consensus seems to be that we do not need this feature (non-issue) and if we can survive ( no demand for this feature from community) without the need for a SAR in tekton-controller/webhook since namespaces becomes the natural boundary for isolation then it does keep the codebase simpler.

I am happy for us to close this issue and revisit when there is a demand.

@dibyom
Copy link
Member

dibyom commented Mar 18, 2020

Given @sthaha 's comment I'm going to close this issue for now. We can revisit it again in the future
/close

@tekton-robot
Copy link
Collaborator

@dibyom: Closing this issue.

In response to this:

Given @sthaha 's comment I'm going to close this issue for now. We can revisit it again in the future
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done.
Projects
None yet