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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Jan 14, 2017
Merged

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

merged 2 commits into from
Jan 14, 2017

Conversation

mitake
Copy link
Contributor

@mitake mitake commented Nov 24, 2016

This PR lets maintenance services require root role. But Snapshot() isn't protected yet because it doesn't have ctx in its parameter. I'm seeking how to obtain credential information in the function.

@heyitsanthony
Copy link
Contributor

@mitake have you tried srv.Context() for getting the auth info for the snapshot stream?

@mitake
Copy link
Contributor Author

mitake commented Nov 29, 2016

@heyitsanthony I didn't notice the interface... I'll try it, thanks!

@mitake
Copy link
Contributor Author

mitake commented Dec 7, 2016

@heyitsanthony I added a protection mechanism to Snapshot() with srv.Context() based on your comment. Also e2e test cases were added for Defragment() and Snapshot(). But cases for Hash() and Status() aren't added yet because they lacks corresponding etcdctl commands. Maybe integration pkg is suitable place for the cases?

heyitsanthony
heyitsanthony suggested changes Dec 7, 2016
View reviewed changes
Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

approach looks OK

e2e/ctl_v3_defrag_test.go

func defragTest(cx ctlCtx) {
func initKeys(cx ctlCtx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maintenanceInitKeys?

e2e/ctl_v3_snapshot_test.go
func TestCtlV3SnapshotWithAuth(t *testing.T) { testCtl(t, snapshotTestWithAuth) }

func snapshotTestWithAuth(cx ctlCtx) {
var kvs = []kv{{"key", "val1"}, {"key", "val2"}, {"key", "val3"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

maintenanceInitKeys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

etcdserver/api/v3rpc/maintenance.go
}

as := ms.ag.AuthStore()
err = as.IsAdminPermitted(authInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

return ms.ag.AuthStore().IsAdminPermitted(authInfo)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it after finishing another ongoing PR: #6903

etcdserver/api/v3rpc/maintenance.go
AuthInfoFromCtx(ctx context.Context) (*auth.AuthInfo, error)
AuthStore() auth.AuthStore
}

type maintenanceServer struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

can there be a separate type returned by NewMaintenanceServer:

type authMaintenanceServer struct {
    ag AuthGetter
    ms *maintenanceServer
}

that does the auth checks before calling into the auth-free maintenance server? This buys type-checking that auth covers all calls for the service since if it misses a check it won't match the interface. I'd like to avoid mingling auth code with non-auth code if it's not strictly necessary...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll try it

etcdserver/v3_server.go
@@ -639,7 +639,7 @@ func (s *EtcdServer) isValidSimpleToken(token string) bool {
}
}

func (s *EtcdServer) authInfoFromCtx(ctx context.Context) (*auth.AuthInfo, error) {
func (s *EtcdServer) AuthInfoFromCtx(ctx context.Context) (*auth.AuthInfo, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like this belongs in the auth package if it's important enough to export? can probably pass a func f(idx uint64) <-chan to get around the etcdserver dependency in isValidToken where it waits for the revision update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll try to move the function to auth pkg.

@mitake mitake force-pushed the auth-maintain branch 2 times, most recently from 9962061 to 8e6e6e5 Compare January 13, 2017 06:00
@mitake
Copy link
Contributor Author

mitake commented Jan 13, 2017

@heyitsanthony updated based on your comments, PTAL.

heyitsanthony
heyitsanthony suggested changes Jan 13, 2017
View reviewed changes
Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

minor nit about warnings

etcdserver/api/v3rpc/maintenance.go

func (ams *authMaintenanceServer) Defragment(ctx context.Context, sr *pb.DefragmentRequest) (*pb.DefragmentResponse, error) {
if err := ams.isAuthenticated(ctx); err != nil {
plog.Warningf("invalid Degragment request was issued: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

drop the warnings? other auth failures don't warn, so it's inconsistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll remove the warnings.

@heyitsanthony
Copy link
Contributor

@mitake maintenance commands with auth enabled will still work even if the cluster lost quorum, right? Otherwise, lgtm.

defer to @xiang90

xiang90
xiang90 reviewed Jan 13, 2017
View reviewed changes
auth/store.go
@@ -153,6 +156,9 @@ type AuthStore interface {

// Close does cleanup of AuthStore
Close() error

// AuthInfoFromCtx gets AuthInfo from gRPC's context
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the mention of gRPC? then this interface seems more generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In AuthInfoFromCtx(), gRPC's metadata is obtained by metadata.FromContext(). So I think the context cannot be said generic.

@xiang90
Copy link
Contributor

xiang90 commented Jan 13, 2017

LGTM.

@xiang90
Copy link
Contributor

xiang90 commented Jan 13, 2017

@heyitsanthony Can you approve this pr? We can add a test for the quorum loss case. it is important to ensure that we can still get a snapshot at least to recover the cluster.

@mitake
Copy link
Contributor Author

mitake commented Jan 14, 2017

@heyitsanthony as @xiang90 says, I also think that quorum loss cases shouldn't prevent the maintenance RPCs and its auth. They are similar to serializable range requests.

mitake added 2 commits January 14, 2017 19:36
@mitake
auth, etcdserver: let maintenance services require root role
9886e94 
This commit lets maintenance services require root privilege. It also
moves AuthInfoFromCtx() from etcdserver to auth pkg for cleaning purpose.
@mitake
e2e: add cases for defrag and snapshot with authentication
783eaf9
@mitake
Copy link
Contributor Author

mitake commented Jan 14, 2017

@heyitsanthony @xiang90 removed the needless warnings. I kept the comment that mentions gRPC, how do you think?

@codecov-io
Copy link

codecov-io commented Jan 14, 2017 •
edited
Loading

Current coverage is 64.01% (diff: 36.11%)

No coverage report found for master at 118fd18.

Powered by Codecov. Last update 118fd18...1315a2b

@xiang90
Copy link
Contributor

xiang90 commented Jan 14, 2017

ok. lgtm. @heyitsanthony Can you approve this pr?

@heyitsanthony
Copy link
Contributor

heyitsanthony commented Jan 14, 2017 •
edited
Loading

just wanted to confirm it'd work under quorum loss, sorry for the confusion!

lgtm. Thanks!

@xiang90 xiang90 merged commit 26d9926 into etcd-io:master Jan 14, 2017
@mitake mitake deleted the auth-maintain branch January 15, 2017 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiang90 xiang90 xiang90 left review comments

@heyitsanthony heyitsanthony heyitsanthony approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mitake @heyitsanthony @xiang90 @codecov-io @gyuho