Merge pull request #176 from ntnn/cluster-groups

Add system:cluster:... groups to effective users

Author: mjudeikis

pkg/registry/rbac/validation/kcp.go

@@ -3,6 +3,7 @@ package validation
 import (
 	"context"
 	"fmt"
+	"slices"
 	"strings"
 
 	"github.com/kcp-dev/logicalcluster/v3"
@@ -32,6 +33,10 @@ const (
 	// are and'ed.
 	ScopeExtraKey = "authentication.kcp.io/scopes"
 
+	// ClusterExtraKey is the key used in a user's "extra" to specify
+	// the origin cluster of the user.
+	ClusterExtraKey = "authentication.kcp.io/cluster-name"
+
 	// ClusterPrefix is the prefix for cluster scopes.
 	clusterPrefix = "cluster:"
 )
@@ -79,6 +84,35 @@ func withScopesAndWarrants(appliesToUser appliesToUserFunc) appliesToUserFuncCtx
 	}
 }
 
+// EffectiveScopes takes the scopes of a user and returns the
+// intersection of all scopes.
+func EffectiveScopes(in []string) []string {
+	if len(in) == 0 {
+		return in
+	}
+
+	effectiveScopes := strings.Split(in[0], ",")
+	if len(effectiveScopes) == 0 || len(in) == 1 {
+		return effectiveScopes
+	}
+
+	for _, further := range in[1:] {
+		furtherScopes := strings.Split(further, ",")
+		newEffective := make([]string, 0, len(effectiveScopes))
+		for _, es := range effectiveScopes {
+			if slices.Contains(furtherScopes, es) {
+				newEffective = append(newEffective, es)
+			}
+		}
+		effectiveScopes = newEffective
+		if len(effectiveScopes) == 0 {
+			return []string{}
+		}
+	}
+
+	return effectiveScopes
+}
+
 var (
 	authenticated   = &user.DefaultInfo{Name: user.Anonymous, Groups: []string{user.AllAuthenticated}}
 	unauthenticated = &user.DefaultInfo{Name: user.Anonymous, Groups: []string{user.AllUnauthenticated}}
@@ -96,6 +130,10 @@ func EffectiveUsers(clusterName logicalcluster.Name, u user.Info) []user.Info {
 	recursive = func(u user.Info) {
 		if IsInScope(u, clusterName) {
 			ret = append(ret, u)
+			// TODO(ntnn): Add system:cluster:<cluster> group?
+			// Though the user is in scope so it shouldn't need the
+			// group added but it might be better for consistency?
+			// Leaving it out for now but could be revisited.
 		} else {
 			found := false
 			for _, g := range u.GetGroups() {
@@ -119,17 +157,29 @@ func EffectiveUsers(clusterName logicalcluster.Name, u user.Info) []user.Info {
 					if g == user.AllAuthenticated {
 						rewritten.Groups = append(rewritten.Groups, user.AllAuthenticated)
 					}
-					// system:cluster:* groups are used to allow
-					// controlling binding APIExports between workspaces
-					// via RBAC
-					if strings.HasPrefix(g, "system:cluster:") {
-						rewritten.Groups = append(rewritten.Groups, g)
+				}
+				// Add the system:cluster:<cluster> group if the SA has
+				// an effective scope for the cluster the effective
+				// users are calculated for.
+				for _, g := range EffectiveScopes(u.GetExtra()[ScopeExtraKey]) {
+					if strings.HasPrefix(g, clusterPrefix) {
+						rewritten.Groups = append(rewritten.Groups, "system:"+g)
 					}
 				}
 				ret = append(ret, rewritten)
 			}
 		}
 
+		if len(u.GetExtra()[WarrantExtraKey]) > 0 && len(u.GetExtra()[ClusterExtraKey]) > 0 && !IsServiceAccount(u) {
+			// Users that are not SAs and have a cluster extra key
+			// cannot have warrants, as they would be coming from
+			// per-workspace authentication.
+			// If this happens it is either a misconfiguration from the
+			// admin or a malicious actor that tries to escalate
+			// privileges through warrants.
+			return
+		}
+
 		for _, v := range u.GetExtra()[WarrantExtraKey] {
 			var w Warrant
 			if err := json.Unmarshal([]byte(v), &w); err != nil {
@@ -143,9 +193,17 @@ func EffectiveUsers(clusterName logicalcluster.Name, u user.Info) []user.Info {
 				Extra:  w.Extra,
 			}
 			if IsServiceAccount(wu) && len(w.Extra[authserviceaccount.ClusterNameKey]) == 0 {
-				// warrants must be scoped to a cluster
+				// For SAs warrants must be scoped to a cluster.
 				continue
 			}
+			// Add the system:cluster:<cluster> group if the warrant has
+			// an effective scope for the cluster the effective
+			// users are calculated for.
+			for _, g := range EffectiveScopes(wu.GetExtra()[ScopeExtraKey]) {
+				if strings.HasPrefix(g, clusterPrefix) {
+					wu.Groups = append(wu.Groups, "system:"+g)
+				}
+			}
 			recursive(wu)
 		}
 	}
@@ -156,11 +214,9 @@ func EffectiveUsers(clusterName logicalcluster.Name, u user.Info) []user.Info {
 			Name:   user.Anonymous,
 			Groups: []string{user.AllAuthenticated},
 		}
-		for _, g := range u.GetGroups() {
-			// system:cluster:* groups are used to allow controlling
-			// binding APIExports between workspaces via RBAC
-			if strings.HasPrefix(g, "system:cluster:") {
-				authed.Groups = append(authed.Groups, g)
+		for _, g := range EffectiveScopes(u.GetExtra()[ScopeExtraKey]) {
+			if strings.HasPrefix(g, clusterPrefix) {
+				authed.Groups = append(authed.Groups, "system:"+g)
 			}
 		}
 		ret = append(ret, authed)

pkg/registry/rbac/validation/kcp_test.go

@@ -176,12 +176,30 @@ func TestAppliesToUserWithWarrantsAndScopes(t *testing.T) {
 			sub:  rbacv1.Subject{Kind: "User", Name: "user-a"},
 			want: true,
 		},
+		{
+			name: "simple matching user with warrants and from this cluster",
+			user: &user.DefaultInfo{Name: "user-a", Extra: map[string][]string{
+				WarrantExtraKey: {`{"user":"user-b"}`},
+				ClusterExtraKey: {"this"},
+			}},
+			sub:  rbacv1.Subject{Kind: "User", Name: "user-a"},
+			want: true, // user is subject
+		},
 		{
 			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 matching warrants but with cluster-name",
+			user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{
+				WarrantExtraKey: {`{"user":"user-a"}`},
+				ClusterExtraKey: {"this"},
+			}},
+			sub:  rbacv1.Subject{Kind: "User", Name: "user-a"},
+			want: false, // Warrants are ineffective on users with cluster
+		},
 		{
 			name: "simple non-matching user with non-matching warrants",
 			user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-b"}`}}},
@@ -194,6 +212,15 @@ func TestAppliesToUserWithWarrantsAndScopes(t *testing.T) {
 			sub:  rbacv1.Subject{Kind: "User", Name: "user-a"},
 			want: true,
 		},
+		{
+			name: "simple non-matching user with multiple warrants and cluster-name",
+			user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{
+				WarrantExtraKey: {`{"user":"user-b"}`, `{"user":"user-a"}`, `{"user":"user-c"}`},
+				ClusterExtraKey: {"this"},
+			}},
+			sub:  rbacv1.Subject{Kind: "User", Name: "user-a"},
+			want: false, // Warrants are ineffective on users with cluster
+		},
 		{
 			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/warrant":["{\"user\":\"user-a\"}"]}}`}}},
@@ -210,13 +237,13 @@ func TestAppliesToUserWithWarrantsAndScopes(t *testing.T) {
 		},
 		{
 			name: "non-cluster-aware service account with this scope",
-			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:this"}}},
+			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{ScopeExtraKey: {"cluster:this"}}},
 			sub:  rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
 			want: true,
 		},
 		{
 			name: "non-cluster-aware service account with other scope",
-			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:other"}}},
+			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{ScopeExtraKey: {"cluster:other"}}},
 			sub:  rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
 			want: false,
 		},
@@ -230,37 +257,46 @@ func TestAppliesToUserWithWarrantsAndScopes(t *testing.T) {
 		// service accounts with cluster
 		{
 			name: "local service account",
-			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/cluster-name": {"this"}}},
+			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{ClusterExtraKey: {"this"}}},
 			sub:  rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
 			want: true,
 		},
 		{
 			name: "foreign service account",
-			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/cluster-name": {"other"}}},
+			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{ClusterExtraKey: {"other"}}},
 			sub:  rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
 			want: false,
 		},
 		{
 			name: "foreign service account with local warrant",
-			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/cluster-name": {"other"}, WarrantExtraKey: {`{"user":"system:serviceaccount:ns:sa","extra":{"authentication.kcp.io/cluster-name":["this"]}}`}}},
+			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{
+				ClusterExtraKey: {"other"},
+				WarrantExtraKey: {`{"user":"system:serviceaccount:ns:sa","extra":{"authentication.kcp.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.kcp.io/cluster-name": {"other"}, WarrantExtraKey: {`{"user":"system:serviceaccount:ns:sa","extra":{"authentication.kcp.io/cluster-name":["other"]}}`}}},
+			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{
+				ClusterExtraKey: {"other"},
+				WarrantExtraKey: {`{"user":"system:serviceaccount:ns:sa","extra":{"authentication.kcp.io/cluster-name":["other"]}}`},
+			}},
 			sub:  rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
 			want: false,
 		},
 		{
 			name: "local service account with multiple clusters",
-			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/cluster-name": {"this", "this"}}},
+			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{ClusterExtraKey: {"this", "this"}}},
 			sub:  rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
 			want: false,
 		},
 		{
 			name: "out-of-scope local service account",
-			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/cluster-name": {"this"}, "authentication.kcp.io/scopes": {"cluster:other"}}},
+			user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{
+				ClusterExtraKey: {"this"},
+				ScopeExtraKey:   {"cluster:other"},
+			}},
 			sub:  rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
 			want: false,
 		},
@@ -496,3 +532,68 @@ func TestPrefixUser(t *testing.T) {
 		})
 	}
 }
+
+func TestEffectiveUsers(t *testing.T) {
+	tests := map[string]struct {
+		in   []string
+		want []string
+	}{
+		"empty": {
+			in:   []string{},
+			want: []string{},
+		},
+		"one scope entry, one cluster": {
+			in:   []string{"cluster:this"},
+			want: []string{"cluster:this"},
+		},
+		"one scope entry, multiple clusters": {
+			in:   []string{"cluster:this,cluster:that"},
+			want: []string{"cluster:this", "cluster:that"},
+		},
+		"multiple scope entries, multiple clusters, empty result": {
+			in: []string{
+				"cluster:this,cluster:that",
+				"cluster:other",
+			},
+			want: []string{},
+		},
+		"multiple scope entries, multiple clusters, non-empty result": {
+			in: []string{
+				"cluster:this,cluster:that",
+				"cluster:other,cluster:this",
+			},
+			want: []string{"cluster:this"},
+		},
+		"multiple scopes entries, multiple clusters, multiple others": {
+			in: []string{
+				"cluster:this,foo:bar",
+				"cluster:this,cluster:other,foo:bar",
+				"cluster:third,foo:bar,foo:baz",
+			},
+			want: []string{
+				"foo:bar",
+			},
+		},
+		"multiple equal scopes entries": {
+			in: []string{
+				"cluster:this,cluster:other,foo:bar",
+				"cluster:this,cluster:other,foo:bar",
+				"cluster:this,cluster:other,foo:bar",
+			},
+			want: []string{
+				"cluster:this",
+				"cluster:other",
+				"foo:bar",
+			},
+		},
+	}
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			got := EffectiveScopes(tt.in)
+			if diff := cmp.Diff(tt.want, got); diff != "" {
+				t.Errorf("EffectiveScopes() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}

staging/src/k8s.io/apiserver/pkg/authentication/serviceaccount/util.go

@@ -52,6 +52,8 @@ const (
 	NodeUIDKey = "authentication.kubernetes.io/node-uid"
 	// ClusterNameKey is the logical cluster name this service-account comes from.
 	ClusterNameKey = "authentication.kcp.io/cluster-name"
+	// ClusterScopeKey additionally sets the scope for the service-account to the cluster it belongs to.
+	ClusterScopeKey = "authentication.kcp.io/scopes"
 )
 
 // MakeUsername generates a username from the given namespace and ServiceAccount name.
@@ -170,6 +172,7 @@ func (sa *ServiceAccountInfo) UserInfo() user.Info {
 		info.Extra = map[string][]string{}
 	}
 	info.Extra[ClusterNameKey] = []string{sa.ClusterName.String()}
+	info.Extra[ClusterScopeKey] = []string{"cluster:" + sa.ClusterName.String()}
 	return info
 }