Skip to content

Commit c4d5d8f

Browse files
lioukbertinatto
liouk
authored and
bertinatto
committed
UPSTREAM: <carry>: selfsubjectaccessreview: grant user:full scope to self-SARs that have user:check-access
Otherwise, the request will inherit any scopes that an access token might have and the scopeAuthorizer will deny the access review if the scopes do not include user:full
1 parent 937cf4c commit c4d5d8f

File tree

4 files changed

+249
-0
lines changed

4 files changed

+249
-0
lines changed

‎pkg/registry/authorization/selfsubjectaccessreview/rest.go

+3
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,9 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
8787
}
8888
}
8989

90+
// when using a scoped token, set the required scopes to perform the self SAR if any is missing
91+
userToCheck = userWithRequiredScopes(userToCheck)
92+
9093
var authorizationAttributes authorizer.AttributesRecord
9194
if selfSAR.Spec.ResourceAttributes != nil {
9295
authorizationAttributes = authorizationutil.ResourceAttributesFrom(userToCheck, *selfSAR.Spec.ResourceAttributes)

‎pkg/registry/authorization/selfsubjectaccessreview/rest_patch.go

+55
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
package selfsubjectaccessreview
2+
3+
import (
4+
"reflect"
5+
"sort"
6+
7+
"k8s.io/apiserver/pkg/authentication/user"
8+
9+
authorizationv1 "github.com/openshift/api/authorization/v1"
10+
authorizationscope "github.com/openshift/apiserver-library-go/pkg/authorization/scope"
11+
)
12+
13+
func userWithRequiredScopes(userToCheck user.Info) user.Info {
14+
userExtra := userToCheck.GetExtra()
15+
if userExtra == nil || !scopesNeedUserFull(userExtra[authorizationv1.ScopesKey]) {
16+
return userToCheck
17+
}
18+
19+
userExtraCopy := make(map[string][]string)
20+
for k, v := range userExtra {
21+
userExtraCopy[k] = v
22+
}
23+
userExtraCopy[authorizationv1.ScopesKey] = append(userExtraCopy[authorizationv1.ScopesKey], authorizationscope.UserFull)
24+
25+
userWithFullScope := &user.DefaultInfo{
26+
Name: userToCheck.GetName(),
27+
UID: userToCheck.GetUID(),
28+
Groups: userToCheck.GetGroups(),
29+
Extra: userExtraCopy,
30+
}
31+
32+
return userWithFullScope
33+
}
34+
35+
// a self-SAR request must be authorized as if it has either the full user's permissions
36+
// or the permissions of the user's role set on the request (if applicable) in order
37+
// to be able to perform the access review
38+
func scopesNeedUserFull(scopes []string) bool {
39+
if len(scopes) == 0 {
40+
return false
41+
}
42+
43+
sort.Strings(scopes)
44+
switch {
45+
case
46+
// all scope slices used here must be sorted
47+
reflect.DeepEqual(scopes, []string{authorizationscope.UserAccessCheck}),
48+
reflect.DeepEqual(scopes, []string{authorizationscope.UserAccessCheck, authorizationscope.UserInfo}),
49+
reflect.DeepEqual(scopes, []string{authorizationscope.UserAccessCheck, authorizationscope.UserListAllProjects}),
50+
reflect.DeepEqual(scopes, []string{authorizationscope.UserAccessCheck, authorizationscope.UserInfo, authorizationscope.UserListAllProjects}):
51+
return true
52+
}
53+
54+
return false
55+
}

‎pkg/registry/authorization/selfsubjectaccessreview/rest_patch_test.go

+55
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
package selfsubjectaccessreview
2+
3+
import (
4+
"testing"
5+
6+
authorizationscope "github.com/openshift/apiserver-library-go/pkg/authorization/scope"
7+
)
8+
9+
func TestScopesNeedUserFull(t *testing.T) {
10+
roleScope := "role:testrole:testns"
11+
tests := []struct {
12+
want bool
13+
scopes []string
14+
}{
15+
{true, []string{authorizationscope.UserAccessCheck}},
16+
{true, []string{authorizationscope.UserInfo, authorizationscope.UserAccessCheck}},
17+
{true, []string{authorizationscope.UserListAllProjects, authorizationscope.UserAccessCheck}},
18+
{true, []string{authorizationscope.UserListAllProjects, authorizationscope.UserInfo, authorizationscope.UserAccessCheck}},
19+
{false, nil},
20+
{false, []string{}},
21+
{false, []string{authorizationscope.UserInfo}},
22+
{false, []string{authorizationscope.UserListAllProjects}},
23+
{false, []string{authorizationscope.UserFull}},
24+
{false, []string{roleScope}},
25+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserFull}},
26+
{false, []string{authorizationscope.UserAccessCheck, roleScope}},
27+
{false, []string{authorizationscope.UserInfo, authorizationscope.UserListAllProjects}},
28+
{false, []string{authorizationscope.UserInfo, authorizationscope.UserFull}},
29+
{false, []string{authorizationscope.UserInfo, roleScope}},
30+
{false, []string{authorizationscope.UserListAllProjects, authorizationscope.UserFull}},
31+
{false, []string{authorizationscope.UserListAllProjects, roleScope}},
32+
{false, []string{authorizationscope.UserFull, roleScope}},
33+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserInfo, authorizationscope.UserFull}},
34+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserInfo, roleScope}},
35+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserListAllProjects, authorizationscope.UserFull}},
36+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserListAllProjects, roleScope}},
37+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserFull, roleScope}},
38+
{false, []string{authorizationscope.UserInfo, authorizationscope.UserListAllProjects, authorizationscope.UserFull}},
39+
{false, []string{authorizationscope.UserInfo, authorizationscope.UserListAllProjects, roleScope}},
40+
{false, []string{authorizationscope.UserInfo, authorizationscope.UserFull, roleScope}},
41+
{false, []string{authorizationscope.UserListAllProjects, authorizationscope.UserFull, roleScope}},
42+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserInfo, authorizationscope.UserListAllProjects, authorizationscope.UserFull}},
43+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserInfo, authorizationscope.UserListAllProjects, roleScope}},
44+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserInfo, authorizationscope.UserFull, roleScope}},
45+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserListAllProjects, authorizationscope.UserFull, roleScope}},
46+
{false, []string{authorizationscope.UserInfo, authorizationscope.UserListAllProjects, authorizationscope.UserFull, roleScope}},
47+
{false, []string{authorizationscope.UserAccessCheck, authorizationscope.UserInfo, authorizationscope.UserListAllProjects, authorizationscope.UserFull, roleScope}},
48+
}
49+
50+
for _, tt := range tests {
51+
if got := scopesNeedUserFull(tt.scopes); got != tt.want {
52+
t.Errorf("scopes %v; got %v; want %v", tt.scopes, got, tt.want)
53+
}
54+
}
55+
}

‎pkg/registry/authorization/selfsubjectaccessreview/rest_test.go

+136
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
package selfsubjectaccessreview
2+
3+
import (
4+
"context"
5+
"reflect"
6+
"testing"
7+
8+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
9+
"k8s.io/apiserver/pkg/authentication/user"
10+
"k8s.io/apiserver/pkg/authorization/authorizer"
11+
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
12+
"k8s.io/apiserver/pkg/registry/rest"
13+
14+
authorizationv1 "github.com/openshift/api/authorization/v1"
15+
authorizationscope "github.com/openshift/apiserver-library-go/pkg/authorization/scope"
16+
17+
authorizationapi "k8s.io/kubernetes/pkg/apis/authorization"
18+
)
19+
20+
type fakeAuthorizer struct {
21+
attrs authorizer.Attributes
22+
}
23+
24+
func (f *fakeAuthorizer) Authorize(ctx context.Context, attrs authorizer.Attributes) (authorizer.Decision, string, error) {
25+
f.attrs = attrs
26+
return authorizer.DecisionNoOpinion, "", nil
27+
}
28+
29+
func TestCreate(t *testing.T) {
30+
userNilExtra := &user.DefaultInfo{}
31+
32+
userNoExtra := &user.DefaultInfo{
33+
Extra: make(map[string][]string),
34+
}
35+
36+
userNoScopes := &user.DefaultInfo{
37+
Extra: map[string][]string{
38+
"extra": {"ex1", "ex2"},
39+
},
40+
}
41+
42+
userWithScopesNoCheckAccess := &user.DefaultInfo{
43+
Extra: map[string][]string{
44+
"extra": {"ex1", "ex2"},
45+
authorizationv1.ScopesKey: {
46+
authorizationscope.UserInfo,
47+
authorizationscope.UserListAllProjects,
48+
},
49+
},
50+
}
51+
52+
userWithScopesWithCheckAccess := &user.DefaultInfo{
53+
Extra: map[string][]string{
54+
"extra": {"ex1", "ex2"},
55+
authorizationv1.ScopesKey: {
56+
authorizationscope.UserAccessCheck,
57+
authorizationscope.UserInfo,
58+
},
59+
},
60+
}
61+
62+
userWithScopeUserFull := &user.DefaultInfo{
63+
Extra: map[string][]string{
64+
"extra": {"ex1", "ex2"},
65+
authorizationv1.ScopesKey: {
66+
authorizationscope.UserAccessCheck,
67+
authorizationscope.UserInfo,
68+
authorizationscope.UserFull,
69+
},
70+
},
71+
}
72+
73+
userWithRoleScope := &user.DefaultInfo{
74+
Extra: map[string][]string{
75+
"extra": {"ex1", "ex2"},
76+
authorizationv1.ScopesKey: {
77+
authorizationscope.UserAccessCheck,
78+
"role:testrole:testns",
79+
},
80+
},
81+
}
82+
83+
testcases := map[string]struct {
84+
user user.Info
85+
expectedUser user.Info
86+
}{
87+
"nil extra": {
88+
user: userNilExtra,
89+
expectedUser: userNilExtra,
90+
},
91+
92+
"no extra": {
93+
user: userNoExtra,
94+
expectedUser: userNoExtra,
95+
},
96+
97+
"no scopes": {
98+
user: userNoScopes,
99+
expectedUser: userNoScopes,
100+
},
101+
102+
"scopes exclude user:check-access": {
103+
user: userWithScopesNoCheckAccess,
104+
expectedUser: userWithScopesNoCheckAccess,
105+
},
106+
107+
"scopes include user:check-access": {
108+
user: userWithScopesWithCheckAccess,
109+
expectedUser: userWithScopeUserFull,
110+
},
111+
112+
"scopes include role scope": {
113+
user: userWithRoleScope,
114+
expectedUser: userWithRoleScope,
115+
},
116+
}
117+
118+
for k, tc := range testcases {
119+
auth := &fakeAuthorizer{}
120+
storage := NewREST(auth)
121+
spec := authorizationapi.SelfSubjectAccessReviewSpec{
122+
NonResourceAttributes: &authorizationapi.NonResourceAttributes{Verb: "get", Path: "/mypath"},
123+
}
124+
125+
ctx := genericapirequest.WithUser(genericapirequest.NewContext(), tc.user)
126+
_, err := storage.Create(ctx, &authorizationapi.SelfSubjectAccessReview{Spec: spec}, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
127+
if err != nil {
128+
t.Errorf("%s: %v", k, err)
129+
continue
130+
}
131+
132+
if !reflect.DeepEqual(auth.attrs.GetUser(), tc.expectedUser) {
133+
t.Errorf("%s: expected\n%#v\ngot\n%#v", k, tc.expectedUser, auth.attrs.GetUser())
134+
}
135+
}
136+
}

0 commit comments

Comments
 (0)