search

argoproj / argo-workflows

Workflow Engine for Kubernetes
https://argo-workflows.readthedocs.io/
Apache License 2.0
15.11k stars 3.21k forks source link

fix: don't mount SA token when `automountServiceAccountToken: false`. Fixes #12848 #13820

Closed MinyiZ closed 3 weeks ago

MinyiZ commented 3 weeks ago

Fixes #12848

Motivation

https://github.com/argoproj/argo-workflows/pull/10945 introduced a regression as it is mounting a serviceaccount token on the main container when automountServiceAccountToken: false.

The security docs say

The main container will have the service account token mounted, allowing the main container to patch pods (among other permissions). Set automountServiceAccountToken to false to prevent this.

https://github.com/argoproj/argo-workflows/issues/10937 was not a bug. It is possible to explicitly mount a token to container and script template workflows even if automountServiceAccountToken: false so the code change was not needed.

Modifications

Verification

Tests passed

MinyiZ commented 3 weeks ago

Regarding the tests here and from #10945, a resource template should correctly fail if automountServiceAccountToken: false since, well, there is no SA token for it to use

I've fixed the tests from https://github.com/argoproj/argo-workflows/pull/10945 to demonstrate that it is possible to explicitly mount a token to container and script template workflows even if automountServiceAccountToken: false so the code change was not needed.

I believe resource templates shouldn't fail if automountServiceAccountToken: false because the exec token is mounted at https://github.com/argoproj/argo-workflows/blob/25bbb71cced32b671f9ad35f0ffd1f0ddb8226ee/workflow/controller/operator.go#L3453 and I've added an e2e test to demonstrate that.

agilgur5 commented 3 weeks ago

I believe resource templates shouldn't fail if automountServiceAccountToken: false because the exec token is mounted at

Ah nice, I wasn't sure if resource templates handled executor.serviceAccountName since they use a main container.

I've fixed the tests from #10945 to demonstrate that it is possible to explicitly mount a token to container and script template workflows even if automountServiceAccountToken: false so the code change was not needed.

I saw that, but I don't think that test is very functional / necessary. Basically you're manually mounting the token instead of automounting it. A user shouldn't really ever do that manually though, they should just automount. That is also testing k8s behavior rather than Argo behavior at that point. I think both those tests should just be removed.

agilgur5 changed the title ~fix: resource template etting duplicate exec-sa-token volume mounts with automountServiceAccountToken: false. Fixes #12848~ fix: don't mount SA token when automountServiceAccountToken: false. Fixes #12848 12 hours ago

MinyiZ changed the title ~fix: don't mount SA token when automountServiceAccountToken: false. Fixes #12848~ fix: resource template getting duplicate exec-sa-token volume mounts with automountServiceAccountToken: false. Fixes #12848 5 hours ago

Regarding the PR title, my change was intentional. The PR title should summarize what the PR does, not what the issue is. The PR title and issue title should not be equivalent, as they are now. The actual source code change is not specific to resource templates either, as the code path does not check for a resource template, which is why I did not label it as such

MinyiZ commented 3 weeks ago

I think both those tests should just be removed.

Yep agreed. Done.

Regarding the PR title, my change was intentional.

Ooops sorry about that. I unintentionally reverted your change when I was fixing up a typo. I've changed it back now.