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

fix(argo-cd): make automountServiceAccountToken configurable #2625

Conversation

stefan-caraiman
Copy link
Contributor

@stefan-caraiman stefan-caraiman commented Apr 4, 2024

Keeps the default behaviour of having automountServiceAccountToken set to true for deployments/statefulsets.

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

@stefan-caraiman stefan-caraiman force-pushed the fix/configurable-automountServiceAccountToken branch from 31e8f70 to 5968839 Compare April 4, 2024 13:10
@stefan-caraiman stefan-caraiman changed the title fix: make automountServiceAccountToken configurable fix(argo-cd): make automountServiceAccountToken configurable Apr 4, 2024
@stefan-caraiman stefan-caraiman force-pushed the fix/configurable-automountServiceAccountToken branch from 5968839 to 5c27b10 Compare April 4, 2024 13:12
@mkilchhofer
Copy link
Member

Just to probably learn something new: is there a difference setting it on the deployment/statefulset or ServiceAccount?
Because this toggle is already supported on all ServiceAccounts:

$ grep automount README.md
| controller.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |
| repoServer.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |
| server.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |
| dex.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |
| redis.serviceAccount.automountServiceAccountToken | bool | `false` | Automount API credentials for the Service Account |
| applicationSet.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |
| notifications.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |

@stefan-caraiman
Copy link
Contributor Author

Just to probably learn something new: is there a difference setting it on the deployment/statefulset or ServiceAccount? Because this toggle is already supported on all ServiceAccounts:

$ grep automount README.md
| controller.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |
| repoServer.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |
| server.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |
| dex.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |
| redis.serviceAccount.automountServiceAccountToken | bool | `false` | Automount API credentials for the Service Account |
| applicationSet.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |
| notifications.serviceAccount.automountServiceAccountToken | bool | `true` | Automount API credentials for the Service Account |

Yep 😄 as per the K8s docs:

The pod spec takes precedence over the service account if both specify a automountServiceAccountToken value.

Setting automountServiceAccountToken on a ServiceAccount applies the setting to all pods using that account, defaulting them to either mount or not mount the service account token. Setting it on a Deployment or StatefulSet allows for specific control over that particular workload, overriding the ServiceAccount's default.

@mkilchhofer
Copy link
Member

Setting automountServiceAccountToken on a ServiceAccount applies the setting to all pods using that account, defaulting them to either mount or not mount the service account token. Setting it on a Deployment or StatefulSet allows for specific control over that particular workload, overriding the ServiceAccount's default.

Ok but do you create your own SA's or why do you like this change? By default every deployment/component of Argo CD has its own SA :)

@stefan-caraiman
Copy link
Contributor Author

stefan-caraiman commented Apr 4, 2024

Not really no, we do not create our own SA, we rely on the defaults, but due to the following Azure AKS Security policy:

"Kubernetes clusters should disable automounting API credentials" - Disable automounting API credentials to prevent a potentially compromised Pod resource to run API commands against Kubernetes clusters.

TLDR: We need to basically set automountServiceAccountToken: false to both the SA and Deployments/StatefulSets for ArgoCD and then mount the serviceaccounttoken through extra volumes/volumeMounts for resources that do need access to the API. (similar to what K8s Admission controller already does for automountServiceAccountToken behind the curtains )

Example values & issue from cert-manager on this topic.

In the end the result is the same as with automountServiceAccountToken: true, but it is a "hardening" requirement to be compliant with the Azure Security Score 😅

@mkilchhofer
Copy link
Member

In the end the result is the same as with automountServiceAccountToken: true, but it is a "hardening" requirement to be compliant with the Azure Security Score 😅

Oh boy 🙈 . I now definitively learned something new. But IMHO this is only to satisfy the security scanner, it doesn't really improve security.
There was a good talk at KubeCon: Malicious compliance ;)
IMAGE ALT TEXT HERE

Lets wait on some other opinions of my colleagues.

@stefan-caraiman
Copy link
Contributor Author

Yep totally agree. No harm though in adding in the option to customize it since it defaults to true just as before. 😄

@mbevc1
Copy link
Collaborator

mbevc1 commented Apr 4, 2024

Hah, nice work Microsoft :)

Good reference @mkilchhofer 👍

Happy we add it in for people on Azure as long as the defaults are not changing.

@stefan-caraiman
Copy link
Contributor Author

@mbevc1 indeed, no defaults are changing so the change is harmless 👍

@stefan-caraiman stefan-caraiman force-pushed the fix/configurable-automountServiceAccountToken branch from 5c27b10 to bfb6745 Compare April 5, 2024 04:42
@github-actions github-actions bot added size/M and removed size/L labels Apr 5, 2024
Keeps the default behaviour of having automountServiceAccountToken
set to true for deployments/statefulsets.

Signed-off-by: Stefan Caraiman <stefanc.caraiman@gmail.com>
@stefan-caraiman stefan-caraiman force-pushed the fix/configurable-automountServiceAccountToken branch from bfb6745 to 979bf1c Compare April 5, 2024 05:02
@stefan-caraiman
Copy link
Contributor Author

@mbevc1 @mkilchhofer just to exemplify further, the following 2 example values achieve the same injection of the Service account token into the pod in order to make K8s api calls.

# Example 1 ( the default now and with this MR too)
# Automatically injection done as explained here
# https://kubernetes.io/docs/reference/access-authn-authz/service-accounts-admin/#serviceaccount-admission-controller
controller:
  automountServiceAccountToken: true
  serviceAccount:
    automountServiceAccountToken: true

same as with

# Example 2 values for disabling auto-mounting and opting to inject it via volumes instead
# for "hardened" AKS setups
controller:
  automountServiceAccountToken: false
  serviceAccount:
    automountServiceAccountToken: false
  volumeMounts:
    - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
      name: serviceaccount-token
      readOnly: true
  volumes:
    - name: serviceaccount-token
      projected:
        defaultMode: 0444
        sources:
        - serviceAccountToken:
            expirationSeconds: 3607
            path: token
        - configMap:
            name: kube-root-ca.crt
            items:
            - key: ca.crt
              path: ca.crt
        - downwardAPI:
            items:
            - path: namespace
              fieldRef:
                apiVersion: v1
                fieldPath: metadata.namespace

Copy link
Collaborator

@yu-croco yu-croco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. LGTM.

@mbevc1
Copy link
Collaborator

mbevc1 commented Apr 10, 2024

Thanks @stefan-caraiman

@mkilchhofer mkilchhofer merged commit f42e0e1 into argoproj:main Apr 10, 2024
6 checks passed
@stefan-caraiman stefan-caraiman deleted the fix/configurable-automountServiceAccountToken branch April 11, 2024 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants