fix(api): prevent IDOR in TenantNamespace Get and Watch handlers - #2471

Calling hasAccessToNamespace inside the watch loop is inefficient because it re-retrieves the user from the context and re-builds the groups map for every single event. Since the user's identity and groups are static for the duration of the watch request, consider extracting the user info and groups map once before starting the goroutine and passing them to a helper function for the access check.

Good catch. Hoisted username and the groups map out of the watch goroutine in commit 61ed7ad, and added an inner helper hasAccessToNamespaceForUser that takes the cached identity. The watch loop now does only the per-namespace RoleBinding list and subject match per event.

The logic for matching a user against RoleBinding subjects is duplicated between filterAccessible and hasAccessToNamespace. This includes the logic for Group, User, and the recently fixed ServiceAccount namespace handling. Refactoring this into a shared helper method (e.g., userMatchesSubjects) would improve maintainability and ensure consistency across the List, Get, and Watch handlers.

Already addressed in commit 7257b6ae — both filterAccessible and hasAccessToNamespace now go through the shared matchesSubject helper (rest.go:360), which centralizes Group/User/ServiceAccount matching including the empty-namespace fallback.

Use structured logging with klog.ErrorS instead of klog.Errorf to adhere to the project's Go code standards and Kubernetes best practices for structured logging.

Switched to klog.ErrorS in commit 61ed7ad.

Returning 403 error if user doesn't have enough privileges better maps to the kube api rbac strategy.

• Resource exists but user doesn't have access? 403 "user is not allowed to get resources of kind X in namespace Y"
• Resource doesn't exist and user doesn't have access? 403 "user is not allowed to get resource of kind X in namespace Y"
• Resource doesn't exist and user has access? 404 "resource X does not exist"
• Resource exist and user has access? 200

Now the error probably will be 500 since it's a custom error

Addressed in commit 7257b6ae — denied access now returns apierrors.NewForbidden instead of NewNotFound, matching the kube RBAC convention you described.

⚠️ Potential issue | 🟠 Major

#!/bin/bash
# Verify whether tenantnamespace already has dedicated Watch tests elsewhere.
# Expected: security coverage for REST.Watch, especially mixed streams and unauthorized-event filtering.
rg -n --type=go -C3 'func Test.*Watch|\.Watch\(' pkg/registry/core/tenantnamespace

# Get the full Watch implementation from rest.go
head -200 pkg/registry/core/tenantnamespace/rest.go | tail -50

# Check full rest.go around the Watch method
sed -n '150,200p' pkg/registry/core/tenantnamespace/rest.go

# Search entire rest_test.go for any Watch reference (not just test functions)
rg -n --type=go 'Watch' pkg/registry/core/tenantnamespace/rest_test.go

# Confirm the file size and check if there's more content beyond line 501
wc -l pkg/registry/core/tenantnamespace/rest_test.go

# Check for any Watch tests in sibling test files or integration tests
find pkg/registry/core/tenantnamespace -name '*test*.go' -type f | xargs rg -l 'Watch' 2>/dev/null || echo "No Watch tests found"

# Get the full Watch implementation to see if it includes IDOR/authorization checks
sed -n '150,250p' pkg/registry/core/tenantnamespace/rest.go

# Check if Watch calls hasAccessToNamespace or any authorization logic
rg -A30 'func \(r \*REST\) Watch' pkg/registry/core/tenantnamespace/rest.go

# Get the complete Watch implementation by searching for the end of the method
rg -A80 'func \(r \*REST\) Watch' pkg/registry/core/tenantnamespace/rest.go | head -100

# Check TestGet_WithAccess assertions more carefully
sed -n '361,410p' pkg/registry/core/tenantnamespace/rest_test.go

# Check what the actual Get implementation returns
rg -B5 -A25 'func \(r \*REST\) Get' pkg/registry/core/tenantnamespace/rest.go | head -50

# Verify the full Get method to understand what object should be returned
sed -n '100,150p' pkg/registry/core/tenantnamespace/rest.go

Add regression tests for Watch authorization filtering and strengthen Get assertions.

The Watch implementation includes per-event hasAccessToNamespace checks to filter unauthorized namespaces, but this critical security path has zero test coverage. This leaves the event-by-event authorization filtering vulnerable to silent regressions, especially mixed authorized/unauthorized streams and selector propagation.

Additionally, TestGet_WithAccess only verifies obj != nil; it does not assert the object type, Kind, APIVersion, or metadata fields, which would catch object wrapping or metadata mapping regressions.

Verify each finding against the current code and only fix it if needed.

In `@pkg/registry/core/tenantnamespace/rest_test.go` around lines 50 - 501, Add
regression tests covering REST.Watch's per-event authorization filtering by
creating mixed streams of RoleBindings/namespaces so some events should be
filtered and asserting only authorized namespace events are delivered and
label/field selectors are preserved; also strengthen TestGet_WithAccess to
validate the returned object's concrete type and metadata (Kind, APIVersion,
Name/Namespace/Labels) from REST.Get and ensure it matches the tenant namespace
resource shape rather than only checking non-nil. Locate tests around
hasAccessToNamespace, Get, and Watch in the REST test file to implement these
assertions.

Get assertions strengthened in commit 61ed7ad to check the concrete *TenantNamespace type plus Name, Kind, and APIVersion. The dedicated Watch test suite (mixed authorized/unauthorized streams, selector propagation) is tracked as a follow-up — hasAccessToNamespace itself has full coverage and the Watch loop is a thin wrapper around it. Will land separately so this security fix can ship.

@myasnikovdaniil, thanks for the update!

The TestGet_WithAccess strengthening looks good in the current file — the type assertion to *corev1alpha1.TenantNamespace along with the Name, Kind, and APIVersion checks are exactly what was needed there.

For the Watch test suite, the deferral reasoning is sound — hasAccessToNamespace is fully covered and the per-event loop is a straightforward wrapper around it. Just make sure a tracking issue is opened so the follow-up doesn't slip. Would you like me to open one now?