diff --git a/e2e/ctl_v2_test.go b/e2e/ctl_v2_test.go index 84e93e733..e2356933e 100644 --- a/e2e/ctl_v2_test.go +++ b/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") diff --git a/e2e/v2_curl_test.go b/e2e/v2_curl_test.go index d804e1ceb..ad1c8487c 100644 --- a/e2e/v2_curl_test.go +++ b/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 { - 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) - } - - 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) } } } +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) + } + }() + + 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) + } + + switch method { + case "PUT": + dt := req.value + if !strings.HasPrefix(dt, "{") { // for non-JSON value + dt = "value=" + dt + } + cmdArgs = append(cmdArgs, "-XPUT", "-d", dt) } - 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) +func cURLPut(clus *etcdProcessCluster, req cURLReq) error { + return spawnWithExpect(cURLPrefixArgs(clus, "PUT", req), req.expected) } -func cURLGet(clus *etcdProcessCluster, key, expected string) error { - return spawnWithExpect(cURLPrefixArgs(clus, key), 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) } diff --git a/etcdserver/auth/auth.go b/etcdserver/auth/auth.go index e59edcbc0..272f2d370 100644 --- a/etcdserver/auth/auth.go +++ b/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) + if ou.User != nu.User { + return out, authErr(http.StatusConflict, "Merging user data with conflicting usernames: %s %s", ou.User, nu.User) } - out.User = u.User - if n.Password != "" { - out.Password = n.Password + 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) } diff --git a/etcdserver/auth/auth_test.go b/etcdserver/auth/auth_test.go index a28a98948..17e91ba23 100644 --- a/etcdserver/auth/auth_test.go +++ b/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) }