protecting lease revoking with auth #8031

mitake · 2017-06-05T07:31:39Z

Current leases can be revoked by anyone. This PR limits the users who can revoke leases. If the user isn't granted write permission of keys which are attached the lease, the revoking request will be denied.

/cc @yudai

heyitsanthony · 2017-06-05T16:56:19Z

e2e/ctl_v3_auth_test.go

@@ -739,3 +740,36 @@ func authTestWatch(cx ctlCtx) {
 	}

 }
+
+func authTestLease(cx ctlCtx) {


this should probably be in integration/v3_auth_test.go using raw grpc calls since it's testing etcdserver behavior, not etcdctl

OK I'll move the test to integration

heyitsanthony · 2017-06-05T16:57:29Z

approach looks OK, test should be an integration test though

yudai · 2017-06-05T22:03:16Z

If userB creates an entry at a key which userA doesn't have writer permission, but with a lease userA have created, userA cannot revoke the lease any longer?
You might want to add a similar check on Put(), like if the lease is already used by an entry where the user doesn't have write permission, make the Put() request fail.

mitake · 2017-06-06T04:05:14Z

The check seems to be reasonable but it isn't perfect because the write permission can be revoked after attaching the lease and before revoking it. Anyway there wouldn't be a clean solution so I'll add the checking mechanism to Put() in the next update. Assuming that admins don't change the permission during the operation would be realistic.

mitake · 2017-06-06T06:55:58Z

@heyitsanthony updated and now the test is moved to integration. Also I added a new field Expire to LeaseRevokeRequest for distinguishing lease revoking triggered by time progress from explicit requests. In the case of time triggered revoking, permission checking isn't required.

codecov-io · 2017-06-06T08:38:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a36d62a). Click here to learn what that means.
The diff coverage is 50%.

@@            Coverage Diff            @@
##             master    #8031   +/-   ##
=========================================
  Coverage          ?   76.25%           
=========================================
  Files             ?      341           
  Lines             ?    26502           
  Branches          ?        0           
=========================================
  Hits              ?    20210           
  Misses            ?     4820           
  Partials          ?     1472

Impacted Files	Coverage Δ
etcdserver/server.go	`80.13% <100%> (ø)`
auth/simple_token.go	`86.79% <100%> (ø)`
etcdserver/apply.go	`89.04% <100%> (ø)`
auth/store.go	`80.41% <14.28%> (ø)`
etcdserver/apply_auth.go	`75.2% <86.66%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a36d62a...7b68318. Read the comment docs.

heyitsanthony · 2017-06-06T17:45:23Z

etcdserver/apply_auth.go

+	if !lc.Expire {
+		// If lc.Expire == true, it means LeaseRevoke() is invoked by time progress,
+		// no permission check is required.
+		lease := aa.lessor.Lookup(lease.LeaseID(lc.ID))


dedup this code into if err := aa.checkLeasePuts(leaseId); err != nil { return nil, err }?

heyitsanthony · 2017-06-06T17:46:19Z

etcdserver/etcdserverpb/rpc.proto

@@ -679,6 +679,9 @@ message LeaseGrantResponse {
 message LeaseRevokeRequest {
  // ID is the lease ID to revoke. When the ID is revoked, all associated keys will be deleted.
  int64 ID = 1;
+  // expire is the flag for indicating that the revoke request is invoked by expire.
+  // In such a case, permission checking can be skipped.
+  bool expire = 2;


would it be cleaner to submit the expire request with root creds instead of having a flag in the rpc message?

yes, in addition, having a flag in rpc struct is a bad idea because user can specify it even though the API layer can overwrite it...

mitake · 2017-06-07T02:55:12Z

@heyitsanthony updated based on your comments, PTAL.

heyitsanthony · 2017-06-07T03:19:29Z

etcdserver/server.go

@@ -747,7 +747,8 @@ func (s *EtcdServer) run() {
 					}
 					lid := lease.ID
 					s.goAttach(func() {
-						s.LeaseRevoke(s.ctx, &pb.LeaseRevokeRequest{ID: int64(lid)})
+						ctx := context.WithValue(s.ctx, "LeaseRevokeExpire", struct{}{})


Checking for a context value each time in AuthInfoFromCtx just for lease expiration seems wasteful. How about something like:

ctx := s.authStore.WithRoot(s.ctx)

You mean generating a dummy token and attach it to the context? I'm not sure it is effective than current approach because in a case of simple token, it needs to generate and install a token just for one lease revoking, and in a case of jwt token, it needs to sign on the temporal token. Of course adding the branch just for revoking is ugly so I don't have strong opinions about these two options. How do you think?

generating a token should be OK; expiration will be a little more expensive but puts/gets/etc don't have to pay for the context value check. If generating the token is too expensive, it could be amortized by reusing the token/context across multiple lease revoke calls.

ok, then I'll change it in the next update.

mitake · 2017-06-07T08:24:07Z

@heyitsanthony updated for internal token generation, PTAL.

heyitsanthony · 2017-06-07T17:21:07Z

etcdserver/apply_auth.go

@@ -63,6 +65,15 @@ func (aa *authApplierV3) Put(txn mvcc.TxnWrite, r *pb.PutRequest) (*pb.PutRespon
 	if err := aa.as.IsPutPermitted(&aa.authInfo, r.Key); err != nil {
 		return nil, err
 	}
+
+	if err := aa.checkLeasePuts(lease.LeaseID(r.Lease)); err != nil {
+		// The especified lease is already attached with a key that cannot


s/especified/specified

heyitsanthony · 2017-06-07T17:21:53Z

lgtm thanks

Currently clients can revoke any lease without permission. This commit lets etcdserver protect revoking with write permission. This commit adds a mechanism for generating internal token. It is used for indicating that LeaseRevoke was issued internally so it should be able to delete any attached keys.

mitake force-pushed the lease-revoke-auth branch 2 times, most recently from 3b743fe to 27e9afc Compare June 5, 2017 13:40

heyitsanthony reviewed Jun 5, 2017

View reviewed changes

mitake force-pushed the lease-revoke-auth branch from 27e9afc to b396b03 Compare June 6, 2017 06:54

mitake force-pushed the lease-revoke-auth branch from b396b03 to f11118e Compare June 6, 2017 07:40

heyitsanthony suggested changes Jun 6, 2017

View reviewed changes

mitake force-pushed the lease-revoke-auth branch from f11118e to 22a54a6 Compare June 7, 2017 02:49

heyitsanthony reviewed Jun 7, 2017

View reviewed changes

mitake force-pushed the lease-revoke-auth branch from 22a54a6 to 76b1209 Compare June 7, 2017 08:23

heyitsanthony reviewed Jun 7, 2017

View reviewed changes

heyitsanthony approved these changes Jun 7, 2017

View reviewed changes

mitake added 2 commits June 7, 2017 17:46

integration: add test cases for lease revoking with auth

7b68318

mitake force-pushed the lease-revoke-auth branch from 76b1209 to 7b68318 Compare June 8, 2017 00:46

mitake merged commit fa4903c into etcd-io:master Jun 8, 2017

mitake deleted the lease-revoke-auth branch June 8, 2017 04:34

gyuho added the kind/release-note label Jun 8, 2017

gyuho added this to the v3.3.0 milestone Jun 8, 2017

gyuho mentioned this pull request Nov 6, 2017

auth: context.WithValue has string type keys #8826

Closed

gyuho removed the kind/need-release-note-v3.3 label Nov 22, 2017

This was referenced Dec 15, 2017

*: fix server-side lease expire when auth is enabled #9018

Merged

On master, key doesn't be deleted when bound lease expires #9006

Closed

tangcong mentioned this pull request Mar 11, 2020

a data corruption bug in revoking lease when upgrading cluster from v3.2 to v3.3/v3.4+ #11689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protecting lease revoking with auth #8031

protecting lease revoking with auth #8031

mitake commented Jun 5, 2017

heyitsanthony Jun 5, 2017

mitake Jun 6, 2017

heyitsanthony commented Jun 5, 2017

yudai commented Jun 5, 2017

mitake commented Jun 6, 2017

mitake commented Jun 6, 2017

codecov-io commented Jun 6, 2017 •

edited

heyitsanthony Jun 6, 2017

mitake Jun 7, 2017

heyitsanthony Jun 6, 2017

mitake Jun 7, 2017

mitake commented Jun 7, 2017

heyitsanthony Jun 7, 2017

mitake Jun 7, 2017

heyitsanthony Jun 7, 2017

mitake Jun 7, 2017

mitake commented Jun 7, 2017

heyitsanthony Jun 7, 2017

heyitsanthony commented Jun 7, 2017

protecting lease revoking with auth #8031

protecting lease revoking with auth #8031

Conversation

mitake commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyitsanthony commented Jun 5, 2017

yudai commented Jun 5, 2017

mitake commented Jun 6, 2017

mitake commented Jun 6, 2017

codecov-io commented Jun 6, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitake commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitake commented Jun 7, 2017

Choose a reason for hiding this comment

heyitsanthony commented Jun 7, 2017

codecov-io commented Jun 6, 2017 •

edited