Skip to content

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

Closed
github-actions[bot] wants to merge 4 commits into
release-1.2from
backport-2524-to-release-1.2
Closed

github-actions[bot] wants to merge 4 commits into
release-1.2from
backport-2524-to-release-1.2

Conversation

@github-actions

Copy link
Copy Markdown

Description

Backport of #2524 to release-1.2.

IvanHunters and others added 4 commits April 28, 2026 17:42
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)
(cherry picked from commit 82fb799)
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)
(cherry picked from commit 79c23d8)
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)
(cherry picked from commit e1f6692)
- 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)
(cherry picked from commit 5c68447)
@dosubot dosubot Bot added size/L This PR changes 100-499 lines, ignoring generated files area/api Issues or PRs related to the cozystack-api aggregated API server 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 labels Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api Issues or PRs related to the cozystack-api aggregated API server 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 size/L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants