Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protect LeaseTimeToLive with RBAC #15656

Merged
merged 2 commits into from
May 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion server/etcdserver/v3_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,32 @@ func (s *EtcdServer) LeaseRenew(ctx context.Context, id lease.LeaseID) (int64, e
return -1, errors.ErrCanceled
}

func (s *EtcdServer) LeaseTimeToLive(ctx context.Context, r *pb.LeaseTimeToLiveRequest) (*pb.LeaseTimeToLiveResponse, error) {
func (s *EtcdServer) checkLeaseTimeToLive(ctx context.Context, leaseID lease.LeaseID) (error, uint64) {
rev := s.AuthStore().Revision()
if !s.AuthStore().IsAuthEnabled() {
return nil, rev
}
authInfo, err := s.AuthInfoFromCtx(ctx)
if err != nil {
return err, rev
}
if authInfo == nil {
return auth.ErrUserEmpty, rev
}

l := s.lessor.Lookup(leaseID)
if l != nil {
for _, key := range l.Keys() {
if err := s.AuthStore().IsRangePermitted(authInfo, []byte(key), []byte{}); err != nil {
return err, 0
}
}
}
Comment on lines +333 to +340
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before iterating over all the keys, I think we also need to check LeaseTimeToLiveRequest.Keys; if it's false, it means users don't want to query al the keys at all, then no need to check the permission either on the server side.

We also need to add test to cover such case, such as the client has no range permission on a key, but it intentionally set LeaseTimeToLiveRequest.Keys to false, then it should not get an error response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment, I'll update tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, could you check @ahrtr ?


return nil, rev
}

func (s *EtcdServer) leaseTimeToLive(ctx context.Context, r *pb.LeaseTimeToLiveRequest) (*pb.LeaseTimeToLiveResponse, error) {
if s.isLeader() {
if err := s.waitAppliedIndex(); err != nil {
return nil, err
Expand Down Expand Up @@ -367,6 +392,30 @@ func (s *EtcdServer) LeaseTimeToLive(ctx context.Context, r *pb.LeaseTimeToLiveR
return nil, errors.ErrCanceled
}

func (s *EtcdServer) LeaseTimeToLive(ctx context.Context, r *pb.LeaseTimeToLiveRequest) (*pb.LeaseTimeToLiveResponse, error) {
var rev uint64
var err error
if r.Keys {
// check RBAC permission only if Keys is true
err, rev = s.checkLeaseTimeToLive(ctx, lease.LeaseID(r.ID))
if err != nil {
return nil, err
}
}

resp, err := s.leaseTimeToLive(ctx, r)
if err != nil {
return nil, err
}

if r.Keys {
if s.AuthStore().IsAuthEnabled() && rev != s.AuthStore().Revision() {
return nil, auth.ErrAuthOldRevision
}
}
return resp, nil
}

func (s *EtcdServer) newHeader() *pb.ResponseHeader {
return &pb.ResponseHeader{
ClusterId: uint64(s.cluster.ID()),
Expand Down
49 changes: 49 additions & 0 deletions tests/e2e/ctl_v3_auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ func TestCtlV3AuthJWTExpire(t *testing.T) {
}
func TestCtlV3AuthRevisionConsistency(t *testing.T) { testCtl(t, authTestRevisionConsistency) }
func TestCtlV3AuthTestCacheReload(t *testing.T) { testCtl(t, authTestCacheReload) }
func TestCtlV3AuthLeaseTimeToLive(t *testing.T) { testCtl(t, authTestLeaseTimeToLive) }

func authEnable(cx ctlCtx) error {
// create root user with root role
Expand Down Expand Up @@ -634,3 +635,51 @@ func authTestCacheReload(cx ctlCtx) {
cx.t.Fatal(err)
}
}

func authTestLeaseTimeToLive(cx ctlCtx) {
if err := authEnable(cx); err != nil {
cx.t.Fatal(err)
}
cx.user, cx.pass = "root", "root"

authSetupTestUser(cx)

cx.user = "test-user"
cx.pass = "pass"

leaseID, err := ctlV3LeaseGrant(cx, 10)
if err != nil {
cx.t.Fatal(err)
}

err = ctlV3Put(cx, "foo", "val", leaseID)
if err != nil {
cx.t.Fatal(err)
}

err = ctlV3LeaseTimeToLive(cx, leaseID, true)
if err != nil {
cx.t.Fatal(err)
}

cx.user = "root"
cx.pass = "root"
err = ctlV3Put(cx, "bar", "val", leaseID)
if err != nil {
cx.t.Fatal(err)
}

cx.user = "test-user"
cx.pass = "pass"
// the lease is attached to bar, which test-user cannot access
err = ctlV3LeaseTimeToLive(cx, leaseID, true)
if err == nil {
cx.t.Fatal("test-user must not be able to access to the lease, because it's attached to the key bar")
}

// without --keys, access should be allowed
err = ctlV3LeaseTimeToLive(cx, leaseID, false)
if err != nil {
cx.t.Fatal(err)
}
}
8 changes: 8 additions & 0 deletions tests/e2e/ctl_v3_lease_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,11 @@ func ctlV3LeaseRevoke(cx ctlCtx, leaseID string) error {
cmdArgs := append(cx.PrefixArgs(), "lease", "revoke", leaseID)
return e2e.SpawnWithExpectWithEnv(cmdArgs, cx.envMap, fmt.Sprintf("lease %s revoked", leaseID))
}

func ctlV3LeaseTimeToLive(cx ctlCtx, leaseID string, withKeys bool) error {
cmdArgs := append(cx.PrefixArgs(), "lease", "timetolive", leaseID)
if withKeys {
cmdArgs = append(cmdArgs, "--keys")
}
return e2e.SpawnWithExpectWithEnv(cmdArgs, cx.envMap, fmt.Sprintf("lease %s granted with", leaseID))
}
91 changes: 86 additions & 5 deletions tests/integration/v3_auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,12 +178,10 @@ func testV3AuthWithLeaseRevokeWithRoot(t *testing.T, ccfg integration.ClusterCon
// wait for lease expire
time.Sleep(3 * time.Second)

tresp, terr := api.Lease.LeaseTimeToLive(
tresp, terr := rootc.TimeToLive(
context.TODO(),
&pb.LeaseTimeToLiveRequest{
ID: int64(leaseID),
Keys: true,
},
leaseID,
clientv3.WithAttachedKeys(),
)
if terr != nil {
t.Error(terr)
Expand Down Expand Up @@ -585,3 +583,86 @@ func TestV3AuthWatchErrorAndWatchId0(t *testing.T) {

<-watchEndCh
}

func TestV3AuthWithLeaseTimeToLive(t *testing.T) {
integration.BeforeTest(t)
clus := integration.NewCluster(t, &integration.ClusterConfig{Size: 1})
defer clus.Terminate(t)

users := []user{
{
name: "user1",
password: "user1-123",
role: "role1",
key: "k1",
end: "k3",
},
{
name: "user2",
password: "user2-123",
role: "role2",
key: "k2",
end: "k4",
},
}
authSetupUsers(t, integration.ToGRPC(clus.Client(0)).Auth, users)

authSetupRoot(t, integration.ToGRPC(clus.Client(0)).Auth)

user1c, cerr := integration.NewClient(t, clientv3.Config{Endpoints: clus.Client(0).Endpoints(), Username: "user1", Password: "user1-123"})
if cerr != nil {
t.Fatal(cerr)
}
defer user1c.Close()

user2c, cerr := integration.NewClient(t, clientv3.Config{Endpoints: clus.Client(0).Endpoints(), Username: "user2", Password: "user2-123"})
if cerr != nil {
t.Fatal(cerr)
}
defer user2c.Close()

leaseResp, err := user1c.Grant(context.TODO(), 90)
if err != nil {
t.Fatal(err)
}
leaseID := leaseResp.ID
_, err = user1c.Put(context.TODO(), "k1", "val", clientv3.WithLease(leaseID))
if err != nil {
t.Fatal(err)
}
// k2 can be accessed from both user1 and user2
_, err = user1c.Put(context.TODO(), "k2", "val", clientv3.WithLease(leaseID))
if err != nil {
t.Fatal(err)
}

_, err = user1c.TimeToLive(context.TODO(), leaseID)
if err != nil {
t.Fatal(err)
}

_, err = user2c.TimeToLive(context.TODO(), leaseID)
if err != nil {
t.Fatal(err)
}

_, err = user2c.TimeToLive(context.TODO(), leaseID, clientv3.WithAttachedKeys())
if err == nil {
t.Fatal("timetolive from user2 should be failed with permission denied")
}

rootc, cerr := integration.NewClient(t, clientv3.Config{Endpoints: clus.Client(0).Endpoints(), Username: "root", Password: "123"})
if cerr != nil {
t.Fatal(cerr)
}
defer rootc.Close()

if _, err := rootc.RoleRevokePermission(context.TODO(), "role1", "k1", "k3"); err != nil {
t.Fatal(err)
}

_, err = user1c.TimeToLive(context.TODO(), leaseID, clientv3.WithAttachedKeys())
if err == nil {
t.Fatal("timetolive from user2 should be failed with permission denied")
}
}