protect membership change RPCs with auth #6903

mitake · 2016-11-28T08:20:29Z

This PR add a mechanism for protecting the membership change RPCs with auth.

heyitsanthony · 2016-11-28T19:36:35Z

auth/store.go

@@ -150,6 +150,9 @@ type AuthStore interface {

 	// CheckPassword checks a given pair of username and password is correct
 	CheckPassword(username, password string) (uint64, error)
+
+	// IsAuthEnabled checks authentication is enabled or not
+	IsAuthEnabled() bool


IsEnabled stuff leads to a lot of split paths. Ideally it shouldn't be necesary; if auth is disabled either the authstore would be niled out (still has path splitting, though) or the authstore would use a dummy auth that passes everything through.

OK, I'll try to eliminate IsEnabled() in the next update.

So long as it's unexported it's OK. I'd like to get rid of the in-package usage, but what I have in mind would be another patchset since it'll need some changes to the etcdserver apply stack.

Ah, my writing was bad. Of course I didn't intend to remove the function, just only make it local again. I'll add conditions based on authStore is nil or not to etcdserver side.

heyitsanthony · 2016-11-28T19:37:07Z

this seems too invasive; I don't think raftpb should know anything about auth revisions. Maybe reuse Context?

mitake · 2016-11-29T05:46:40Z

@heyitsanthony Which Context do you mean? I agree that changing raftpb is a large change, but the information must be propagated via network. So at least one message struct must be modified. Or is the http header of rafthttp more suitable for the auth revision?

heyitsanthony · 2016-11-30T01:33:02Z

@mitake there's a Context field in raftpb.ConfChange that's being filled out with a json structure in etcdserver. If the auth rev is going to be passed through raft, it probably belongs inside of that (there's already a comment saying it needs to be changed to protobuf). The only drawback is etcd will have to support both the json and protobuf message for a while.

I don't think raft/ or rafthttp/ should know anything about auth if possible since those are lower level packages than etcdserver and auth.

Discussed the raft bits with @xiang90 yesterday; he's OK with doing a check at the api layer (instead of putting it through raft) and risking ignoring a permission revoke between that check and the apply. I assume you want stronger consistency guarantees, though.

mitake · 2016-11-30T06:29:41Z

@heyitsanthony sorry, I missed the field... anyway, both of permission revoke and membership change require the root role, so the risk would be able to be avoided by admins' careful operations. I'll remove the apply side checking and retry in the API layer in the next update.

mitake · 2016-12-06T07:11:28Z

@heyitsanthony updated for your comments.

now authStore.isAuthEnabled() is private again
the version number validation of confchange was removed (instead of it I added a comment of the potential problem)

Could you take a look?

heyitsanthony

approach looks OK in general

heyitsanthony · 2016-12-07T18:39:49Z

e2e/ctl_v3_auth_test.go

+
+	// ordinal user cannot add a new member
+	cx.user, cx.pass = "test-user", "pass"
+	peerURL := fmt.Sprintf("http://localhost:%d", etcdProcessBasePort+11)


these should probably be made full-fledged functions in ctl_v3_member_test.go like func ctlV3MemberAdd(cx ctlCtx) error, etc

OK, I'll add new fledged functions and modify callers in both of ctl_v3_auth_test.go and ctl_v3_member_test.go.

heyitsanthony · 2016-12-07T18:40:57Z

e2e/ctl_v3_auth_test.go

+	// ordinal user cannot remove a member
+	cx.user, cx.pass = "test-user", "pass"
+	cmdArgs := append(cx.PrefixArgs(), "member", "remove", memIDToRemove)
+	if err := spawnWithExpect(cmdArgs, "Error:  etcdserver: permission denied"); err != nil {


err := ctlV3MemberRemove(cx, resp.Header.MemberId, resp.Header.ClusterId)

I'll use the function, thanks.

heyitsanthony · 2016-12-07T18:51:09Z

etcdserver/server.go

+		return err
+	}
+
+	err = s.AuthStore().IsAdminPermitted(authInfo)


return s.AuthStore().IsAdminPermitted(authInfo)

OK. But if we allow EtcdServer.authStore == nil, the return value of s.AuthStore() cannot be used directly?

yeah it would need if s.AuthStore() == nil { return nil } at the start of the function...

heyitsanthony · 2016-12-07T18:54:40Z

etcdserver/server.go

@@ -1002,6 +1002,26 @@ func (s *EtcdServer) LeaderStats() []byte {

 func (s *EtcdServer) StoreStats() []byte { return s.store.JsonStats() }

+func (s *EtcdServer) checkMembershipOperationPermission(ctx context.Context) error {


can this return nil if authStore is nil instead of needing the dummy stuff?

Allowing EtcdServer.authStore == nil is ok for me but it requires many changes because the callers of EtcdServer.AuthStore() must be changed and there are more than 20 callers. Is it ok?

I thought the tests only needed to be updated with non-nil because it started hitting this path? Or is it breaking somewhere else?

For example, we'll need changes in auth related functions of applierV3Backend? The changes won't be costly but will make this PR a little bit bloated.

Huh? Why would all that be necessary? It was working fine allowing authStore == nil in the tests. The server_test.go membership tests now need != nil plus the dummy auth code because this function does not do a nil check. If there's a nil check in this function, all that new code can be removed, right?

Ahh, I misunderstood your intention. You wanted to add the nil check mainly for test code, but I thought that you were talking about the idea of detecting auth is enabled or not with EtcdServer.authStore is nil or not instead of calling EtcdServer.isAuthEnabled() (yes, if this understanding is correct, two ways of detection coexist...). Of course I agree with your idea of reducing the dummy code and add nil check to test code. Sorry for that.

mitake · 2016-12-12T06:45:28Z

@heyitsanthony updated based on your comments, PTAL.

added a nil check to EtcdServer.checkMembershipOperationPermission() and reduced the dummy auth store
unified membership management functions for cleaning e2e

heyitsanthony · 2016-12-12T19:23:23Z

lgtm /cc @xiang90

xiang90 · 2016-12-14T17:39:55Z

etcdserver/server.go

@@ -1015,6 +1036,10 @@ func (s *EtcdServer) AddMember(ctx context.Context, memb membership.Member) erro
 		}
 	}

+	if err := s.checkMembershipOperationPermission(ctx); err != nil {


we probably should move before line 1027.

Seems reasonable, I'll fix it soon.

xiang90 · 2016-12-14T17:40:05Z

etcdserver/server.go

@@ -1034,6 +1059,10 @@ func (s *EtcdServer) RemoveMember(ctx context.Context, id uint64) error {
 		return err
 	}

+	if err := s.checkMembershipOperationPermission(ctx); err != nil {


move this before line 1057?

xiang90 · 2016-12-14T17:40:28Z

Minor issues. LGTM after fixing them. Thanks a lot!

This commit protects membership change operations with auth. Only users that have root role can issue the operations. Implements etcd-io#6899

mitake · 2016-12-15T13:55:21Z

@xiang90 updated for the comments, PTAL.

xiang90 · 2016-12-15T16:04:21Z

lgtm

heyitsanthony reviewed Nov 28, 2016

View reviewed changes

mitake force-pushed the auth-member branch from 35120b3 to 2ab9435 Compare December 6, 2016 07:09

mitake force-pushed the auth-member branch from 2ab9435 to d3e5e8d Compare December 6, 2016 07:17

heyitsanthony suggested changes Dec 7, 2016

View reviewed changes

mitake mentioned this pull request Dec 8, 2016

RFC, WIP: etcdserver: let maintenance services require root role #6898

Merged

mitake force-pushed the auth-member branch from d3e5e8d to ff663f8 Compare December 12, 2016 06:44

mitake force-pushed the auth-member branch from ff663f8 to 0e29f19 Compare December 12, 2016 06:48

heyitsanthony approved these changes Dec 12, 2016

View reviewed changes

xiang90 reviewed Dec 14, 2016

View reviewed changes

mitake added 2 commits December 15, 2016 22:54

auth, etcdserver: protect membership change operations with auth

86d7390

This commit protects membership change operations with auth. Only users that have root role can issue the operations. Implements etcd-io#6899

e2e: test cases of protecting membership change with auth

d46b753

mitake force-pushed the auth-member branch from 0e29f19 to d46b753 Compare December 15, 2016 13:55

xiang90 merged commit 35fd5dc into etcd-io:master Dec 15, 2016

mitake deleted the auth-member branch December 16, 2016 01:24

mitake mentioned this pull request Jan 18, 2017

Member can be removed without authentication even when auth is enabled. #7181

Closed

gyuho mentioned this pull request Jan 24, 2018

Improve membership reconfiguration test coverage #9150

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protect membership change RPCs with auth #6903

protect membership change RPCs with auth #6903

mitake commented Nov 28, 2016

heyitsanthony Nov 28, 2016

mitake Nov 29, 2016

heyitsanthony Nov 30, 2016

mitake Nov 30, 2016

heyitsanthony commented Nov 28, 2016

mitake commented Nov 29, 2016

heyitsanthony commented Nov 30, 2016

mitake commented Nov 30, 2016

mitake commented Dec 6, 2016

heyitsanthony left a comment

heyitsanthony Dec 7, 2016

mitake Dec 8, 2016

heyitsanthony Dec 7, 2016

mitake Dec 8, 2016

heyitsanthony Dec 7, 2016

mitake Dec 8, 2016

heyitsanthony Dec 8, 2016

heyitsanthony Dec 7, 2016

mitake Dec 8, 2016

heyitsanthony Dec 8, 2016

mitake Dec 8, 2016

heyitsanthony Dec 9, 2016

mitake Dec 12, 2016

mitake commented Dec 12, 2016

heyitsanthony commented Dec 12, 2016

xiang90 Dec 14, 2016

mitake Dec 15, 2016

xiang90 Dec 14, 2016

mitake Dec 15, 2016

xiang90 commented Dec 14, 2016

mitake commented Dec 15, 2016

xiang90 commented Dec 15, 2016

		@@ -1002,6 +1002,26 @@ func (s *EtcdServer) LeaderStats() []byte {

		func (s *EtcdServer) StoreStats() []byte { return s.store.JsonStats() }

		func (s *EtcdServer) checkMembershipOperationPermission(ctx context.Context) error {

protect membership change RPCs with auth #6903

protect membership change RPCs with auth #6903

Conversation

mitake commented Nov 28, 2016

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

heyitsanthony commented Nov 28, 2016

mitake commented Nov 29, 2016

heyitsanthony commented Nov 30, 2016

mitake commented Nov 30, 2016

mitake commented Dec 6, 2016

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

Choose a reason for hiding this comment

mitake commented Dec 12, 2016

heyitsanthony commented Dec 12, 2016

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 Dec 14, 2016

mitake commented Dec 15, 2016

xiang90 commented Dec 15, 2016