From 93894a86ab9c82093a7d14ec7ad7971fb98bb073 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacob=20L=C3=A4rfors?= <1135394+jlarfors@users.noreply.github.com> Date: Sat, 16 Nov 2024 22:43:12 +0200 Subject: [PATCH] refactor roles to use a verb list, this is much more ergonomic --- pkg/auth/rbac.go | 98 ++++++++++++------------------------ pkg/auth/rbac_server_test.go | 24 ++++----- pkg/auth/rbac_test.go | 38 +++++++------- pkg/auth/role.go | 23 ++++----- 4 files changed, 72 insertions(+), 111 deletions(-) diff --git a/pkg/auth/rbac.go b/pkg/auth/rbac.go index e0f4084..af7fdc8 100644 --- a/pkg/auth/rbac.go +++ b/pkg/auth/rbac.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "log/slog" + "slices" "strings" "sync" "time" @@ -124,8 +125,8 @@ type Group struct { } type Permissions struct { - Allow []Verbs `json:"allow"` - Deny []Verbs `json:"deny"` + Allow []Rule `json:"allow"` + Deny []Rule `json:"deny"` } type Verb string @@ -146,6 +147,7 @@ const ( VerbDelete Verb = "delete" // VerbRun allows a user to run actions for an actor. VerbRun Verb = "run" + VerbAll Verb = "*" ) type RBACRequest struct { @@ -174,8 +176,8 @@ func (r *RBAC) Check(ctx context.Context, req RBACRequest) bool { // Merge wildcard and namespace permissions. if ns == nil { ns = &Permissions{ - Allow: []Verbs{}, - Deny: []Verbs{}, + Allow: []Rule{}, + Deny: []Rule{}, } } if wildcardNS != nil { @@ -185,25 +187,7 @@ func (r *RBAC) Check(ctx context.Context, req RBACRequest) bool { if !isAllow { for _, allow := range ns.Allow { - switch req.Verb { - case VerbRead: - isAllow = checkVerbFilter(allow.Read, req.Object) || - checkVerbFilter(allow.Update, req.Object) || - checkVerbFilter(allow.Create, req.Object) || - checkVerbFilter(allow.Delete, req.Object) - - case VerbUpdate: - isAllow = checkVerbFilter(allow.Update, req.Object) - case VerbCreate: - isAllow = checkVerbFilter(allow.Create, req.Object) - case VerbDelete: - isAllow = checkVerbFilter(allow.Delete, req.Object) - case VerbRun: - isAllow = checkVerbFilter(allow.Run, req.Object) - default: - // Unknown verb. - return false - } + isAllow = checkVerb(allow, req.Verb, req.Object) if isAllow { break } @@ -211,24 +195,7 @@ func (r *RBAC) Check(ctx context.Context, req RBACRequest) bool { } if !isDeny { for _, deny := range ns.Deny { - switch req.Verb { - case VerbRead: - isDeny = checkVerbFilter(deny.Read, req.Object) - case VerbUpdate: - isDeny = checkVerbFilter(deny.Read, req.Object) || - checkVerbFilter(deny.Update, req.Object) - case VerbCreate: - isDeny = checkVerbFilter(deny.Read, req.Object) || - checkVerbFilter(deny.Create, req.Object) - case VerbDelete: - isDeny = checkVerbFilter(deny.Read, req.Object) || - checkVerbFilter(deny.Delete, req.Object) - case VerbRun: - isDeny = checkVerbFilter(deny.Run, req.Object) - default: - // Unknown verb. - return false - } + isDeny = checkVerb(deny, req.Verb, req.Object) if isDeny { break } @@ -238,20 +205,22 @@ func (r *RBAC) Check(ctx context.Context, req RBACRequest) bool { return isAllow && !isDeny } -func checkVerbFilter(vf *VerbFilter, obj hz.ObjectKeyer) bool { - if vf == nil { - return false +func checkVerb(rule Rule, verb Verb, obj hz.ObjectKeyer) bool { + // If the rule does not specify the wildcard ("*") verb, check if the verb is allowed. + if !slices.Contains(rule.Verbs, VerbAll) { + if !slices.Contains(rule.Verbs, verb) { + return false + } } - if !checkStringPattern(vf.Group, obj.ObjectGroup()) { + if !checkStringPattern(rule.Group, obj.ObjectGroup()) { return false } - if !checkStringPattern(vf.Kind, obj.ObjectKind()) { + if !checkStringPattern(rule.Kind, obj.ObjectKind()) { return false } - if !checkStringPattern(vf.Name, obj.ObjectName()) { + if !checkStringPattern(rule.Name, obj.ObjectName()) { return false } - return true } @@ -355,8 +324,8 @@ func (r *RBAC) refresh() { permissions, ok := group.Namespaces[roleBinding.Namespace] if !ok { permissions = &Permissions{ - Allow: []Verbs{}, - Deny: []Verbs{}, + Allow: []Rule{}, + Deny: []Rule{}, } group.Namespaces[roleBinding.Namespace] = permissions } @@ -403,18 +372,16 @@ func (r *RBAC) refresh() { rootNS, ok := group.Namespaces[hz.RootNamespace] if !ok { rootNS = &Permissions{ - Allow: []Verbs{}, - Deny: []Verbs{}, + Allow: []Rule{}, + Deny: []Rule{}, } group.Namespaces[hz.RootNamespace] = rootNS } - rootNS.Allow = append(rootNS.Allow, Verbs{ - Read: &VerbFilter{ - Name: &localNS, - Kind: hz.P(core.ObjectKindNamespace), - // Group: hz.P("TODO"), - }, + rootNS.Allow = append(rootNS.Allow, Rule{ + Name: &localNS, + Kind: hz.P(core.ObjectKindNamespace), + Verbs: []Verb{VerbRead}, }) } } @@ -433,17 +400,16 @@ func (r *RBAC) refresh() { ns, ok := group.Namespaces["*"] if !ok { ns = &Permissions{ - Allow: []Verbs{}, - Deny: []Verbs{}, + Allow: []Rule{}, + Deny: []Rule{}, } group.Namespaces["*"] = ns } - ns.Allow = append(ns.Allow, Verbs{ - Read: &VerbFilter{}, - Update: &VerbFilter{}, - Create: &VerbFilter{}, - Delete: &VerbFilter{}, - Run: &VerbFilter{}, + ns.Allow = append(ns.Allow, Rule{ + Group: hz.P("*"), + Kind: hz.P("*"), + Name: hz.P("*"), + Verbs: []Verb{VerbAll}, }) } diff --git a/pkg/auth/rbac_server_test.go b/pkg/auth/rbac_server_test.go index 8c3a36b..deb8690 100644 --- a/pkg/auth/rbac_server_test.go +++ b/pkg/auth/rbac_server_test.go @@ -12,7 +12,7 @@ import ( "github.com/verifa/horizon/pkg/testutil" ) -func TestRBACServer(t *testing.T) { +func TestServerRBAC(t *testing.T) { ctx := context.Background() ts := server.Test(t, ctx) @@ -45,13 +45,12 @@ func TestRBACServer(t *testing.T) { Namespace: "team-a", }, Spec: auth.RoleSpec{ - Allow: []auth.Verbs{ + Allow: []auth.Rule{ { - Create: &auth.VerbFilter{ - Group: hz.P("*"), - Kind: hz.P("*"), - Name: hz.P("*"), - }, + Group: hz.P("*"), + Kind: hz.P("*"), + Name: hz.P("*"), + Verbs: []auth.Verb{auth.VerbCreate}, }, }, }, @@ -89,13 +88,12 @@ func TestRBACServer(t *testing.T) { Namespace: "team-b", }, Spec: auth.RoleSpec{ - Allow: []auth.Verbs{ + Allow: []auth.Rule{ { - Create: &auth.VerbFilter{ - Group: hz.P("*"), - Kind: hz.P("*"), - Name: hz.P("*"), - }, + Group: hz.P("*"), + Kind: hz.P("*"), + Name: hz.P("*"), + Verbs: []auth.Verb{auth.VerbCreate}, }, }, }, diff --git a/pkg/auth/rbac_test.go b/pkg/auth/rbac_test.go index ef0cadd..45c5b87 100644 --- a/pkg/auth/rbac_test.go +++ b/pkg/auth/rbac_test.go @@ -34,12 +34,11 @@ func TestRBAC(t *testing.T) { Namespace: "namespace-test", }, Spec: RoleSpec{ - Allow: []Verbs{ + Allow: []Rule{ { - Create: &VerbFilter{ - Kind: hz.P("object-test"), - Group: hz.P("group-test"), - }, + Kind: hz.P("object-test"), + Group: hz.P("group-test"), + Verbs: []Verb{VerbRead, VerbCreate}, }, }, }, @@ -157,12 +156,11 @@ func TestRBAC(t *testing.T) { Namespace: "namespace-test", }, Spec: RoleSpec{ - Allow: []Verbs{ + Allow: []Rule{ { - Run: &VerbFilter{ - Group: hz.P("group-test"), - Kind: hz.P("object-test"), - }, + Group: hz.P("group-test"), + Kind: hz.P("object-test"), + Verbs: []Verb{VerbRun}, }, }, }, @@ -214,13 +212,12 @@ func TestRBAC(t *testing.T) { Namespace: "namespace-test", }, Spec: RoleSpec{ - Allow: []Verbs{ + Allow: []Rule{ { - Read: &VerbFilter{}, - Update: &VerbFilter{}, - Create: &VerbFilter{}, - Delete: &VerbFilter{}, - Run: &VerbFilter{}, + Group: hz.P("*"), + Name: hz.P("*"), + Kind: hz.P("*"), + Verbs: []Verb{VerbAll}, }, }, }, @@ -231,9 +228,12 @@ func TestRBAC(t *testing.T) { Namespace: "namespace-test", }, Spec: RoleSpec{ - Deny: []Verbs{ + Deny: []Rule{ { - Delete: &VerbFilter{}, + Group: hz.P("*"), + Name: hz.P("*"), + Kind: hz.P("*"), + Verbs: []Verb{VerbDelete}, }, }, }, @@ -368,7 +368,7 @@ func TestRBAC(t *testing.T) { for index, tc := range test.cases { ok := rbac.Check(ctx, tc.req) if ok != tc.expect { - t.Fatal("test case failed: ", index) + t.Fatal("test case failed: ", index, tc.req) } } }) diff --git a/pkg/auth/role.go b/pkg/auth/role.go index 69ff8d7..6798ddc 100644 --- a/pkg/auth/role.go +++ b/pkg/auth/role.go @@ -23,20 +23,17 @@ func (Role) ObjectKind() string { } type RoleSpec struct { - Allow []Verbs `json:"allow,omitempty"` - Deny []Verbs `json:"deny,omitempty"` + Allow []Rule `json:"allow,omitempty"` + Deny []Rule `json:"deny,omitempty"` } -type Verbs struct { - Read *VerbFilter `json:"read,omitempty"` - Update *VerbFilter `json:"update,omitempty"` - Create *VerbFilter `json:"create,omitempty"` - Delete *VerbFilter `json:"delete,omitempty"` - Run *VerbFilter `json:"run,omitempty"` -} - -type VerbFilter struct { - Name *string `json:"name,omitempty" cue:""` - Kind *string `json:"kind,omitempty" cue:""` +type Rule struct { + // Name of a resource that this rule targets. + Name *string `json:"name,omitempty" cue:""` + // Kind of a resource that this rule targets. + Kind *string `json:"kind,omitempty" cue:""` + // Group of a resource that this rule targets. Group *string `json:"group,omitempty" cue:""` + // Verbs that this rule enforces. + Verbs []Verb `json:"verbs,omitempty" cue:""` }