Merge pull request #5196 from gyuho/password_check

etcdserver/auth: check empty password

e2e/ctl_v2_test.go

@@ -158,7 +158,6 @@ func testCtlV2Watch(t *testing.T, cfg *etcdProcessClusterConfig, noSync bool) {
 
 func TestCtlV2GetRoleUser(t *testing.T)          { testCtlV2GetRoleUser(t, &configNoTLS) }
 func TestCtlV2GetRoleUserWithProxy(t *testing.T) { testCtlV2GetRoleUser(t, &configWithProxy) }
-
 func testCtlV2GetRoleUser(t *testing.T, cfg *etcdProcessClusterConfig) {
 	defer testutil.AfterTest(t)
 
@@ -181,6 +180,7 @@ func testCtlV2GetRoleUser(t *testing.T, cfg *etcdProcessClusterConfig) {
 	if err := etcdctlUserGet(epc, "username"); err != nil {
 		t.Fatalf("failed to get user (%v)", err)
 	}
+
 	// ensure double grant gives an error; was crashing in 2.3.1
 	regrantArgs := etcdctlPrefixArgs(epc)
 	regrantArgs = append(regrantArgs, "user", "grant", "--roles", "foo", "username")
@@ -191,7 +191,6 @@ func testCtlV2GetRoleUser(t *testing.T, cfg *etcdProcessClusterConfig) {
 
 func TestCtlV2UserListUsername(t *testing.T) { testCtlV2UserList(t, "username") }
 func TestCtlV2UserListRoot(t *testing.T)     { testCtlV2UserList(t, "root") }
-
 func testCtlV2UserList(t *testing.T, username string) {
 	defer testutil.AfterTest(t)
 
@@ -325,6 +324,11 @@ func etcdctlUserList(clus *etcdProcessCluster, expectedUser string) error {
 	return spawnWithExpect(cmdArgs, expectedUser)
 }
 
+func etcdctlAuthEnable(clus *etcdProcessCluster) error {
+	cmdArgs := append(etcdctlPrefixArgs(clus), "auth", "enable")
+	return spawnWithExpect(cmdArgs, "Authentication Enabled")
+}
+
 func mustEtcdctl(t *testing.T) {
 	if !fileutil.Exist("../bin/etcdctl") {
 		t.Fatalf("could not find etcdctl binary")

e2e/v2_curl_test.go

@@ -15,7 +15,9 @@
 package e2e
 
 import (
+	"fmt"
 	"math/rand"
+	"strings"
 	"testing"
 
 	"github.com/coreos/etcd/pkg/testutil"
@@ -30,7 +32,6 @@ func TestV2CurlProxyNoTLS(t *testing.T)   { testCurlPutGet(t, &configWithProxy)
 func TestV2CurlProxyTLS(t *testing.T)     { testCurlPutGet(t, &configWithProxyTLS) }
 func TestV2CurlProxyPeerTLS(t *testing.T) { testCurlPutGet(t, &configWithProxyPeerTLS) }
 func TestV2CurlClientBoth(t *testing.T)   { testCurlPutGet(t, &configClientBoth) }
-
 func testCurlPutGet(t *testing.T, cfg *etcdProcessClusterConfig) {
 	defer testutil.AfterTest(t)
 
@@ -48,66 +49,124 @@ func testCurlPutGet(t *testing.T, cfg *etcdProcessClusterConfig) {
 		}
 	}()
 
-	expectPut := `{"action":"set","node":{"key":"/testKey","value":"foo","`
-	expectGet := `{"action":"get","node":{"key":"/testKey","value":"foo","`
-
+	var (
+		expectPut = `{"action":"set","node":{"key":"/foo","value":"bar","`
+		expectGet = `{"action":"get","node":{"key":"/foo","value":"bar","`
+	)
+	if err := cURLPut(epc, cURLReq{endpoint: "/v2/keys/foo", value: "bar", expected: expectPut}); err != nil {
+		t.Fatalf("failed put with curl (%v)", err)
+	}
+	if err := cURLGet(epc, cURLReq{endpoint: "/v2/keys/foo", expected: expectGet}); err != nil {
+		t.Fatalf("failed get with curl (%v)", err)
+	}
 	if cfg.clientTLS == clientTLSAndNonTLS {
-		if err := cURLPut(epc, "testKey", "foo", expectPut); err != nil {
-			t.Fatalf("failed put with curl (%v)", err)
-		}
-
-		if err := cURLGet(epc, "testKey", expectGet); err != nil {
+		if err := cURLGet(epc, cURLReq{endpoint: "/v2/keys/foo", expected: expectGet, isTLS: true}); err != nil {
 			t.Fatalf("failed get with curl (%v)", err)
 		}
-		if err := cURLGetUseTLS(epc, "testKey", expectGet); err != nil {
-			t.Fatalf("failed get with curl (%v)", err)
-		}
-	} else {
-		if err := cURLPut(epc, "testKey", "foo", expectPut); err != nil {
-			t.Fatalf("failed put with curl (%v)", err)
+	}
+}
+
+func TestV2CurlIssue5182(t *testing.T) {
+	defer testutil.AfterTest(t)
+
+	epc := setupEtcdctlTest(t, &configNoTLS, false)
+	defer func() {
+		if err := epc.Close(); err != nil {
+			t.Fatalf("error closing etcd processes (%v)", err)
 		}
+	}()
 
-		if err := cURLGet(epc, "testKey", expectGet); err != nil {
-			t.Fatalf("failed get with curl (%v)", err)
+	expectPut := `{"action":"set","node":{"key":"/foo","value":"bar","`
+	if err := cURLPut(epc, cURLReq{endpoint: "/v2/keys/foo", value: "bar", expected: expectPut}); err != nil {
+		t.Fatal(err)
+	}
+
+	expectUserAdd := `{"user":"foo","roles":null}`
+	if err := cURLPut(epc, cURLReq{endpoint: "/v2/auth/users/foo", value: `{"user":"foo", "password":"pass"}`, expected: expectUserAdd}); err != nil {
+		t.Fatal(err)
+	}
+	expectRoleAdd := `{"role":"foo","permissions":{"kv":{"read":["/foo/*"],"write":null}}`
+	if err := cURLPut(epc, cURLReq{endpoint: "/v2/auth/roles/foo", value: `{"role":"foo", "permissions": {"kv": {"read": ["/foo/*"]}}}`, expected: expectRoleAdd}); err != nil {
+		t.Fatal(err)
+	}
+	expectUserUpdate := `{"user":"foo","roles":["foo"]}`
+	if err := cURLPut(epc, cURLReq{endpoint: "/v2/auth/users/foo", value: `{"user": "foo", "grant": ["foo"]}`, expected: expectUserUpdate}); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := etcdctlUserAdd(epc, "root", "a"); err != nil {
+		t.Fatal(err)
+	}
+	if err := etcdctlAuthEnable(epc); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := cURLGet(epc, cURLReq{endpoint: "/v2/keys/foo/", username: "root", password: "a", expected: "bar"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := cURLGet(epc, cURLReq{endpoint: "/v2/keys/foo/", username: "foo", password: "pass", expected: "bar"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := cURLGet(epc, cURLReq{endpoint: "/v2/keys/foo/", username: "foo", password: "", expected: "bar"}); err != nil {
+		if !strings.Contains(err.Error(), `The request requires user authentication`) {
+			t.Fatalf("expected 'The request requires user authentication' error, got %v", err)
 		}
+	} else {
+		t.Fatalf("expected 'The request requires user authentication' error")
 	}
 }
 
+type cURLReq struct {
+	username string
+	password string
+
+	isTLS bool
+
+	endpoint string
+
+	value    string
+	expected string
+}
+
 // cURLPrefixArgs builds the beginning of a curl command for a given key
 // addressed to a random URL in the given cluster.
-func cURLPrefixArgs(clus *etcdProcessCluster, key string) []string {
-	cmdArgs := []string{"curl"}
-	acurl := clus.procs[rand.Intn(clus.cfg.clusterSize)].cfg.acurl
-
-	if clus.cfg.clientTLS == clientTLS {
+func cURLPrefixArgs(clus *etcdProcessCluster, method string, req cURLReq) []string {
+	var (
+		cmdArgs = []string{"curl"}
+		acurl   = clus.procs[rand.Intn(clus.cfg.clusterSize)].cfg.acurl
+	)
+	if req.isTLS {
+		if clus.cfg.clientTLS != clientTLSAndNonTLS {
+			panic("should not use cURLPrefixArgsUseTLS when serving only TLS or non-TLS")
+		}
+		cmdArgs = append(cmdArgs, "--cacert", caPath, "--cert", certPath, "--key", privateKeyPath)
+		acurl = clus.procs[rand.Intn(clus.cfg.clusterSize)].cfg.acurltls
+	} else if clus.cfg.clientTLS == clientTLS {
 		cmdArgs = append(cmdArgs, "--cacert", caPath, "--cert", certPath, "--key", privateKeyPath)
 	}
-	keyURL := acurl + "/v2/keys/testKey"
-	cmdArgs = append(cmdArgs, "-L", keyURL)
-	return cmdArgs
-}
+	ep := acurl + req.endpoint
 
-func cURLPrefixArgsUseTLS(clus *etcdProcessCluster, key string) []string {
-	cmdArgs := []string{"curl"}
-	if clus.cfg.clientTLS != clientTLSAndNonTLS {
-		panic("should not use cURLPrefixArgsUseTLS when serving only TLS or non-TLS")
+	if req.username != "" || req.password != "" {
+		cmdArgs = append(cmdArgs, "-L", "-u", fmt.Sprintf("%s:%s", req.username, req.password), ep)
+	} else {
+		cmdArgs = append(cmdArgs, "-L", ep)
 	}
-	cmdArgs = append(cmdArgs, "--cacert", caPath, "--cert", certPath, "--key", privateKeyPath)
-	acurl := clus.procs[rand.Intn(clus.cfg.clusterSize)].cfg.acurltls
-	keyURL := acurl + "/v2/keys/testKey"
-	cmdArgs = append(cmdArgs, "-L", keyURL)
-	return cmdArgs
-}
 
-func cURLPut(clus *etcdProcessCluster, key, val, expected string) error {
-	args := append(cURLPrefixArgs(clus, key), "-XPUT", "-d", "value="+val)
-	return spawnWithExpect(args, expected)
+	switch method {
+	case "PUT":
+		dt := req.value
+		if !strings.HasPrefix(dt, "{") { // for non-JSON value
+			dt = "value=" + dt
+		}
+		cmdArgs = append(cmdArgs, "-XPUT", "-d", dt)
+	}
+	return cmdArgs
 }
 
-func cURLGet(clus *etcdProcessCluster, key, expected string) error {
-	return spawnWithExpect(cURLPrefixArgs(clus, key), expected)
+func cURLPut(clus *etcdProcessCluster, req cURLReq) error {
+	return spawnWithExpect(cURLPrefixArgs(clus, "PUT", req), req.expected)
 }
 
-func cURLGetUseTLS(clus *etcdProcessCluster, key, expected string) error {
-	return spawnWithExpect(cURLPrefixArgsUseTLS(clus, key), expected)
+func cURLGet(clus *etcdProcessCluster, req cURLReq) error {
+	return spawnWithExpect(cURLPrefixArgs(clus, "GET", req), req.expected)
 }

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)
 		}