mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-06 12:03:39 +09:00
myasnikovdaniil
a961a90357
## Summary Fixed two IDOR (Insecure Direct Object Reference) vulnerabilities in the TenantNamespace API handlers that allowed authenticated users to access metadata of tenant namespaces without proper authorization checks. ## Changes ### New optimized function: hasAccessToNamespace() - Lists RoleBindings **only in the target namespace** instead of all cluster RoleBindings - Used by Get() and Watch() for single-namespace access checks - Order of magnitude faster than the previous approach ### Get() handler - Uses `hasAccessToNamespace()` for efficient authorization - Returns `NotFound` instead of `Forbidden` to prevent tenant enumeration - Now correctly enforces RoleBinding-based access control ### Watch() handler - Uses `hasAccessToNamespace()` for per-event authorization - Logs authorization errors with klog for security audit - Events for unauthorized namespaces are silently filtered out - Ensures users only receive watch events for namespaces they have access to ### Additional fixes - Fixed ServiceAccount subject handling when namespace is empty (defaults to RoleBinding namespace) - Added proper error logging in Watch handler ## Performance Impact | Handler | Before | After | |---------|--------|-------| | List() | List all RoleBindings × 1 | No change ✅ | | Get() | List all RoleBindings × 1 | List RoleBindings in 1 namespace 🚀 | | Watch() | List all RoleBindings × N events | List RoleBindings in 1 namespace × N events 🚀 | **For Watch with 100 events:** - Before: 100 × (all cluster RoleBindings) = catastrophic - After: 100 × (1-5 RoleBindings in namespace) = fast + cached by controller-runtime ## Security Impact **Before**: Any authenticated user could: - Read metadata (labels, annotations, creation time) of any `tenant-*` namespace via `Get()` - Stream all tenant namespace events via `Watch()`, including creation/modification/deletion **After**: Users can only access tenant namespaces they have explicit RoleBindings for, matching the behavior of the `List()` handler. ## Testing Manually verified: - Users can only `get` their own tenant namespaces - Users can only `watch` events for their own tenant namespaces - Unauthorized access returns `NotFound` (not `Forbidden`) to prevent enumeration - `List()` behavior remains unchanged and consistent with `Get()` and `Watch()` - Authorization errors are logged for security audit ## Checklist - [x] Code follows project style and conventions - [x] Security vulnerability is fully mitigated - [x] Authorization logic is consistent across List/Get/Watch handlers - [x] Performance optimized per feedback from @timofei.larkin - [x] Error logging added for security audit - [ ] Unit tests added (can be done in follow-up PR) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enforced per-namespace access control for Get and Watch; inaccessible namespaces return Forbidden or NotFound as appropriate * Forwarded field/label selectors to upstream watches and added defensive filtering to skip inaccessible events (logged) * Improved ServiceAccount subject namespace fallback and privileged-group bypass * **Tests** * Added security tests covering RoleBinding subjects, groups, privileged bypasses, ServiceAccounts, and access-denied behaviors <!-- end of auto-generated comment: release notes by coderabbit.ai -->