diff --git a/e2e/ctl_v3_auth_test.go b/e2e/ctl_v3_auth_test.go index 605762e25..0b15cfd63 100644 --- a/e2e/ctl_v3_auth_test.go +++ b/e2e/ctl_v3_auth_test.go @@ -29,6 +29,11 @@ func TestCtlV3AuthUserDeleteDuringOps(t *testing.T) { testCtl(t, authUserDeleteD func TestCtlV3AuthRoleRevokeDuringOps(t *testing.T) { testCtl(t, authRoleRevokeDuringOpsTest) } func TestCtlV3AuthTxn(t *testing.T) { testCtl(t, authTestTxn) } func TestCtlV3AuthPerfixPerm(t *testing.T) { testCtl(t, authTestPrefixPerm) } +func TestCtlV3AuthMemberAdd(t *testing.T) { testCtl(t, authTestMemberAdd) } +func TestCtlV3AuthMemberRemove(t *testing.T) { + testCtl(t, authTestMemberRemove, withQuorum(), withNoStrictReconfig()) +} +func TestCtlV3AuthMemberUpdate(t *testing.T) { testCtl(t, authTestMemberUpdate) } func authEnableTest(cx ctlCtx) { if err := authEnable(cx); err != nil { @@ -450,3 +455,91 @@ func authTestPrefixPerm(cx ctlCtx) { cx.t.Fatal(err) } } + +func authTestMemberAdd(cx ctlCtx) { + if err := authEnable(cx); err != nil { + cx.t.Fatal(err) + } + + cx.user, cx.pass = "root", "root" + authSetupTestUser(cx) + + peerURL := fmt.Sprintf("http://localhost:%d", etcdProcessBasePort+11) + // ordinal user cannot add a new member + cx.user, cx.pass = "test-user", "pass" + if err := ctlV3MemberAdd(cx, peerURL); err == nil { + cx.t.Fatalf("ordinal user must not be allowed to add a member") + } + + // root can add a new member + cx.user, cx.pass = "root", "root" + if err := ctlV3MemberAdd(cx, peerURL); err != nil { + cx.t.Fatal(err) + } +} + +func authTestMemberRemove(cx ctlCtx) { + if err := authEnable(cx); err != nil { + cx.t.Fatal(err) + } + + cx.user, cx.pass = "root", "root" + authSetupTestUser(cx) + + n1 := cx.cfg.clusterSize + if n1 < 2 { + cx.t.Fatalf("%d-node is too small to test 'member remove'", n1) + } + resp, err := getMemberList(cx) + if err != nil { + cx.t.Fatal(err) + } + if n1 != len(resp.Members) { + cx.t.Fatalf("expected %d, got %d", n1, len(resp.Members)) + } + + var ( + memIDToRemove = fmt.Sprintf("%x", resp.Header.MemberId) + clusterID = fmt.Sprintf("%x", resp.Header.ClusterId) + ) + + // ordinal user cannot remove a member + cx.user, cx.pass = "test-user", "pass" + if err = ctlV3MemberRemove(cx, memIDToRemove, clusterID); err == nil { + cx.t.Fatalf("ordinal user must not be allowed to remove a member") + } + + // root can remove a member + cx.user, cx.pass = "root", "root" + if err = ctlV3MemberRemove(cx, memIDToRemove, clusterID); err != nil { + cx.t.Fatal(err) + } +} + +func authTestMemberUpdate(cx ctlCtx) { + if err := authEnable(cx); err != nil { + cx.t.Fatal(err) + } + + cx.user, cx.pass = "root", "root" + authSetupTestUser(cx) + + mr, err := getMemberList(cx) + if err != nil { + cx.t.Fatal(err) + } + + // ordinal user cannot update a member + cx.user, cx.pass = "test-user", "pass" + peerURL := fmt.Sprintf("http://localhost:%d", etcdProcessBasePort+11) + memberID := fmt.Sprintf("%x", mr.Members[0].ID) + if err = ctlV3MemberUpdate(cx, memberID, peerURL); err == nil { + cx.t.Fatalf("ordinal user must not be allowed to update a member") + } + + // root can update a member + cx.user, cx.pass = "root", "root" + if err = ctlV3MemberUpdate(cx, memberID, peerURL); err != nil { + cx.t.Fatal(err) + } +} diff --git a/e2e/ctl_v3_member_test.go b/e2e/ctl_v3_member_test.go index c272a60a7..cfd99ac2e 100644 --- a/e2e/ctl_v3_member_test.go +++ b/e2e/ctl_v3_member_test.go @@ -98,13 +98,16 @@ func ctlV3MemberRemove(cx ctlCtx, memberID, clusterID string) error { } func memberAddTest(cx ctlCtx) { - peerURL := fmt.Sprintf("http://localhost:%d", etcdProcessBasePort+11) - cmdArgs := append(cx.PrefixArgs(), "member", "add", "newmember", fmt.Sprintf("--peer-urls=%s", peerURL)) - if err := spawnWithExpect(cmdArgs, " added to cluster "); err != nil { + if err := ctlV3MemberAdd(cx, fmt.Sprintf("http://localhost:%d", etcdProcessBasePort+11)); err != nil { cx.t.Fatal(err) } } +func ctlV3MemberAdd(cx ctlCtx, peerURL string) error { + cmdArgs := append(cx.PrefixArgs(), "member", "add", "newmember", fmt.Sprintf("--peer-urls=%s", peerURL)) + return spawnWithExpect(cmdArgs, " added to cluster ") +} + func memberUpdateTest(cx ctlCtx) { mr, err := getMemberList(cx) if err != nil { @@ -112,8 +115,13 @@ func memberUpdateTest(cx ctlCtx) { } peerURL := fmt.Sprintf("http://localhost:%d", etcdProcessBasePort+11) - cmdArgs := append(cx.PrefixArgs(), "member", "update", fmt.Sprintf("%x", mr.Members[0].ID), fmt.Sprintf("--peer-urls=%s", peerURL)) - if err = spawnWithExpect(cmdArgs, " updated in cluster "); err != nil { + memberID := fmt.Sprintf("%x", mr.Members[0].ID) + if err = ctlV3MemberUpdate(cx, memberID, peerURL); err != nil { cx.t.Fatal(err) } } + +func ctlV3MemberUpdate(cx ctlCtx, memberID, peerURL string) error { + cmdArgs := append(cx.PrefixArgs(), "member", "update", memberID, fmt.Sprintf("--peer-urls=%s", peerURL)) + return spawnWithExpect(cmdArgs, " updated in cluster ") +} diff --git a/etcdserver/server.go b/etcdserver/server.go index 6341f6b57..b62d29191 100644 --- a/etcdserver/server.go +++ b/etcdserver/server.go @@ -1007,7 +1007,32 @@ func (s *EtcdServer) LeaderStats() []byte { func (s *EtcdServer) StoreStats() []byte { return s.store.JsonStats() } +func (s *EtcdServer) checkMembershipOperationPermission(ctx context.Context) error { + if s.authStore == nil { + // In the context of ordinal etcd process, s.authStore will never be nil. + // This branch is for handling cases in server_test.go + return nil + } + + // Note that this permission check is done in the API layer, + // so TOCTOU problem can be caused potentially in a schedule like this: + // update membership with user A -> revoke root role of A -> apply membership change + // in the state machine layer + // However, both of membership change and role management requires the root privilege. + // So careful operation by admins can prevent the problem. + authInfo, err := s.authInfoFromCtx(ctx) + if err != nil { + return err + } + + return s.AuthStore().IsAdminPermitted(authInfo) +} + func (s *EtcdServer) AddMember(ctx context.Context, memb membership.Member) error { + if err := s.checkMembershipOperationPermission(ctx); err != nil { + return err + } + if s.Cfg.StrictReconfigCheck { // by default StrictReconfigCheck is enabled; reject new members if unhealthy if !s.cluster.IsReadyToAddNewMember() { @@ -1034,6 +1059,10 @@ func (s *EtcdServer) AddMember(ctx context.Context, memb membership.Member) erro } func (s *EtcdServer) RemoveMember(ctx context.Context, id uint64) error { + if err := s.checkMembershipOperationPermission(ctx); err != nil { + return err + } + // by default StrictReconfigCheck is enabled; reject removal if leads to quorum loss if err := s.mayRemoveMember(types.ID(id)); err != nil { return err @@ -1073,8 +1102,12 @@ func (s *EtcdServer) mayRemoveMember(id types.ID) error { } func (s *EtcdServer) UpdateMember(ctx context.Context, memb membership.Member) error { - b, err := json.Marshal(memb) - if err != nil { + b, merr := json.Marshal(memb) + if merr != nil { + return merr + } + + if err := s.checkMembershipOperationPermission(ctx); err != nil { return err } cc := raftpb.ConfChange{