diff --git a/auth/store.go b/auth/store.go index a7102164b..ac5b344c6 100644 --- a/auth/store.go +++ b/auth/store.go @@ -61,6 +61,7 @@ var ( ErrAuthOldRevision = errors.New("auth: revision in header is old") ErrInvalidAuthToken = errors.New("auth: invalid auth token") ErrInvalidAuthOpts = errors.New("auth: invalid auth options") + ErrInvalidAuthMgmt = errors.New("auth: invalid auth management") // BcryptCost is the algorithm cost / strength for hashing auth passwords BcryptCost = bcrypt.DefaultCost @@ -352,6 +353,11 @@ func (as *authStore) UserAdd(r *pb.AuthUserAddRequest) (*pb.AuthUserAddResponse, } func (as *authStore) UserDelete(r *pb.AuthUserDeleteRequest) (*pb.AuthUserDeleteResponse, error) { + if as.enabled && strings.Compare(r.Name, rootUser) == 0 { + plog.Errorf("the user root must not be deleted") + return nil, ErrInvalidAuthMgmt + } + tx := as.be.BatchTx() tx.Lock() defer tx.Unlock() @@ -477,6 +483,11 @@ func (as *authStore) UserList(r *pb.AuthUserListRequest) (*pb.AuthUserListRespon } func (as *authStore) UserRevokeRole(r *pb.AuthUserRevokeRoleRequest) (*pb.AuthUserRevokeRoleResponse, error) { + if as.enabled && strings.Compare(r.Name, rootUser) == 0 && strings.Compare(r.Role, rootRole) == 0 { + plog.Errorf("the role root must not be revoked from the user root") + return nil, ErrInvalidAuthMgmt + } + tx := as.be.BatchTx() tx.Lock() defer tx.Unlock() @@ -579,17 +590,10 @@ func (as *authStore) RoleRevokePermission(r *pb.AuthRoleRevokePermissionRequest) } func (as *authStore) RoleDelete(r *pb.AuthRoleDeleteRequest) (*pb.AuthRoleDeleteResponse, error) { - // TODO(mitake): current scheme of role deletion allows existing users to have the deleted roles - // - // Assume a case like below: - // create a role r1 - // create a user u1 and grant r1 to u1 - // delete r1 - // - // After this sequence, u1 is still granted the role r1. So if admin create a new role with the name r1, - // the new r1 is automatically granted u1. - // In some cases, it would be confusing. So we need to provide an option for deleting the grant relation - // from all users. + if as.enabled && strings.Compare(r.Role, rootRole) == 0 { + plog.Errorf("the role root must not be deleted") + return nil, ErrInvalidAuthMgmt + } tx := as.be.BatchTx() tx.Lock() @@ -602,6 +606,28 @@ func (as *authStore) RoleDelete(r *pb.AuthRoleDeleteRequest) (*pb.AuthRoleDelete delRole(tx, r.Role) + users := getAllUsers(tx) + for _, user := range users { + updatedUser := &authpb.User{ + Name: user.Name, + Password: user.Password, + } + + for _, role := range user.Roles { + if strings.Compare(role, r.Role) != 0 { + updatedUser.Roles = append(updatedUser.Roles, role) + } + } + + if len(updatedUser.Roles) == len(user.Roles) { + continue + } + + putUser(tx, updatedUser) + + as.invalidateCachedPerm(string(user.Name)) + } + as.commitRevision(tx) plog.Noticef("deleted role %s", r.Role) diff --git a/e2e/ctl_v3_auth_test.go b/e2e/ctl_v3_auth_test.go index 5677c2efc..61e982144 100644 --- a/e2e/ctl_v3_auth_test.go +++ b/e2e/ctl_v3_auth_test.go @@ -33,8 +33,10 @@ func TestCtlV3AuthMemberAdd(t *testing.T) { testCtl(t, authTestMemberA func TestCtlV3AuthMemberRemove(t *testing.T) { testCtl(t, authTestMemberRemove, withQuorum(), withNoStrictReconfig()) } -func TestCtlV3AuthMemberUpdate(t *testing.T) { testCtl(t, authTestMemberUpdate) } -func TestCtlV3AuthCertCN(t *testing.T) { testCtl(t, authTestCertCN, withCfg(configClientTLSCertAuth)) } +func TestCtlV3AuthMemberUpdate(t *testing.T) { testCtl(t, authTestMemberUpdate) } +func TestCtlV3AuthCertCN(t *testing.T) { testCtl(t, authTestCertCN, withCfg(configClientTLSCertAuth)) } +func TestCtlV3AuthRevokeWithDelete(t *testing.T) { testCtl(t, authTestRevokeWithDelete) } +func TestCtlV3AuthInvalidMgmt(t *testing.T) { testCtl(t, authTestInvalidMgmt) } func authEnableTest(cx ctlCtx) { if err := authEnable(cx); err != nil { @@ -562,3 +564,52 @@ func authTestCertCN(cx ctlCtx) { cx.t.Fatal(err) } } + +func authTestRevokeWithDelete(cx ctlCtx) { + if err := authEnable(cx); err != nil { + cx.t.Fatal(err) + } + + cx.user, cx.pass = "root", "root" + authSetupTestUser(cx) + + // create a new role + cx.user, cx.pass = "root", "root" + if err := ctlV3Role(cx, []string{"add", "test-role2"}, "Role test-role2 created"); err != nil { + cx.t.Fatal(err) + } + + // grant the new role to the user + if err := ctlV3User(cx, []string{"grant-role", "test-user", "test-role2"}, "Role test-role2 is granted to user test-user", nil); err != nil { + cx.t.Fatal(err) + } + + // check the result + if err := ctlV3User(cx, []string{"get", "test-user"}, "Roles: test-role test-role2", nil); err != nil { + cx.t.Fatal(err) + } + + // delete the role, test-role2 must be revoked from test-user + if err := ctlV3Role(cx, []string{"delete", "test-role2"}, "Role test-role2 deleted"); err != nil { + cx.t.Fatal(err) + } + + // check the result + if err := ctlV3User(cx, []string{"get", "test-user"}, "Roles: test-role", nil); err != nil { + cx.t.Fatal(err) + } +} + +func authTestInvalidMgmt(cx ctlCtx) { + if err := authEnable(cx); err != nil { + cx.t.Fatal(err) + } + + if err := ctlV3Role(cx, []string{"delete", "root"}, "Error: etcdserver: invalid auth management"); err == nil { + cx.t.Fatal("deleting the role root must not be allowed") + } + + if err := ctlV3User(cx, []string{"revoke-role", "root", "root"}, "Error: etcdserver: invalid auth management", []string{}); err == nil { + cx.t.Fatal("revoking the role root from the user root must not be allowed") + } +} diff --git a/etcdserver/api/v3rpc/rpctypes/error.go b/etcdserver/api/v3rpc/rpctypes/error.go index 0ac594051..0907e902c 100644 --- a/etcdserver/api/v3rpc/rpctypes/error.go +++ b/etcdserver/api/v3rpc/rpctypes/error.go @@ -56,6 +56,7 @@ var ( ErrGRPCPermissionNotGranted = grpc.Errorf(codes.FailedPrecondition, "etcdserver: permission is not granted to the role") ErrGRPCAuthNotEnabled = grpc.Errorf(codes.FailedPrecondition, "etcdserver: authentication is not enabled") ErrGRPCInvalidAuthToken = grpc.Errorf(codes.Unauthenticated, "etcdserver: invalid auth token") + ErrGRPCInvalidAuthMgmt = grpc.Errorf(codes.InvalidArgument, "etcdserver: invalid auth management") ErrGRPCNoLeader = grpc.Errorf(codes.Unavailable, "etcdserver: no leader") ErrGRPCNotCapable = grpc.Errorf(codes.Unavailable, "etcdserver: not capable") @@ -102,6 +103,7 @@ var ( grpc.ErrorDesc(ErrGRPCPermissionNotGranted): ErrGRPCPermissionNotGranted, grpc.ErrorDesc(ErrGRPCAuthNotEnabled): ErrGRPCAuthNotEnabled, grpc.ErrorDesc(ErrGRPCInvalidAuthToken): ErrGRPCInvalidAuthToken, + grpc.ErrorDesc(ErrGRPCInvalidAuthMgmt): ErrGRPCInvalidAuthMgmt, grpc.ErrorDesc(ErrGRPCNoLeader): ErrGRPCNoLeader, grpc.ErrorDesc(ErrGRPCNotCapable): ErrGRPCNotCapable, @@ -148,6 +150,7 @@ var ( ErrPermissionNotGranted = Error(ErrGRPCPermissionNotGranted) ErrAuthNotEnabled = Error(ErrGRPCAuthNotEnabled) ErrInvalidAuthToken = Error(ErrGRPCInvalidAuthToken) + ErrInvalidAuthMgmt = Error(ErrGRPCInvalidAuthMgmt) ErrNoLeader = Error(ErrGRPCNoLeader) ErrNotCapable = Error(ErrGRPCNotCapable) diff --git a/etcdserver/api/v3rpc/util.go b/etcdserver/api/v3rpc/util.go index 733bb72e6..cb514a2dc 100644 --- a/etcdserver/api/v3rpc/util.go +++ b/etcdserver/api/v3rpc/util.go @@ -97,6 +97,8 @@ func togRPCError(err error) error { return rpctypes.ErrGRPCAuthNotEnabled case auth.ErrInvalidAuthToken: return rpctypes.ErrGRPCInvalidAuthToken + case auth.ErrInvalidAuthMgmt: + return rpctypes.ErrGRPCInvalidAuthMgmt default: return grpc.Errorf(codes.Unknown, err.Error()) }