Skip to content

Commit d9dc93c

Browse files
IevaVasiljevagamab
IevaVasiljeva
and
gamab
authored
AuthZService: improve authz caching (#103633)
* remove the use of client side cache for in-proc authz client Co-authored-by: Gabriel MABILLE <[email protected]> * add a permission denial cache, fetch perms if not in either of the caches Co-authored-by: Gabriel MABILLE <[email protected]> * Clean up tests Co-authored-by: Ieva <[email protected]> * Cache tests Co-authored-by: Ieva <[email protected]> * Add test to list + cache Co-authored-by: Ieva <[email protected]> * Add outdated cache test Co-authored-by: Ieva <[email protected]> * Re-organize metrics Co-authored-by: Ieva <[email protected]> --------- Co-authored-by: Gabriel MABILLE <[email protected]>
1 parent e650fa7 commit d9dc93c

File tree

4 files changed

+240
-33
lines changed

4 files changed

+240
-33
lines changed

‎pkg/services/authz/rbac.go‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,11 @@ func ProvideAuthZClient(
9898
return ctx, nil
9999
}))
100100
authzv1.RegisterAuthzServiceServer(channel, server)
101-
rbacClient := newRBACClient(channel, tracer)
101+
rbacClient := authzlib.NewClient(
102+
channel,
103+
authzlib.WithCacheClientOption(&NoopCache{}),
104+
authzlib.WithTracerClientOption(tracer),
105+
)
102106

103107
if features.IsEnabledGlobally(featuremgmt.FlagZanzana) {
104108
return zanzana.WithShadowClient(rbacClient, zanzanaClient, reg)
@@ -153,18 +157,16 @@ func newRemoteRBACClient(clientCfg *authzClientSettings, tracer trace.Tracer) (a
153157
return nil, fmt.Errorf("failed to create authz client to remote server: %w", err)
154158
}
155159

156-
return newRBACClient(conn, tracer), nil
157-
}
158-
159-
func newRBACClient(conn grpc.ClientConnInterface, tracer trace.Tracer) authlib.AccessClient {
160-
return authzlib.NewClient(
160+
client := authzlib.NewClient(
161161
conn,
162162
authzlib.WithCacheClientOption(cache.NewLocalCache(cache.Config{
163163
Expiry: 30 * time.Second,
164164
CleanupInterval: 2 * time.Minute,
165165
})),
166166
authzlib.WithTracerClientOption(tracer),
167167
)
168+
169+
return client, nil
168170
}
169171

170172
func RegisterRBACAuthZService(
@@ -233,3 +235,17 @@ func (t tokenExhangeRoundTripper) RoundTrip(r *http.Request) (*http.Response, er
233235
r.Header.Set("X-Access-Token", "Bearer "+res.Token)
234236
return t.rt.RoundTrip(r)
235237
}
238+
239+
type NoopCache struct{}
240+
241+
func (lc *NoopCache) Get(ctx context.Context, key string) ([]byte, error) {
242+
return nil, cache.ErrNotFound
243+
}
244+
245+
func (lc *NoopCache) Set(ctx context.Context, key string, data []byte, exp time.Duration) error {
246+
return nil
247+
}
248+
249+
func (lc *NoopCache) Delete(ctx context.Context, key string) error {
250+
return nil
251+
}

‎pkg/services/authz/rbac/cache.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@ func userPermCacheKey(namespace, userUID, action string) string {
2727
return namespace + ".perm_" + userUID + "_" + action
2828
}
2929

30+
func userPermDenialCacheKey(namespace, userUID, action, name, parent string) string {
31+
return namespace + ".perm_" + userUID + "_" + action + "_" + name + "_" + parent
32+
}
33+
3034
func userBasicRoleCacheKey(namespace, userUID string) string {
3135
return namespace + ".basic_role_" + userUID
3236
}

‎pkg/services/authz/rbac/service.go‎

Lines changed: 74 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,12 @@ type Service struct {
5353
sf *singleflight.Group
5454

5555
// Cache for user permissions, user team memberships and user basic roles
56-
idCache *cacheWrap[store.UserIdentifiers]
57-
permCache *cacheWrap[map[string]bool]
58-
teamCache *cacheWrap[[]int64]
59-
basicRoleCache *cacheWrap[store.BasicRole]
60-
folderCache *cacheWrap[folderTree]
56+
idCache *cacheWrap[store.UserIdentifiers]
57+
permCache *cacheWrap[map[string]bool]
58+
permDenialCache *cacheWrap[bool]
59+
teamCache *cacheWrap[[]int64]
60+
basicRoleCache *cacheWrap[store.BasicRole]
61+
folderCache *cacheWrap[folderTree]
6162
}
6263

6364
func NewService(
@@ -81,6 +82,7 @@ func NewService(
8182
mapper: newMapper(),
8283
idCache: newCacheWrap[store.UserIdentifiers](cache, logger, longCacheTTL),
8384
permCache: newCacheWrap[map[string]bool](cache, logger, shortCacheTTL),
85+
permDenialCache: newCacheWrap[bool](cache, logger, shortCacheTTL),
8486
teamCache: newCacheWrap[[]int64](cache, logger, shortCacheTTL),
8587
basicRoleCache: newCacheWrap[store.BasicRole](cache, logger, shortCacheTTL),
8688
folderCache: newCacheWrap[folderTree](cache, logger, shortCacheTTL),
@@ -111,6 +113,29 @@ func (s *Service) Check(ctx context.Context, req *authzv1.CheckRequest) (*authzv
111113
attribute.String("folder", checkReq.ParentFolder),
112114
)
113115

116+
permDenialKey := userPermDenialCacheKey(checkReq.Namespace.Value, checkReq.UserUID, checkReq.Action, checkReq.Name, checkReq.ParentFolder)
117+
if _, ok := s.permDenialCache.Get(ctx, permDenialKey); ok {
118+
s.metrics.permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc()
119+
s.metrics.requestCount.WithLabelValues("false", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc()
120+
return &authzv1.CheckResponse{Allowed: false}, nil
121+
}
122+
123+
cachedPerms, err := s.getCachedIdentityPermissions(ctx, checkReq.Namespace, checkReq.IdentityType, checkReq.UserUID, checkReq.Action)
124+
if err == nil {
125+
allowed, err := s.checkPermission(ctx, cachedPerms, checkReq)
126+
if err != nil {
127+
ctxLogger.Error("could not check permission", "error", err)
128+
s.metrics.requestCount.WithLabelValues("true", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc()
129+
return deny, err
130+
}
131+
if allowed {
132+
s.metrics.permissionCacheUsage.WithLabelValues("true", checkReq.Action).Inc()
133+
s.metrics.requestCount.WithLabelValues("false", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc()
134+
return &authzv1.CheckResponse{Allowed: allowed}, nil
135+
}
136+
}
137+
s.metrics.permissionCacheUsage.WithLabelValues("false", checkReq.Action).Inc()
138+
114139
permissions, err := s.getIdentityPermissions(ctx, checkReq.Namespace, checkReq.IdentityType, checkReq.UserUID, checkReq.Action)
115140
if err != nil {
116141
ctxLogger.Error("could not get user permissions", "subject", req.GetSubject(), "error", err)
@@ -125,6 +150,10 @@ func (s *Service) Check(ctx context.Context, req *authzv1.CheckRequest) (*authzv
125150
return deny, err
126151
}
127152

153+
if !allowed {
154+
s.permDenialCache.Set(ctx, permDenialKey, true)
155+
}
156+
128157
s.metrics.requestCount.WithLabelValues("false", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc()
129158
return &authzv1.CheckResponse{Allowed: allowed}, nil
130159
}
@@ -148,11 +177,18 @@ func (s *Service) List(ctx context.Context, req *authzv1.ListRequest) (*authzv1.
148177
attribute.String("action", listReq.Action),
149178
)
150179

151-
permissions, err := s.getIdentityPermissions(ctx, listReq.Namespace, listReq.IdentityType, listReq.UserUID, listReq.Action)
152-
if err != nil {
153-
ctxLogger.Error("could not get user permissions", "subject", req.GetSubject(), "error", err)
154-
s.metrics.requestCount.WithLabelValues("true", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc()
155-
return nil, err
180+
permissions, err := s.getCachedIdentityPermissions(ctx, listReq.Namespace, listReq.IdentityType, listReq.UserUID, listReq.Action)
181+
if err == nil {
182+
s.metrics.permissionCacheUsage.WithLabelValues("true", listReq.Action).Inc()
183+
} else {
184+
s.metrics.permissionCacheUsage.WithLabelValues("false", listReq.Action).Inc()
185+
186+
permissions, err = s.getIdentityPermissions(ctx, listReq.Namespace, listReq.IdentityType, listReq.UserUID, listReq.Action)
187+
if err != nil {
188+
ctxLogger.Error("could not get user permissions", "subject", req.GetSubject(), "error", err)
189+
s.metrics.requestCount.WithLabelValues("true", "true", req.GetVerb(), req.GetGroup(), req.GetResource()).Inc()
190+
return nil, err
191+
}
156192
}
157193

158194
resp, err := s.listPermission(ctx, permissions, listReq)
@@ -303,6 +339,34 @@ func (s *Service) getIdentityPermissions(ctx context.Context, ns types.Namespace
303339
}
304340
}
305341

342+
func (s *Service) getCachedIdentityPermissions(ctx context.Context, ns types.NamespaceInfo, idType types.IdentityType, userID, action string) (map[string]bool, error) {
343+
ctx, span := s.tracer.Start(ctx, "authz_direct_db.service.getCachedIdentityPermissions")
344+
defer span.End()
345+
346+
switch idType {
347+
case types.TypeAnonymous:
348+
anonPermKey := anonymousPermCacheKey(ns.Value, action)
349+
if cached, ok := s.permCache.Get(ctx, anonPermKey); ok {
350+
return cached, nil
351+
}
352+
return nil, cache.ErrNotFound
353+
case types.TypeRenderService:
354+
return nil, cache.ErrNotFound
355+
case types.TypeUser, types.TypeServiceAccount:
356+
userIdentifiers, err := s.GetUserIdentifiers(ctx, ns, userID)
357+
if err != nil {
358+
return nil, err
359+
}
360+
userPermKey := userPermCacheKey(ns.Value, userIdentifiers.UID, action)
361+
if cached, ok := s.permCache.Get(ctx, userPermKey); ok {
362+
return cached, nil
363+
}
364+
return nil, cache.ErrNotFound
365+
default:
366+
return nil, fmt.Errorf("unsupported identity type: %s", idType)
367+
}
368+
}
369+
306370
func (s *Service) getUserPermissions(ctx context.Context, ns types.NamespaceInfo, userID, action string, actionSets []string) (map[string]bool, error) {
307371
ctx, span := s.tracer.Start(ctx, "authz_direct_db.service.getUserPermissions")
308372
defer span.End()
@@ -313,12 +377,6 @@ func (s *Service) getUserPermissions(ctx context.Context, ns types.NamespaceInfo
313377
}
314378

315379
userPermKey := userPermCacheKey(ns.Value, userIdentifiers.UID, action)
316-
if cached, ok := s.permCache.Get(ctx, userPermKey); ok {
317-
s.metrics.permissionCacheUsage.WithLabelValues("true", action).Inc()
318-
return cached, nil
319-
}
320-
s.metrics.permissionCacheUsage.WithLabelValues("false", action).Inc()
321-
322380
res, err, _ := s.sf.Do(userPermKey+"_getUserPermissions", func() (interface{}, error) {
323381
basicRoles, err := s.getUserBasicRole(ctx, ns, userIdentifiers)
324382
if err != nil {
@@ -363,9 +421,6 @@ func (s *Service) getAnonymousPermissions(ctx context.Context, ns types.Namespac
363421
defer span.End()
364422

365423
anonPermKey := anonymousPermCacheKey(ns.Value, action)
366-
if cached, ok := s.permCache.Get(ctx, anonPermKey); ok {
367-
return cached, nil
368-
}
369424
res, err, _ := s.sf.Do(anonPermKey+"_getAnonymousPermissions", func() (interface{}, error) {
370425
permissions, err := s.permissionStore.GetUserPermissions(ctx, ns, store.PermissionsQuery{Action: action, ActionSets: actionSets, Role: "Viewer"})
371426
if err != nil {

‎pkg/services/authz/rbac/service_test.go‎

Lines changed: 140 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -339,14 +339,6 @@ func TestService_getUserPermissions(t *testing.T) {
339339
}
340340

341341
testCases := []testCase{
342-
{
343-
name: "should return permissions from cache if available",
344-
permissions: []accesscontrol.Permission{
345-
{Action: "dashboards:read", Scope: "dashboards:uid:some_dashboard"},
346-
},
347-
cacheHit: true,
348-
expectedPerms: map[string]bool{"dashboards:uid:some_dashboard": true},
349-
},
350342
{
351343
name: "should return permissions from store if not in cache",
352344
permissions: []accesscontrol.Permission{
@@ -898,6 +890,111 @@ func TestService_Check(t *testing.T) {
898890
})
899891
}
900892

893+
func TestService_CacheCheck(t *testing.T) {
894+
callingService := authn.NewAccessTokenAuthInfo(authn.Claims[authn.AccessTokenClaims]{
895+
Claims: jwt.Claims{
896+
Subject: types.NewTypeID(types.TypeAccessPolicy, "some-service"),
897+
Audience: []string{"authzservice"},
898+
},
899+
Rest: authn.AccessTokenClaims{Namespace: "org-12"},
900+
})
901+
902+
ctx := types.WithAuthInfo(context.Background(), callingService)
903+
userID := &store.UserIdentifiers{UID: "test-uid", ID: 1}
904+
905+
t.Run("Allow based on cached permissions", func(t *testing.T) {
906+
s := setupService()
907+
908+
s.idCache.Set(ctx, userIdentifierCacheKey("org-12", "test-uid"), *userID)
909+
s.permCache.Set(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": true})
910+
911+
resp, err := s.Check(ctx, &authzv1.CheckRequest{
912+
Namespace: "org-12",
913+
Subject: "user:test-uid",
914+
Group: "dashboard.grafana.app",
915+
Resource: "dashboards",
916+
Verb: "get",
917+
Name: "dash1",
918+
})
919+
require.NoError(t, err)
920+
assert.True(t, resp.Allowed)
921+
})
922+
t.Run("Fallback to the database on cache miss", func(t *testing.T) {
923+
s := setupService()
924+
925+
// Populate database permission but not the cache
926+
store := &fakeStore{
927+
userID: userID,
928+
userPermissions: []accesscontrol.Permission{{Action: "dashboards:read", Scope: "dashboards:uid:dash2"}},
929+
}
930+
931+
s.store = store
932+
s.permissionStore = store
933+
934+
s.idCache.Set(ctx, userIdentifierCacheKey("org-12", "test-uid"), *userID)
935+
936+
resp, err := s.Check(ctx, &authzv1.CheckRequest{
937+
Namespace: "org-12",
938+
Subject: "user:test-uid",
939+
Group: "dashboard.grafana.app",
940+
Resource: "dashboards",
941+
Verb: "get",
942+
Name: "dash2",
943+
})
944+
require.NoError(t, err)
945+
assert.True(t, resp.Allowed)
946+
})
947+
t.Run("Fallback to the database on outdated cache", func(t *testing.T) {
948+
s := setupService()
949+
950+
store := &fakeStore{
951+
userID: userID,
952+
userPermissions: []accesscontrol.Permission{{Action: "dashboards:read", Scope: "dashboards:uid:dash2"}},
953+
}
954+
955+
s.store = store
956+
s.permissionStore = store
957+
958+
s.idCache.Set(ctx, userIdentifierCacheKey("org-12", "test-uid"), *userID)
959+
// The cache does not have the permission for dash2 (outdated)
960+
s.permCache.Set(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": true})
961+
962+
resp, err := s.Check(ctx, &authzv1.CheckRequest{
963+
Namespace: "org-12",
964+
Subject: "user:test-uid",
965+
Group: "dashboard.grafana.app",
966+
Resource: "dashboards",
967+
Verb: "get",
968+
Name: "dash2",
969+
})
970+
require.NoError(t, err)
971+
assert.True(t, resp.Allowed)
972+
})
973+
t.Run("Should deny on explicit cache deny entry", func(t *testing.T) {
974+
s := setupService()
975+
976+
s.idCache.Set(ctx, userIdentifierCacheKey("org-12", "test-uid"), *userID)
977+
978+
// Explicitly deny access to the dashboard
979+
s.permDenialCache.Set(ctx, userPermDenialCacheKey("org-12", "test-uid", "dashboards:read", "dash1", "fold1"), true)
980+
981+
// Allow access to the dashboard to prove this is not checked
982+
s.permCache.Set(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": false})
983+
984+
resp, err := s.Check(ctx, &authzv1.CheckRequest{
985+
Namespace: "org-12",
986+
Subject: "user:test-uid",
987+
Group: "dashboard.grafana.app",
988+
Resource: "dashboards",
989+
Verb: "get",
990+
Name: "dash1",
991+
Folder: "fold1",
992+
})
993+
require.NoError(t, err)
994+
assert.False(t, resp.Allowed)
995+
})
996+
}
997+
901998
func TestService_List(t *testing.T) {
902999
callingService := authn.NewAccessTokenAuthInfo(authn.Claims[authn.AccessTokenClaims]{
9031000
Claims: jwt.Claims{
@@ -1191,6 +1288,40 @@ func TestService_List(t *testing.T) {
11911288
})
11921289
}
11931290

1291+
func TestService_CacheList(t *testing.T) {
1292+
callingService := authn.NewAccessTokenAuthInfo(authn.Claims[authn.AccessTokenClaims]{
1293+
Claims: jwt.Claims{
1294+
Subject: types.NewTypeID(types.TypeAccessPolicy, "some-service"),
1295+
Audience: []string{"authzservice"},
1296+
},
1297+
Rest: authn.AccessTokenClaims{Namespace: "org-12"},
1298+
})
1299+
1300+
t.Run("List based on cached permissions", func(t *testing.T) {
1301+
s := setupService()
1302+
ctx := types.WithAuthInfo(context.Background(), callingService)
1303+
userID := &store.UserIdentifiers{UID: "test-uid", ID: 1}
1304+
s.idCache.Set(ctx, userIdentifierCacheKey("org-12", "test-uid"), *userID)
1305+
s.permCache.Set(ctx,
1306+
userPermCacheKey("org-12", "test-uid", "dashboards:read"),
1307+
map[string]bool{"dashboards:uid:dash1": true, "dashboards:uid:dash2": true, "folders:uid:fold1": true},
1308+
)
1309+
s.identityStore = &fakeIdentityStore{}
1310+
1311+
resp, err := s.List(ctx, &authzv1.ListRequest{
1312+
Namespace: "org-12",
1313+
Subject: "user:test-uid",
1314+
Group: "dashboard.grafana.app",
1315+
Resource: "dashboards",
1316+
Verb: "list",
1317+
})
1318+
1319+
require.NoError(t, err)
1320+
require.ElementsMatch(t, resp.Items, []string{"dash1", "dash2"})
1321+
require.ElementsMatch(t, resp.Folders, []string{"fold1"})
1322+
})
1323+
}
1324+
11941325
func setupService() *Service {
11951326
cache := cache.NewLocalCache(cache.Config{Expiry: 5 * time.Minute, CleanupInterval: 5 * time.Minute})
11961327
logger := log.New("authz-rbac-service")
@@ -1202,6 +1333,7 @@ func setupService() *Service {
12021333
metrics: newMetrics(nil),
12031334
idCache: newCacheWrap[store.UserIdentifiers](cache, logger, longCacheTTL),
12041335
permCache: newCacheWrap[map[string]bool](cache, logger, shortCacheTTL),
1336+
permDenialCache: newCacheWrap[bool](cache, logger, shortCacheTTL),
12051337
teamCache: newCacheWrap[[]int64](cache, logger, shortCacheTTL),
12061338
basicRoleCache: newCacheWrap[store.BasicRole](cache, logger, longCacheTTL),
12071339
folderCache: newCacheWrap[folderTree](cache, logger, shortCacheTTL),

0 commit comments

Comments
 (0)