Skip to content

Consider supporting groups instead of single users only and create their tests #351

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: sig-auth-acceptance
Choose a base branch
from
Open

Consider supporting groups instead of single users only and create their tests #351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 26 additions & 7 deletions pkg/authorization/static/static.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"

"k8s.io/apiserver/pkg/authorization/authorizer"
"k8s.io/utils/set"
)

// StaticAuthorizationConfig describes what is needed to specify a static
Expand All @@ -38,8 +39,9 @@ type StaticAuthorizationConfig struct {
}

type UserConfig struct {
Name string `json:"name,omitempty"`
Groups []string `json:"groups,omitempty"`
Name string `json:"name,omitempty"`
Groups []string `json:"groups,omitempty"`
GroupSet set.Set[string]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we split the external, serialized config, and its internal representation?
Make it so that only one of username/group can be specified.

}

type staticAuthorizer struct {
Expand All @@ -48,8 +50,12 @@ type staticAuthorizer struct {

// NewStaticAuthorizer creates an authorizer for static SubjectAccessReviews
func NewStaticAuthorizer(config []StaticAuthorizationConfig) (*staticAuthorizer, error) {
for _, c := range config {
if c.ResourceRequest != (c.Path == "") {
for c := range config {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we need some internal representation of the config anyway, it might be easier to implement this part as a unionauthorizer.New([]staticAuthorizer{...}) (union authorizer constructor) where each staticAuthorizer in the above mentioned slice represents each element from []StaticAuthorizationConfig here.

That way func (saConfig StaticAuthorizationConfig) Matches(a authorizer.Attributes) bool
changes into
func (sa staticAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error).
Each return true changes into authorizer.DecisionAllow and each return false into authorizer.DecisionNoOpinion.

The constructor might then look something like this:

func NewStaticAuthorizer(config []StaticAuthorizationConfig) (authorizer.Authorizer, error) {
   var authorizers []staticAuthorizer
   for _, c := range config {
       authz, err := newStaticAuthorizers(&c)
       // handle error
       authorizers = append(authorizers, authz)
   }
   return unionauthorizer.New(authorizers...)
}

WDYT?

cc @ibihim

Copy link
Collaborator

@ibihim ibihim Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

lol. Yes, I agree. This is what I tried to convey. We need an internal and an external representation. The internal one has a Set, the other has a slice of groups potentially.

WRT the I have no strong opinions. We could leave it as is, but why invent our own logic, if we try to satisfy the authorizer interface, right?

if config[c].User.Groups != nil {
config[c].User.GroupSet = set.New(config[c].User.Groups...)
}

if config[c].ResourceRequest != (config[c].Path == "") {
return nil, fmt.Errorf("invalid configuration: resource requests must not include a path: %v", config)
}
}
Expand All @@ -60,17 +66,30 @@ func (saConfig StaticAuthorizationConfig) Matches(a authorizer.Attributes) bool
isAllowed := func(staticConf string, requestVal string) bool {
if staticConf == "" {
return true
} else {
return staticConf == requestVal
}
return staticConf == requestVal
}
isGroupAllowed := func(requestGroups []string) bool {
if len(saConfig.User.GroupSet) == 0 {
return true
}
for _, group := range requestGroups {
if _, exists := saConfig.User.GroupSet[group]; exists {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use the set.Has() method

return true
}
}
return false
}

userName := ""
userGroups := []string{}
if a.GetUser() != nil {
userName = a.GetUser().GetName()
userGroups = a.GetUser().GetGroups()
}

if isAllowed(saConfig.User.Name, userName) &&
if (saConfig.User.Name == "" || isAllowed(saConfig.User.Name, userName)) &&
isGroupAllowed(userGroups) &&
isAllowed(saConfig.Verb, a.GetVerb()) &&
isAllowed(saConfig.Namespace, a.GetNamespace()) &&
isAllowed(saConfig.APIGroup, a.GetAPIGroup()) &&
Expand Down
49 changes: 49 additions & 0 deletions pkg/authorization/static/static_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,55 @@ func TestStaticAuthorizer(t *testing.T) {
authorizer.AttributesRecord{Verb: "get", Resource: "services", ResourceRequest: true},
},
},
{
name: "groupMatch",
config: []StaticAuthorizationConfig{
{User: UserConfig{Groups: []string{"admin", "editors"}}, Verb: "get", Resource: "namespaces", ResourceRequest: true},
},
shouldPass: []authorizer.Attributes{
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "user1", Groups: []string{"admin"}}, Verb: "get", Resource: "namespaces", ResourceRequest: true},
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "user2", Groups: []string{"editors"}}, Verb: "get", Resource: "namespaces", ResourceRequest: true},
},
shouldNoOpinion: []authorizer.Attributes{
// User with non-matching group
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "user3", Groups: []string{"viewers"}}, Verb: "get", Resource: "namespaces", ResourceRequest: true},
// User with no groups
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "user4"}, Verb: "get", Resource: "namespaces", ResourceRequest: true},
},
},
{
name: "groupAndUserMatch",
config: []StaticAuthorizationConfig{
{User: UserConfig{Name: "system:foo", Groups: []string{"admin"}}, Verb: "get", Resource: "pods", ResourceRequest: true},
},
shouldPass: []authorizer.Attributes{
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "system:foo", Groups: []string{"admin", "dev"}}, Verb: "get", Resource: "pods", ResourceRequest: true},
},
shouldNoOpinion: []authorizer.Attributes{
// User name matches, but group does not
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "system:foo", Groups: []string{"viewers"}}, Verb: "get", Resource: "pods", ResourceRequest: true},
// Group matches, but user name does not
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "system:bar", Groups: []string{"admin"}}, Verb: "get", Resource: "pods", ResourceRequest: true},
// Neither user name nor group matches
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "system:baz", Groups: []string{"viewers"}}, Verb: "get", Resource: "pods", ResourceRequest: true},
},
},
{
name: "groupWildcard",
config: []StaticAuthorizationConfig{
{User: UserConfig{Groups: []string{}}, Verb: "get", Resource: "services", ResourceRequest: true},
},
shouldPass: []authorizer.Attributes{
// Any group should be allowed as no groups are specified
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "user1", Groups: []string{"viewers"}}, Verb: "get", Resource: "services", ResourceRequest: true},
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "user2", Groups: []string{"admins", "viewers"}}, Verb: "get", Resource: "services", ResourceRequest: true},
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "user3"}, Verb: "get", Resource: "services", ResourceRequest: true},
},
shouldNoOpinion: []authorizer.Attributes{
// Wrong verb
authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "user1", Groups: []string{"viewers"}}, Verb: "update", Resource: "services", ResourceRequest: true},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
Loading