protect watch with auth #7809

mitake · 2017-04-25T05:52:29Z

@heyitsanthony how do you think about this change? I couldn't separate auth and watch in v3rpc in a clean manner (like maintenance RPCs) because its stream RPC structure was hard to decompose. I'd like to hear your opinion.

heyitsanthony · 2017-04-25T19:47:31Z

etcdserver/api/v3rpc/watch.go

@@ -180,6 +200,19 @@ func (sws *serverWatchStream) recvLoop() error {
 				// support  >= key queries
 				creq.RangeEnd = []byte{}
 			}
+
+			if !sws.isWatchPermitted(creq) {
+				wr := &pb.WatchResponse{


This should fill out all of the fields like the regular error path below. Older clients will be confused by this behavior since they'll get id=0 and not know about permission_denied and think the watch was successfully created.

heyitsanthony · 2017-04-25T19:50:24Z

clientv3/watch.go

@@ -462,6 +471,9 @@ func (w *watchGrpcStream) run() {
 					close(ws.recvc)
 					closing[ws] = struct{}{}
 				}
+			case pbresp.PermissionDenied:


this should be handled through the pbresp.Created path if it's only going to happen on create events; it should work as-is if returning a createresponse with canceled=true and id=-1.

heyitsanthony · 2017-04-25T19:52:40Z

etcdserver/etcdserverpb/rpc.proto

@@ -653,6 +653,9 @@ message WatchResponse {
  // The client should treat the watcher as canceled and should not try to create any
  // watcher with the same start_revision again.
  int64 compact_revision  = 5;
+  // permissionDenied is set to true if the requesting user doesn't have a
+  // read permission of watched keys.
+  bool permission_denied = 6;


I'm hesitant to include boolean flags to indicate errors since errors will be mutually exclusive. Maybe a string that can be translated back to a grpc error?

Thanks for your review and sorry for my late reply. I added the field (and other changes you pointed out problems) for expressing the error of permission denied. But as you say, if it can be expressed in the error code, it would be more straightfoward. I'll change the error propagation style in the next update.

heyitsanthony · 2017-04-25T19:55:56Z

clientv3/client.go

@@ -93,7 +93,10 @@ func NewFromURL(url string) (*Client, error) {
 // Close shuts down the client's etcd connections.
 func (c *Client) Close() error {
 	c.cancel()
-	c.Watcher.Close()
+	err := c.Watcher.Close()
+	if err != nil {


Close should probably keep going regardless of whether if there's an error, otherwise it'll leak lease resources if there's any watch error

heyitsanthony · 2017-04-25T19:56:17Z

clientv3/watch.go

@@ -324,6 +329,10 @@ func (w *watchGrpcStream) Close() (err error) {
 	case err = <-w.errc:
 	default:
 	}
+
+	if w.closeErr != nil {


why is this needed now?

heyitsanthony · 2017-04-25T19:59:11Z

e2e/ctl_v3_auth_test.go

+					cx.t.Fatalf("watchTest #%d-%d: ctlV3Put error (%v)", i, j, err)
+				}
+			}
+			close(donec)


defer close(donec) at top of goroutine; otherwise the fatal will cause the test to wait until go test timeout

heyitsanthony · 2017-04-25T20:02:23Z

e2e/ctl_v3_watch_test.go

@@ -116,3 +116,30 @@ func ctlV3Watch(cx ctlCtx, args []string, kvs ...kv) error {
 	}
 	return proc.Stop()
 }
+


share setup code from cmdArgs := to if cx.interactive { ... } instead of duplicating: ctlV3WatchProc(cx ctlCtx, args []string) (*expect.ExpectProcess, error)

xiang90 · 2017-05-15T18:52:19Z

@mitake kindly ping. Can we get this in soon? it blocks our 3.2 release. The approach looks good.

mitake · 2017-05-16T01:54:18Z

@xiang90 sorry for that, I'll update this PR today!

mitake · 2017-05-16T04:06:36Z

@heyitsanthony @xiang90 In this version, I tried to propagate the permission denied error via WatchResponse. As @heyitsanthony pointed out, it is overkill and the error should be propagated an ordinary error.

I think watchGrpcStream.closeErr would be a suitable way for the propagation. But they are removed too early here: https://github.com/coreos/etcd/blob/master/clientv3/watch.go#L412 so the path of watcher.Close() cannot obtain the error with the variable. Is it ok to defer deleting the streams? I think w.streams shouldn't be deleted here https://github.com/coreos/etcd/blob/master/clientv3/watch.go#L334-L336 because watcher.Close() should collect errors from the streams here: https://github.com/coreos/etcd/blob/master/clientv3/watch.go#L312-L316

I'm still thinking about this change breaks semantics of clientv3. I'd like to hear your comments.

mitake · 2017-05-16T04:23:00Z

mitake@e1103cc
This is a WIP version of my idea.

mitake · 2017-05-16T09:05:46Z

@heyitsanthony @xiang90 Anyway, I updated this PR based on your feedback. If the way of error propagation isn't suitable, I'll revisit it again.

mitake · 2017-05-16T12:33:10Z

clientv3/client.go

@@ -93,8 +93,11 @@ func NewFromURL(url string) (*Client, error) {
 // Close shuts down the client's etcd connections.
 func (c *Client) Close() error {
 	c.cancel()
-	c.Watcher.Close()
+	closeErr := c.Watcher.Close()


This line is breaking TestGRPCResolver and TestGRPCResolverMulti because it changes behaviour of integration.ClusterV3.Terminate()...

Anyway, current Client.Close() ignores an error from c.Watcher.Close(), do you have good idea for receiving the watcher error? @heyitsanthony @xiang90

heyitsanthony · 2017-05-16T17:09:44Z

@mitake an auth error probably shouldn't take down the entire watch stream (e.g., by returning an error on the stream); there would have to be an extra error path outside of watch channels just for handling auth errors. Sending a response with Canceled = true from the etcd server on watch create should be enough to close down the watch (although the reason for the cancel won't be clear)

mitake · 2017-05-17T02:07:48Z

@heyitsanthony I see, then as you mentioned in the comment (#7809 (comment)), a new member for indicating the reason should be in WatchResponse. I think string is too generic, how about adding a new enum?

mitake · 2017-05-17T03:37:06Z

@heyitsanthony I updated with the enum approach, how about this?

mitake · 2017-05-17T04:23:22Z

@heyitsanthony sorry it seems that I misunderstood your intention. An auth error shouldn't do closing an entire watch stream like this: https://github.com/coreos/etcd/pull/7809/files#diff-bdef56e462f289cae8e13ebbe16ba455R476 ? The error should be handled in the receiver side, is this ok?

mitake · 2017-05-17T06:15:35Z

@heyitsanthony @xiang90 I let clientv3 return canceled events to its client. I think the latest PR can handle the permission denied error in a suitable manner and preserve the semantics of existing watch APIs. Could you take a look?

heyitsanthony · 2017-05-17T17:12:05Z

@mitake using string for the error is important here since the strings can map directly to errors etcd already has via rpctypes.Error(). Adding an enum for watch errors means another path for translating rpc errors into client-side error types; it's simpler to return arbitrary etcd errors because it avoids the need to handle a separate class of errors...

heyitsanthony · 2017-05-17T17:29:20Z

clientv3/watch.go

-					// signal to stream goroutine to update closingc
-					close(ws.recvc)
-					closing[ws] = struct{}{}
+				if pbresp.CancelReason == pb.WatchResponse_PERMISSION_DENIED {


does this really need special handling? why isn't the regular cancel=true path enough?

I thought the special handling is required because the error of permission denied will be replied from etcd to the client before registering a substream so the new dispatchCanceledEvent() is needed. Otherwise, the client cannot know the reason of the cancel. In a case of etcdctl, the command cannot print a detailed reason for a user.

mitake · 2017-05-18T02:12:42Z

@heyitsanthony ok then I'll change the type of the filed for cancel reason from enum to string in the next update.

mitake · 2017-05-18T05:10:32Z

@heyitsanthony updated for changing the reason field from enum to string. Could you take a look? The way of propagating the error isn't changed now. If the change is required, I'll do it in next update.

heyitsanthony

in general OK; I believe the patch can be simplified by setting Created: true in the error response like the other Canceled: true path (tried it out, seems to work OK)

heyitsanthony · 2017-05-18T20:17:10Z

clientv3/watch.go

@@ -65,6 +66,9 @@ type WatchResponse struct {
 	Created bool

 	closeErr error
+
+	// CancelReason is a reason of canceling watch
+	CancelReason string


cancelReason? data can be accessed through Err()

Ah, it should be private. I'll fix it in the next update. Thanks.

heyitsanthony · 2017-05-18T20:18:15Z

clientv3/watch.go

@@ -85,6 +89,9 @@ func (wr *WatchResponse) Err() error {
 	case wr.CompactRevision != 0:
 		return v3rpc.ErrCompacted
 	case wr.Canceled:
+		if len(wr.CancelReason) != 0 {
+			return errors.New(wr.CancelReason)


should be able to reuse rpctypes errors here:

return v3rpc.Error(grpc.Errorf(codes.FailedPrecondition, "%s", wr.cancelReason))

Thanks, I'll ueuse the code.

heyitsanthony · 2017-05-18T20:19:30Z

etcdserver/etcdserverpb/rpc.proto

@@ -654,6 +654,9 @@ message WatchResponse {
  // watcher with the same start_revision again.
  int64 compact_revision  = 5;

+  // cancel_reason indicates a reason of cancel.


s/a reason of cancel/the reason for canceling the watcher./

I'll fix it in the next update.

heyitsanthony · 2017-05-18T20:38:37Z

etcdctl/ctlv3/command/watch_command.go

@@ -129,6 +129,9 @@ func getWatchChan(c *clientv3.Client, args []string) (clientv3.WatchChan, error)

 func printWatchCh(ch clientv3.WatchChan) {
 	for resp := range ch {
+		if resp.Canceled {


"watch was canceled (%v)\n"?

heyitsanthony · 2017-05-18T20:44:06Z

etcdserver/api/v3rpc/watch.go

+					Header:       sws.newResponseHeader(sws.watchStream.Rev()),
+					WatchId:      -1,
+					Canceled:     true,
+					CancelReason: rpctypes.ErrGRPCPermissionDenied.Error(),


add Created: true since it's a response to a watch create

heyitsanthony · 2017-05-18T20:49:03Z

clientv3/watch.go

-					// signal to stream goroutine to update closingc
-					close(ws.recvc)
-					closing[ws] = struct{}{}
+				if len(pbresp.CancelReason) != 0 {


client probably doesn't need this new path if the server sets Created to true when returning the error response

I tried the idea of setting Created as true, but from the cancel reason couldn't propagated to a client. This is because pbresp.WatchId is -1 so the existing path of pbresp.Created cannot reply the watch response to the client. It is because handling the canceled response with the path of created will close watchGrpcStream.revc in watchGrpcStream.addSubstream().
This is why I added the new path and new function dispatchCanceledEvent(): https://github.com/coreos/etcd/pull/7809/files#diff-bdef56e462f289cae8e13ebbe16ba455R571

Of course the watching can be canceled but the client cannot know the reason. How do you think?

heyitsanthony · 2017-05-18T20:50:39Z

clientv3/watch.go

@@ -605,7 +640,7 @@ func (w *watchGrpcStream) serveSubstream(ws *watcherStream, resumec chan struct{
 				return
 			}

-			if wr.Created {
+			if wr.Created || wr.Canceled {


|| wr.Canceled not needed if Created is true in the server response?

mitake · 2017-05-19T08:26:54Z

@heyitsanthony anyway, I updated minor coding style problems. The error propagation way are currently kept as before. If the cancel reason can be propagated with a more simpler way, of course I'll change it.

heyitsanthony · 2017-05-19T17:16:14Z

@mitake Seriously, it needs Created: true (see https://github.com/heyitsanthony/etcd/tree/auth-watch-created-true) or old clients will hang:

With created=true:

[anthony@etcd]$ ./bin/etcd-create-true &
[anthony@etcd]$ ETCDCTL_API=3 ./bin/etcdctl user add root:root
User root created
[anthony@etcd]$ ETCDCTL_API=3 ./bin/etcdctl auth enable
Authentication Enabled
[anthony@etcd]$ ETCDCTL_API=3 ./bin/etcdctl-create-true watch abc
Error:  watch is canceled by the server
[anthony@etcd]$ ETCDCTL_API=3 ./bin/etcdctl-3.1.8 watch abc
Error:  watch is canceled by the server

Without created=true:

[anthony@etcd]$ ./bin/etcd-no-create-true &
[anthony@etcd]$ ETCDCTL_API=3 ./bin/etcdctl-3.1.8 watch abc
[hangs]
[anthony@etcd]$ ETCDCTL_API=3 ./bin/etcdctl-no-create-true -w simple watch abc
watch was canceled (rpc error: code = FailedPrecondition desc = rpc error: code = PermissionDenied desc = etcdserver: permission denied)
Error:  watch is canceled by the server

I'm more concerned about clients hanging than trying to propagate the specific error through the client at this point.

mitake · 2017-05-20T02:36:26Z

@heyitsanthony I see, the problem of hanging seems to be important. I picked and squashed your commit and added a todo of e2e (https://github.com/coreos/etcd/pull/7809/files#diff-12807139fa89b9ce1993cb698466b7ecR141). Could you take a look?

heyitsanthony · 2017-05-20T02:46:51Z

lgtm once CI greenlights. thanks!

mitake · 2017-05-20T04:23:26Z

@heyitsanthony thanks for your detailed review! CI passed so I'm merging it.

heyitsanthony suggested changes Apr 25, 2017

View reviewed changes

mitake force-pushed the auth-watch branch from aa589ee to 4b4d239 Compare May 16, 2017 09:05

mitake commented May 16, 2017

View reviewed changes

mitake force-pushed the auth-watch branch from 4b4d239 to 3443df3 Compare May 17, 2017 03:36

mitake force-pushed the auth-watch branch from 3443df3 to a8e525e Compare May 17, 2017 06:14

mitake force-pushed the auth-watch branch 2 times, most recently from bb0876e to a82370c Compare May 17, 2017 06:26

heyitsanthony reviewed May 17, 2017

View reviewed changes

mitake force-pushed the auth-watch branch from a82370c to 99b1c3c Compare May 18, 2017 05:09

heyitsanthony suggested changes May 18, 2017

View reviewed changes

mitake force-pushed the auth-watch branch from 99b1c3c to ee5c1f9 Compare May 19, 2017 08:25

mitake added 2 commits May 20, 2017 11:34

clientv3, etcdserver: support auth in Watch()

939912c

e2e: add a new test case for protecting watch with auth

881903b

mitake force-pushed the auth-watch branch from ee5c1f9 to 881903b Compare May 20, 2017 02:34

heyitsanthony approved these changes May 20, 2017

View reviewed changes

mitake merged commit 4cd5e7e into etcd-io:master May 20, 2017

mitake deleted the auth-watch branch May 20, 2017 04:23

mitake mentioned this pull request May 25, 2017

revisit error code propagation for protecting watch with auth #7988

Closed

protect watch with auth #7809

protect watch with auth #7809

Conversation

mitake commented Apr 25, 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

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

xiang90 commented May 15, 2017

mitake commented May 16, 2017

mitake commented May 16, 2017

mitake commented May 16, 2017

mitake commented May 16, 2017

Choose a reason for hiding this comment

heyitsanthony commented May 16, 2017

mitake commented May 17, 2017

mitake commented May 17, 2017

mitake commented May 17, 2017

mitake commented May 17, 2017

heyitsanthony commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitake commented May 18, 2017

mitake commented May 18, 2017 • edited Loading

heyitsanthony left a comment

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

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

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

Choose a reason for hiding this comment

mitake commented May 19, 2017

heyitsanthony commented May 19, 2017

mitake commented May 20, 2017

heyitsanthony commented May 20, 2017

mitake commented May 20, 2017

mitake commented May 18, 2017 •

edited

Loading