From de09174a3f3a11621913428c0fb053b2a56bcdac 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 | 49 ++++++++----- server/auth/store.go | 36 +++++----- server/auth/store_test.go | 90 ++++++++++++++++++++++-- server/storage/schema/auth.go | 3 + server/storage/schema/auth_users.go | 24 ++++--- server/storage/schema/auth_users_test.go | 2 +- 6 files changed, 153 insertions(+), 51 deletions(-) diff --git a/server/auth/range_perm_cache.go b/server/auth/range_perm_cache.go index 2ebe5439b..5bd38db84 100644 --- a/server/auth/range_perm_cache.go +++ b/server/auth/range_perm_cache.go @@ -103,34 +103,49 @@ func checkKeyPoint(lg *zap.Logger, cachedPerms *unifiedRangePermissions, key []b return false } -func (as *authStore) isRangeOpPermitted(tx AuthReadTx, userName string, key, rangeEnd []byte, permtyp authpb.Permission_Type) bool { +func (as *authStore) isRangeOpPermitted(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 { + as.lg.Error( + "user doesn't exist", + zap.String("user-name", userName), + ) + return false + } + + if len(rangeEnd) == 0 { + return checkKeyPoint(as.lg, rangePerm, key, permtyp) + } + + return checkKeyInterval(as.lg, rangePerm, key, rangeEnd, permtyp) +} + +func (as *authStore) refreshRangePermCache(tx AuthReadTx) { + // 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) + + users := tx.UnsafeGetAllUsers() + for _, user := range users { + userName := string(user.Name) perms := getMergedPerms(tx, userName) if perms == nil { as.lg.Error( "failed to create a merged permission", zap.String("user-name", userName), ) - return false + continue } as.rangePermCache[userName] = perms } - - if len(rangeEnd) == 0 { - return checkKeyPoint(as.lg, as.rangePermCache[userName], key, permtyp) - } - - return checkKeyInterval(as.lg, as.rangePermCache[userName], key, rangeEnd, permtyp) -} - -func (as *authStore) clearCachedPerm() { - as.rangePermCache = make(map[string]*unifiedRangePermissions) -} - -func (as *authStore) invalidateCachedPerm(userName string) { - delete(as.rangePermCache, userName) } type unifiedRangePermissions struct { diff --git a/server/auth/store.go b/server/auth/store.go index 2d978a011..a27f9fdc2 100644 --- a/server/auth/store.go +++ b/server/auth/store.go @@ -235,7 +235,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 @@ -268,7 +275,7 @@ func (as *authStore) AuthEnable() error { as.enabled = true as.tokenProvider.enable() - as.rangePermCache = make(map[string]*unifiedRangePermissions) + as.refreshRangePermCache(tx) as.setRevision(tx.UnsafeReadAuthRevision()) @@ -437,6 +444,7 @@ func (as *authStore) UserAdd(r *pb.AuthUserAddRequest) (*pb.AuthUserAddResponse, tx.UnsafePutUser(newUser) as.commitRevision(tx) + as.refreshRangePermCache(tx) as.lg.Info("added a user", zap.String("user-name", r.Name)) return &pb.AuthUserAddResponse{}, nil @@ -459,8 +467,8 @@ func (as *authStore) UserDelete(r *pb.AuthUserDeleteRequest) (*pb.AuthUserDelete tx.UnsafeDeleteUser(r.Name) as.commitRevision(tx) + as.refreshRangePermCache(tx) - as.invalidateCachedPerm(r.Name) as.tokenProvider.invalidateUser(r.Name) as.lg.Info( @@ -500,8 +508,8 @@ func (as *authStore) UserChangePassword(r *pb.AuthUserChangePasswordRequest) (*p tx.UnsafePutUser(updatedUser) as.commitRevision(tx) + as.refreshRangePermCache(tx) - as.invalidateCachedPerm(r.Name) as.tokenProvider.invalidateUser(r.Name) as.lg.Info( @@ -545,9 +553,8 @@ func (as *authStore) UserGrantRole(r *pb.AuthUserGrantRoleRequest) (*pb.AuthUser tx.UnsafePutUser(user) - as.invalidateCachedPerm(r.User) - as.commitRevision(tx) + as.refreshRangePermCache(tx) as.lg.Info( "granted a role to a user", @@ -617,9 +624,8 @@ func (as *authStore) UserRevokeRole(r *pb.AuthUserRevokeRoleRequest) (*pb.AuthUs tx.UnsafePutUser(updatedUser) - as.invalidateCachedPerm(r.Name) - as.commitRevision(tx) + as.refreshRangePermCache(tx) as.lg.Info( "revoked a role from a user", @@ -682,11 +688,8 @@ func (as *authStore) RoleRevokePermission(r *pb.AuthRoleRevokePermissionRequest) tx.UnsafePutRole(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", @@ -734,10 +737,10 @@ func (as *authStore) RoleDelete(r *pb.AuthRoleDeleteRequest) (*pb.AuthRoleDelete tx.UnsafePutUser(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 @@ -822,11 +825,8 @@ func (as *authStore) RoleGrantPermission(r *pb.AuthRoleGrantPermissionRequest) ( tx.UnsafePutRole(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", @@ -871,7 +871,7 @@ func (as *authStore) isOpPermitted(userName string, revision uint64, key, rangeE return nil } - if as.isRangeOpPermitted(tx, userName, key, rangeEnd, permTyp) { + if as.isRangeOpPermitted(userName, key, rangeEnd, permTyp) { return nil } diff --git a/server/auth/store_test.go b/server/auth/store_test.go index 192bd72d8..498ca0cd1 100644 --- a/server/auth/store_test.go +++ b/server/auth/store_test.go @@ -29,6 +29,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" "golang.org/x/crypto/bcrypt" "google.golang.org/grpc/metadata" ) @@ -141,7 +142,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) @@ -155,6 +157,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) { @@ -204,7 +211,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) @@ -218,6 +226,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) { @@ -540,17 +589,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) } @@ -559,12 +635,12 @@ func TestUserRevokePermission(t *testing.T) { assert.Equal(t, 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) } diff --git a/server/storage/schema/auth.go b/server/storage/schema/auth.go index aa695bb1d..2375f0666 100644 --- a/server/storage/schema/auth.go +++ b/server/storage/schema/auth.go @@ -111,6 +111,9 @@ func (atx *authBatchTx) Lock() { func (atx *authBatchTx) Unlock() { atx.tx.Unlock() + // Calling Commit() for defensive purpose. If the number of pending writes doesn't exceed batchLimit, + // ReadTx can miss some writes issued by its predecessor BatchTx. + atx.tx.Commit() } func (atx *authReadTx) UnsafeReadAuthEnabled() bool { diff --git a/server/storage/schema/auth_users.go b/server/storage/schema/auth_users.go index c3e7a92ff..2f7229e3d 100644 --- a/server/storage/schema/auth_users.go +++ b/server/storage/schema/auth_users.go @@ -31,13 +31,6 @@ func (atx *authBatchTx) UnsafeGetUser(username string) *authpb.User { return arx.UnsafeGetUser(username) } -func (abe *authBackend) GetAllUsers() []*authpb.User { - tx := abe.BatchTx() - tx.Lock() - defer tx.Unlock() - return tx.UnsafeGetAllUsers() -} - func (atx *authBatchTx) UnsafeGetAllUsers() []*authpb.User { arx := &authReadTx{tx: atx.tx, lg: atx.lg} return arx.UnsafeGetAllUsers() @@ -73,8 +66,23 @@ func (atx *authReadTx) UnsafeGetUser(username string) *authpb.User { return user } +func (abe *authBackend) GetAllUsers() []*authpb.User { + tx := abe.BatchTx() + tx.Lock() + defer tx.Unlock() + return tx.UnsafeGetAllUsers() +} + func (atx *authReadTx) UnsafeGetAllUsers() []*authpb.User { - _, vs := atx.tx.UnsafeRange(AuthUsers, []byte{0}, []byte{0xff}, -1) + var vs [][]byte + err := atx.tx.UnsafeForEach(AuthUsers, func(k []byte, v []byte) error { + vs = append(vs, v) + return nil + }) + if err != nil { + atx.lg.Panic("failed to get users", + zap.Error(err)) + } if len(vs) == 0 { return nil } diff --git a/server/storage/schema/auth_users_test.go b/server/storage/schema/auth_users_test.go index c25aef336..ed0d0f5b8 100644 --- a/server/storage/schema/auth_users_test.go +++ b/server/storage/schema/auth_users_test.go @@ -113,7 +113,7 @@ func TestGetAllUsers(t *testing.T) { be2 := backend.NewDefaultBackend(lg, tmpPath) defer be2.Close() abe2 := NewAuthBackend(lg, be2) - users := abe2.GetAllUsers() + users := abe2.ReadTx().UnsafeGetAllUsers() assert.Equal(t, tc.want, users) })