Skip to content

Commit 3dfd955

Browse files
ntnnntnn
committed
UPSTREAM: <carry>: Add system:cluster:<cluster> group to effective users
Signed-off-by: Nelo-T. Wallus <[email protected]> Signed-off-by: Nelo-T. Wallus <[email protected]>
1 parent 525f42a commit 3dfd955

File tree

2 files changed

+177
-20
lines changed

2 files changed

+177
-20
lines changed

‎pkg/registry/rbac/validation/kcp.go‎

Lines changed: 67 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package validation
33
import (
44
"context"
55
"fmt"
6+
"slices"
67
"strings"
78

89
"github.com/kcp-dev/logicalcluster/v3"
@@ -32,6 +33,10 @@ const (
3233
// are and'ed.
3334
ScopeExtraKey = "authentication.kcp.io/scopes"
3435

36+
// ClusterExtraKey is the key used in a user's "extra" to specify
37+
// the origin cluster of the user.
38+
ClusterExtraKey = "authentication.kcp.io/cluster-name"
39+
3540
// ClusterPrefix is the prefix for cluster scopes.
3641
clusterPrefix = "cluster:"
3742
)
@@ -79,6 +84,35 @@ func withScopesAndWarrants(appliesToUser appliesToUserFunc) appliesToUserFuncCtx
7984
}
8085
}
8186

87+
// EffectiveScopes takes the scopes of a user and returns the
88+
// intersection of all scopes.
89+
func EffectiveScopes(in []string) []string {
90+
if len(in) == 0 {
91+
return in
92+
}
93+
94+
effectiveScopes := strings.Split(in[0], ",")
95+
if len(effectiveScopes) == 0 || len(in) == 1 {
96+
return effectiveScopes
97+
}
98+
99+
for _, further := range in[1:] {
100+
furtherScopes := strings.Split(further, ",")
101+
newEffective := make([]string, 0, len(effectiveScopes))
102+
for _, es := range effectiveScopes {
103+
if slices.Contains(furtherScopes, es) {
104+
newEffective = append(newEffective, es)
105+
}
106+
}
107+
effectiveScopes = newEffective
108+
if len(effectiveScopes) == 0 {
109+
return []string{}
110+
}
111+
}
112+
113+
return effectiveScopes
114+
}
115+
82116
var (
83117
authenticated = &user.DefaultInfo{Name: user.Anonymous, Groups: []string{user.AllAuthenticated}}
84118
unauthenticated = &user.DefaultInfo{Name: user.Anonymous, Groups: []string{user.AllUnauthenticated}}
@@ -96,6 +130,10 @@ func EffectiveUsers(clusterName logicalcluster.Name, u user.Info) []user.Info {
96130
recursive = func(u user.Info) {
97131
if IsInScope(u, clusterName) {
98132
ret = append(ret, u)
133+
// TODO(ntnn): Add system:cluster:<cluster> group?
134+
// Though the user is in scope so it shouldn't need the
135+
// group added but it might be better for consistency?
136+
// Leaving it out for now but could be revisited.
99137
} else {
100138
found := false
101139
for _, g := range u.GetGroups() {
@@ -119,17 +157,29 @@ func EffectiveUsers(clusterName logicalcluster.Name, u user.Info) []user.Info {
119157
if g == user.AllAuthenticated {
120158
rewritten.Groups = append(rewritten.Groups, user.AllAuthenticated)
121159
}
122-
// system:cluster:* groups are used to allow
123-
// controlling binding APIExports between workspaces
124-
// via RBAC
125-
if strings.HasPrefix(g, "system:cluster:") {
126-
rewritten.Groups = append(rewritten.Groups, g)
160+
}
161+
// Add the system:cluster:<cluster> group if the SA has
162+
// an effective scope for the cluster the effective
163+
// users are calculated for.
164+
for _, g := range EffectiveScopes(u.GetExtra()[ScopeExtraKey]) {
165+
if strings.HasPrefix(g, clusterPrefix) {
166+
rewritten.Groups = append(rewritten.Groups, "system:"+g)
127167
}
128168
}
129169
ret = append(ret, rewritten)
130170
}
131171
}
132172

173+
if len(u.GetExtra()[WarrantExtraKey]) > 0 && len(u.GetExtra()[ClusterExtraKey]) > 0 && !IsServiceAccount(u) {
174+
// Users that are not SAs and have a cluster extra key
175+
// cannot have warrants, as they would be coming from
176+
// per-workspace authentication.
177+
// If this happens it is either a misconfiguration from the
178+
// admin or a malicious actor that tries to escalate
179+
// privileges through warrants.
180+
return
181+
}
182+
133183
for _, v := range u.GetExtra()[WarrantExtraKey] {
134184
var w Warrant
135185
if err := json.Unmarshal([]byte(v), &w); err != nil {
@@ -143,9 +193,17 @@ func EffectiveUsers(clusterName logicalcluster.Name, u user.Info) []user.Info {
143193
Extra: w.Extra,
144194
}
145195
if IsServiceAccount(wu) && len(w.Extra[authserviceaccount.ClusterNameKey]) == 0 {
146-
// warrants must be scoped to a cluster
196+
// For SAs warrants must be scoped to a cluster.
147197
continue
148198
}
199+
// Add the system:cluster:<cluster> group if the warrant has
200+
// an effective scope for the cluster the effective
201+
// users are calculated for.
202+
for _, g := range EffectiveScopes(wu.GetExtra()[ScopeExtraKey]) {
203+
if strings.HasPrefix(g, clusterPrefix) {
204+
wu.Groups = append(wu.Groups, "system:"+g)
205+
}
206+
}
149207
recursive(wu)
150208
}
151209
}
@@ -156,11 +214,9 @@ func EffectiveUsers(clusterName logicalcluster.Name, u user.Info) []user.Info {
156214
Name: user.Anonymous,
157215
Groups: []string{user.AllAuthenticated},
158216
}
159-
for _, g := range u.GetGroups() {
160-
// system:cluster:* groups are used to allow controlling
161-
// binding APIExports between workspaces via RBAC
162-
if strings.HasPrefix(g, "system:cluster:") {
163-
authed.Groups = append(authed.Groups, g)
217+
for _, g := range EffectiveScopes(u.GetExtra()[ScopeExtraKey]) {
218+
if strings.HasPrefix(g, clusterPrefix) {
219+
authed.Groups = append(authed.Groups, "system:"+g)
164220
}
165221
}
166222
ret = append(ret, authed)

‎pkg/registry/rbac/validation/kcp_test.go‎

Lines changed: 110 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -172,12 +172,30 @@ func TestAppliesToUserWithWarrantsAndScopes(t *testing.T) {
172172
sub: rbacv1.Subject{Kind: "User", Name: "user-a"},
173173
want: true,
174174
},
175+
{
176+
name: "simple matching user with warrants and from this cluster",
177+
user: &user.DefaultInfo{Name: "user-a", Extra: map[string][]string{
178+
WarrantExtraKey: {`{"user":"user-b"}`},
179+
ClusterExtraKey: {"this"},
180+
}},
181+
sub: rbacv1.Subject{Kind: "User", Name: "user-a"},
182+
want: true, // user is subject
183+
},
175184
{
176185
name: "simple non-matching user with matching warrants",
177186
user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-a"}`}}},
178187
sub: rbacv1.Subject{Kind: "User", Name: "user-a"},
179188
want: true,
180189
},
190+
{
191+
name: "simple non-matching user with matching warrants but with cluster-name",
192+
user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{
193+
WarrantExtraKey: {`{"user":"user-a"}`},
194+
ClusterExtraKey: {"this"},
195+
}},
196+
sub: rbacv1.Subject{Kind: "User", Name: "user-a"},
197+
want: false, // Warrants are ineffective on users with cluster
198+
},
181199
{
182200
name: "simple non-matching user with non-matching warrants",
183201
user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-b"}`}}},
@@ -190,6 +208,15 @@ func TestAppliesToUserWithWarrantsAndScopes(t *testing.T) {
190208
sub: rbacv1.Subject{Kind: "User", Name: "user-a"},
191209
want: true,
192210
},
211+
{
212+
name: "simple non-matching user with multiple warrants and cluster-name",
213+
user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{
214+
WarrantExtraKey: {`{"user":"user-b"}`, `{"user":"user-a"}`, `{"user":"user-c"}`},
215+
ClusterExtraKey: {"this"},
216+
}},
217+
sub: rbacv1.Subject{Kind: "User", Name: "user-a"},
218+
want: false, // Warrants are ineffective on users with cluster
219+
},
193220
{
194221
name: "simple non-matching user with nested warrants",
195222
user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"user-b","extra":{"authorization.kcp.io/warrant":["{\"user\":\"user-a\"}"]}}`}}},
@@ -206,18 +233,18 @@ func TestAppliesToUserWithWarrantsAndScopes(t *testing.T) {
206233
},
207234
{
208235
name: "non-cluster-aware service account with this scope",
209-
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:this"}}},
236+
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{ScopeExtraKey: {"cluster:this"}}},
210237
sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
211238
want: true,
212239
},
213240
{
214241
name: "non-cluster-aware service account with other scope",
215-
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/scopes": {"cluster:other"}}},
242+
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{ScopeExtraKey: {"cluster:other"}}},
216243
sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
217244
want: false,
218245
},
219246
{
220-
name: "non-cluster-aware service account as warrant",
247+
name: "non-cluster-aware service account as warrant", // TODO what is this supposed to test?
221248
user: &user.DefaultInfo{Name: "user-b", Extra: map[string][]string{WarrantExtraKey: {`{"user":"system:serviceaccount:ns:sa"}`}}},
222249
sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
223250
want: false,
@@ -226,37 +253,46 @@ func TestAppliesToUserWithWarrantsAndScopes(t *testing.T) {
226253
// service accounts with cluster
227254
{
228255
name: "local service account",
229-
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/cluster-name": {"this"}}},
256+
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{ClusterExtraKey: {"this"}}},
230257
sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
231258
want: true,
232259
},
233260
{
234261
name: "foreign service account",
235-
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/cluster-name": {"other"}}},
262+
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{ClusterExtraKey: {"other"}}},
236263
sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
237264
want: false,
238265
},
239266
{
240267
name: "foreign service account with local warrant",
241-
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"]}}`}}},
268+
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{
269+
ClusterExtraKey: {"other"},
270+
WarrantExtraKey: {`{"user":"system:serviceaccount:ns:sa","extra":{"authentication.kcp.io/cluster-name":["this"]}}`},
271+
}},
242272
sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
243273
want: true,
244274
},
245275
{
246276
name: "foreign service account with foreign warrant",
247-
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"]}}`}}},
277+
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{
278+
ClusterExtraKey: {"other"},
279+
WarrantExtraKey: {`{"user":"system:serviceaccount:ns:sa","extra":{"authentication.kcp.io/cluster-name":["other"]}}`},
280+
}},
248281
sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
249282
want: false,
250283
},
251284
{
252285
name: "local service account with multiple clusters",
253-
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/cluster-name": {"this", "this"}}},
286+
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{ClusterExtraKey: {"this", "this"}}},
254287
sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
255288
want: false,
256289
},
257290
{
258291
name: "out-of-scope local service account",
259-
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{"authentication.kcp.io/cluster-name": {"this"}, "authentication.kcp.io/scopes": {"cluster:other"}}},
292+
user: &user.DefaultInfo{Name: "system:serviceaccount:ns:sa", Extra: map[string][]string{
293+
ClusterExtraKey: {"this"},
294+
ScopeExtraKey: {"cluster:other"},
295+
}},
260296
sub: rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"},
261297
want: false,
262298
},
@@ -487,3 +523,68 @@ func TestPrefixUser(t *testing.T) {
487523
})
488524
}
489525
}
526+
527+
func TestEffectiveUsers(t *testing.T) {
528+
tests := map[string]struct {
529+
in []string
530+
want []string
531+
}{
532+
"empty": {
533+
in: []string{},
534+
want: []string{},
535+
},
536+
"one scope entry, one cluster": {
537+
in: []string{"cluster:this"},
538+
want: []string{"cluster:this"},
539+
},
540+
"one scope entry, multiple clusters": {
541+
in: []string{"cluster:this,cluster:that"},
542+
want: []string{"cluster:this", "cluster:that"},
543+
},
544+
"multiple scope entries, multiple clusters, empty result": {
545+
in: []string{
546+
"cluster:this,cluster:that",
547+
"cluster:other",
548+
},
549+
want: []string{},
550+
},
551+
"multiple scope entries, multiple clusters, non-empty result": {
552+
in: []string{
553+
"cluster:this,cluster:that",
554+
"cluster:other,cluster:this",
555+
},
556+
want: []string{"cluster:this"},
557+
},
558+
"multiple scopes entries, multiple clusters, multiple others": {
559+
in: []string{
560+
"cluster:this,foo:bar",
561+
"cluster:this,cluster:other,foo:bar",
562+
"cluster:third,foo:bar,foo:baz",
563+
},
564+
want: []string{
565+
"foo:bar",
566+
},
567+
},
568+
"multiple equal scopes entries": {
569+
in: []string{
570+
"cluster:this,cluster:other,foo:bar",
571+
"cluster:this,cluster:other,foo:bar",
572+
"cluster:this,cluster:other,foo:bar",
573+
},
574+
want: []string{
575+
"cluster:this",
576+
"cluster:other",
577+
"foo:bar",
578+
},
579+
},
580+
}
581+
for name, tt := range tests {
582+
t.Run(name, func(t *testing.T) {
583+
t.Parallel()
584+
got := EffectiveScopes(tt.in)
585+
if diff := cmp.Diff(tt.want, got); diff != "" {
586+
t.Errorf("EffectiveScopes() mismatch (-want +got):\n%s", diff)
587+
}
588+
})
589+
}
590+
}

0 commit comments

Comments
 (0)