Skip to content

Commit 09edf16

Browse files
myasnikovdaniilmyasnikovdaniil
authored
[Backport release-1.3] fix(api): prevent IDOR in TenantNamespace Get and Watch handlers (#2524)
# Description Backport of #2471 to `release-1.3`.
2 parents 5eb52f0 + 5c68447 commit 09edf16

2 files changed

Lines changed: 612 additions & 19 deletions

File tree

‎pkg/registry/core/tenantnamespace/rest.go‎

Lines changed: 139 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,16 @@ import (
1717
apierrors "k8s.io/apimachinery/pkg/api/errors"
1818
metainternal "k8s.io/apimachinery/pkg/apis/meta/internalversion"
1919
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
20+
"k8s.io/apimachinery/pkg/fields"
21+
"k8s.io/apimachinery/pkg/labels"
2022
"k8s.io/apimachinery/pkg/runtime"
2123
"k8s.io/apimachinery/pkg/runtime/schema"
2224
"k8s.io/apimachinery/pkg/types"
2325
"k8s.io/apimachinery/pkg/util/duration"
2426
"k8s.io/apimachinery/pkg/watch"
2527
"k8s.io/apiserver/pkg/endpoints/request"
2628
"k8s.io/apiserver/pkg/registry/rest"
29+
"k8s.io/klog/v2"
2730
"sigs.k8s.io/controller-runtime/pkg/client"
2831

2932
corev1alpha1 "github.com/cozystack/cozystack/pkg/apis/core/v1alpha1"
@@ -123,8 +126,18 @@ func (r *REST) Get(
123126
return nil, apierrors.NewNotFound(r.gvr.GroupResource(), name)
124127
}
125128

129+
// Check if user has access to this namespace
130+
hasAccess, err := r.hasAccessToNamespace(ctx, name)
131+
if err != nil {
132+
return nil, err
133+
}
134+
if !hasAccess {
135+
// Return Forbidden to follow standard K8s RBAC behavior
136+
return nil, apierrors.NewForbidden(r.gvr.GroupResource(), name, fmt.Errorf("access denied"))
137+
}
138+
126139
ns := &corev1.Namespace{}
127-
err := r.c.Get(ctx, types.NamespacedName{Namespace: "", Name: name}, ns, &client.GetOptions{Raw: opts})
140+
err = r.c.Get(ctx, types.NamespacedName{Namespace: "", Name: name}, ns, &client.GetOptions{Raw: opts})
128141
if err != nil {
129142
return nil, err
130143
}
@@ -143,11 +156,33 @@ func (r *REST) Get(
143156
// -----------------------------------------------------------------------------
144157

145158
func (r *REST) Watch(ctx context.Context, opts *metainternal.ListOptions) (watch.Interface, error) {
159+
// Extract user identity once for the lifetime of the watch — it does not
160+
// change between events and rebuilding it per event is wasteful.
161+
u, ok := request.UserFrom(ctx)
162+
if !ok {
163+
return nil, apierrors.NewUnauthorized("user missing in context")
164+
}
165+
username := u.GetName()
166+
groups := make(map[string]struct{})
167+
for _, group := range u.GetGroups() {
168+
groups[group] = struct{}{}
169+
}
170+
146171
nsList := &corev1.NamespaceList{}
147-
nsWatch, err := r.w.Watch(ctx, nsList, &client.ListOptions{Raw: &metav1.ListOptions{
172+
173+
// Build upstream watch options with field and label selectors
174+
rawOpts := &metav1.ListOptions{
148175
Watch: true,
149176
ResourceVersion: opts.ResourceVersion,
150-
}})
177+
}
178+
if opts.FieldSelector != nil {
179+
rawOpts.FieldSelector = opts.FieldSelector.String()
180+
}
181+
if opts.LabelSelector != nil {
182+
rawOpts.LabelSelector = opts.LabelSelector.String()
183+
}
184+
185+
nsWatch, err := r.w.Watch(ctx, nsList, &client.ListOptions{Raw: rawOpts})
151186
if err != nil {
152187
return nil, err
153188
}
@@ -189,6 +224,30 @@ func (r *REST) Watch(ctx context.Context, opts *metainternal.ListOptions) (watch
189224
continue
190225
}
191226

227+
// Apply defensive filtering for field and label selectors
228+
if opts.FieldSelector != nil {
229+
if !opts.FieldSelector.Matches(fields.Set{"metadata.name": ns.Name}) {
230+
continue
231+
}
232+
}
233+
if opts.LabelSelector != nil {
234+
if !opts.LabelSelector.Matches(labels.Set(ns.Labels)) {
235+
continue
236+
}
237+
}
238+
239+
// Check if user has access to this namespace using the cached
240+
// identity — avoids re-extracting user/groups on every event.
241+
hasAccess, err := r.hasAccessToNamespaceForUser(ctx, ns.Name, username, groups)
242+
if err != nil {
243+
klog.ErrorS(err, "Failed to check access for namespace in watch", "namespace", ns.Name)
244+
continue
245+
}
246+
if !hasAccess {
247+
// User doesn't have access, skip this event
248+
continue
249+
}
250+
192251
out := &corev1alpha1.TenantNamespace{
193252
TypeMeta: metav1.TypeMeta{
194253
APIVersion: corev1alpha1.SchemeGroupVersion.String(),
@@ -309,6 +368,26 @@ func (r *REST) makeList(src *corev1.NamespaceList, allowed []string) *corev1alph
309368
return out
310369
}
311370

371+
// matchesSubject checks if a RoleBinding subject matches the user's identity.
372+
// It handles Group, User, and ServiceAccount subjects with proper namespace fallback.
373+
func matchesSubject(subj rbacv1.Subject, bindingNamespace, username string, groups map[string]struct{}) bool {
374+
switch subj.Kind {
375+
case "Group":
376+
_, ok := groups[subj.Name]
377+
return ok
378+
case "User":
379+
return subj.Name == username
380+
case "ServiceAccount":
381+
saNamespace := subj.Namespace
382+
if saNamespace == "" {
383+
saNamespace = bindingNamespace
384+
}
385+
return username == fmt.Sprintf("system:serviceaccount:%s:%s", saNamespace, subj.Name)
386+
default:
387+
return false
388+
}
389+
}
390+
312391
func (r *REST) filterAccessible(
313392
ctx context.Context,
314393
names []string,
@@ -347,22 +426,9 @@ func (r *REST) filterAccessible(
347426
subjectLoop:
348427
for j := range rbs.Items[i].Subjects {
349428
subj := rbs.Items[i].Subjects[j]
350-
switch subj.Kind {
351-
case "Group":
352-
if _, ok = groups[subj.Name]; ok {
353-
allowedNameSet[rbs.Items[i].Namespace] = struct{}{}
354-
break subjectLoop
355-
}
356-
case "User":
357-
if subj.Name == u.GetName() {
358-
allowedNameSet[rbs.Items[i].Namespace] = struct{}{}
359-
break subjectLoop
360-
}
361-
case "ServiceAccount":
362-
if u.GetName() == fmt.Sprintf("system:serviceaccount:%s:%s", subj.Namespace, subj.Name) {
363-
allowedNameSet[rbs.Items[i].Namespace] = struct{}{}
364-
break subjectLoop
365-
}
429+
if matchesSubject(subj, rbs.Items[i].Namespace, u.GetName(), groups) {
430+
allowedNameSet[rbs.Items[i].Namespace] = struct{}{}
431+
break subjectLoop
366432
}
367433
}
368434
}
@@ -373,6 +439,60 @@ func (r *REST) filterAccessible(
373439
return allowed, nil
374440
}
375441

442+
// hasAccessToNamespace checks if the user has access to a single namespace.
443+
// This is optimized for Get/Watch operations where we check one namespace at a time.
444+
// It lists RoleBindings only in the target namespace instead of all cluster RoleBindings.
445+
func (r *REST) hasAccessToNamespace(
446+
ctx context.Context,
447+
namespace string,
448+
) (bool, error) {
449+
u, ok := request.UserFrom(ctx)
450+
if !ok {
451+
return false, fmt.Errorf("user missing in context")
452+
}
453+
groups := make(map[string]struct{})
454+
for _, group := range u.GetGroups() {
455+
groups[group] = struct{}{}
456+
}
457+
return r.hasAccessToNamespaceForUser(ctx, namespace, u.GetName(), groups)
458+
}
459+
460+
// hasAccessToNamespaceForUser is the inner check that does not re-extract user
461+
// identity from context. Use this in hot paths (e.g. the Watch loop) where the
462+
// caller has already cached the user name and groups.
463+
func (r *REST) hasAccessToNamespaceForUser(
464+
ctx context.Context,
465+
namespace, username string,
466+
groups map[string]struct{},
467+
) (bool, error) {
468+
// Check privileged groups
469+
if _, ok := groups["system:masters"]; ok {
470+
return true, nil
471+
}
472+
if _, ok := groups["cozystack-cluster-admin"]; ok {
473+
return true, nil
474+
}
475+
476+
// List RoleBindings only in the target namespace
477+
rbs := &rbacv1.RoleBindingList{}
478+
err := r.c.List(ctx, rbs, client.InNamespace(namespace))
479+
if err != nil {
480+
return false, fmt.Errorf("failed to list rolebindings in %s: %w", namespace, err)
481+
}
482+
483+
// Check if user is in any RoleBinding subjects
484+
for i := range rbs.Items {
485+
for j := range rbs.Items[i].Subjects {
486+
subj := rbs.Items[i].Subjects[j]
487+
if matchesSubject(subj, rbs.Items[i].Namespace, username, groups) {
488+
return true, nil
489+
}
490+
}
491+
}
492+
493+
return false, nil
494+
}
495+
376496
// -----------------------------------------------------------------------------
377497
// Boiler-plate
378498
// -----------------------------------------------------------------------------

0 commit comments

Comments
 (0)