AuthZService: improve authz caching - #2
Conversation
* remove the use of client side cache for in-proc authz client Co-authored-by: Gabriel MABILLE <gabriel.mabille@grafana.com> * add a permission denial cache, fetch perms if not in either of the caches Co-authored-by: Gabriel MABILLE <gabriel.mabille@grafana.com> * Clean up tests Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Cache tests Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Add test to list + cache Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Add outdated cache test Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> * Re-organize metrics Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> --------- Co-authored-by: Gabriel MABILLE <gabriel.mabille@grafana.com>
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bugbot free trial expires on August 11, 2025
Learn more in the Cursor dashboard.
| s.permDenialCache.Set(ctx, userPermDenialCacheKey("org-12", "test-uid", "dashboards:read", "dash1", "fold1"), true) | ||
|
|
||
| // Allow access to the dashboard to prove this is not checked | ||
| s.permCache.Set(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": false}) |
There was a problem hiding this comment.
Bug: Permission Cache Test Setup Error
The TestService_CacheCheck test case "Should deny on explicit cache deny entry" incorrectly sets a permission in s.permCache to false. The permission cache is designed to store only true values for granted permissions; the absence of a key indicates no permission. This behavior is consistent with the getScopeMap function and prevents invalid cache states. The current setup contradicts the test's comment "Allow access to the dashboard to prove this is not checked". To properly demonstrate that the denial cache takes precedence, the permission cache should be set to true (allowing access) while the denial cache explicitly denies it.
Locations (1)
| return &authzv1.CheckResponse{Allowed: allowed}, nil | ||
| } | ||
| } | ||
| s.metrics.permissionCacheUsage.WithLabelValues("false", checkReq.Action).Inc() |
There was a problem hiding this comment.
Bug: Cache Trust Asymmetry Causes Security Vulnerability
The Check operation exhibits asymmetric cache trust logic: cached permission grants are trusted and returned immediately, but cached denials from the same permission cache are ignored, leading to a fresh database lookup. This allows stale cached grants to provide access to revoked resources, posing a security risk. Additionally, this behavior causes metrics to incorrectly report cache misses for these ignored denials.
Locations (1)
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
3 participants
Test 2