fix: root check the role list if rootShouldBindRole is true (#39713)

- issue: #39712 Signed-off-by: SimFG <[email protected]>
milvus-io · Feb 13, 2025 · b5b15ff · b5b15ff
1 parent 0345753
commit b5b15ff
Show file tree

Hide file tree

Showing 8 changed files with 162 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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