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

feat: additional RBAC configmaps #9976

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/argocd/commands/admin/settings_rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ func getPolicyConfigMap(ctx context.Context, client kubernetes.Interface, namesp
// checkPolicy checks whether given subject is allowed to execute specified
// action against specified resource
func checkPolicy(subject, action, resource, subResource, builtinPolicy, userPolicy, defaultRole, matchMode string, strict bool) bool {
enf := rbac.NewEnforcer(nil, "argocd", "argocd-rbac-cm", nil)
enf := rbac.NewEnforcer(nil, "argocd", common.ArgoCDRBACConfigMapName, nil)
enf.SetDefaultRole(defaultRole)
enf.SetMatchMode(matchMode)
if builtinPolicy != "" {
Expand All @@ -329,7 +329,7 @@ func checkPolicy(subject, action, resource, subResource, builtinPolicy, userPoli
log.Fatalf("invalid user policy: %v", err)
return false
}
if err := enf.SetUserPolicy(userPolicy); err != nil {
if err := enf.SetUserPolicy(common.ArgoCDRBACConfigMapName, userPolicy); err != nil {
log.Fatalf("could not set user policy: %v", err)
return false
}
Expand Down
9 changes: 9 additions & 0 deletions common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -293,3 +293,12 @@ const (
// Keep alive is 2x enforcement minimum to ensure network jitter does not introduce ENHANCE_YOUR_CALM errors
GRPCKeepAliveTime = 2 * GRPCKeepAliveEnforcementMinimum
)

// Security log levels
const (
SecurityField = "security"
SecurityCritical = "4"
SecurityHigh = "3"
SecurityMedium = "2"
SecurityLow = "1"
)
27 changes: 27 additions & 0 deletions docs/operator-manual/rbac.md
Original file line number Diff line number Diff line change
Expand Up @@ -212,3 +212,30 @@ Or against the group defined:
$ argocd admin settings rbac can db-admins get applications 'staging-db-admins/*' --policy-file policy.csv
Yes
```

### Adding additional RBAC configmaps
Since v2.5, you now have the ability to create additional ConfigMaps to augment the policy specified in `argocd-rbac-cm`.

Simply create a ConfigMap in the `argocd` namespace with the label `argocd.argoproj.io/cm-type=additional-rbac` and specify the `policy.csv` key.

```yaml
apiVersion: v1
kind: ConfigMap
metadata:
name: argocd-rbac-cm-extra
namespace: argocd
labels:
argocd.argoproj.io/cm-type: additional-rbac
data:
policy.csv: |
notfromstatefarm marked this conversation as resolved.
Show resolved Hide resolved
p, role:org-admin, applications, *, */*, allow
p, role:org-admin, clusters, get, *, allow
p, role:org-admin, repositories, get, *, allow
p, role:org-admin, repositories, create, *, allow
p, role:org-admin, repositories, update, *, allow
p, role:org-admin, repositories, delete, *, allow
p, role:org-admin, logs, get, *, allow
p, role:org-admin, exec, create, */*, allow

g, your-github-org:your-team, role:org-admin
```
2 changes: 1 addition & 1 deletion server/application/application_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ g, group-47, role:test2
p, role:test3, applications, *, proj-20/*, allow
g, group-49, role:test3
`
_ = enf.SetUserPolicy(policy)
_ = enf.SetUserPolicy(common.ArgoCDRBACConfigMapName, policy)
}
appServer := newTestAppServerWithEnforcerConfigure(f, objects...)

Expand Down
6 changes: 3 additions & 3 deletions server/rbacpolicy/rbacpolicy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func TestEnforceAllPolicies(t *testing.T) {
enf := rbac.NewEnforcer(kubeclientset, test.FakeArgoCDNamespace, common.ArgoCDConfigMapName, nil)
enf.EnableLog(true)
_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj/*, allow` + "\n" + `p, alice, logs, get, my-proj/*, allow` + "\n" + `p, alice, exec, create, my-proj/*, allow`)
_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj/*, allow` + "\n" + `p, bob, logs, get, my-proj/*, allow` + "\n" + `p, bob, exec, create, my-proj/*, allow`)
_ = enf.SetUserPolicy(common.ArgoCDRBACConfigMapName, `p, bob, applications, create, my-proj/*, allow`+"\n"+`p, bob, logs, get, my-proj/*, allow`+"\n"+`p, bob, exec, create, my-proj/*, allow`)
rbacEnf := NewRBACPolicyEnforcer(enf, projLister)
enf.SetClaimsEnforcerFunc(rbacEnf.EnforceClaims)

Expand Down Expand Up @@ -128,7 +128,7 @@ func TestInvalidatedCache(t *testing.T) {
enf := rbac.NewEnforcer(kubeclientset, test.FakeArgoCDNamespace, common.ArgoCDConfigMapName, nil)
enf.EnableLog(true)
_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj/*, allow` + "\n" + `p, alice, logs, get, my-proj/*, allow` + "\n" + `p, alice, exec, create, my-proj/*, allow`)
_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj/*, allow` + "\n" + `p, bob, logs, get, my-proj/*, allow` + "\n" + `p, bob, exec, create, my-proj/*, allow`)
_ = enf.SetUserPolicy(common.ArgoCDRBACConfigMapName, `p, bob, applications, create, my-proj/*, allow`+"\n"+`p, bob, logs, get, my-proj/*, allow`+"\n"+`p, bob, exec, create, my-proj/*, allow`)
rbacEnf := NewRBACPolicyEnforcer(enf, projLister)
enf.SetClaimsEnforcerFunc(rbacEnf.EnforceClaims)

Expand All @@ -143,7 +143,7 @@ func TestInvalidatedCache(t *testing.T) {
assert.True(t, enf.Enforce(claims, "exec", "create", "my-proj/my-app"))

_ = enf.SetBuiltinPolicy(`p, alice, applications, create, my-proj2/*, allow` + "\n" + `p, alice, logs, get, my-proj2/*, allow` + "\n" + `p, alice, exec, create, my-proj2/*, allow`)
_ = enf.SetUserPolicy(`p, bob, applications, create, my-proj2/*, allow` + "\n" + `p, bob, logs, get, my-proj2/*, allow` + "\n" + `p, bob, exec, create, my-proj2/*, allow`)
_ = enf.SetUserPolicy(common.ArgoCDRBACConfigMapName, `p, bob, applications, create, my-proj2/*, allow`+"\n"+`p, bob, logs, get, my-proj2/*, allow`+"\n"+`p, bob, exec, create, my-proj2/*, allow`)
claims = jwt.MapClaims{"sub": "alice"}
assert.True(t, enf.Enforce(claims, "applications", "create", "my-proj2/my-app"))
assert.True(t, enf.Enforce(claims, "logs", "get", "my-proj2/my-app"))
Expand Down
2 changes: 1 addition & 1 deletion server/repository/repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ func TestRepositoryServerGetAppDetails(t *testing.T) {
repoServerClient := mocks.RepoServerServiceClient{}
repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}
enforcer := newEnforcer(kubeclientset)
_ = enforcer.SetUserPolicy("p, role:readrepos, repositories, get, *, allow")
_ = enforcer.SetUserPolicy(common.ArgoCDRBACConfigMapName, "p, role:readrepos, repositories, get, *, allow")
enforcer.SetDefaultRole("role:readrepos")

url := "https://test"
Expand Down
2 changes: 1 addition & 1 deletion server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ func TestEnforceClaims(t *testing.T) {
g, org2:team2, role:admin
g, bob, role:admin
`
_ = enf.SetUserPolicy(policy)
_ = enf.SetUserPolicy(common.ArgoCDRBACConfigMapName, policy)
allowed := []jwt.Claims{
jwt.MapClaims{"groups": []string{"org1:team1", "org2:team2"}},
jwt.RegisteredClaims{Subject: "admin"},
Expand Down
103 changes: 97 additions & 6 deletions util/rbac/rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"sync"
"time"

"github.com/argoproj/argo-cd/v2/common"
"github.com/argoproj/argo-cd/v2/util/assets"
"github.com/argoproj/argo-cd/v2/util/glob"
jwtutil "github.com/argoproj/argo-cd/v2/util/jwt"
Expand Down Expand Up @@ -71,6 +72,8 @@ type Enforcer struct {
model model.Model
defaultRole string
matchMode string
policyCache map[string]string
policyLock sync.Mutex
}

// cachedEnforcer holds the Casbin enforcer instances and optional custom project policy
Expand Down Expand Up @@ -168,6 +171,7 @@ func NewEnforcer(clientset kubernetes.Interface, namespace, configmap string, cl
model: builtInModel,
claimsEnforcerFunc: claimsEnforcer,
enabled: true,
policyCache: make(map[string]string),
}
}

Expand Down Expand Up @@ -317,6 +321,17 @@ func enforce(enf CasbinEnforcer, defaultRole string, claimsEnforcerFunc ClaimsEn
return ok && err == nil
}

// buildPolicy merges all the user policies into one
func (e *Enforcer) buildPolicy() string {
e.policyLock.Lock()
fullPolicy := ""
for _, policy := range e.policyCache {
fullPolicy = fmt.Sprintf("%s%s\n", fullPolicy, policy)
}
e.policyLock.Unlock()
return fullPolicy
}

// SetBuiltinPolicy sets a built-in policy, which augments any user defined policies
func (e *Enforcer) SetBuiltinPolicy(policy string) error {
e.invalidateCache(func() {
Expand All @@ -326,14 +341,28 @@ func (e *Enforcer) SetBuiltinPolicy(policy string) error {
}

// SetUserPolicy sets a user policy, augmenting the built-in policy
func (e *Enforcer) SetUserPolicy(policy string) error {
func (e *Enforcer) SetUserPolicy(policyName string, policy string) error {
e.policyLock.Lock()
e.policyCache[policyName] = policy
e.policyLock.Unlock()
e.invalidateCache(func() {
e.adapter.userDefinedPolicy = policy
e.adapter.userDefinedPolicy = e.buildPolicy()
})
return e.LoadPolicy()
}

// newInformers returns an informer which watches updates on the rbac configmap
// DeleteUserPolicy deletes a user policy
func (e *Enforcer) DeleteUserPolicy(policyName string) error {
e.policyLock.Lock()
delete(e.policyCache, policyName)
e.policyLock.Unlock()
e.invalidateCache(func() {
e.adapter.userDefinedPolicy = e.buildPolicy()
})
return e.LoadPolicy()
}

// newInformer returns an informer which watches updates on the rbac configmap
func (e *Enforcer) newInformer() cache.SharedIndexInformer {
tweakConfigMap := func(options *metav1.ListOptions) {
cmFieldSelector := fields.ParseSelectorOrDie(fmt.Sprintf("metadata.name=%s", e.configmap))
Expand All @@ -343,6 +372,15 @@ func (e *Enforcer) newInformer() cache.SharedIndexInformer {
return v1.NewFilteredConfigMapInformer(e.clientset, e.namespace, defaultRBACSyncPeriod, indexers, tweakConfigMap)
}

// newAdditionalInformer returns an informer which watches updates on the rbac configmap
func (e *Enforcer) newAdditionalInformer() cache.SharedIndexInformer {
tweakConfigMap := func(options *metav1.ListOptions) {
options.LabelSelector = "argocd.argoproj.io/cm-type=additional-rbac"
Copy link
Member

Choose a reason for hiding this comment

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

What happens if I remove the label? Will the policy be dropped from the combined policy?

Choose a reason for hiding this comment

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

since we are using v1.NewFilteredConfigMapInformer with our specified labelselector, it will drop the policy from combined policy:

time="2022-11-21T23:04:23Z" level=info msg="RBAC Additional ConfigMap 'argocd-rbac-cm-extra' deleted" security=2

Copy link
Member

Choose a reason for hiding this comment

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

Should additional-rbac become a constant?

And also, I'm in favor for dropping additional and just having it named rbac

}
indexers := cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}
return v1.NewFilteredConfigMapInformer(e.clientset, e.namespace, defaultRBACSyncPeriod, indexers, tweakConfigMap)
}

// RunPolicyLoader runs the policy loader which watches policy updates from the configmap and reloads them
func (e *Enforcer) RunPolicyLoader(ctx context.Context, onUpdated func(cm *apiv1.ConfigMap) error) error {
cm, err := e.clientset.CoreV1().ConfigMaps(e.namespace).Get(ctx, e.configmap, metav1.GetOptions{})
Expand All @@ -356,6 +394,7 @@ func (e *Enforcer) RunPolicyLoader(ctx context.Context, onUpdated func(cm *apiv1
return err
}
}
go e.runAdditionalInformer(ctx)
e.runInformer(ctx, onUpdated)
return nil
}
Expand All @@ -370,7 +409,7 @@ func (e *Enforcer) runInformer(ctx context.Context, onUpdated func(cm *apiv1.Con
if err != nil {
log.Error(err)
} else {
log.Infof("RBAC ConfigMap '%s' added", e.configmap)
log.WithField(common.SecurityField, common.SecurityMedium).Infof("RBAC ConfigMap '%s' added", cm.Name)
}
}
},
Expand All @@ -384,7 +423,7 @@ func (e *Enforcer) runInformer(ctx context.Context, onUpdated func(cm *apiv1.Con
if err != nil {
log.Error(err)
} else {
log.Infof("RBAC ConfigMap '%s' updated", e.configmap)
log.WithField(common.SecurityField, common.SecurityMedium).Infof("RBAC ConfigMap '%s' updated", newCM.Name)
}
},
},
Expand All @@ -405,7 +444,59 @@ func (e *Enforcer) syncUpdate(cm *apiv1.ConfigMap, onUpdated func(cm *apiv1.Conf
if err := onUpdated(cm); err != nil {
return err
}
return e.SetUserPolicy(policyCSV)
return e.SetUserPolicy(cm.Name, policyCSV)
}

func (e *Enforcer) runAdditionalInformer(ctx context.Context) {
cmInformer := e.newAdditionalInformer()
cmInformer.AddEventHandler(
cache.ResourceEventHandlerFuncs{
AddFunc: func(obj interface{}) {
if cm, ok := obj.(*apiv1.ConfigMap); ok {
err := e.syncAdditionalUpdate(cm)
if err != nil {
log.Error(err)
} else {
log.WithField(common.SecurityField, common.SecurityMedium).Infof("RBAC Additional ConfigMap '%s' added", cm.Name)
}
}
},
UpdateFunc: func(old, new interface{}) {
oldCM := old.(*apiv1.ConfigMap)
newCM := new.(*apiv1.ConfigMap)
if oldCM.ResourceVersion == newCM.ResourceVersion {
return
}
err := e.syncAdditionalUpdate(newCM)
if err != nil {
log.Error(err)
} else {
log.WithField(common.SecurityField, common.SecurityMedium).Infof("RBAC Additional ConfigMap '%s' updated", newCM.Name)
}
},
DeleteFunc: func(obj interface{}) {
cm := obj.(*apiv1.ConfigMap)
err := e.DeleteUserPolicy(cm.Name)
if err != nil {
log.Error(err)
} else {
log.WithField(common.SecurityField, common.SecurityMedium).Infof("RBAC Additional ConfigMap '%s' deleted", cm.Name)
}
},
},
)
log.Info("Starting additional rbac config informer")
cmInformer.Run(ctx.Done())
log.Info("rbac additional configmap informer cancelled")
}

// syncUpdate updates the enforcer
func (e *Enforcer) syncAdditionalUpdate(cm *apiv1.ConfigMap) error {
policyCSV, ok := cm.Data[ConfigMapPolicyCSVKey]
if !ok {
policyCSV = ""
}
return e.SetUserPolicy(cm.Name, policyCSV)
}

// ValidatePolicy verifies a policy string is acceptable to casbin
Expand Down
3 changes: 2 additions & 1 deletion util/rbac/rbac_norace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"
"time"

"github.com/argoproj/argo-cd/v2/common"
"github.com/stretchr/testify/assert"
apiv1 "k8s.io/api/core/v1"
"k8s.io/client-go/kubernetes/fake"
Expand Down Expand Up @@ -78,7 +79,7 @@ p, trudy, applications/secrets, get, foo/obj, deny
p, danny, applications, get, */obj, allow
p, danny, applications, get, proj1/a*p1, allow
`
_ = enf.SetUserPolicy(policy)
_ = enf.SetUserPolicy(common.ArgoCDRBACConfigMapName, policy)

// Verify the resource wildcard
assert.True(t, enf.Enforce("alice", "applications", "get", "foo/obj"))
Expand Down
Loading