server/auth: fix auth panic bug when user changes password

[tangcong]

What happened?

When the etcd cluster is migrated from the old version before 3.5 to the version 3.5+, if user changes the password will trigger a panic bug.

Reason

Old etcd version user's option is nil. I checked the code and found that user.Options.NoPassword is always false. It seems that there is no need to check here, and it is not allowed to set an empty password.

[ahrtr]

Please also add unit tests

cc @mitake

[tangcong]

Please also add unit tests

cc @mitake

done.

[ahrtr]

LGTM

Thank you @tangcong

cc @mitake

[mitake]

Thanks a lot for finding and fixing the issue @tangcong! I commented for 2 minor points, could you take a look? The overall change LGTM.

[serathius]

This shows lack of etcd upgrade testing.

[mitake]

LGTM, I'm merging. Thanks a lot @tangcong !
@serathius I agree, it's motivating upgrade testing.

[ahrtr]

@mitake The PR #9817 is included in release-3.4, but (*authStore) UserChangePassword() in 3.4 doesn't check user.Options.NoPassword at all. I guess it's missed when implementing #9817?

see

etcd/auth/store.go

Lines 518 to 569 in 4b91b6d

	func (as *authStore) UserChangePassword(r *pb.AuthUserChangePasswordRequest) (*pb.AuthUserChangePasswordResponse, error) {
	// TODO(mitake): measure the cost of bcrypt.GenerateFromPassword()
	// If the cost is too high, we should move the encryption to outside of the raft
	hashed, err := bcrypt.GenerateFromPassword([]byte(r.Password), as.bcryptCost)
	if err != nil {
	if as.lg != nil {
	as.lg.Warn(
	"failed to bcrypt hash password",
	zap.String("user-name", r.Name),
	zap.Error(err),
	)
	} else {
	plog.Errorf("failed to hash password: %s", err)
	}
	return nil, err
	}
	tx := as.be.BatchTx()
	tx.Lock()
	defer tx.Unlock()
	user := getUser(as.lg, tx, r.Name)
	if user == nil {
	return nil, ErrUserNotFound
	}
	updatedUser := &authpb.User{
	Name: []byte(r.Name),
	Roles: user.Roles,
	Password: hashed,
	Options: user.Options,
	}
	putUser(as.lg, tx, updatedUser)
	as.commitRevision(tx)
	as.saveConsistentIndex(tx)
	as.refreshRangePermCache(tx)
	as.tokenProvider.invalidateUser(r.Name)
	if as.lg != nil {
	as.lg.Info(
	"changed a password of a user",
	zap.String("user-name", r.Name),
	zap.Strings("user-roles", user.Roles),
	)
	} else {
	plog.Noticef("changed a password of a user: %s", r.Name)
	}
	return &pb.AuthUserChangePasswordResponse{}, nil
	}

[ahrtr]

Just as we discussed in slack, 3.4 doesn't have this issue because it doesn't check user.Options.NoPassword at all, so it will not panic. Let's leave 3.4 as it's.