Skip to content

[Backport release-1.2] fix(api): prevent IDOR in TenantNamespace Get and Watch handlers - #2523

Merged
Andrei Kvapil (kvaps) merged 4 commits into
release-1.2from
backport-2471-to-release-1.2
Apr 30, 2026
Merged

Andrei Kvapil (kvaps) merged 4 commits into
release-1.2from
backport-2471-to-release-1.2

Conversation

@github-actions

Copy link
Copy Markdown

Description

Backport of #2471 to release-1.2.

IvanHunters and others added 4 commits April 28, 2026 14:07
Fixed two IDOR vulnerabilities allowing authenticated users to access
metadata of any tenant namespace without proper authorization.

Changes:
- Added hasAccessToNamespace() for efficient single-namespace access checks
- Get() now verifies access before returning namespace metadata
- Watch() filters events per-namespace with proper authorization
- Returns NotFound (not Forbidden) to prevent tenant enumeration

Performance optimization:
- hasAccessToNamespace() lists RoleBindings only in target namespace
  instead of listing all cluster RoleBindings (order of magnitude faster)
- Watch handler logs authorization errors for security audit

Additional fixes:
- Handle ServiceAccount subjects with empty namespace correctly
- Add klog error logging for failed authorization checks

Signed-off-by: IvanHunters <[email protected]>
(cherry picked from commit 64a3edf)
Added comprehensive unit tests for authorization logic:

- hasAccessToNamespace() tests:
  - User subject access (positive and negative cases)
  - Group subject access
  - ServiceAccount subject access
  - ServiceAccount with empty namespace (defaults to RoleBinding ns)
  - Privileged groups (system:masters, cozystack-cluster-admin)

- Get() handler tests:
  - Returns namespace when user has access
  - Returns NotFound when user lacks access (not Forbidden)
  - Returns NotFound for non-tenant namespaces

All tests verify that authorization correctly enforces RoleBinding-based
access control and prevents IDOR vulnerabilities.

Signed-off-by: IvanHunters <[email protected]>
(cherry picked from commit 5b2501d)
Address review feedback from sircthulhu and CodeRabbit:

1. Return Forbidden instead of NotFound for unauthorized access
   - Get() now returns 403 Forbidden to follow standard K8s RBAC behavior
   - Previously returned 404 NotFound for security-by-obscurity
   - Updated test expectations to match new behavior

2. Propagate field and label selectors in Watch handler
   - Pass opts.FieldSelector and opts.LabelSelector to upstream Watch
   - Add defensive filtering before authorization to prevent RBAC bypass
   - Fixes potential issue with resourceNames restrictions

3. Refactor subject-matching logic to eliminate duplication
   - Extract matchesSubject() helper for Group/User/ServiceAccount checks
   - Remove duplicated code from filterAccessible and hasAccessToNamespace
   - Consolidates ServiceAccount namespace fallback logic

All tests pass successfully.

Signed-off-by: IvanHunters <[email protected]>
(cherry picked from commit 7257b6a)
- Hoist user identity extraction out of the Watch goroutine; reuse a
  cached username and groups map across events instead of re-fetching
  them per event. Watch now returns Unauthorized up front when no user
  is present in the context, rather than failing silently per event.
- Switch the per-event access-check error log to structured klog.ErrorS
  to comply with the project Go style guide.
- Strengthen TestGet_WithAccess to assert the concrete *TenantNamespace
  type plus Name, Kind, and APIVersion, so type or metadata regressions
  fail fast.

Assisted-By: Claude <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>
(cherry picked from commit 61ed7ad)
@dosubot dosubot Bot added size/L This PR changes 100-499 lines, ignoring generated files area/release Issues or PRs related to release tooling (changelog, backport, release pipeline) kind/backport-previous Categorizes issue or PR as requiring a backport to the previous release line kind/bug Categorizes issue or PR as related to a bug security/fixed Fix released labels Apr 28, 2026
@kvaps
Andrei Kvapil (kvaps) merged commit edaf0b0 into release-1.2 Apr 30, 2026
11 checks passed
@kvaps
Andrei Kvapil (kvaps) deleted the backport-2471-to-release-1.2 branch April 30, 2026 13:14
@github-actions

Copy link
Copy Markdown
Author

Backport failed for release-1.2, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-1.2
git worktree add -d .worktree/backport-2523-to-release-1.2 origin/release-1.2
cd .worktree/backport-2523-to-release-1.2
git switch --create backport-2523-to-release-1.2
git cherry-pick -x 911c83f6dc291d5b424d52f080dbc116ce9f7dae e628b57509e42911de639e500c378c2f60988308 4d3cc85a066f4da58e51ad72ff6006d4b90ab144 a98ed6a1e1437bb518fdd7c2140d2e57d7d57b7f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/release Issues or PRs related to release tooling (changelog, backport, release pipeline) kind/backport-previous Categorizes issue or PR as requiring a backport to the previous release line kind/bug Categorizes issue or PR as related to a bug security/fixed Fix released size/L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants