From 8ffc58bbc7c4143c902c10e026ff08e9ec678caf Mon Sep 17 00:00:00 2001 From: Mangirdas Judeikis Date: Sat, 23 Nov 2024 12:13:14 +0200 Subject: [PATCH] Merge pull request #145 from sttts/sttts-warrants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ✨ authz: add warrants to default rule resolver --- pkg/registry/rbac/validation/kcp.go | 126 ++++++++++++ pkg/registry/rbac/validation/kcp_test.go | 238 ++++++++++++++++++++++ pkg/registry/rbac/validation/rule.go | 8 +- pkg/registry/rbac/validation/rule_test.go | 3 +- 4 files changed, 370 insertions(+), 5 deletions(-) create mode 100644 pkg/registry/rbac/validation/kcp.go create mode 100644 pkg/registry/rbac/validation/kcp_test.go diff --git a/pkg/registry/rbac/validation/kcp.go b/pkg/registry/rbac/validation/kcp.go new file mode 100644 index 0000000000000..a680095d64607 --- /dev/null +++ b/pkg/registry/rbac/validation/kcp.go @@ -0,0 +1,126 @@ +package validation + +import ( + "context" + "encoding/json" + "strings" + + "github.com/kcp-dev/logicalcluster/v3" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/util/sets" + authserviceaccount "k8s.io/apiserver/pkg/authentication/serviceaccount" + "k8s.io/apiserver/pkg/authentication/user" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" +) + +const ( + // WarrantExtraKey is the key used in a user's "extra" to specify + // JSON-encoded user infos for attached extra permissions for that user + // evaluated by the authorizer. + WarrantExtraKey = "authorization.kcp.io/warrants" + + // ScopeExtraKey is the key used in a user's "extra" to specify + // that the user is restricted to a given scope. Valid values are: + // - "cluster:" + // In the future, we might add: + // - "interval:from:to". + // Scopes are and'ed. Scoping to multiple clusters invalidates it for all. + ScopeExtraKey = "authentication.kcp.io/scopes" +) + +// Warrant is serialized into the user's "extra" field authorization.kcp.io/warrants +// to hold user information for extra permissions. +type Warrant struct { + // User is the user you're testing for. + // If you specify "User" but not "Groups", then is it interpreted as "What if User were not a member of any groups + // +optional + User string `json:"user,omitempty"` + // Groups is the groups you're testing for. + // +optional + // +listType=atomic + Groups []string `json:"groups,omitempty"` + // Extra corresponds to the user.Info.GetExtra() method from the authenticator. Since that is input to the authorizer + // it needs a reflection here. + // +optional + Extra map[string][]string `json:"extra,omitempty"` + // UID information about the requesting user. + // +optional + UID string `json:"uid,omitempty"` +} + +type appliesToUserFunc func(user user.Info, subject rbacv1.Subject, namespace string) bool +type appliesToUserFuncCtx func(ctx context.Context, user user.Info, subject rbacv1.Subject, namespace string) bool + +var appliesToUserWithWarrants = withWarrants(appliesToUser) + +// withWarrants wraps the appliesToUser predicate to check for the base user and any warrants. +func withWarrants(appliesToUser appliesToUserFunc) appliesToUserFuncCtx { + var recursive appliesToUserFuncCtx + recursive = func(ctx context.Context, u user.Info, bindingSubject rbacv1.Subject, namespace string) bool { + cluster := genericapirequest.ClusterFrom(ctx) + if IsInScope(u, cluster.Name) && appliesToUser(u, bindingSubject, namespace) { + return true + } + + for _, v := range u.GetExtra()[WarrantExtraKey] { + var w Warrant + if err := json.Unmarshal([]byte(v), &w); err != nil { + continue + } + + wu := &user.DefaultInfo{ + Name: w.User, + UID: w.UID, + Groups: w.Groups, + Extra: w.Extra, + } + if IsServiceAccount(wu) && len(w.Extra[authserviceaccount.ClusterNameKey]) == 0 { + continue + } + if recursive(ctx, wu, bindingSubject, namespace) { + return true + } + } + + return false + } + return recursive +} + +// IsServiceAccount returns true if the user is a service account. +func IsServiceAccount(attr user.Info) bool { + return strings.HasPrefix(attr.GetName(), "system:serviceaccount:") +} + +// IsForeign returns true if the service account is not from the given cluster. +func IsForeign(attr user.Info, cluster logicalcluster.Name) bool { + clusters := attr.GetExtra()[authserviceaccount.ClusterNameKey] + if clusters == nil { + // an unqualified service account is considered local: think of some + // local SubjectAccessReview specifying a service account without the + // cluster scope. + return false + } + return !sets.New(clusters...).Has(string(cluster)) +} + +// IsInScope checks if the user is valid for the given cluster. +func IsInScope(attr user.Info, cluster logicalcluster.Name) bool { + if IsServiceAccount(attr) && IsForeign(attr, cluster) { + return false + } + + scopes := attr.GetExtra()[ScopeExtraKey] + for _, scope := range scopes { + switch { + case strings.HasPrefix(scope, "cluster:"): + if scope != "cluster:"+string(cluster) { + return false + } + default: + // Unknown scope, ignore. + } + } + + return true +} diff --git a/pkg/registry/rbac/validation/kcp_test.go b/pkg/registry/rbac/validation/kcp_test.go new file mode 100644 index 0000000000000..395ccec988f4e --- /dev/null +++ b/pkg/registry/rbac/validation/kcp_test.go @@ -0,0 +1,238 @@ +package validation + +import ( + "context" + "testing" + + "github.com/kcp-dev/logicalcluster/v3" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/endpoints/request" +) + +func TestIsInScope(t *testing.T) { + tests := []struct { + name string + info user.DefaultInfo + cluster logicalcluster.Name + want bool + }{ + {name: "empty", cluster: logicalcluster.Name("cluster"), want: true}, + { + name: "serviceaccount from other cluster", + info: user.DefaultInfo{Name: "system:serviceaccount:default:foo", Extra: map[string][]string{"authentication.kubernetes.io/cluster-name": {"anotherws"}}}, + cluster: logicalcluster.Name("this"), + want: false, + }, + { + name: "serviceaccount from same cluster", + info: user.DefaultInfo{Name: "system:serviceaccount:default:foo", Extra: map[string][]string{"authentication.kubernetes.io/cluster-name": {"this"}}}, + cluster: logicalcluster.Name("this"), + want: true, + }, + { + name: "serviceaccount without a cluster", + info: user.DefaultInfo{Name: "system:serviceaccount:default:foo"}, + cluster: logicalcluster.Name("this"), + // an unqualified service account is considered local: think of some + // local SubjectAccessReview specifying a service account without the + // cluster scope. + want: true, + }, + { + name: "scoped user", + info: user.DefaultInfo{Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:this"}}}, + cluster: logicalcluster.Name("this"), + want: true, + }, + { + name: "scoped user to another cluster", + info: user.DefaultInfo{Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:another"}}}, + cluster: logicalcluster.Name("this"), + want: false, + }, + { + name: "scoped user to multiple clusters", + info: user.DefaultInfo{Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:this", "cluster:another"}}}, + cluster: logicalcluster.Name("this"), + want: false, + }, + { + name: "unknown scope", + info: user.DefaultInfo{Extra: map[string][]string{"authentication.kcp.io/scopes": {"unknown:foo"}}}, + cluster: logicalcluster.Name("this"), + want: true, + }, + { + name: "scoped service account", + info: user.DefaultInfo{Name: "system:serviceaccount:default:foo", Extra: map[string][]string{ + "authentication.kubernetes.io/cluster-name": {"this"}, + "authentication.kcp.io/scopes": {"cluster:this"}, + }}, + cluster: logicalcluster.Name("this"), + want: true, + }, + { + name: "scoped foreign service account", + info: user.DefaultInfo{Name: "system:serviceaccount:default:foo", Extra: map[string][]string{ + "authentication.kubernetes.io/cluster-name": {"another"}, + "authentication.kcp.io/scopes": {"cluster:this"}, + }}, + cluster: logicalcluster.Name("this"), + want: false, + }, + { + name: "scoped service account to another clusters", + info: user.DefaultInfo{Name: "system:serviceaccount:default:foo", Extra: map[string][]string{ + "authentication.kubernetes.io/cluster-name": {"this"}, + "authentication.kcp.io/scopes": {"cluster:another"}, + }}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsInScope(&tt.info, tt.cluster); got != tt.want { + t.Errorf("IsInScope() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestAppliesToUserWithWarrants(t *testing.T) { + tests := []struct { + name string + user user.Info + sub rbacv1.Subject + want bool + }{ + { + name: "simple matching user without warrants", + user: &user.DefaultInfo{Name: "user-a"}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: true, + }, + { + name: "simple non-matching user without warrants", + user: &user.DefaultInfo{Name: "user-a"}, + sub: rbacv1.Subject{Kind: "User", Name: "user-b"}, + want: false, + }, + { + name: "simple matching user with warrants", + user: &user.DefaultInfo{Name: "user-a", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-b"}`}}}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: true, + }, + { + name: "simple non-matching user with matching warrants", + user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-a"}`}}}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: true, + }, + { + name: "simple non-matching user with non-matching warrants", + user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-b"}`}}}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: false, + }, + { + name: "simple non-matching user with multiple warrants", + user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-b"}`, `{"user":"user-a"}`, `{"user":"user-c"}`}}}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: true, + }, + { + name: "simple non-matching user with nested warrants", + user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-b","extra":{"authorization.kcp.io/warrants":["{\"user\":\"user-a\"}"]}}`}}}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: true, + }, + { + name: "foreign service account", + user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kubernetes.io/cluster-name": {"other"}}}, + sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"}, + want: false, + }, + { + name: "local service account", + user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kubernetes.io/cluster-name": {"this"}}}, + sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"}, + want: true, + }, + { + name: "foreign service account with local warrant", + user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kubernetes.io/cluster-name": {"other"}, WarrantExtraKey: {`{"user":"system:serviceaccount:ns:sa","extra":{"authentication.kubernetes.io/cluster-name":["this"]}}`}}}, + sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"}, + want: true, + }, + { + name: "foreign service account with foreign warrant", + user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kubernetes.io/cluster-name": {"other"}, WarrantExtraKey: {`{"user":"system:serviceaccount:ns:sa","extra":{"authentication.kubernetes.io/cluster-name":["other"]}}`}}}, + sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"}, + want: false, + }, + { + name: "non-cluster-aware service account", + user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa"}, + sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"}, + want: true, + }, + { + name: "non-cluster-aware service account as warrant", + user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"system:serviceaccount:ns:sa"}`}}}, + sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"}, + want: false, + }, + { + name: "in-scope scoped user", + user: &user.DefaultInfo{Name: "user-a", Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:this"}}}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: true, + }, + { + name: "out-of-scope user", + user: &user.DefaultInfo{Name: "user-a", Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:other"}}}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: false, + }, + { + name: "out-of-scope user with warrent", + user: &user.DefaultInfo{Name: "user-a", Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:other"}, WarrantExtraKey: {`{"user":"user-a"}`}}}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: true, + }, + { + name: "out-of-scope warrant", + user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-a","extra":{"authentication.kcp.io/scopes":["cluster:other"]}}`}}}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: false, + }, + { + name: "in-scope warrant", + user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-a","extra":{"authentication.kcp.io/scopes":["cluster:this"]}}`}}}, + sub: rbacv1.Subject{Kind: "User", Name: "user-a"}, + want: true, + }, + { + name: "in-scope service account", + user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:this"}}}, + sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"}, + want: true, + }, + { + name: "out-of-scope service account", + user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:other"}}}, + sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := request.WithCluster(context.Background(), request.Cluster{Name: "this"}) + if got := appliesToUserWithWarrants(ctx, tt.user, tt.sub, "ns"); got != tt.want { + t.Errorf("withWarrants(base) = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/registry/rbac/validation/rule.go b/pkg/registry/rbac/validation/rule.go index 5322c7419feb4..6f6af30edc16a 100644 --- a/pkg/registry/rbac/validation/rule.go +++ b/pkg/registry/rbac/validation/rule.go @@ -184,7 +184,7 @@ func (r *DefaultRuleResolver) VisitRulesFor(ctx context.Context, user user.Info, } else { sourceDescriber := &clusterRoleBindingDescriber{} for _, clusterRoleBinding := range clusterRoleBindings { - subjectIndex, applies := appliesTo(user, clusterRoleBinding.Subjects, "") + subjectIndex, applies := appliesTo(ctx, user, clusterRoleBinding.Subjects, "") if !applies { continue } @@ -213,7 +213,7 @@ func (r *DefaultRuleResolver) VisitRulesFor(ctx context.Context, user user.Info, } else { sourceDescriber := &roleBindingDescriber{} for _, roleBinding := range roleBindings { - subjectIndex, applies := appliesTo(user, roleBinding.Subjects, namespace) + subjectIndex, applies := appliesTo(ctx, user, roleBinding.Subjects, namespace) if !applies { continue } @@ -260,9 +260,9 @@ func (r *DefaultRuleResolver) GetRoleReferenceRules(ctx context.Context, roleRef // appliesTo returns whether any of the bindingSubjects applies to the specified subject, // and if true, the index of the first subject that applies -func appliesTo(user user.Info, bindingSubjects []rbacv1.Subject, namespace string) (int, bool) { +func appliesTo(ctx context.Context, user user.Info, bindingSubjects []rbacv1.Subject, namespace string) (int, bool) { for i, bindingSubject := range bindingSubjects { - if appliesToUser(user, bindingSubject, namespace) { + if appliesToUserWithWarrants(ctx, user, bindingSubject, namespace) { return i, true } } diff --git a/pkg/registry/rbac/validation/rule_test.go b/pkg/registry/rbac/validation/rule_test.go index 459d9c21ae5cd..fe9c963600a23 100644 --- a/pkg/registry/rbac/validation/rule_test.go +++ b/pkg/registry/rbac/validation/rule_test.go @@ -17,6 +17,7 @@ limitations under the License. package validation import ( + "context" "hash/fnv" "io" "reflect" @@ -267,7 +268,7 @@ func TestAppliesTo(t *testing.T) { } for _, tc := range tests { - gotIndex, got := appliesTo(tc.user, tc.subjects, tc.namespace) + gotIndex, got := appliesTo(context.Background(), tc.user, tc.subjects, tc.namespace) if got != tc.appliesTo { t.Errorf("case %q want appliesTo=%t, got appliesTo=%t", tc.testCase, tc.appliesTo, got) }