Bug RHOAIENG-46523: Allow colon in RBAC subject names (system:authenticated)

[Gkrumbach07]

Description

Fixes workspace permission grants for groups/users with colons in their names. The UI now correctly allows granting permissions to system:authenticated and other valid RBAC subjects.

Root Cause

Permission validation incorrectly used isValidKubernetesName() which only accepts DNS-1123 labels (lowercase alphanumeric and hyphens). RBAC subjects are NOT Kubernetes resource names - they're identity references that legitimately contain colons.

Changes

• Added isValidRBACSubject() in middleware.go to validate RBAC subjects
  • Accepts colons, dots, uppercase, email addresses (all valid RBAC subjects)
  • Max length 253 chars (DNS subdomain limit)
  • Rejects control characters for security
• Updated AddProjectPermission() in permissions.go to use isValidRBACSubject()
• Updated tests in permissions_test.go to validate system:authenticated and similar subjects

Valid RBAC Subjects Now Accepted

• system:authenticated (all authenticated users)
• system:unauthenticated (anonymous access)
• system:serviceaccount:namespace:name (service accounts)
• user@domain.com (email addresses)

Testing

• Updated unit tests to cover new validation logic
• Tests verify control characters are still rejected for security
• Tests verify maximum length (253 chars) is enforced

Security Considerations

• Control characters (including newlines, tabs, null bytes) are explicitly rejected
• Empty strings are rejected
• Length validation prevents overflow attacks

Files Changed

• components/backend/handlers/middleware.go (line 51-70)
• components/backend/handlers/permissions.go (line 160-163)
• components/backend/handlers/permissions_test.go (line 738-787)

Issue

https://issues.redhat.com/browse/RHOAIENG-46523

[github-actions]

Claude Code Review

Summary

This PR correctly fixes a critical validation bug that prevented granting workspace permissions to valid RBAC subjects containing colons (e.g., system:authenticated). The implementation introduces a new isValidRBACSubject() validation function that properly distinguishes between Kubernetes resource names (DNS-1123 labels) and RBAC subject identifiers (RFC 1123 subdomains).

Overall Assessment: ✅ APPROVED - This is a well-implemented fix with good security practices and comprehensive test coverage.

---

Issues by Severity

🚫 Blocker Issues

None - No blocking issues found.

---

🔴 Critical Issues

None - No critical issues found.

---

🟡 Major Issues

1. Unicode Normalization Missing

Location: components/backend/handlers/middleware.go:58-70

While the function rejects control characters (ASCII 0-31, 127), it doesn't handle potentially dangerous Unicode characters that could cause security issues:

• Zero-width characters (U+200B, U+200C, U+200D, U+FEFF)
• Bi-directional text override characters (U+202E, U+202D)
• Confusable characters that could be used in homograph attacks

Recommendation: Consider adding Unicode normalization and checking for potentially dangerous Unicode sequences, especially if RBAC subjects are displayed in the UI or logs.

Example:

func isValidRBACSubject(name string) bool {
    if len(name) == 0 || len(name) > 253 {
        return false
    }
    
    // Reject control characters and dangerous Unicode
    for _, r := range name {
        // ASCII control characters
        if r < 32 || r == 127 {
            return false
        }
        // Zero-width characters
        if r == '\u200B' || r == '\u200C' || r == '\u200D' || r == '\uFEFF' {
            return false
        }
        // Bi-directional override (homograph attacks)
        if r == '\u202E' || r == '\u202D' {
            return false
        }
    }
    return true
}

Priority: Medium (security hardening, not immediate vulnerability)

---

🔵 Minor Issues

1. Error Message Could Be More Specific

Location: components/backend/handlers/permissions.go:162

c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid subject name format"})

The error message is generic. Consider making it more helpful by indicating what makes a valid RBAC subject:

c.JSON(http.StatusBadRequest, gin.H{
    "error": "Invalid subject name: must be 1-253 characters without control characters"
})

Priority: Low (usability improvement)

---

2. Missing Test Coverage for Edge Cases

Location: components/backend/handlers/permissions_test.go:740-787

Current tests are good but could be enhanced with:

• Empty string test - Explicitly verify "" is rejected (currently implied by binding validation)
• Exactly 253 chars test - Already present ✅
• Exactly 254 chars test - Already present ✅
• Unicode characters test - Test that valid Unicode (e.g., "user-名前") is accepted
• Mixed case test - Already present (User-Name) ✅

Priority: Low (nice-to-have)

---

3. Documentation Comment Could Reference Kubernetes RBAC Spec

Location: components/backend/handlers/middleware.go:51-57

The function comment is excellent, but consider adding a reference to the Kubernetes documentation for maintainability:

// isValidRBACSubject validates RBAC subject identifiers (user/group names in RoleBindings)
// RBAC subjects can contain colons (e.g., system:authenticated, system:serviceaccount:ns:name)
// Unlike Kubernetes resource names, RBAC subjects follow a different format (RFC 1123 subdomain)
// See: https://kubernetes.io/docs/reference/access-authn-authz/rbac/#referring-to-subjects
// Returns false if:
//   - name is empty (prevents empty string injection)
//   - name exceeds 253 characters (max length for DNS subdomain)
//   - name contains control characters (prevents injection attacks)

Priority: Low (documentation improvement)

---

Positive Highlights

✅ Excellent Root Cause Analysis - The PR description clearly explains why the old validation was incorrect and what RBAC subjects actually are.

✅ Security-First Approach - Explicit control character rejection prevents injection attacks (newlines, tabs, null bytes).

✅ Comprehensive Test Coverage - Tests cover all the new valid cases (system:authenticated, service accounts, emails, etc.) and verify security boundaries.

✅ Correct Validation Logic - The 253-character limit matches RFC 1123 subdomain specifications (same as Kubernetes uses internally for RBAC subjects).

✅ No Breaking Changes - The change relaxes validation (allows more valid inputs), so existing valid subjects continue to work.

✅ Follows Project Standards - Implementation follows the error handling patterns from .claude/patterns/error-handling.md:

• Logs validation failures appropriately
• Returns 400 Bad Request for invalid input
• Uses structured error messages

✅ Good Separation of Concerns - Validation logic in middleware.go, usage in permissions.go, comprehensive tests in permissions_test.go.

---

Recommendations

Prioritized Action Items

Before Merge (Optional):

1. ⚠️ Consider Unicode hardening - Add checks for dangerous Unicode characters if RBAC subjects are user-facing (Major issue Outcome: Reduce Refinement Time with agent System #1)
2. ℹ️ Improve error message - Make validation errors more helpful (Minor issue Outcome: Reduce Refinement Time with agent System #1)

Post-Merge (Nice-to-have):

1. 📚 Add Kubernetes RBAC docs reference - Helps future maintainers (Minor issue Epic: Data Source Integration #3)
2. 🧪 Add Unicode test cases - Verify behavior with international characters (Minor issue Epic: RAT Architecture & Design #2)

---

Security Analysis

✅ Authentication Pattern: Not applicable - this is a validation function, not an authentication endpoint

✅ Input Validation: Excellent

• Empty strings rejected ✅
• Length limits enforced (253 chars) ✅
• Control characters explicitly blocked ✅
• No SQL/command injection risk (used in RBAC subjects, not shell commands) ✅

✅ Error Handling: Follows established patterns

• Returns 400 Bad Request for invalid input ✅
• Logs with appropriate context ✅
• Doesn't expose internal implementation details ✅

✅ RBAC Integration: Correctly validates subjects used in RoleBindings

• Follows Kubernetes RBAC subject format ✅
• Allows system groups (system:authenticated) ✅
• Allows service account references ✅

---

Code Quality Checklist

Based on CLAUDE.md standards:

• Error Handling - Follows .claude/patterns/error-handling.md patterns
• Validation - Input validation at entry point (handler)
• Testing - Comprehensive unit tests with positive and negative cases
• Documentation - Clear function comments explaining validation rules
• Security - Control character rejection, length limits
• No Panic - Returns errors, doesn't panic
• Type Safety - Uses string validation, no unsafe type assertions

---

Final Verdict

Status: ✅ APPROVED WITH MINOR SUGGESTIONS

This is a solid fix that correctly addresses the root cause. The implementation is secure, well-tested, and follows project standards. The suggested improvements are optional enhancements that can be addressed in follow-up PRs if desired.

Recommendation: Merge after addressing Unicode hardening (Major issue #1) if RBAC subjects are displayed in user-facing contexts, otherwise merge as-is.

---

Review performed by Claude Code with comprehensive analysis of:

• ✅ CLAUDE.md project standards
• ✅ Backend development patterns (.claude/context/backend-development.md)
• ✅ Security standards (.claude/context/security-standards.md)
• ✅ Error handling patterns (.claude/patterns/error-handling.md)
• ✅ All changed files and test coverage

---

🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

1. CLAUDE.md - Master project instructions, development standards
2. backend-development.md - Go backend, K8s integration patterns
3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
4. security-standards.md - Auth, RBAC, token handling
5. k8s-client-usage.md - User token vs service account patterns
6. error-handling.md - Consistent error patterns
7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.