From b5b15ff84052b750fe41c926b2f04329401fe01e Mon Sep 17 00:00:00 2001 From: SimFG Date: Thu, 13 Feb 2025 21:18:13 +0800 Subject: [PATCH] fix: root check the role list if `rootShouldBindRole` is true (#39713) - issue: #39712 Signed-off-by: SimFG --- configs/milvus.yaml | 1 + internal/rootcoord/list_db_task.go | 5 +- internal/rootcoord/list_db_task_test.go | 44 +++++++++++++ internal/rootcoord/root_coord.go | 2 +- internal/rootcoord/root_coord_test.go | 29 +++++++-- internal/rootcoord/show_collection_task.go | 5 +- .../rootcoord/show_collection_task_test.go | 62 +++++++++++++++++++ pkg/util/paramtable/component_param.go | 34 ++++++---- 8 files changed, 162 insertions(+), 20 deletions(-) diff --git a/configs/milvus.yaml b/configs/milvus.yaml index 1899ef8807995..f224189c0f353 100644 --- a/configs/milvus.yaml +++ b/configs/milvus.yaml @@ -832,6 +832,7 @@ common: superUsers: defaultRootPassword: "Milvus" # default password for root user. The maximum length is 72 characters, and double quotes are required. rootShouldBindRole: false # Whether the root user should bind a role when the authorization is enabled. + enablePublicPrivilege: true # Whether to enable public privilege rbac: overrideBuiltInPrivilegeGroups: enabled: false # Whether to override build-in privilege groups diff --git a/internal/rootcoord/list_db_task.go b/internal/rootcoord/list_db_task.go index a4e7891428f34..49438db873be4 100644 --- a/internal/rootcoord/list_db_task.go +++ b/internal/rootcoord/list_db_task.go @@ -51,7 +51,7 @@ func (t *listDatabaseTask) Execute(ctx context.Context) error { } curUser, err := contextutil.GetCurUserFromContext(ctx) // it will fail if the inner node server use the list database API - if err != nil || curUser == util.UserRoot { + if err != nil || (curUser == util.UserRoot && !Params.CommonCfg.RootShouldBindRole.GetAsBool()) { if err != nil { log.Ctx(ctx).Warn("get current user from context failed", zap.Error(err)) } @@ -72,6 +72,9 @@ func (t *listDatabaseTask) Execute(ctx context.Context) error { privilegeDBs.Insert(util.AnyWord) return privilegeDBs, nil } + if role.GetName() == util.RolePublic { + continue + } entities, err := t.core.meta.SelectGrant(ctx, "", &milvuspb.GrantEntity{ Role: role, DbName: util.AnyWord, diff --git a/internal/rootcoord/list_db_task_test.go b/internal/rootcoord/list_db_task_test.go index 868391ae39c6a..e7c1774a3b9ff 100644 --- a/internal/rootcoord/list_db_task_test.go +++ b/internal/rootcoord/list_db_task_test.go @@ -130,6 +130,28 @@ func Test_ListDBTask(t *testing.T) { assert.Equal(t, commonpb.ErrorCode_Success, task.Resp.GetStatus().GetErrorCode()) } + { + // proxy node with root user, root user should bind role + Params.Save(Params.CommonCfg.RootShouldBindRole.Key, "true") + meta.EXPECT().SelectUser(mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return([]*milvuspb.UserResult{ + { + User: &milvuspb.UserEntity{ + Name: "root", + }, + Roles: []*milvuspb.RoleEntity{}, + }, + }, nil).Once() + + ctx := GetContext(context.Background(), "root:root") + task := getTask() + err := task.Execute(ctx) + assert.NoError(t, err) + assert.Equal(t, 0, len(task.Resp.GetDbNames())) + assert.Equal(t, commonpb.ErrorCode_Success, task.Resp.GetStatus().GetErrorCode()) + Params.Reset(Params.CommonCfg.RootShouldBindRole.Key) + } + { // select role fail meta.EXPECT().SelectUser(mock.Anything, mock.Anything, mock.Anything, mock.Anything). @@ -234,6 +256,28 @@ func Test_ListDBTask(t *testing.T) { assert.Equal(t, "fooDB", task.Resp.GetDbNames()[0]) } + { + // normal user and public role + meta.EXPECT().SelectUser(mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return([]*milvuspb.UserResult{ + { + User: &milvuspb.UserEntity{ + Name: "foo", + }, + Roles: []*milvuspb.RoleEntity{ + { + Name: "public", + }, + }, + }, + }, nil).Once() + ctx := GetContext(context.Background(), "foo:root") + task := getTask() + err := task.Execute(ctx) + assert.NoError(t, err) + assert.Equal(t, 0, len(task.Resp.GetDbNames())) + } + { // normal user with any db privilege meta.EXPECT().SelectUser(mock.Anything, mock.Anything, mock.Anything, mock.Anything). diff --git a/internal/rootcoord/root_coord.go b/internal/rootcoord/root_coord.go index a7af848cd5a52..0181c2bc551df 100644 --- a/internal/rootcoord/root_coord.go +++ b/internal/rootcoord/root_coord.go @@ -597,7 +597,7 @@ func (c *Core) initRbac(initCtx context.Context) error { } } - if Params.ProxyCfg.EnablePublicPrivilege.GetAsBool() { + if Params.CommonCfg.EnablePublicPrivilege.GetAsBool() { err = c.initPublicRolePrivilege(initCtx) if err != nil { return err diff --git a/internal/rootcoord/root_coord_test.go b/internal/rootcoord/root_coord_test.go index 1b75db9884552..7e9970aefe976 100644 --- a/internal/rootcoord/root_coord_test.go +++ b/internal/rootcoord/root_coord_test.go @@ -2142,11 +2142,11 @@ func TestCore_InitRBAC(t *testing.T) { meta.EXPECT().OperatePrivilege(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Times(3) Params.Save(Params.RoleCfg.Enabled.Key, "false") - Params.Save(Params.ProxyCfg.EnablePublicPrivilege.Key, "true") + Params.Save(Params.CommonCfg.EnablePublicPrivilege.Key, "true") defer func() { Params.Reset(Params.RoleCfg.Enabled.Key) - Params.Reset(Params.ProxyCfg.EnablePublicPrivilege.Key) + Params.Reset(Params.CommonCfg.EnablePublicPrivilege.Key) }() err := c.initRbac(context.TODO()) @@ -2162,12 +2162,33 @@ func TestCore_InitRBAC(t *testing.T) { Params.Save(Params.RoleCfg.Enabled.Key, "true") Params.Save(Params.RoleCfg.Roles.Key, builtinRoles) - Params.Save(Params.ProxyCfg.EnablePublicPrivilege.Key, "false") + Params.Save(Params.CommonCfg.EnablePublicPrivilege.Key, "false") defer func() { Params.Reset(Params.RoleCfg.Enabled.Key) Params.Reset(Params.RoleCfg.Roles.Key) - Params.Reset(Params.ProxyCfg.EnablePublicPrivilege.Key) + Params.Reset(Params.CommonCfg.EnablePublicPrivilege.Key) + }() + + err := c.initRbac(context.TODO()) + assert.NoError(t, err) + }) + + t.Run("compact proxy setting", func(t *testing.T) { + builtinRoles := `{"db_admin": {"privileges": [{"object_type": "Global", "object_name": "*", "privilege": "CreateCollection", "db_name": "*"}]}}` + meta := mockrootcoord.NewIMetaTable(t) + c := newTestCore(withHealthyCode(), withMeta(meta)) + meta.EXPECT().CreateRole(mock.Anything, mock.Anything, mock.Anything).Return(nil).Times(3) + meta.EXPECT().OperatePrivilege(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() + + Params.Save(Params.RoleCfg.Enabled.Key, "true") + Params.Save(Params.RoleCfg.Roles.Key, builtinRoles) + Params.Save("proxy.enablePublicPrivilege", "false") + + defer func() { + Params.Reset(Params.RoleCfg.Enabled.Key) + Params.Reset(Params.RoleCfg.Roles.Key) + Params.Reset("proxy.enablePublicPrivilege") }() err := c.initRbac(context.TODO()) diff --git a/internal/rootcoord/show_collection_task.go b/internal/rootcoord/show_collection_task.go index 31c268abde57b..92eeaad4a6516 100644 --- a/internal/rootcoord/show_collection_task.go +++ b/internal/rootcoord/show_collection_task.go @@ -58,7 +58,7 @@ func (t *showCollectionTask) Execute(ctx context.Context) error { return privilegeColls, nil } curUser, err := contextutil.GetCurUserFromContext(ctx) - if err != nil || curUser == util.UserRoot { + if err != nil || (curUser == util.UserRoot && !Params.CommonCfg.RootShouldBindRole.GetAsBool()) { if err != nil { log.Ctx(ctx).Warn("get current user from context failed", zap.Error(err)) } @@ -79,6 +79,9 @@ func (t *showCollectionTask) Execute(ctx context.Context) error { privilegeColls.Insert(util.AnyWord) return privilegeColls, nil } + if role.GetName() == util.RolePublic { + continue + } entities, err := t.core.meta.SelectGrant(ctx, "", &milvuspb.GrantEntity{ Role: role, DbName: t.Req.GetDbName(), diff --git a/internal/rootcoord/show_collection_task_test.go b/internal/rootcoord/show_collection_task_test.go index ccc7d6bfebe48..b18792b0adf24 100644 --- a/internal/rootcoord/show_collection_task_test.go +++ b/internal/rootcoord/show_collection_task_test.go @@ -163,6 +163,68 @@ func TestShowCollectionsAuth(t *testing.T) { assert.Equal(t, "foo", task.Rsp.GetCollectionNames()[0]) }) + t.Run("root user", func(t *testing.T) { + Params.Save(Params.CommonCfg.AuthorizationEnabled.Key, "true") + defer Params.Reset(Params.CommonCfg.AuthorizationEnabled.Key) + meta := mockrootcoord.NewIMetaTable(t) + core := newTestCore(withMeta(meta)) + + meta.EXPECT().ListCollections(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*model.Collection{ + { + DBID: 1, + CollectionID: 100, + Name: "foo", + CreateTime: tsoutil.GetCurrentTime(), + }, + }, nil).Once() + + task := &showCollectionTask{ + baseTask: newBaseTask(context.Background(), core), + Req: &milvuspb.ShowCollectionsRequest{DbName: "default"}, + Rsp: &milvuspb.ShowCollectionsResponse{}, + } + + ctx := GetContext(context.Background(), "root:root") + err := task.Execute(ctx) + assert.NoError(t, err) + assert.Equal(t, 1, len(task.Rsp.GetCollectionNames())) + assert.Equal(t, "foo", task.Rsp.GetCollectionNames()[0]) + }) + + t.Run("root user, should bind role", func(t *testing.T) { + Params.Save(Params.CommonCfg.AuthorizationEnabled.Key, "true") + Params.Save(Params.CommonCfg.RootShouldBindRole.Key, "true") + defer Params.Reset(Params.CommonCfg.AuthorizationEnabled.Key) + defer Params.Reset(Params.CommonCfg.RootShouldBindRole.Key) + meta := mockrootcoord.NewIMetaTable(t) + core := newTestCore(withMeta(meta)) + + meta.EXPECT().SelectUser(mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return([]*milvuspb.UserResult{ + { + User: &milvuspb.UserEntity{ + Name: "foo", + }, + Roles: []*milvuspb.RoleEntity{ + { + Name: "public", + }, + }, + }, + }, nil).Once() + + task := &showCollectionTask{ + baseTask: newBaseTask(context.Background(), core), + Req: &milvuspb.ShowCollectionsRequest{DbName: "default"}, + Rsp: &milvuspb.ShowCollectionsResponse{}, + } + + ctx := GetContext(context.Background(), "root:root") + err := task.Execute(ctx) + assert.NoError(t, err) + assert.Equal(t, 0, len(task.Rsp.GetCollectionNames())) + }) + t.Run("fail to select user", func(t *testing.T) { Params.Save(Params.CommonCfg.AuthorizationEnabled.Key, "true") defer Params.Reset(Params.CommonCfg.AuthorizationEnabled.Key) diff --git a/pkg/util/paramtable/component_param.go b/pkg/util/paramtable/component_param.go index 69717f91eeec4..f6029aede7db9 100644 --- a/pkg/util/paramtable/component_param.go +++ b/pkg/util/paramtable/component_param.go @@ -240,10 +240,11 @@ type commonConfig struct { StorageType ParamItem `refreshable:"false"` SimdType ParamItem `refreshable:"false"` - AuthorizationEnabled ParamItem `refreshable:"false"` - SuperUsers ParamItem `refreshable:"true"` - DefaultRootPassword ParamItem `refreshable:"false"` - RootShouldBindRole ParamItem `refreshable:"true"` + AuthorizationEnabled ParamItem `refreshable:"false"` + SuperUsers ParamItem `refreshable:"true"` + DefaultRootPassword ParamItem `refreshable:"false"` + RootShouldBindRole ParamItem `refreshable:"true"` + EnablePublicPrivilege ParamItem `refreshable:"false"` ClusterName ParamItem `refreshable:"false"` @@ -683,6 +684,22 @@ like the old password verification when updating the credential`, } p.RootShouldBindRole.Init(base.mgr) + p.EnablePublicPrivilege = ParamItem{ + Key: "common.security.enablePublicPrivilege", + Formatter: func(originValue string) string { // compatible with old version + fallbackValue := base.Get("proxy.enablePublicPrivilege") + if fallbackValue == "false" { + return "false" + } + return originValue + }, + Version: "2.5.4", + Doc: "Whether to enable public privilege", + DefaultValue: "true", + Export: true, + } + p.EnablePublicPrivilege.Init(base.mgr) + p.ClusterName = ParamItem{ Key: "common.cluster.name", Version: "2.0.0", @@ -1339,7 +1356,6 @@ type proxyConfig struct { MustUsePartitionKey ParamItem `refreshable:"true"` SkipAutoIDCheck ParamItem `refreshable:"true"` SkipPartitionKeyCheck ParamItem `refreshable:"true"` - EnablePublicPrivilege ParamItem `refreshable:"false"` MaxVarCharLength ParamItem `refreshable:"false"` AccessLog AccessLogConfig @@ -1745,14 +1761,6 @@ please adjust in embedded Milvus: false`, } p.SkipPartitionKeyCheck.Init(base.mgr) - p.EnablePublicPrivilege = ParamItem{ - Key: "proxy.enablePublicPrivilege", - Version: "2.4.1", - DefaultValue: "true", - Doc: "switch for whether proxy shall enable public privilege", - } - p.EnablePublicPrivilege.Init(base.mgr) - p.MaxVarCharLength = ParamItem{ Key: "proxy.maxVarCharLength", Version: "2.4.19", // hotfix