etcdserver/auth: check empty password in merge

Fix https://github.com/coreos/etcd/issues/5182.

etcdserver/auth/auth.go

@@ -281,13 +281,7 @@ func (s *store) UpdateUser(user User) (User, error) {
 		return old, err
 	}
 
-	hash, err := s.HashPassword(user.Password)
-	if err != nil {
-		return old, err
-	}
-	user.Password = hash
-
-	newUser, err := old.merge(user)
+	newUser, err := old.merge(user, s.PasswordStore)
 	if err != nil {
 		return old, err
 	}
@@ -448,29 +442,33 @@ func (s *store) DisableAuth() error {
 // is called and returns a new User with these modifications applied. Think of
 // all Users as immutable sets of data. Merge allows you to perform the set
 // operations (desired grants and revokes) atomically
-func (u User) merge(n User) (User, error) {
+func (ou User) merge(nu User, s PasswordStore) (User, error) {
 	var out User
-	if u.User != n.User {
-		return out, authErr(http.StatusConflict, "Merging user data with conflicting usernames: %s %s", u.User, n.User)
-	}
-	out.User = u.User
-	if n.Password != "" {
-		out.Password = n.Password
+	if ou.User != nu.User {
+		return out, authErr(http.StatusConflict, "Merging user data with conflicting usernames: %s %s", ou.User, nu.User)
+	}
+	out.User = ou.User
+	if nu.Password != "" {
+		hash, err := s.HashPassword(nu.Password)
+		if err != nil {
+			return ou, err
+		}
+		out.Password = hash
 	} else {
-		out.Password = u.Password
+		out.Password = ou.Password
 	}
-	currentRoles := types.NewUnsafeSet(u.Roles...)
-	for _, g := range n.Grant {
+	currentRoles := types.NewUnsafeSet(ou.Roles...)
+	for _, g := range nu.Grant {
 		if currentRoles.Contains(g) {
-			plog.Noticef("granting duplicate role %s for user %s", g, n.User)
-			return User{}, authErr(http.StatusConflict, fmt.Sprintf("Granting duplicate role %s for user %s", g, n.User))
+			plog.Noticef("granting duplicate role %s for user %s", g, nu.User)
+			return User{}, authErr(http.StatusConflict, fmt.Sprintf("Granting duplicate role %s for user %s", g, nu.User))
 		}
 		currentRoles.Add(g)
 	}
-	for _, r := range n.Revoke {
+	for _, r := range nu.Revoke {
 		if !currentRoles.Contains(r) {
-			plog.Noticef("revoking ungranted role %s for user %s", r, n.User)
-			return User{}, authErr(http.StatusConflict, fmt.Sprintf("Revoking ungranted role %s for user %s", r, n.User))
+			plog.Noticef("revoking ungranted role %s for user %s", r, nu.User)
+			return User{}, authErr(http.StatusConflict, fmt.Sprintf("Revoking ungranted role %s for user %s", r, nu.User))
 		}
 		currentRoles.Remove(r)
 	}

etcdserver/auth/auth_test.go

@@ -26,6 +26,21 @@ import (
 	"golang.org/x/net/context"
 )
 
+type fakeDoer struct{}
+
+func (_ fakeDoer) Do(context.Context, etcdserverpb.Request) (etcdserver.Response, error) {
+	return etcdserver.Response{}, nil
+}
+
+func TestCheckPassword(t *testing.T) {
+	st := NewStore(fakeDoer{}, 5*time.Second)
+	u := User{Password: "$2a$10$I3iddh1D..EIOXXQtsra4u8AjOtgEa2ERxVvYGfXFBJDo1omXwP.q"}
+	matched := st.CheckPassword(u, "foo")
+	if matched {
+		t.Fatalf("expected false, got %v", matched)
+	}
+}
+
 const testTimeout = time.Millisecond
 
 func TestMergeUser(t *testing.T) {
@@ -71,16 +86,16 @@ func TestMergeUser(t *testing.T) {
 			User{User: "foo", Roles: []string{"role1", "role2"}},
 			false,
 		},
-		{
-			User{User: "foo"},
-			User{User: "foo", Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"},
-			User{User: "foo", Roles: []string{}, Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"},
+		{ // empty password will not overwrite the previous password
+			User{User: "foo", Password: "foo", Roles: []string{}},
+			User{User: "foo", Password: ""},
+			User{User: "foo", Password: "foo", Roles: []string{}},
 			false,
 		},
 	}
 
 	for i, tt := range tbl {
-		out, err := tt.input.merge(tt.merge)
+		out, err := tt.input.merge(tt.merge, passwordStore{})
 		if err != nil && !tt.iserr {
 			t.Fatalf("Got unexpected error on item %d", i)
 		}