From 0c655902f21d4e3fc37094b8e129f2321d1683d9 Mon Sep 17 00:00:00 2001 From: Hitoshi Mitake Date: Mon, 5 Jun 2017 16:20:09 +0900 Subject: [PATCH 1/2] auth, etcdserver: protect revoking lease with auth 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. --- auth/simple_token.go | 3 ++- auth/store.go | 35 +++++++++++++++++++++++++++++++++++ etcdserver/apply.go | 1 + etcdserver/apply_auth.go | 37 ++++++++++++++++++++++++++++++++++--- etcdserver/server.go | 3 ++- 5 files changed, 74 insertions(+), 5 deletions(-) diff --git a/auth/simple_token.go b/auth/simple_token.go index 94d92a115e2..e39678d6cf3 100644 --- a/auth/simple_token.go +++ b/auth/simple_token.go @@ -118,6 +118,8 @@ func (t *tokenSimple) genTokenPrefix() (string, error) { func (t *tokenSimple) assignSimpleTokenToUser(username, token string) { t.simpleTokensMu.Lock() + defer t.simpleTokensMu.Unlock() + _, ok := t.simpleTokens[token] if ok { plog.Panicf("token %s is alredy used", token) @@ -125,7 +127,6 @@ func (t *tokenSimple) assignSimpleTokenToUser(username, token string) { t.simpleTokens[token] = username t.simpleTokenKeeper.addSimpleToken(token) - t.simpleTokensMu.Unlock() } func (t *tokenSimple) invalidateUser(username string) { diff --git a/auth/store.go b/auth/store.go index 20b57f28489..f3cbd6bba17 100644 --- a/auth/store.go +++ b/auth/store.go @@ -162,6 +162,9 @@ type AuthStore interface { // AuthInfoFromTLS gets AuthInfo from TLS info of gRPC's context AuthInfoFromTLS(ctx context.Context) *AuthInfo + + // WithRoot generates and installs a token that can be used as a root credential + WithRoot(ctx context.Context) context.Context } type TokenProvider interface { @@ -1057,3 +1060,35 @@ func NewTokenProvider(tokenOpts string, indexWaiter func(uint64) <-chan struct{} return nil, ErrInvalidAuthOpts } } + +func (as *authStore) WithRoot(ctx context.Context) context.Context { + if !as.isAuthEnabled() { + return ctx + } + + var ctxForAssign context.Context + if ts := as.tokenProvider.(*tokenSimple); ts != nil { + ctx1 := context.WithValue(ctx, "index", uint64(0)) + prefix, err := ts.genTokenPrefix() + if err != nil { + plog.Errorf("failed to generate prefix of internally used token") + return ctx + } + ctxForAssign = context.WithValue(ctx1, "simpleToken", prefix) + } else { + ctxForAssign = ctx + } + + token, err := as.tokenProvider.assign(ctxForAssign, "root", as.Revision()) + if err != nil { + // this must not happen + plog.Errorf("failed to assign token for lease revoking: %s", err) + return ctx + } + + mdMap := map[string]string{ + "token": token, + } + tokenMD := metadata.New(mdMap) + return metadata.NewContext(ctx, tokenMD) +} diff --git a/etcdserver/apply.go b/etcdserver/apply.go index 426f8019531..9d520767720 100644 --- a/etcdserver/apply.go +++ b/etcdserver/apply.go @@ -84,6 +84,7 @@ func (s *EtcdServer) newApplierV3() applierV3 { return newAuthApplierV3( s.AuthStore(), newQuotaApplierV3(s, &applierV3backend{s}), + s.lessor, ) } diff --git a/etcdserver/apply_auth.go b/etcdserver/apply_auth.go index 7da4ae45df5..5cd74018dac 100644 --- a/etcdserver/apply_auth.go +++ b/etcdserver/apply_auth.go @@ -19,12 +19,14 @@ import ( "github.com/coreos/etcd/auth" pb "github.com/coreos/etcd/etcdserver/etcdserverpb" + "github.com/coreos/etcd/lease" "github.com/coreos/etcd/mvcc" ) type authApplierV3 struct { applierV3 - as auth.AuthStore + as auth.AuthStore + lessor lease.Lessor // mu serializes Apply so that user isn't corrupted and so that // serialized requests don't leak data from TOCTOU errors @@ -33,8 +35,8 @@ type authApplierV3 struct { authInfo auth.AuthInfo } -func newAuthApplierV3(as auth.AuthStore, base applierV3) *authApplierV3 { - return &authApplierV3{applierV3: base, as: as} +func newAuthApplierV3(as auth.AuthStore, base applierV3, lessor lease.Lessor) *authApplierV3 { + return &authApplierV3{applierV3: base, as: as, lessor: lessor} } func (aa *authApplierV3) Apply(r *pb.InternalRaftRequest) *applyResult { @@ -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 specified lease is already attached with a key that cannot + // be written by this user. It means the user cannot revoke the + // lease so attaching the lease to the newly written key should + // be forbidden. + return nil, err + } + if r.PrevKv { err := aa.as.IsRangePermitted(&aa.authInfo, r.Key, nil) if err != nil { @@ -158,6 +169,26 @@ func (aa *authApplierV3) Txn(rt *pb.TxnRequest) (*pb.TxnResponse, error) { return aa.applierV3.Txn(rt) } +func (aa *authApplierV3) LeaseRevoke(lc *pb.LeaseRevokeRequest) (*pb.LeaseRevokeResponse, error) { + if err := aa.checkLeasePuts(lease.LeaseID(lc.ID)); err != nil { + return nil, err + } + return aa.applierV3.LeaseRevoke(lc) +} + +func (aa *authApplierV3) checkLeasePuts(leaseID lease.LeaseID) error { + lease := aa.lessor.Lookup(leaseID) + if lease != nil { + for _, key := range lease.Keys() { + if err := aa.as.IsPutPermitted(&aa.authInfo, []byte(key)); err != nil { + return err + } + } + } + + return nil +} + func needAdminPermission(r *pb.InternalRaftRequest) bool { switch { case r.AuthEnable != nil: diff --git a/etcdserver/server.go b/etcdserver/server.go index a410845b781..0bb0885f9c0 100644 --- a/etcdserver/server.go +++ b/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 := s.authStore.WithRoot(s.ctx) + s.LeaseRevoke(ctx, &pb.LeaseRevokeRequest{ID: int64(lid)}) <-c }) } From 7b68318284bd657d46b9842e38a7bc38d01fc84d Mon Sep 17 00:00:00 2001 From: Hitoshi Mitake Date: Tue, 6 Jun 2017 15:53:15 +0900 Subject: [PATCH 2/2] integration: add test cases for lease revoking with auth --- integration/v3_auth_test.go | 160 +++++++++++++++++++++++++++++++++++- 1 file changed, 156 insertions(+), 4 deletions(-) diff --git a/integration/v3_auth_test.go b/integration/v3_auth_test.go index 06ee68ec967..7ee8c0698b0 100644 --- a/integration/v3_auth_test.go +++ b/integration/v3_auth_test.go @@ -20,6 +20,7 @@ import ( "golang.org/x/net/context" + "github.com/coreos/etcd/auth/authpb" "github.com/coreos/etcd/clientv3" "github.com/coreos/etcd/etcdserver/api/v3rpc/rpctypes" pb "github.com/coreos/etcd/etcdserver/etcdserverpb" @@ -104,16 +105,167 @@ func TestV3AuthRevision(t *testing.T) { } } -func authSetupRoot(t *testing.T, auth pb.AuthClient) { - if _, err := auth.UserAdd(context.TODO(), &pb.AuthUserAddRequest{Name: "root", Password: "123"}); err != nil { +type user struct { + name string + password string + role string + key string + end string +} + +func TestV3AuthWithLeaseRevoke(t *testing.T) { + defer testutil.AfterTest(t) + clus := NewClusterV3(t, &ClusterConfig{Size: 1}) + defer clus.Terminate(t) + + users := []user{ + { + name: "user1", + password: "user1-123", + role: "role1", + key: "k1", + end: "k2", + }, + } + authSetupUsers(t, toGRPC(clus.Client(0)).Auth, users) + + authSetupRoot(t, toGRPC(clus.Client(0)).Auth) + + rootc, cerr := clientv3.New(clientv3.Config{Endpoints: clus.Client(0).Endpoints(), Username: "root", Password: "123"}) + if cerr != nil { + t.Fatal(cerr) + } + defer rootc.Close() + + leaseResp, err := rootc.Grant(context.TODO(), 90) + if err != nil { t.Fatal(err) } - if _, err := auth.RoleAdd(context.TODO(), &pb.AuthRoleAddRequest{Name: "root"}); err != nil { + leaseID := leaseResp.ID + // permission of k3 isn't granted to user1 + _, err = rootc.Put(context.TODO(), "k3", "val", clientv3.WithLease(leaseID)) + if err != nil { t.Fatal(err) } - if _, err := auth.UserGrantRole(context.TODO(), &pb.AuthUserGrantRoleRequest{User: "root", Role: "root"}); err != nil { + + userc, cerr := clientv3.New(clientv3.Config{Endpoints: clus.Client(0).Endpoints(), Username: "user1", Password: "user1-123"}) + if cerr != nil { + t.Fatal(cerr) + } + defer userc.Close() + _, err = userc.Revoke(context.TODO(), leaseID) + if err == nil { + t.Fatal("revoking from user1 should be failed with permission denied") + } +} + +func TestV3AuthWithLeaseAttach(t *testing.T) { + defer testutil.AfterTest(t) + clus := NewClusterV3(t, &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, toGRPC(clus.Client(0)).Auth, users) + + authSetupRoot(t, toGRPC(clus.Client(0)).Auth) + + user1c, cerr := clientv3.New(clientv3.Config{Endpoints: clus.Client(0).Endpoints(), Username: "user1", Password: "user1-123"}) + if cerr != nil { + t.Fatal(cerr) + } + defer user1c.Close() + + user2c, cerr := clientv3.New(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 + // permission of k2 is also granted to user2 + _, err = user1c.Put(context.TODO(), "k2", "val", clientv3.WithLease(leaseID)) + if err != nil { + t.Fatal(err) + } + + _, err = user2c.Revoke(context.TODO(), leaseID) + if err != nil { + t.Fatal(err) + } + + leaseResp, err = user1c.Grant(context.TODO(), 90) + if err != nil { + t.Fatal(err) + } + leaseID = leaseResp.ID + // permission of k1 isn't granted to user2 + _, err = user1c.Put(context.TODO(), "k1", "val", clientv3.WithLease(leaseID)) + if err != nil { t.Fatal(err) } + + _, err = user2c.Revoke(context.TODO(), leaseID) + if err == nil { + t.Fatal("revoking from user2 should be failed with permission denied") + } +} + +func authSetupUsers(t *testing.T, auth pb.AuthClient, users []user) { + for _, user := range users { + if _, err := auth.UserAdd(context.TODO(), &pb.AuthUserAddRequest{Name: user.name, Password: user.password}); err != nil { + t.Fatal(err) + } + if _, err := auth.RoleAdd(context.TODO(), &pb.AuthRoleAddRequest{Name: user.role}); err != nil { + t.Fatal(err) + } + if _, err := auth.UserGrantRole(context.TODO(), &pb.AuthUserGrantRoleRequest{User: user.name, Role: user.role}); err != nil { + t.Fatal(err) + } + + if len(user.key) == 0 { + continue + } + + perm := &authpb.Permission{ + PermType: authpb.READWRITE, + Key: []byte(user.key), + RangeEnd: []byte(user.end), + } + if _, err := auth.RoleGrantPermission(context.TODO(), &pb.AuthRoleGrantPermissionRequest{Name: user.role, Perm: perm}); err != nil { + t.Fatal(err) + } + } +} + +func authSetupRoot(t *testing.T, auth pb.AuthClient) { + root := []user{ + { + name: "root", + password: "123", + role: "root", + key: "", + }, + } + authSetupUsers(t, auth, root) if _, err := auth.AuthEnable(context.TODO(), &pb.AuthEnableRequest{}); err != nil { t.Fatal(err) }