argoproj / argo-cd

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

Blank space in Argo RBAC line causes permission issues #20825

Open elmazzun opened 4 days ago

elmazzun commented 4 days ago

Hi!

I found out that, in Argo RBAC permissions, line g, admins, role: admin is not valid and will cause permission issues; to fix this, I had to remove the blank space between : and admin in order to have my permissions back (ie: g, admins, role:admin).

Checklist:

Describe the bug

As said above, a blank space between role keyword and ArgoCD role admin will result in permission denied, even if I was cluster-admin.

To Reproduce

  1. create admins Group and put your user inside
  2. create a ClusterRoleBinding and bind admins Group to cluster-admin ClusterRole
  3. install ArgoCD and login to its dashboard
  4. add the following RBAC permission: g, admins, role: admin
  5. once logged in, go to User Info from left sidebar and check that your logged User is associated with currect entities: in my case it was:
    Groups:
    admins
    system:authenticated
    system:authenticated:oauth
  6. try to perform any operation that you should be able to perform, like adding a Repository: you will get permission denied.
  7. remove extra blank space from g, admins, role: admin so that it will be g, admins, role:admin .
  8. you are now able to perform any operation.

Expected behavior

g, admins, role: admin is a valid CSV row so there should be no problem, yet it is required (apparently) that between role: and admin should be no blank space.

Screenshots

None

Version

Kubernetes cluster: v1.29.8

$ argocd version                                                                     
argocd: v2.10.17+6e33cba
  BuildDate: 2024-09-26T07:23:50Z
  GitCommit: 6e33cba80e65f7f60f1a1e8f677a6e2dce315368
  GitTreeState: clean
  GoVersion: go1.21.13
  Compiler: gc
  Platform: linux/amd64
argocd-server: v2.12.6+4dab5bd
  BuildDate: 2024-10-22T15:24:17Z
  GitCommit: 4dab5bd6a60adea12e084ad23519e35b710060a2
  GitTreeState: clean
  GoVersion: go1.22.7 (Red Hat 1.22.7-1.module+el8.10.0+22325+dc584f75)
  Compiler: gc
  Platform: linux/amd64
  ExtraBuildInfo: {Vendor Information: Red Hat OpenShift GitOps version: v1.14.1}
  Kustomize Version: v5.4.2 2024-10-22T15:25:33Z
  Helm Version: v3.15.2+g1a500d5
  Kubectl Version: v0.29.6
  Jsonnet Version: v0.20.0

Logs

None

andrii-korotkov-verkada commented 4 days ago

What do you think should be a proper fix?

anandf commented 3 days ago

Would trimming the role string help in this case ?

elmazzun commented 3 days ago

What do you think should be a proper fix?

I think that RBAC Argo should reflect the proper CSV standard, which means that the following lines should be both valid:

g, admins, role:admin  # The "right" line
g, admins, role: admin # The line I typed

Would trimming the role string help in this case ?

I guess so but how far do we want to validate these strings? The following lines are valid CSV but would they be still valid to Argo?

g,admins, role:admin
g, admins,role:admin
g,admins,role:admin

Is there any Argo log where it is reported a failed parsing in RBAC configurations?

(thanks for your work!)

andrii-korotkov-verkada commented 3 days ago

We use casbin for handling the permissions check, tho we can post-process the policy file. But yeah, it's unclear what's the exact scope of this should be. Maybe we can log a warning if there are more or less than two spaces in a line?

elmazzun commented 3 days ago

Didn't know Argo is relying on Casbin: Argo may validate RBAC lines by enforcing the format Casbin is expecting. Yet, if this is the first time someone reports an issue like mine, @andrii-korotkov-verkada maybe it is better as you suggested, logging a warning about this would be enough.

devopsjedi commented 1 day ago

I would like to work on this issue.

devopsjedi commented 22 hours ago

It seems like casbin does not have any issues with the format or parsing of role: admin. Access is not granted during enforcement since the role named role: admin does not exist.

Should we be adding logs for references to roles that don't exist?

andrii-korotkov-verkada commented 22 hours ago

Yeah, let's do this. Maybe casbin has some functions we can use for that already. But if not, let's add them in argo.

andrii-korotkov-verkada commented 4 hours ago

Hm, although the author mentions that removing a space fixes the issue.

elmazzun commented 4 hours ago

I tried at least five CSV online validators: according to all of them, my Argo policy CSV was valid even with that blank space, yet Argo (Casbin?) did not tolerate g, admins, role: admin.

We may further simplify this by just logging somewhere that no spaces are tolerated in Argo RBAC rules composition and (following my proposal) add in https://argo-cd.readthedocs.io/en/stable/operator-manual/rbac/#rbac-model-structure that Argo RBAC model structure use a subset of the CSV where the following lines are _not_ valid, etc. etc..

Again, if I am the only one reporting such issue I don't want you guys to work too much on this, I guess there are more important tasks to be dealt.