Fix password verification miss cache (#18730)

Signed-off-by: Nyun.zhang <yun.zhang@zilliz.com>
Signed-off-by: Nyun.zhang <yun.zhang@zilliz.com>

internal/proxy/authentication_interceptor.go

@@ -6,9 +6,6 @@ import (
 
 	"github.com/milvus-io/milvus/internal/util"
 
-	"go.uber.org/zap"
-
-	"github.com/milvus-io/milvus/internal/log"
 	"github.com/milvus-io/milvus/internal/util/crypto"
 
 	"google.golang.org/grpc/metadata"
@@ -31,13 +28,7 @@ func validAuth(ctx context.Context, authorization []string) bool {
 	username := secrets[0]
 	password := secrets[1]
 
-	credInfo, err := globalMetaCache.GetCredentialInfo(ctx, username)
-	if err != nil {
-		log.Error("found no credential", zap.String("username", username), zap.Error(err))
-		return false
-	}
-
-	return crypto.PasswordVerify(password, credInfo)
+	return passwordVerify(ctx, username, password, globalMetaCache)
 }
 
 func validSourceID(ctx context.Context, authorization []string) bool {

internal/proxy/impl.go

@@ -3950,16 +3950,8 @@ func (node *Proxy) UpdateCredential(ctx context.Context, req *milvuspb.UpdateCre
 			Reason:    err.Error(),
 		}, nil
 	}
-	// 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) {
+
+	if !passwordVerify(ctx, req.Username, rawOldPassword, globalMetaCache) {
 		return &commonpb.Status{
 			ErrorCode: commonpb.ErrorCode_UpdateCredentialFailure,
 			Reason:    "old password is not correct:" + req.Username,

internal/proxy/meta_cache.go

@@ -536,7 +536,6 @@ func (m *MetaCache) GetCredentialInfo(ctx context.Context, username string) (*in
 			Username:          resp.Username,
 			EncryptedPassword: resp.Password,
 		}
-		m.UpdateCredential(credInfo)
 	}
 
 	return credInfo, nil
@@ -552,15 +551,15 @@ func (m *MetaCache) RemoveCredential(username string) {
 func (m *MetaCache) UpdateCredential(credInfo *internalpb.CredentialInfo) {
 	m.credMut.Lock()
 	defer m.credMut.Unlock()
-	// update credMap
 	username := credInfo.Username
 	_, ok := m.credMap[username]
 	if !ok {
 		m.credMap[username] = &internalpb.CredentialInfo{}
 	}
+
+	// Do not cache encrypted password content
 	m.credMap[username].Username = username
 	m.credMap[username].Sha256Password = credInfo.Sha256Password
-	m.credMap[username].EncryptedPassword = credInfo.EncryptedPassword
 }
 
 // GetShards update cache if withCache == false

internal/proxy/proxy_test.go

@@ -2270,9 +2270,7 @@ func TestProxy(t *testing.T) {
 		getResp, err := rootCoordClient.GetCredential(ctx, getCredentialReq)
 		assert.NoError(t, err)
 		assert.Equal(t, commonpb.ErrorCode_Success, getResp.Status.ErrorCode)
-		assert.True(t, crypto.PasswordVerify(newPassword, &internalpb.CredentialInfo{
-			EncryptedPassword: getResp.Password,
-		}))
+		assert.True(t, passwordVerify(ctx, username, newPassword, globalMetaCache))
 
 		getCredentialReq.Username = "("
 		getResp, err = rootCoordClient.GetCredential(ctx, getCredentialReq)

internal/proxy/rootcoord_mock_test.go

@@ -1144,6 +1144,7 @@ type DescribeSegmentsFunc func(ctx context.Context, request *rootcoordpb.Describ
 type ImportFunc func(ctx context.Context, req *milvuspb.ImportRequest) (*milvuspb.ImportResponse, error)
 type DropCollectionFunc func(ctx context.Context, request *milvuspb.DropCollectionRequest) (*commonpb.Status, error)
 type GetIndexStateFunc func(ctx context.Context, request *milvuspb.GetIndexStateRequest) (*indexpb.GetIndexStatesResponse, error)
+type GetGetCredentialFunc func(ctx context.Context, req *rootcoordpb.GetCredentialRequest) (*rootcoordpb.GetCredentialResponse, error)
 
 type mockRootCoord struct {
 	types.RootCoord
@@ -1155,6 +1156,14 @@ type mockRootCoord struct {
 	ImportFunc
 	DropCollectionFunc
 	GetIndexStateFunc
+	GetGetCredentialFunc
+}
+
+func (m *mockRootCoord) GetCredential(ctx context.Context, request *rootcoordpb.GetCredentialRequest) (*rootcoordpb.GetCredentialResponse, error) {
+	if m.GetGetCredentialFunc != nil {
+		return m.GetGetCredentialFunc(ctx, request)
+	}
+	return nil, errors.New("mock")
 }
 
 func (m *mockRootCoord) DescribeCollection(ctx context.Context, request *milvuspb.DescribeCollectionRequest) (*milvuspb.DescribeCollectionResponse, error) {

internal/proxy/util.go

@@ -24,6 +24,8 @@ import (
 	"strings"
 	"time"
 
+	"golang.org/x/crypto/bcrypt"
+
 	"github.com/milvus-io/milvus/internal/log"
 	"go.uber.org/zap"
 
@@ -701,3 +703,32 @@ func GetRole(username string) ([]string, error) {
 	}
 	return globalMetaCache.GetUserRole(username), nil
 }
+
+// PasswordVerify verify password
+func passwordVerify(ctx context.Context, username, rawPwd string, globalMetaCache Cache) bool {
+	// it represents the cache miss if Sha256Password is empty within credInfo, which shall be updated first connection.
+	// meanwhile, generating Sha256Password depends on raw password and encrypted password will not cache.
+	credInfo, err := globalMetaCache.GetCredentialInfo(ctx, username)
+	if err != nil {
+		log.Error("found no credential", zap.String("username", username), zap.Error(err))
+		return false
+	}
+
+	// hit cache
+	sha256Pwd := crypto.SHA256(rawPwd, credInfo.Username)
+	if credInfo.Sha256Password != "" {
+		return sha256Pwd == credInfo.Sha256Password
+	}
+
+	// miss cache, verify against encrypted password from etcd
+	if err := bcrypt.CompareHashAndPassword([]byte(credInfo.EncryptedPassword), []byte(rawPwd)); err != nil {
+		log.Error("Verify password failed", zap.Error(err))
+		return false
+	}
+
+	// update cache after miss cache
+	credInfo.Sha256Password = sha256Pwd
+	log.Debug("get credential miss cache, update cache with", zap.Any("credential", credInfo))
+	globalMetaCache.UpdateCredential(credInfo)
+	return true
+}

internal/proxy/util_test.go

@@ -23,6 +23,10 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/milvus-io/milvus/internal/proto/rootcoordpb"
+
+	"github.com/milvus-io/milvus/internal/proto/internalpb"
+
 	"github.com/milvus-io/milvus/internal/proto/commonpb"
 	"github.com/milvus-io/milvus/internal/proto/schemapb"
 	"github.com/milvus-io/milvus/internal/util"
@@ -721,3 +725,47 @@ func TestGetRole(t *testing.T) {
 	assert.Nil(t, err)
 	assert.Equal(t, 1, len(roles))
 }
+
+func TestPasswordVerify(t *testing.T) {
+	username := "user-test00"
+	password := "PasswordVerify"
+
+	// credential does not exist within cache
+	credCache := make(map[string]*internalpb.CredentialInfo, 0)
+	invokedCount := 0
+
+	mockedRootCoord := newMockRootCoord()
+	mockedRootCoord.GetGetCredentialFunc = func(ctx context.Context, req *rootcoordpb.GetCredentialRequest) (*rootcoordpb.GetCredentialResponse, error) {
+		invokedCount++
+		return nil, fmt.Errorf("get cred not found credential")
+	}
+
+	metaCache := &MetaCache{
+		credMap:   credCache,
+		rootCoord: mockedRootCoord,
+	}
+	ret, ok := credCache[username]
+	assert.False(t, ok)
+	assert.Nil(t, ret)
+	assert.False(t, passwordVerify(context.TODO(), username, password, metaCache))
+	assert.Equal(t, 1, invokedCount)
+
+	// Sha256Password has not been filled into cache during establish connection firstly
+	encryptedPwd, err := crypto.PasswordEncrypt(password)
+	assert.Nil(t, err)
+	credCache[username] = &internalpb.CredentialInfo{
+		Username:          username,
+		EncryptedPassword: encryptedPwd,
+	}
+	assert.True(t, passwordVerify(context.TODO(), username, password, metaCache))
+	ret, ok = credCache[username]
+	assert.True(t, ok)
+	assert.NotNil(t, ret)
+	assert.Equal(t, username, ret.Username)
+	assert.NotNil(t, ret.Sha256Password)
+	assert.Equal(t, 1, invokedCount)
+
+	// Sha256Password already exists within cache
+	assert.True(t, passwordVerify(context.TODO(), username, password, metaCache))
+	assert.Equal(t, 1, invokedCount)
+}

internal/util/crypto/crypto.go

@@ -5,9 +5,6 @@ import (
 	"encoding/base64"
 	"encoding/hex"
 
-	"github.com/milvus-io/milvus/internal/log"
-	"github.com/milvus-io/milvus/internal/proto/internalpb"
-	"go.uber.org/zap"
 	"golang.org/x/crypto/bcrypt"
 )
 
@@ -30,23 +27,6 @@ func PasswordEncrypt(pwd string) (string, error) {
 	return string(bytes), err
 }
 
-// PasswordVerify verify password
-func PasswordVerify(rawPwd string, credInfo *internalpb.CredentialInfo) bool {
-	// 1. hit cache
-	if credInfo.Sha256Password != "" {
-		encryped := SHA256(rawPwd, credInfo.Username)
-		return encryped == credInfo.Sha256Password
-	}
-
-	// 2. miss cache, verify against encrypted password from etcd
-	err := bcrypt.CompareHashAndPassword([]byte(credInfo.EncryptedPassword), []byte(rawPwd))
-	if err != nil {
-		log.Error("Verify password failed", zap.Error(err))
-	}
-
-	return err == nil
-}
-
 func Base64Decode(pwd string) (string, error) {
 	bytes, err := base64.StdEncoding.DecodeString(pwd)
 	if err != nil {

internal/util/crypto/crypto_test.go

@@ -3,32 +3,10 @@ package crypto
 import (
 	"testing"
 
-	"github.com/milvus-io/milvus/internal/proto/internalpb"
 	"github.com/stretchr/testify/assert"
 	"golang.org/x/crypto/bcrypt"
 )
 
-func TestPasswordVerify_HitCache(t *testing.T) {
-	wrongPassword := "test_my_name"
-	correctPassword := "test_my_pass_new"
-	credInfo := &internalpb.CredentialInfo{
-		Username:       "root",
-		Sha256Password: "bcca79df9650cef1d7ed9f63449d7f8a27843d2678f5666f38ca65ab77d99a13",
-	}
-	assert.True(t, PasswordVerify(correctPassword, credInfo))
-	assert.False(t, PasswordVerify(wrongPassword, credInfo))
-}
-
-func TestPasswordVerify_MissCache(t *testing.T) {
-	wrongPassword := "test_my_name"
-	correctPassword := "test_my_pass_new"
-	credInfo := &internalpb.CredentialInfo{
-		EncryptedPassword: "$2a$10$3H9DLiHyPxJ29bMWRNyueOrGkbzJfE3BAR159ju3UetytAoKk7Ne2",
-	}
-	assert.True(t, PasswordVerify(correctPassword, credInfo))
-	assert.False(t, PasswordVerify(wrongPassword, credInfo))
-}
-
 //func BenchmarkPasswordVerify(b *testing.B) {
 //	correctPassword := "test_my_pass_new"
 //	credInfo := &internalpb.CredentialInfo{