AuthZService: improve authz caching

[zaibkhan]

Mirrors ai-code-review-evaluation#2 for like-for-like benchmarking.

• Base: cache-optimization-baseline
• Head: authz-service-improve-caching-pr

Original PR excerpt:

Test 2

[codoki-pr-intelligence]

Codoki PR Review

Summary: Harden denial cache keys, prevent collisions
What’s good: Denial and permission caches are integrated cleanly into Check/List, reducing DB hits on repeated outcomes and preserving observability via metrics.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity	Issue	Why it matters
High	Correctness — Ambiguous denial cache key can collide across resources	…/rbac/cache.go Cache key concatenation is ambiguous: different (name, parent) pairs can produce the same key when values contain underscores (e.g., name="a_b", parent="c" vs name="a", parent="b_c"). This can cause cross-resource false denies. Use an unambiguous separator or escape the dynamic components.

Showing top 1 issues. Critical: 0, High: 1. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: The denial cache key concatenation is ambiguous and can collide for different (name, parent) pairs; use an unambiguous separator or escaping to avoid cross-resource false denies.
• Testing: Consider adding a test where Name and Parent include underscores or other separators (e.g., name='a_b', parent='c' vs name='a', parent='b_c') to verify the denial cache never cross-applies between distinct resources.
• Documentation: Document the intended precedence and TTL semantics for permDenialCache (deny short-circuit even if allow exists in permCache), since it affects how quickly newly granted permissions become effective.
• Compatibility: newRBACClient was removed; ensure no other internal packages import it. The replacement logic in ProvideAuthZClient/newRemoteRBACClient appears equivalent, but downstream references (if any) will break.
• Performance: Early return on permDenialCache hit will save DB work on repeated denies; ensure the cache TTL stays small to limit staleness.
• Security: Ambiguous cache keys for denies can misattribute authorization decisions across resources if keys collide, potentially denying access incorrectly.
• Open questions: Is a 30s stale deny acceptable for newly granted permissions, or should we re-check on a recent deny or add an invalidation signal on permission changes?

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Service
    Caller->>Service: Check()
    opt build permDenialKey
        alt permDenialCache hit
            Service-->>Caller: Allowed=false
        else cache miss
            Service->>Service: getCachedIdentityPermissions()
            alt cached perms found
                Service->>Service: checkPermission()
                alt allowed
                    Service-->>Caller: Allowed=true
                else not allowed
                    Service->>Service: getIdentityPermissions()
                    Service->>Service: checkPermission()
                    alt not allowed
                        Service->>Service: permDenialCache.Set()
                        Service-->>Caller: Allowed=false
                    else allowed
                        Service-->>Caller: Allowed=true
                    end
                end
            else not found
                Service->>Service: getIdentityPermissions()
                Service->>Service: checkPermission()
                alt not allowed
                    Service->>Service: permDenialCache.Set()
                    Service-->>Caller: Allowed=false
                else allowed
                    Service-->>Caller: Allowed=true
                end
            end
        end
    end

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

⚠️ High: Cache key concatenation is ambiguous: different (name, parent) pairs can produce the same key when values contain underscores (e.g., name="a_b", parent="c" vs name="a", parent="b_c"). This can cause cross-resource false denies. Use an unambiguous separator or escape the dynamic components.

Suggested change

	return namespace + ".perm_" + userUID + "_" + action + "_" + name + "_" + parent
	return namespace + ".perm_" + userUID + "_" + action + "\x1F" + name + "\x1F" + parent