argoproj / argo-cd

Declarative Continuous Deployment for Kubernetes
https://argo-cd.readthedocs.io
Apache License 2.0
17.65k stars 5.38k forks source link

`argo-manager` serviceaccount is bypassed if kubeconfig is using certificate using argocd-cli #17150

Open JorTurFer opened 8 months ago

JorTurFer commented 8 months ago

Checking the code, I've found that the argo-manager service account is only used if the using kubeconfig doesn't have cert/key, and if it has, ArgoCD uses cert/key. This overrides the RBAC we could build around ArgoCD's service account as the cert/key is admin of the cluster and the code is confusing to me:

// Bearer token will preferentially be used for auth if present,
// Even in presence of key/cert credentials
// So set bearer token only if the key/cert data is absent
if len(tlsClientConfig.CertData) == 0 || len(tlsClientConfig.KeyData) == 0 {
    clst.Config.BearerToken = managerBearerToken
}

If bearer is preferentially used even in presence of key/cert, we should use it always and not if key/cert is absent.

We use argocd-cli to manage our clusters (adding or removing them, and also updating the labels and annotations). The current situation that we faced with is that our provider has changed how the kubeconfig is generated in their backend, and now instead of using token, it uses short term certificates (from 1 hour to 6 months). After this change on their side, ArgoCD has started using the certificate instead of the bearer token from argo-manager serviceAccount, bypassing the RBAC that we could have for ArgoCD, because now it's using an admin certificate.

I guess that I could mitigate this just generating my own serviceAccount and using its token for argocd-cli operations, and in that case argo-manager token will be used, but this is a bit hacky.

As I have no idea about why the code above does that although the comment suggests the opposite I suggest adding a new parameter to argocd cluster add to enforce the usage of the bearer token (removing the cert information and just keeping the bearer token). To not break anything, we can disable this as default. users that want to enforce the usage of argo-manager service account (because they have build the RBAC based on it) can enable by adding the new parameter and that's it

JorTurFer commented 7 months ago

Is there any update here? @crenshaw-dev @blakepettersson ? Can I do something to push this discussion?

ChristianCiach commented 5 months ago

I just encountered this and we are seeing this as a major security issue. This leaks the kubernetes admin mTLS credentials into the cluster. Even worse, these credentials are usually not easily revocable. This means that the documented revocation process doesn't apply here.

Why is the ServiceAccount even created inside the target cluster if ArgoCD doesn't use it?

To make matters even worse: Administrators will probably not even notice that ArgoCD uses the wrong credentials until they try to revoke access or some other bad thing happened.

@crenshaw-dev Just pinging random people. Maybe this issue should even be converted into a security advisory.

JorTurFer commented 1 month ago

Hello ✋ Is there any update here?

VannTen commented 3 weeks ago

The present process is very surprising. Since argocd cluster add create a service account/role/rolebinding on the managed cluster, I would expect it uses it to access the cluster, not grab my certs.

VannTen commented 3 weeks ago

This appear to comes from #3655 and be an intentional change.

ChristianCiach commented 3 weeks ago

I still think this is a major security issue, because this is not documented and silently leaks the (usually kubernetes admin) credentials into the argocd cluster. I believe this issue needs a CVE entry.

VannTen commented 3 weeks ago

Agreed, I was merely digging why it was made. Reading the linked issue, it does not appear that this particular problem was considered.