From 380a5d639e389064c07d9c039f30ef927dcad4e4 Mon Sep 17 00:00:00 2001 From: Hitoshi Mitake Date: Sun, 17 Apr 2022 23:06:30 +0900 Subject: [PATCH] server/auth: protect rangePermCache with a RW lock Signed-off-by: Hitoshi Mitake --- server/auth/range_perm_cache.go | 38 +++++++++++--- server/auth/store.go | 34 ++++++------- server/auth/store_test.go | 90 ++++++++++++++++++++++++++++++--- 3 files changed, 131 insertions(+), 31 deletions(-) diff --git a/server/auth/range_perm_cache.go b/server/auth/range_perm_cache.go index 54be7d25f076..3cfd6a1c7611 100644 --- a/server/auth/range_perm_cache.go +++ b/server/auth/range_perm_cache.go @@ -107,7 +107,10 @@ func checkKeyPoint(lg *zap.Logger, cachedPerms *unifiedRangePermissions, key []b func (as *authStore) isRangeOpPermitted(tx backend.ReadTx, userName string, key, rangeEnd []byte, permtyp authpb.Permission_Type) bool { // assumption: tx is Lock()ed - _, ok := as.rangePermCache[userName] + as.rangePermCacheMu.RLock() + defer as.rangePermCacheMu.RUnlock() + + rangePerm, ok := as.rangePermCache[userName] if !ok { perms := getMergedPerms(as.lg, tx, userName) if perms == nil { @@ -118,21 +121,42 @@ func (as *authStore) isRangeOpPermitted(tx backend.ReadTx, userName string, key, return false } as.rangePermCache[userName] = perms + as.lg.Error( + "user doesn't exist", + zap.String("user-name", userName), + ) + return false } if len(rangeEnd) == 0 { - return checkKeyPoint(as.lg, as.rangePermCache[userName], key, permtyp) + return checkKeyPoint(as.lg, rangePerm, key, permtyp) } - return checkKeyInterval(as.lg, as.rangePermCache[userName], key, rangeEnd, permtyp) + return checkKeyInterval(as.lg, rangePerm, key, rangeEnd, permtyp) } -func (as *authStore) clearCachedPerm() { +func (as *authStore) refreshRangePermCache(tx backend.ReadTx) { + // Note that every authentication configuration update calls this method and it invalidates the entire + // rangePermCache and reconstruct it based on information of users and roles stored in the backend. + // This can be a costly operation. + as.rangePermCacheMu.Lock() + defer as.rangePermCacheMu.Unlock() + as.rangePermCache = make(map[string]*unifiedRangePermissions) -} -func (as *authStore) invalidateCachedPerm(userName string) { - delete(as.rangePermCache, userName) + users := getAllUsers(as.lg, tx) + for _, user := range users { + userName := string(user.Name) + perms := getMergedPerms(as.lg, tx, userName) + if perms == nil { + as.lg.Error( + "failed to create a merged permission", + zap.String("user-name", userName), + ) + continue + } + as.rangePermCache[userName] = perms + } } type unifiedRangePermissions struct { diff --git a/server/auth/store.go b/server/auth/store.go index 749eae4785cd..bc3c6a36c2a6 100644 --- a/server/auth/store.go +++ b/server/auth/store.go @@ -208,7 +208,14 @@ type authStore struct { enabled bool enabledMu sync.RWMutex - rangePermCache map[string]*unifiedRangePermissions // username -> unifiedRangePermissions + // rangePermCache needs to be protected by rangePermCacheMu + // rangePermCacheMu needs to be write locked only in initialization phase or configuration changes + // Hot paths like Range(), needs to acquire read lock for improving performance + // + // Note that BatchTx and ReadTx cannot be a mutex for rangePermCache because they are independent resources + // see also: https://github.com/etcd-io/etcd/pull/13920#discussion_r849114855 + rangePermCache map[string]*unifiedRangePermissions // username -> unifiedRangePermissions + rangePermCacheMu sync.RWMutex tokenProvider TokenProvider bcryptCost int // the algorithm cost / strength for hashing auth passwords @@ -243,7 +250,7 @@ func (as *authStore) AuthEnable() error { as.enabled = true as.tokenProvider.enable() - as.rangePermCache = make(map[string]*unifiedRangePermissions) + as.refreshRangePermCache(tx) as.setRevision(getRevision(tx)) @@ -422,6 +429,7 @@ func (as *authStore) UserAdd(r *pb.AuthUserAddRequest) (*pb.AuthUserAddResponse, putUser(as.lg, tx, newUser) as.commitRevision(tx) + as.refreshRangePermCache(tx) as.lg.Info("added a user", zap.String("user-name", r.Name)) return &pb.AuthUserAddResponse{}, nil @@ -445,8 +453,8 @@ func (as *authStore) UserDelete(r *pb.AuthUserDeleteRequest) (*pb.AuthUserDelete delUser(tx, r.Name) as.commitRevision(tx) + as.refreshRangePermCache(tx) - as.invalidateCachedPerm(r.Name) as.tokenProvider.invalidateUser(r.Name) as.lg.Info( @@ -487,8 +495,8 @@ func (as *authStore) UserChangePassword(r *pb.AuthUserChangePasswordRequest) (*p putUser(as.lg, tx, updatedUser) as.commitRevision(tx) + as.refreshRangePermCache(tx) - as.invalidateCachedPerm(r.Name) as.tokenProvider.invalidateUser(r.Name) as.lg.Info( @@ -532,9 +540,8 @@ func (as *authStore) UserGrantRole(r *pb.AuthUserGrantRoleRequest) (*pb.AuthUser putUser(as.lg, tx, user) - as.invalidateCachedPerm(r.User) - as.commitRevision(tx) + as.refreshRangePermCache(tx) as.lg.Info( "granted a role to a user", @@ -610,9 +617,8 @@ func (as *authStore) UserRevokeRole(r *pb.AuthUserRevokeRoleRequest) (*pb.AuthUs putUser(as.lg, tx, updatedUser) - as.invalidateCachedPerm(r.Name) - as.commitRevision(tx) + as.refreshRangePermCache(tx) as.lg.Info( "revoked a role from a user", @@ -678,11 +684,8 @@ func (as *authStore) RoleRevokePermission(r *pb.AuthRoleRevokePermissionRequest) putRole(as.lg, tx, updatedRole) - // TODO(mitake): currently single role update invalidates every cache - // It should be optimized. - as.clearCachedPerm() - as.commitRevision(tx) + as.refreshRangePermCache(tx) as.lg.Info( "revoked a permission on range", @@ -730,10 +733,10 @@ func (as *authStore) RoleDelete(r *pb.AuthRoleDeleteRequest) (*pb.AuthRoleDelete putUser(as.lg, tx, updatedUser) - as.invalidateCachedPerm(string(user.Name)) } as.commitRevision(tx) + as.refreshRangePermCache(tx) as.lg.Info("deleted a role", zap.String("role-name", r.Role)) return &pb.AuthRoleDeleteResponse{}, nil @@ -818,11 +821,8 @@ func (as *authStore) RoleGrantPermission(r *pb.AuthRoleGrantPermissionRequest) ( putRole(as.lg, tx, role) - // TODO(mitake): currently single role update invalidates every cache - // It should be optimized. - as.clearCachedPerm() - as.commitRevision(tx) + as.refreshRangePermCache(tx) as.lg.Info( "granted/updated a permission to a user", diff --git a/server/auth/store_test.go b/server/auth/store_test.go index 4091352e4870..3f3d76767848 100644 --- a/server/auth/store_test.go +++ b/server/auth/store_test.go @@ -27,6 +27,7 @@ import ( "go.etcd.io/etcd/api/v3/authpb" pb "go.etcd.io/etcd/api/v3/etcdserverpb" "go.etcd.io/etcd/api/v3/v3rpc/rpctypes" + "go.etcd.io/etcd/pkg/v3/adt" "go.etcd.io/etcd/server/v3/mvcc/backend" betesting "go.etcd.io/etcd/server/v3/mvcc/backend/testing" @@ -153,7 +154,8 @@ func TestUserAdd(t *testing.T) { as, tearDown := setupAuthStore(t) defer tearDown(t) - ua := &pb.AuthUserAddRequest{Name: "foo", Options: &authpb.UserAddOptions{NoPassword: false}} + const userName = "foo" + ua := &pb.AuthUserAddRequest{Name: userName, Options: &authpb.UserAddOptions{NoPassword: false}} _, err := as.UserAdd(ua) // add an existing user if err == nil { t.Fatalf("expected %v, got %v", ErrUserAlreadyExist, err) @@ -167,6 +169,11 @@ func TestUserAdd(t *testing.T) { if err != ErrUserEmpty { t.Fatal(err) } + + if _, ok := as.rangePermCache[userName]; !ok { + t.Fatalf("user %s should be added but it doesn't exist in rangePermCache", userName) + + } } func TestRecover(t *testing.T) { @@ -216,7 +223,8 @@ func TestUserDelete(t *testing.T) { defer tearDown(t) // delete an existing user - ud := &pb.AuthUserDeleteRequest{Name: "foo"} + const userName = "foo" + ud := &pb.AuthUserDeleteRequest{Name: userName} _, err := as.UserDelete(ud) if err != nil { t.Fatal(err) @@ -230,6 +238,47 @@ func TestUserDelete(t *testing.T) { if err != ErrUserNotFound { t.Fatalf("expected %v, got %v", ErrUserNotFound, err) } + + if _, ok := as.rangePermCache[userName]; ok { + t.Fatalf("user %s should be deleted but it exists in rangePermCache", userName) + + } +} + +func TestUserDeleteAndPermCache(t *testing.T) { + as, tearDown := setupAuthStore(t) + defer tearDown(t) + + // delete an existing user + const deletedUserName = "foo" + ud := &pb.AuthUserDeleteRequest{Name: deletedUserName} + _, err := as.UserDelete(ud) + if err != nil { + t.Fatal(err) + } + + // delete a non-existing user + _, err = as.UserDelete(ud) + if err != ErrUserNotFound { + t.Fatalf("expected %v, got %v", ErrUserNotFound, err) + } + + if _, ok := as.rangePermCache[deletedUserName]; ok { + t.Fatalf("user %s should be deleted but it exists in rangePermCache", deletedUserName) + } + + // add a new user + const newUser = "bar" + ua := &pb.AuthUserAddRequest{Name: newUser, HashedPassword: encodePassword("pwd1"), Options: &authpb.UserAddOptions{NoPassword: false}} + _, err = as.UserAdd(ua) + if err != nil { + t.Fatal(err) + } + + if _, ok := as.rangePermCache[newUser]; !ok { + t.Fatalf("user %s should exist but it doesn't exist in rangePermCache", deletedUserName) + + } } func TestUserChangePassword(t *testing.T) { @@ -524,17 +573,44 @@ func TestUserRevokePermission(t *testing.T) { t.Fatal(err) } - _, err = as.UserGrantRole(&pb.AuthUserGrantRoleRequest{User: "foo", Role: "role-test"}) + const userName = "foo" + _, err = as.UserGrantRole(&pb.AuthUserGrantRoleRequest{User: userName, Role: "role-test"}) if err != nil { t.Fatal(err) } - _, err = as.UserGrantRole(&pb.AuthUserGrantRoleRequest{User: "foo", Role: "role-test-1"}) + _, err = as.UserGrantRole(&pb.AuthUserGrantRoleRequest{User: userName, Role: "role-test-1"}) if err != nil { t.Fatal(err) } - u, err := as.UserGet(&pb.AuthUserGetRequest{Name: "foo"}) + perm := &authpb.Permission{ + PermType: authpb.WRITE, + Key: []byte("WriteKeyBegin"), + RangeEnd: []byte("WriteKeyEnd"), + } + _, err = as.RoleGrantPermission(&pb.AuthRoleGrantPermissionRequest{ + Name: "role-test-1", + Perm: perm, + }) + if err != nil { + t.Fatal(err) + } + + if _, ok := as.rangePermCache[userName]; !ok { + t.Fatalf("User %s should have its entry in rangePermCache", userName) + } + unifiedPerm := as.rangePermCache[userName] + pt1 := adt.NewBytesAffinePoint([]byte("WriteKeyBegin")) + if !unifiedPerm.writePerms.Contains(pt1) { + t.Fatal("rangePermCache should contain WriteKeyBegin") + } + pt2 := adt.NewBytesAffinePoint([]byte("OutOfRange")) + if unifiedPerm.writePerms.Contains(pt2) { + t.Fatal("rangePermCache should not contain OutOfRange") + } + + u, err := as.UserGet(&pb.AuthUserGetRequest{Name: userName}) if err != nil { t.Fatal(err) } @@ -544,12 +620,12 @@ func TestUserRevokePermission(t *testing.T) { t.Fatalf("expected %v, got %v", expected, u.Roles) } - _, err = as.UserRevokeRole(&pb.AuthUserRevokeRoleRequest{Name: "foo", Role: "role-test-1"}) + _, err = as.UserRevokeRole(&pb.AuthUserRevokeRoleRequest{Name: userName, Role: "role-test-1"}) if err != nil { t.Fatal(err) } - u, err = as.UserGet(&pb.AuthUserGetRequest{Name: "foo"}) + u, err = as.UserGet(&pb.AuthUserGetRequest{Name: userName}) if err != nil { t.Fatal(err) }