Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[3.5] server/auth: protect rangePermCache with a RW lock #14227

Merged
merged 1 commit into from
Jul 20, 2022
Merged

[3.5] server/auth: protect rangePermCache with a RW lock #14227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 32 additions & 18 deletions server/auth/range_perm_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,34 +105,48 @@ func checkKeyPoint(lg *zap.Logger, cachedPerms *unifiedRangePermissions, key []b
return false
}

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]
func (as *authStore) isRangeOpPermitted(userName string, key, rangeEnd []byte, permtyp authpb.Permission_Type) bool {
as.rangePermCacheMu.RLock()
defer as.rangePermCacheMu.RUnlock()

rangePerm, ok := as.rangePermCache[userName]
if !ok {
perms := getMergedPerms(as.lg, tx, userName)
if perms == nil {
as.lg.Error(
"failed to create a merged permission",
zap.String("user-name", userName),
)
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 {
Expand Down
46 changes: 27 additions & 19 deletions server/auth/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))

Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -867,7 +867,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
}

Expand Down Expand Up @@ -929,7 +929,15 @@ func getUser(lg *zap.Logger, tx backend.ReadTx, username string) *authpb.User {
}

func getAllUsers(lg *zap.Logger, tx backend.ReadTx) []*authpb.User {
_, vs := tx.UnsafeRange(buckets.AuthUsers, []byte{0}, []byte{0xff}, -1)
var vs [][]byte
err := tx.UnsafeForEach(buckets.AuthUsers, func(k []byte, v []byte) error {
vs = append(vs, v)
return nil
})
if err != nil {
lg.Panic("failed to get users",
zap.Error(err))
}
if len(vs) == 0 {
return nil
}
Expand Down
90 changes: 83 additions & 7 deletions server/auth/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down