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

Kubernetes 1.24: Token-handling code assumes auto-created service account token secrets #8320

Closed
liggitt opened this issue Apr 5, 2022 · 38 comments · Fixed by #9620
Closed
Labels
area/api Argo Server API area/controller Controller issues, panics area/server area/sso-rbac solution/workaround There's a workaround, might not be great, but exists type/feature Feature request type/security Security related

Comments

@liggitt
Copy link

liggitt commented Apr 5, 2022

Sweeping token-scraping of auto-generated Kubernetes token secrets in preparation for Kubernetes 1.24 showed the following code locations assume auto-generated tokens will exist:

return account.Secrets[0].Name, nil

tokenSecret, err := secretsInterface.Get(ctx, serviceAccount.Secrets[0].Name, metav1.GetOptions{})

secret, err := s.cache.SecretLister.Secrets(serviceAccount.GetNamespace()).Get(serviceAccount.Secrets[0].Name)

That assumption is not universally correct.

In 1.21+, secret-based tokens are no longer used for mounting into pods (ephemeral time-limited tokens are), and the token controller can be turned off.

In 1.24+, secret-based tokens are no longer auto-created by default for new service accounts.

Using ephemeral time-bound tokens is preferred in 1.21+ (see the TokenRequest API) if possible.

If a secret-based token is still desired, one can be created manually, but will not be referenced from the service account's .secrets list.


Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.

@liggitt liggitt changed the title Token-handling code assumes auto-created service account token secrets Kubernetes 1.24: Token-handling code assumes auto-created service account token secrets Apr 5, 2022
@alexec
Copy link
Contributor

alexec commented Apr 5, 2022

@liggitt do you have some thoughts on how to adapt the code to address this?

@alexec alexec added type/security Security related area/api Argo Server API labels Apr 5, 2022
@liggitt
Copy link
Author

liggitt commented Apr 5, 2022

if you need tokens to use outside the context of a pod (where they continue to be mounted in automatically for use), either request one using the TokenRequest API, or create a secret to hold one.

@alexec
Copy link
Contributor

alexec commented Apr 5, 2022

None of these SAs are useful if they do not have a token. The user could presumably create a token?

@liggitt
Copy link
Author

liggitt commented Apr 5, 2022

Service account tokens are not automatically ambient in secrets in 1.24+ (and possibly in 1.21+ if the cluster owner turned off the token controller since it is no longer required for getting tokens into pods).

Tokens are created automatically for mounting into pods (but are not persisted in Secret API objects).

If a secret-based token is desired/needed, it can be created manually, but would not be referenced from the ServiceAccount's secrets list field.

@liggitt
Copy link
Author

liggitt commented Apr 5, 2022

it looks like at least some of the uses of getServiceAccountTokenName are to get the name of a Secret object in order to create a volume mount mounting in the token?

If you can't resolve a secret name, I'd recommend using projected token volumes instead, which let you request a token be mounted into a pod without needing to look up a Secret name.

See https://kubernetes.io/docs/reference/access-authn-authz/service-accounts-admin/#bound-service-account-token-volume for details on how to replicate the content of an old secret-based token volume with the new projected token volumes

@alexec
Copy link
Contributor

alexec commented Apr 5, 2022

Do project volumes work on GKE auto-pilot?

@liggitt
Copy link
Author

liggitt commented Apr 5, 2022

yes

@alexec
Copy link
Contributor

alexec commented Apr 5, 2022

@jessesuen @jannfis FYI

@alexec
Copy link
Contributor

alexec commented Apr 5, 2022

Project volumes will not work for our case. We don't know the name of the service account token ahead of time. A single pod read many different tokens.

I think users will have to create the tokens themselve. We should update the code to returrn an error (rather than panic) so that users know what to do.

@liggitt thoughts?

@liggitt
Copy link
Author

liggitt commented Apr 5, 2022

We don't know the name of the service account token ahead of time.

The name of the token, or the name of the service account?

A single pod read many different tokens.

Projected token volumes can be used multiple times, but only for a single service account (the one referenced by the pod spec)

I think users will have to create the tokens themselves

If so, they'll either need to give you the names of the secrets they created (since they won't be referenced from the serviceaccount's .secrets field), or you'll have to query for the secrets (filtering by type, then matching up the kubernetes.io/service-account.name annotation to the name of the service account you're looking for).

Use of secret-based tokens (even manually created ones) is discouraged, since it exposes token content via the API.

@alexec
Copy link
Contributor

alexec commented Apr 5, 2022

We don't know the name of the service account token ahead of time.

The name of the token, or the name of the service account?

Sorry. We don't know the service account name ahead of time (i.e. when pod starts up).

If so, they'll either need to give you the names of the secrets they created (since they won't be referenced from the serviceaccount's .secrets field), or you'll have to query for the secrets (filtering by type, then matching up the kubernetes.io/service-account.name annotation to the name of the service account you're looking for).

Use of secret-based tokens (even manually created ones) is discouraged, since it exposes token content via the API.

I understand.

@alexec alexec added type/feature Feature request and removed type/bug triage labels Apr 5, 2022
@lknite
Copy link

lknite commented May 7, 2022

This looks related "argocd cluster add ...", timing out waiting for a secret.

@jrhoward
Copy link

jrhoward commented Sep 9, 2022

looking at the code you could change it so that it retrieves the secret by annotation
kubernetes.io/service-account.name: "service account name " in the method getServiceAccountTokenName using the operator.go as an example but that requires users to explicitly create the secret. This means you would be missing out on the security enhancements provided in 1.24.

It seems to me that the long term solution is to create a token at run time from the argoexe container using something like

...CoreV1().ServiceAccounts(Namespace).CreateToken(ctx, name, &tokenRequest, metav1.CreateOptions{})

based on the name of the service account and inject it into the the container as a tmpfs volume as there will be no way to mount a pre-existing service account secret

@urbantz-technical-support

I found another place where we assume tokens to be auto generated:

if len(serviceAccount.Secrets) == 0 {

This essentially means that you can't run argo-workflows on k8s 1.24 (like we do) with sso enabled.

Maybe until it gets addressed we should add something to the documentation, or maybe pinned this issue explaining it is not compatible with k8s 1.24

@alexec
Copy link
Contributor

alexec commented Sep 14, 2022

@juliev0 @sarabala1979 this issue needs to scheduled and fixed.

@sarabala1979
Copy link
Member

@alexec I will add it in RTB we will look at this after 3.4.0. we can target and include it in 3.4.1

@alexec
Copy link
Contributor

alexec commented Sep 14, 2022

v3.5 would be fine I think.

@jrhoward
Copy link

Don't think I am qualified to take over the PR but I'll help out where I can as I have the branch running locally with 1.24.

I have installed the sample executor plugin and successfully run a sample workflow based on the revised instructions with a service account token suffixed with .service-account-token.

@jrhoward
Copy link

interceptor.go is missing updates. I modified and built locally.

if len(serviceAccount.Secrets) == 0 {
return fmt.Errorf("failed to get secret for service account \"%s\": no secrets", serviceAccountName)
}
tokenSecret, err := secretsInterface.Get(ctx, serviceAccount.Secrets[0].Name, metav1.GetOptions{})

Remove lines 87 to 89 and change line 90 from serviceAccount.Secrets[0].Name to leverage the new util:

tokenSecret, err := secretsInterface.Get(ctx, secrets.SecretName(serviceAccount), metav1.GetOptions{})

terrytangyuan pushed a commit to terrytangyuan/argo-workflows that referenced this issue Sep 25, 2022
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@gcsfred2
Copy link

@terrytangyuan
Copy link
Member

@gcsfred2 That is unrelated. Try creating a new local cluster and start again.

@gcsfred2
Copy link

Retrying https://github.com/terrytangyuan/argo-workflows/tree/support-k8s-124 and running locally. My recent error: https://pastebin.com/hBySXzWV . There was no error running "make start UI=true" and the Hello World example.

@jrhoward
Copy link

jrhoward commented Oct 7, 2022

ok I just discovered that there is a work around which does not require a code change on argoworkflow.

Not sure if it is documented so I can't vouch for how long it will last but you can manually insert the secret name into the service account manifest which means you can deploy workflow without a fix into 1.24, as an example this works on server version:
{Major:"1", Minor:"24", GitVersion:"v1.24.4+k3s1}, yes tested with k3d > k3s

note the trick is to define the secret name in the service account manifest

I applied the following manifests:

apiVersion: v1
kind: Secret
type: kubernetes.io/service-account-token
metadata:
  name: uber-argo
  annotations:
    kubernetes.io/service-account.name: "uber-argo"
---

apiVersion: v1
kind: ServiceAccount
metadata:
  name: uber-argo
secrets:
- name: uber-argo

functionality has been removed but the spec is still valid

@terrytangyuan terrytangyuan removed their assignment Oct 7, 2022
@terrytangyuan
Copy link
Member

I won't have time to finish #9680 given other priorities, so I am closing that PR and freeing up this issue if anyone else wants to continue the work. The TokenRequest API suggested by @jessesuen seems like a better approach. ArgoCD has done this so feel free to use the following resources as reference:

If you are blocked by the error failed to get token volumes: service account argo/default does not have any secrets, please use the above workaround for now.

@alexec
Copy link
Contributor

alexec commented Oct 7, 2022

@jrhoward awesome work finding the work-around. It does appear to be documented:

https://kubernetes.io/docs/reference/access-authn-authz/service-accounts-admin/

The short term fix myself and @sarabala1979 were considering was exactly the same as this.

@liggitt
Copy link
Author

liggitt commented Oct 7, 2022

Manually adding token secrets to the list in the service account object is not expected. That list is specifically for secrets which are mounted into pods, and secret based tokens no longer are in 1.21+

@sarabala1979
Copy link
Member

@terrytangyuan can you update the PR with @jrhoward examples for Short term solutions? ServiceAccount will have an annotation with a secret name. So the controller will look at the annotation if there is no Secret on ServiceAccount.

terrytangyuan pushed a commit that referenced this issue Oct 7, 2022
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@terrytangyuan
Copy link
Member

@terrytangyuan can you update the PR with @jrhoward examples for Short term solutions? ServiceAccount will have an annotation with a secret name. So the controller will look at the annotation if there is no Secret on ServiceAccount.

I couldn't get some tests passing and didn't have much time to investigate further. The remaining issues are documented in #9680 (comment) in case anyone else wants to pick up.

@alexec
Copy link
Contributor

alexec commented Oct 11, 2022

Hmmm.kubernetes.io/service-account.name is an annotation on the secret, not the service account. We actually need the service account to point to the secret.

alexec added a commit that referenced this issue Oct 15, 2022
Signed-off-by: Alex Collins <alex_collins@intuit.com>
alexec added a commit that referenced this issue Oct 19, 2022
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alexec@users.noreply.github.com>
juchaosong pushed a commit to juchaosong/argo-workflows that referenced this issue Nov 3, 2022
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alexec@users.noreply.github.com>
Signed-off-by: juchao <juchao@coscene.io>
@agilgur5 agilgur5 added the solution/workaround There's a workaround, might not be great, but exists label Sep 12, 2023
@rubenpetrosyan1
Copy link

Hi team,
Is the issue assumed as fixed?
I'm asking because this comes up pretty frequently on Slack.

@zhaorizhao
Copy link

zhaorizhao commented Oct 9, 2023

set secrets to make this field accessible:

---
apiVersion: v1
kind: ServiceAccount
metadata:
  name: read-only
  namespace: argo
  annotations:
    workflows.argoproj.io/rbac-rule: "true"
    workflows.argoproj.io/rbac-rule-precedence: "0"
secrets: 
- name: read-only-token-manual
---
apiVersion: v1
kind: Secret
metadata:
  name: read-only-token-manual
  namespace: argo
  annotations:
    kubernetes.io/service-account.name: "read-only"
type: kubernetes.io/service-account-token

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Argo Server API area/controller Controller issues, panics area/server area/sso-rbac solution/workaround There's a workaround, might not be great, but exists type/feature Feature request type/security Security related
Projects
None yet