From b496ecaccf472b850e66cd5248204ca6596ff193 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacob=20L=C3=A4rfors?= <1135394+jlarfors@users.noreply.github.com> Date: Fri, 22 Nov 2024 11:32:31 +0200 Subject: [PATCH] allow only one admin group and make roles in root namespace apply to all namespaces --- pkg/auth/auth.go | 8 ++-- pkg/auth/rbac.go | 78 +++++++++++++++++-------------------- pkg/auth/rbac_test.go | 89 +++++++++++++++++++++++++++++++++++++++---- 3 files changed, 121 insertions(+), 54 deletions(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 5f46520..9ed076b 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -12,16 +12,16 @@ import ( "github.com/verifa/horizon/pkg/hz" ) -func WithAdminGroups(groups ...string) Option { +func WithAdminGroups(group string) Option { return func(o *authorizerOptions) { - o.adminGroups = append(o.adminGroups, groups...) + o.adminGroup = group } } type Option func(*authorizerOptions) type authorizerOptions struct { - adminGroups []string + adminGroup string } var defaultAuthorizerOptions = authorizerOptions{} @@ -98,7 +98,7 @@ func (a *Auth) Start( RoleBindings: make(map[string]RoleBinding), Roles: make(map[string]Role), Permissions: make(map[string]*Group), - AdminGroups: ao.adminGroups, + AdminGroup: ao.adminGroup, } if err := rbac.Start(ctx); err != nil { return fmt.Errorf("starting rbac: %w", err) diff --git a/pkg/auth/rbac.go b/pkg/auth/rbac.go index 1266e38..142e5b6 100644 --- a/pkg/auth/rbac.go +++ b/pkg/auth/rbac.go @@ -19,12 +19,12 @@ type RBAC struct { Conn *nats.Conn // TODO: RoleBindings and Roles maps are not thread safe. // E.g. HandleRoleEvent and refresh both write and read from Roles. - RoleBindings map[string]RoleBinding `json:"roleBindings,omitempty"` - Roles map[string]Role `json:"roles,omitempty"` + RoleBindings map[string]RoleBinding + Roles map[string]Role - Permissions map[string]*Group `json:"permissions,omitempty"` + Permissions map[string]*Group - AdminGroups []string `json:"adminGroups,omitempty"` + AdminGroup string // init is true if the RBAC has been initialised. // RBAC has been initialised if all watchers have been started and @@ -129,6 +129,20 @@ type Permissions struct { Deny []Rule `json:"deny"` } +func (p *Permissions) AllowRules() []Rule { + if p == nil { + return nil + } + return p.Allow +} + +func (p *Permissions) DenyRules() []Rule { + if p == nil { + return nil + } + return p.Deny +} + type Verb string const ( @@ -159,6 +173,10 @@ type RequestSubject struct { } func (r *RBAC) Check(ctx context.Context, req Request) bool { + // Members of the admin group is allowed to do anything. + if slices.Contains(req.Subject.Groups, r.AdminGroup) { + return true + } r.mx.RLock() defer r.mx.RUnlock() @@ -169,26 +187,26 @@ func (r *RBAC) Check(ctx context.Context, req Request) bool { if !ok { continue } - wildcardNS, wildcardOK := group.Namespaces["*"] + rootNS, wildcardOK := group.Namespaces[hz.NamespaceRoot] ns, nsOK := group.Namespaces[req.Object.ObjectNamespace()] - if !wildcardOK && !nsOK { + // Exit early if there are no permissions for either. + if !(wildcardOK || nsOK) { continue } + lenAllow := len(rootNS.AllowRules()) + len(ns.AllowRules()) + lenDeny := len(rootNS.DenyRules()) + len(ns.DenyRules()) + // Merge wildcard and namespace permissions. - if ns == nil { - ns = &Permissions{ - Allow: []Rule{}, - Deny: []Rule{}, - } - } - if wildcardNS != nil { - ns.Allow = append(ns.Allow, wildcardNS.Allow...) - ns.Deny = append(ns.Deny, wildcardNS.Deny...) + perm := &Permissions{ + Allow: make([]Rule, 0, lenAllow), + Deny: make([]Rule, 0, lenDeny), } + perm.Allow = append(rootNS.AllowRules(), ns.AllowRules()...) + perm.Deny = append(rootNS.DenyRules(), ns.DenyRules()...) if !isAllow { - for _, allow := range ns.Allow { + for _, allow := range perm.Allow { isAllow = checkVerb(allow, req.Verb, req.Object) if isAllow { break @@ -196,7 +214,7 @@ func (r *RBAC) Check(ctx context.Context, req Request) bool { } } if !isDeny { - for _, deny := range ns.Deny { + for _, deny := range perm.Deny { isDeny = checkVerb(deny, req.Verb, req.Object) if isDeny { break @@ -390,32 +408,6 @@ func (r *RBAC) refresh() { } } - // Add admin group permissions (if any). - for _, adminGroup := range r.AdminGroups { - group, ok := cache[adminGroup] - if !ok { - group = &Group{ - Name: adminGroup, - Namespaces: make(map[string]*Permissions), - } - cache[adminGroup] = group - } - ns, ok := group.Namespaces["*"] - if !ok { - ns = &Permissions{ - Allow: []Rule{}, - Deny: []Rule{}, - } - group.Namespaces["*"] = ns - } - ns.Allow = append(ns.Allow, Rule{ - Group: hz.P("*"), - Kind: hz.P("*"), - Name: hz.P("*"), - Verbs: []Verb{VerbAll}, - }) - } - r.mx.Lock() defer r.mx.Unlock() r.Permissions = cache diff --git a/pkg/auth/rbac_test.go b/pkg/auth/rbac_test.go index 35e34af..d684a99 100644 --- a/pkg/auth/rbac_test.go +++ b/pkg/auth/rbac_test.go @@ -17,10 +17,10 @@ func TestRBAC(t *testing.T) { } type test struct { - name string - adminGroups []string - roles []Role - bindings []RoleBinding + name string + adminGroup string + roles []Role + bindings []RoleBinding cases []testcase } @@ -342,8 +342,8 @@ func TestRBAC(t *testing.T) { } testAdminGroup := test{ - name: "admin-group", - adminGroups: []string{"admin"}, + name: "admin-group", + adminGroup: "admin", cases: []testcase{ { req: Request{ @@ -363,11 +363,86 @@ func TestRBAC(t *testing.T) { }, } + testAllowRootNamespace := test{ + name: "allow-root-namespace", + roles: []Role{ + { + ObjectMeta: hz.ObjectMeta{ + Name: "role-allow-root-namespace", + Namespace: hz.NamespaceRoot, + }, + Spec: RoleSpec{ + Allow: []Rule{ + { + Group: hz.P("*"), + Kind: hz.P("*"), + Verbs: []Verb{VerbAll}, + }, + }, + }, + }, + }, + bindings: []RoleBinding{ + { + ObjectMeta: hz.ObjectMeta{ + Name: "rolebinding-allow-root-namespace", + Namespace: hz.NamespaceRoot, + }, + Spec: RoleBindingSpec{ + RoleRef: RoleRef{ + Group: "core", + Kind: "Role", + Name: "role-allow-root-namespace", + }, + Subjects: []Subject{ + { + Kind: "Group", + Name: "group-allow-root-namespace", + }, + }, + }, + }, + }, + cases: []testcase{ + { + req: Request{ + Subject: RequestSubject{ + Groups: []string{"group-allow-root-namespace"}, + }, + Verb: "read", + Object: hz.ObjectKey{ + Group: "group-test", + Kind: "object-test", + Namespace: hz.NamespaceRoot, + Name: "superfluous", + }, + }, + expect: true, + }, + { + req: Request{ + Subject: RequestSubject{ + Groups: []string{"group-allow-root-namespace"}, + }, + Verb: "read", + Object: hz.ObjectKey{ + Group: "group-test", + Kind: "object-test", + Namespace: "any-other-namespace", + Name: "superfluous", + }, + }, + expect: true, + }, + }, + } + tests := []test{ testAdminGroup, testCreateAllowsRead, testAllowRun, testDenyDelete, + testAllowRootNamespace, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -375,7 +450,7 @@ func TestRBAC(t *testing.T) { RoleBindings: make(map[string]RoleBinding), Roles: make(map[string]Role), Permissions: make(map[string]*Group), - AdminGroups: test.adminGroups, + AdminGroup: test.adminGroup, } for _, role := range test.roles { _, err := rbac.HandleRoleEvent(event(t, role))