Broken policy.csv crashes ArgoCD-Server

[maxtacu]

Checklist:

• [x] I've searched in the docs and FAQ for my answer: http://bit.ly/argocd-faq.
• [x] I've included steps to reproduce the bug.
• [x] I've pasted the output of argocd version.

Describe the bug

ArgoCD is crashing when a broken csv policy is applied. If ArgoCD is configured to track itself from the git, then the fix of the policy will not be applied because argocd-server crashed and not receiving any updates. Only manual kubectl apply will help to recover argocd-server.

To Reproduce apply the argocd-rbac-cm ConfigMap with a broken policy.csv For Example:

apiVersion: v1
kind: ConfigMap
metadata:
  name: argocd-rbac-cm
data:
  policy.default: role:none
  policy.csv: |
    g, maxim.tacu@example.com role:admin 

  scopes: '[email]'

A comma is missing after the email in the example above!

Expected behavior A policy csv syntax checker would be NICE TO HAVE in argocd before applying it. In case if a broken policy is pushed, the best way would be to discard policies to none or readonly for everyone until a new fix will be released/pushed to the repo which is tracked by argocd and applied by itself without any manual intervention.

Version Currently we just upgraded to the latest one, but all versions are affected.

$ argocd version
argocd: v1.4.0-rc1+5af52f6
  BuildDate: 2020-01-13T17:19:43Z
  GitCommit: 5af52f66988ad8fa0d6b977d7f5aedcdb9f5a521
  GitTreeState: clean
  GoVersion: go1.12.6
  Compiler: gc
  Platform: linux/amd64
argocd-server: v1.4.0-rc1+5af52f6
  BuildDate: 2020-01-13T17:22:26Z
  GitCommit: 5af52f66988ad8fa0d6b977d7f5aedcdb9f5a521
  GitTreeState: clean
  GoVersion: go1.12.6
  Compiler: gc
  Platform: linux/amd64
  Ksonnet Version: v0.13.1
  Kustomize Version: Version: {Version:kustomize/v3.2.1 GitCommit:d89b448c745937f0cf1936162f26a5aac688f840 BuildDate:2019-09-27T00:10:52Z GoOs:linux GoArch:amd64}
  Helm Version: v2.15.2
  Kubectl Version: v1.14.0

Logs Argocd-server crash logs:

panic: grouping policy elements do not meet role definition

goroutine 149 [running]:
github.com/casbin/casbin/model.(*Assertion).buildRoleLinks(0xc00008ef00, 0x254f180, 0xc0005d6700)
    /go/src/github.com/casbin/casbin/model/assertion.go:43 +0x4bd
github.com/casbin/casbin/model.Model.BuildRoleLinks(0xc0004fd020, 0x254f180, 0xc0005d6700)
    /go/src/github.com/casbin/casbin/model/policy.go:26 +0xd4
github.com/casbin/casbin.(*Enforcer).BuildRoleLinks(0xc0007c2d90)
    /go/src/github.com/casbin/casbin/enforcer.go:296 +0x58
github.com/casbin/casbin.(*Enforcer).LoadPolicy(0xc0007c2d90, 0x0, 0x0)
    /go/src/github.com/casbin/casbin/enforcer.go:220 +0xa0
github.com/argoproj/argo-cd/util/rbac.(*Enforcer).SetUserPolicy(...)
    /go/src/github.com/argoproj/argo-cd/util/rbac/rbac.go:162
github.com/argoproj/argo-cd/util/rbac.(*Enforcer).syncUpdate(0xc00008f020, 0xc0008c0280, 0xc0008991d0, 0x0, 0x0)
    /go/src/github.com/argoproj/argo-cd/util/rbac/rbac.go:236 +0x143
github.com/argoproj/argo-cd/util/rbac.(*Enforcer).RunPolicyLoader(0xc00008f020, 0x252d5c0, 0xc0005d8400, 0xc0008991d0, 0x0, 0x0)
    /go/src/github.com/argoproj/argo-cd/util/rbac/rbac.go:183 +0x1ac
github.com/argoproj/argo-cd/server.(*ArgoCDServer).rbacPolicyLoader(0xc0004d2a00, 0x252d5c0, 0xc0005d8400)
    /go/src/github.com/argoproj/argo-cd/server/server.go:387 +0x7f
created by github.com/argoproj/argo-cd/server.(*ArgoCDServer).Run
    /go/src/github.com/argoproj/argo-cd/server/server.go:276 +0x80c

[maxtacu]

Hey guys, any updates on this?

[onurccn]

You need , between the email and the role

g, maxim.tacu@example.com, role:admin 

[maxtacu]

I know what is missing. The problem is that argocd shouldn't crash when there is an invalid policy. Argocd should switch to some default restricting policy (for example with admin only access)

[crenshaw-dev]

I'm not entirely opposed to crashing on a default config. For example, if the desired RBAC is more restrictive than the built-in policy, I wouldn't want to fall back to the less-restrictive built-in policy.

But there should be clear docs and maybe a nicer error message.

[andrii-korotkov-verkada]

I think now it logs Fatal. I agree, it's not clear what the fallback is intended to be, so fataling can make sense.