fix #16504 (#16523)

Signed-off-by: Nkejiang <ke.jiang@zilliz.com>
Co-authored-by: Nkejiang <ke.jiang@zilliz.com>

configs/milvus.yaml

@@ -287,3 +287,5 @@ common:
 
   storageType: minio
 
+  security:
+    authorizationEnabled: false

internal/distributed/proxy/service.go

@@ -727,7 +727,7 @@ func (s *Server) CreateCredential(ctx context.Context, req *milvuspb.CreateCrede
 	return s.proxy.CreateCredential(ctx, req)
 }
 
-func (s *Server) UpdateCredential(ctx context.Context, req *milvuspb.CreateCredentialRequest) (*commonpb.Status, error) {
+func (s *Server) UpdateCredential(ctx context.Context, req *milvuspb.UpdateCredentialRequest) (*commonpb.Status, error) {
 	return s.proxy.UpdateCredential(ctx, req)
 }
 

internal/distributed/proxy/service_test.go

@@ -752,7 +752,7 @@ func (m *MockProxy) CreateCredential(ctx context.Context, req *milvuspb.CreateCr
 	return nil, nil
 }
 
-func (m *MockProxy) UpdateCredential(ctx context.Context, req *milvuspb.CreateCredentialRequest) (*commonpb.Status, error) {
+func (m *MockProxy) UpdateCredential(ctx context.Context, req *milvuspb.UpdateCredentialRequest) (*commonpb.Status, error) {
 	return nil, nil
 }
 

internal/proto/milvus.proto

@@ -69,7 +69,7 @@ service MilvusService {
 
   // https://wiki.lfaidata.foundation/display/MIL/MEP+27+--+Support+Basic+Authentication
   rpc CreateCredential(CreateCredentialRequest) returns (common.Status) {}
-  rpc UpdateCredential(CreateCredentialRequest) returns (common.Status) {}
+  rpc UpdateCredential(UpdateCredentialRequest) returns (common.Status) {}
   rpc DeleteCredential(DeleteCredentialRequest) returns (common.Status) {}
   rpc ListCredUsers(ListCredUsersRequest) returns (ListCredUsersResponse) {}
 }
@@ -878,6 +878,21 @@ message CreateCredentialRequest {
   uint64 modified_utc_timestamps = 5;
 }
 
+message UpdateCredentialRequest {
+  // Not useful for now
+  common.MsgBase base = 1;
+  // username
+  string username = 2;
+  // old password
+  string oldPassword = 3;
+  // new password
+  string newPassword = 4;
+  // create time
+  uint64 created_utc_timestamps = 5;
+  // modify time
+  uint64 modified_utc_timestamps = 6;
+}
+
 message DeleteCredentialRequest {
   // Not useful for now
   common.MsgBase base = 1;

internal/proxy/authentication_interceptor.go

@@ -68,8 +68,7 @@ func AuthenticationInterceptor(ctx context.Context) (context.Context, error) {
 	// check:
 	//	1. if rpc call from a member (like index/query/data component)
 	// 	2. if rpc call from sdk
-	usernames, _ := globalMetaCache.GetCredUsernames(ctx)
-	if len(usernames) > 0 {
+	if Params.CommonCfg.AuthorizationEnabled {
 		if !validSourceID(ctx, md[strings.ToLower(util.HeaderSourceID)]) &&
 			!validAuth(ctx, md[strings.ToLower(util.HeaderAuthorize)]) {
 			return nil, ErrUnauthenticated()

internal/proxy/authentication_interceptor_test.go

@@ -44,6 +44,7 @@ func TestValidSourceID(t *testing.T) {
 
 func TestAuthenticationInterceptor(t *testing.T) {
 	ctx := context.Background()
+	Params.CommonCfg.AuthorizationEnabled = true // mock authorization is turned on
 	// no metadata
 	_, err := AuthenticationInterceptor(ctx)
 	assert.NotNil(t, err)

internal/proxy/impl.go

@@ -4098,10 +4098,17 @@ func (node *Proxy) CreateCredential(ctx context.Context, req *milvuspb.CreateCre
 	return result, err
 }
 
-func (node *Proxy) UpdateCredential(ctx context.Context, req *milvuspb.CreateCredentialRequest) (*commonpb.Status, error) {
+func (node *Proxy) UpdateCredential(ctx context.Context, req *milvuspb.UpdateCredentialRequest) (*commonpb.Status, error) {
 	log.Debug("UpdateCredential", zap.String("role", typeutil.RootCoordRole), zap.String("username", req.Username))
-	// validate params
-	rawPassword, err := crypto.Base64Decode(req.Password)
+	rawOldPassword, err := crypto.Base64Decode(req.OldPassword)
+	if err != nil {
+		log.Error("decode old password fail", zap.String("username", req.Username), zap.Error(err))
+		return &commonpb.Status{
+			ErrorCode: commonpb.ErrorCode_UpdateCredentialFailure,
+			Reason:    "decode old password fail when updating:" + req.Username,
+		}, nil
+	}
+	rawNewPassword, err := crypto.Base64Decode(req.NewPassword)
 	if err != nil {
 		log.Error("decode password fail", zap.String("username", req.Username), zap.Error(err))
 		return &commonpb.Status{
@@ -4109,14 +4116,31 @@ func (node *Proxy) UpdateCredential(ctx context.Context, req *milvuspb.CreateCre
 			Reason:    "decode password fail when updating:" + req.Username,
 		}, nil
 	}
-	if err = ValidatePassword(rawPassword); err != nil {
+	// valid new password
+	if err = ValidatePassword(rawNewPassword); err != nil {
 		log.Error("illegal password", zap.String("username", req.Username), zap.Error(err))
 		return &commonpb.Status{
 			ErrorCode: commonpb.ErrorCode_IllegalArgument,
 			Reason:    err.Error(),
 		}, nil
 	}
-	encryptedPassword, err := crypto.PasswordEncrypt(rawPassword)
+	// check old password is correct
+	oldCredInfo, err := globalMetaCache.GetCredentialInfo(ctx, req.Username)
+	if err != nil {
+		log.Error("found no credential", zap.String("username", req.Username), zap.Error(err))
+		return &commonpb.Status{
+			ErrorCode: commonpb.ErrorCode_UpdateCredentialFailure,
+			Reason:    "found no credential:" + req.Username,
+		}, nil
+	}
+	if !crypto.PasswordVerify(rawOldPassword, oldCredInfo.EncryptedPassword) {
+		return &commonpb.Status{
+			ErrorCode: commonpb.ErrorCode_UpdateCredentialFailure,
+			Reason:    "old password is not correct:" + req.Username,
+		}, nil
+	}
+	// update meta data
+	encryptedPassword, err := crypto.PasswordEncrypt(rawNewPassword)
 	if err != nil {
 		log.Error("encrypt password fail", zap.String("username", req.Username), zap.Error(err))
 		return &commonpb.Status{
@@ -4124,11 +4148,11 @@ func (node *Proxy) UpdateCredential(ctx context.Context, req *milvuspb.CreateCre
 			Reason:    "encrypt password fail when updating:" + req.Username,
 		}, nil
 	}
-	credInfo := &internalpb.CredentialInfo{
+	updateCredReq := &internalpb.CredentialInfo{
 		Username:          req.Username,
 		EncryptedPassword: encryptedPassword,
 	}
-	result, err := node.rootCoord.UpdateCredential(ctx, credInfo)
+	result, err := node.rootCoord.UpdateCredential(ctx, updateCredReq)
 	if err != nil { // for error like conntext timeout etc.
 		log.Error("update credential fail", zap.String("username", req.Username), zap.Error(err))
 		return &commonpb.Status{

internal/proxy/proxy_test.go

@@ -30,14 +30,11 @@ import (
 	"testing"
 	"time"
 
-	"github.com/milvus-io/milvus/internal/proto/rootcoordpb"
-
-	"github.com/milvus-io/milvus/internal/util/crypto"
-
 	ot "github.com/grpc-ecosystem/go-grpc-middleware/tracing/opentracing"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/keepalive"
 
+	"github.com/milvus-io/milvus/internal/util/crypto"
 	"github.com/milvus-io/milvus/internal/util/dependency"
 	"github.com/milvus-io/milvus/internal/util/trace"
 
@@ -58,6 +55,7 @@ import (
 	"github.com/milvus-io/milvus/internal/util/distance"
 
 	"github.com/milvus-io/milvus/internal/proto/proxypb"
+	"github.com/milvus-io/milvus/internal/proto/rootcoordpb"
 
 	"github.com/golang/protobuf/proto"
 
@@ -1972,22 +1970,23 @@ func TestProxy(t *testing.T) {
 		assert.Equal(t, 0, len(resp.CollectionNames))
 	})
 
+	username := "test_username_" + funcutil.RandomString(15)
+	password := "xxx"
+
 	wg.Add(1)
-	t.Run("credential apis", func(t *testing.T) {
+	t.Run("credential CREATE api", func(t *testing.T) {
 		defer wg.Done()
 
 		// 1. create credential
-		password := "xxx"
-		newPassword := "yyy"
-		constructCreateCredentialRequest := func(rand string) *milvuspb.CreateCredentialRequest {
+		constructCreateCredentialRequest := func() *milvuspb.CreateCredentialRequest {
 			return &milvuspb.CreateCredentialRequest{
 				Base:     nil,
-				Username: "test_username_" + rand,
+				Username: username,
 				Password: crypto.Base64Encode(password),
 			}
 		}
 		// success
-		createCredentialReq := constructCreateCredentialRequest(funcutil.RandomString(15))
+		createCredentialReq := constructCreateCredentialRequest()
 		resp, err := proxy.CreateCredential(ctx, createCredentialReq)
 		assert.NoError(t, err)
 		assert.Equal(t, commonpb.ErrorCode_Success, resp.ErrorCode)
@@ -1998,49 +1997,90 @@ func TestProxy(t *testing.T) {
 		assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode)
 
 		// invalid username
-		reqInvalidField := constructCreateCredentialRequest(funcutil.RandomString(15))
-		reqInvalidField.Username = "11_invalid_username"
-		resp, err = proxy.CreateCredential(ctx, reqInvalidField)
+		createCredentialReq.Username = "11_invalid_username"
+		resp, err = proxy.CreateCredential(ctx, createCredentialReq)
 		assert.NoError(t, err)
 		assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode)
 
 		// invalid password (not decode)
-		reqInvalidField = constructCreateCredentialRequest(funcutil.RandomString(15))
-		reqInvalidField.Password = "not_decoded_password"
-		resp, err = proxy.CreateCredential(ctx, reqInvalidField)
+		createCredentialReq.Password = "not_decoded_password"
+		resp, err = proxy.CreateCredential(ctx, createCredentialReq)
 		assert.NoError(t, err)
 		assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode)
 
 		// invalid password (length gt 256)
-		reqInvalidField = constructCreateCredentialRequest(funcutil.RandomString(15))
-		reqInvalidField.Password = "aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeeeffffffffffgggggggggghhhhhhhhhhiiiiiiiiiijjjjjjjjjjkkkkkkkkkkllllllllllmmmmmmmmmnnnnnnnnnnnooooooooooppppppppppqqqqqqqqqqrrrrrrrrrrsssssssssstttttttttttuuuuuuuuuuuvvvvvvvvvvwwwwwwwwwwwxxxxxxxxxxyyyyyyyyyzzzzzzzzzzz"
-		resp, err = proxy.CreateCredential(ctx, reqInvalidField)
+		createCredentialReq.Password = "aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeeeffffffffffgggggggggghhhhhhhhhhiiiiiiiiiijjjjjjjjjjkkkkkkkkkkllllllllllmmmmmmmmmnnnnnnnnnnnooooooooooppppppppppqqqqqqqqqqrrrrrrrrrrsssssssssstttttttttttuuuuuuuuuuuvvvvvvvvvvwwwwwwwwwwwxxxxxxxxxxyyyyyyyyyzzzzzzzzzzz"
+		resp, err = proxy.CreateCredential(ctx, createCredentialReq)
 		assert.NoError(t, err)
 		assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode)
+	})
+
+	wg.Add(1)
+	t.Run("credential UPDATE api", func(t *testing.T) {
+		defer wg.Done()
 
 		// 2. update credential
-		createCredentialReq.Password = crypto.Base64Encode(newPassword)
-		updateResp, err := proxy.UpdateCredential(ctx, createCredentialReq)
+		newPassword := "yyy"
+		constructUpdateCredentialRequest := func() *milvuspb.UpdateCredentialRequest {
+			return &milvuspb.UpdateCredentialRequest{
+				Base:        nil,
+				Username:    username,
+				OldPassword: crypto.Base64Encode(password),
+				NewPassword: crypto.Base64Encode(newPassword),
+			}
+		}
+		// cannot update non-existing user's password
+		updateCredentialReq := constructUpdateCredentialRequest()
+		updateCredentialReq.Username = "test_username_" + funcutil.RandomString(15)
+		updateResp, err := proxy.UpdateCredential(ctx, updateCredentialReq)
+		assert.NoError(t, err)
+		assert.NotEqual(t, commonpb.ErrorCode_Success, updateResp.ErrorCode)
+
+		// success
+		updateCredentialReq.Username = username
+		updateCredentialReq.NewPassword = crypto.Base64Encode(newPassword)
+		updateResp, err = proxy.UpdateCredential(ctx, updateCredentialReq)
 		assert.NoError(t, err)
 		assert.Equal(t, commonpb.ErrorCode_Success, updateResp.ErrorCode)
 
-		// invalid password (not decode)
-		createCredentialReq.Password = newPassword
-		updateResp, err = proxy.UpdateCredential(ctx, createCredentialReq)
+		// invalid old password (not decode)
+		updateCredentialReq.OldPassword = password
+		updateCredentialReq.NewPassword = crypto.Base64Encode(newPassword)
+		updateResp, err = proxy.UpdateCredential(ctx, updateCredentialReq)
+		assert.NoError(t, err)
+		assert.NotEqual(t, commonpb.ErrorCode_Success, updateResp.ErrorCode)
+
+		// invalid new password (not decode)
+		updateCredentialReq.OldPassword = crypto.Base64Encode(password)
+		updateCredentialReq.NewPassword = newPassword
+		updateResp, err = proxy.UpdateCredential(ctx, updateCredentialReq)
 		assert.NoError(t, err)
 		assert.NotEqual(t, commonpb.ErrorCode_Success, updateResp.ErrorCode)
 
 		// invalid password (length gt 256)
-		createCredentialReq.Password = "aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeeeffffffffffgggggggggghhhhhhhhhhiiiiiiiiiijjjjjjjjjjkkkkkkkkkkllllllllllmmmmmmmmmnnnnnnnnnnnooooooooooppppppppppqqqqqqqqqqrrrrrrrrrrsssssssssstttttttttttuuuuuuuuuuuvvvvvvvvvvwwwwwwwwwwwxxxxxxxxxxyyyyyyyyyzzzzzzzzzzz"
-		updateResp, err = proxy.UpdateCredential(ctx, createCredentialReq)
+		updateCredentialReq.NewPassword = "aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeeeffffffffffgggggggggghhhhhhhhhhiiiiiiiiiijjjjjjjjjjkkkkkkkkkkllllllllllmmmmmmmmmnnnnnnnnnnnooooooooooppppppppppqqqqqqqqqqrrrrrrrrrrsssssssssstttttttttttuuuuuuuuuuuvvvvvvvvvvwwwwwwwwwwwxxxxxxxxxxyyyyyyyyyzzzzzzzzzzz"
+		updateResp, err = proxy.UpdateCredential(ctx, updateCredentialReq)
 		assert.NoError(t, err)
 		assert.NotEqual(t, commonpb.ErrorCode_Success, updateResp.ErrorCode)
 
+		// wrong password
+		updateCredentialReq.OldPassword = crypto.Base64Encode("wrong_password")
+		updateCredentialReq.NewPassword = crypto.Base64Encode(newPassword)
+		updateResp, err = proxy.UpdateCredential(ctx, updateCredentialReq)
+		assert.NoError(t, err)
+		assert.NotEqual(t, commonpb.ErrorCode_Success, updateResp.ErrorCode)
+	})
+
+	wg.Add(1)
+	t.Run("credential GET api", func(t *testing.T) {
+		defer wg.Done()
+
 		// 3. get credential
+		newPassword := "yyy"
 		constructGetCredentialRequest := func() *rootcoordpb.GetCredentialRequest {
 			return &rootcoordpb.GetCredentialRequest{
 				Base:     nil,
-				Username: createCredentialReq.Username,
+				Username: username,
 			}
 		}
 		getCredentialReq := constructGetCredentialRequest()
@@ -2052,6 +2092,11 @@ func TestProxy(t *testing.T) {
 		getCredentialReq.Username = "("
 		getResp, err = rootCoordClient.GetCredential(ctx, getCredentialReq)
 		assert.Error(t, err)
+	})
+
+	wg.Add(1)
+	t.Run("credential LIST api", func(t *testing.T) {
+		defer wg.Done()
 
 		// 4. list credential usernames
 		constructListCredUsersRequest := func() *milvuspb.ListCredUsersRequest {
@@ -2063,12 +2108,17 @@ func TestProxy(t *testing.T) {
 		listUsersResp, err := proxy.ListCredUsers(ctx, listCredUsersReq)
 		assert.NoError(t, err)
 		assert.True(t, len(listUsersResp.Usernames) > 0)
+	})
+
+	wg.Add(1)
+	t.Run("credential DELETE api", func(t *testing.T) {
+		defer wg.Done()
 
 		// 5. delete credential
 		constructDelCredRequest := func() *milvuspb.DeleteCredentialRequest {
 			return &milvuspb.DeleteCredentialRequest{
 				Base:     nil,
-				Username: createCredentialReq.Username,
+				Username: username,
 			}
 		}
 		delCredReq := constructDelCredRequest()
@@ -2847,7 +2897,7 @@ func TestProxy(t *testing.T) {
 	wg.Add(1)
 	t.Run("UpdateCredential fail, timeout", func(t *testing.T) {
 		defer wg.Done()
-		resp, err := proxy.UpdateCredential(shortCtx, &milvuspb.CreateCredentialRequest{Username: "xxx"})
+		resp, err := proxy.UpdateCredential(shortCtx, &milvuspb.UpdateCredentialRequest{Username: "xxx"})
 		assert.NoError(t, err)
 		assert.NotEqual(t, commonpb.ErrorCode_Success, resp.ErrorCode)
 	})

internal/rootcoord/meta_table.go

@@ -1360,7 +1360,12 @@ func (mt *MetaTable) AddCredential(credInfo *internalpb.CredentialInfo) error {
 		return fmt.Errorf("username is empty")
 	}
 	k := fmt.Sprintf("%s/%s", CredentialPrefix, credInfo.Username)
-	err := mt.txn.Save(k, credInfo.EncryptedPassword)
+	v, err := proto.Marshal(&internalpb.CredentialInfo{EncryptedPassword: credInfo.EncryptedPassword})
+	if err != nil {
+		log.Error("MetaTable marshal credential info fail", zap.String("key", k), zap.Error(err))
+		return fmt.Errorf("metaTable marshal credential info fail key:%s, err:%w", k, err)
+	}
+	err = mt.txn.Save(k, string(v))
 	if err != nil {
 		log.Error("MetaTable save fail", zap.Error(err))
 		return fmt.Errorf("save credential fail key:%s, err:%w", credInfo.Username, err)
@@ -1379,7 +1384,13 @@ func (mt *MetaTable) getCredential(username string) (*internalpb.CredentialInfo,
 		log.Warn("MetaTable load fail", zap.String("key", k), zap.Error(err))
 		return nil, err
 	}
-	return &internalpb.CredentialInfo{Username: username, EncryptedPassword: v}, nil
+
+	credentialInfo := internalpb.CredentialInfo{}
+	err = proto.Unmarshal([]byte(v), &credentialInfo)
+	if err != nil {
+		return nil, fmt.Errorf("get credential unmarshal err:%w", err)
+	}
+	return &internalpb.CredentialInfo{Username: username, EncryptedPassword: credentialInfo.EncryptedPassword}, nil
 }
 
 // DeleteCredential delete credential

internal/rootcoord/meta_table_test.go

@@ -25,6 +25,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/milvus-io/milvus/internal/proto/internalpb"
+
 	"github.com/golang/protobuf/proto"
 	"github.com/milvus-io/milvus/internal/kv"
 	etcdkv "github.com/milvus-io/milvus/internal/kv/etcd"
@@ -1135,6 +1137,14 @@ func TestMetaTable(t *testing.T) {
 		assert.EqualError(t, err, fmt.Sprintf("cannot find index, id = %d", idxInfo[0].IndexID))
 	})
 
+	wg.Add(1)
+	t.Run("add credential failed", func(t *testing.T) {
+		defer wg.Done()
+
+		err = mt.AddCredential(&internalpb.CredentialInfo{Username: "x", EncryptedPassword: "a\xc5z"})
+		assert.NotNil(t, err)
+	})
+
 	wg.Add(1)
 	t.Run("delete credential failed", func(t *testing.T) {
 		defer wg.Done()

internal/types/types.go

@@ -1134,7 +1134,7 @@ type ProxyComponent interface {
 	// CreateCredential create new user and password
 	CreateCredential(ctx context.Context, req *milvuspb.CreateCredentialRequest) (*commonpb.Status, error)
 	// UpdateCredential update password for a user
-	UpdateCredential(ctx context.Context, req *milvuspb.CreateCredentialRequest) (*commonpb.Status, error)
+	UpdateCredential(ctx context.Context, req *milvuspb.UpdateCredentialRequest) (*commonpb.Status, error)
 	// DeleteCredential delete a user
 	DeleteCredential(ctx context.Context, req *milvuspb.DeleteCredentialRequest) (*commonpb.Status, error)
 	// ListCredUsers list all usernames

internal/util/paramtable/component_param.go

@@ -125,6 +125,8 @@ type commonConfig struct {
 	SimdType       string
 	IndexSliceSize int64
 	StorageType    string
+
+	AuthorizationEnabled bool
 }
 
 func (p *commonConfig) init(base *BaseTable) {
@@ -160,6 +162,8 @@ func (p *commonConfig) init(base *BaseTable) {
 	p.initSimdType()
 	p.initIndexSliceSize()
 	p.initStorageType()
+
+	p.initEnableAuthorization()
 }
 
 func (p *commonConfig) initClusterPrefix() {
@@ -348,6 +352,10 @@ func (p *commonConfig) initStorageType() {
 	p.StorageType = p.Base.LoadWithDefault("storageType", "minio")
 }
 
+func (p *commonConfig) initEnableAuthorization() {
+	p.AuthorizationEnabled = p.Base.ParseBool("common.security.authorizationEnabled", false)
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // --- rootcoord ---
 type rootCoordConfig struct {