This is an automated email from the ASF dual-hosted git repository. littlecui pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/servicecomb-service-center.git
The following commit(s) were added to refs/heads/master by this push: new 6d91dd8 add account lock to manage login failure (#1056) 6d91dd8 is described below commit 6d91dd8410b40babab8b020615eb870d549c4cc8 Author: Shawn <xiaoliang.t...@gmail.com> AuthorDate: Thu Jun 17 09:37:22 2021 +0800 add account lock to manage login failure (#1056) --- datasource/account.go | 18 +++++++ datasource/datasource.go | 1 + datasource/etcd/account_lock.go | 87 ++++++++++++++++++++++++++++++++++ datasource/etcd/etcd.go | 22 ++++++--- datasource/etcd/path/key_generator.go | 7 +++ datasource/manager.go | 3 ++ datasource/mongo/account_lock.go | 60 +++++++++++++++++++++++ datasource/mongo/mongo.go | 18 ++++--- datasource/options.go | 5 +- etc/conf/app.yaml | 2 +- server/server.go | 14 ++++-- server/service/account/account.go | 37 +++++++++++++++ server/service/account/account_test.go | 34 +++++++++++++ server/service/rbac/authr_plugin.go | 4 +- server/service/rbac/blocker.go | 72 +++++++++++----------------- server/service/rbac/blocker_test.go | 15 +++--- server/service/rbac/password.go | 2 +- test/test.go | 6 ++- 18 files changed, 330 insertions(+), 77 deletions(-) diff --git a/datasource/account.go b/datasource/account.go index 89f5c63..1c04cd7 100644 --- a/datasource/account.go +++ b/datasource/account.go @@ -28,12 +28,18 @@ var ( ErrAccountDuplicated = errors.New("account is duplicated") ErrAccountCanNotEdit = errors.New("account can not be edited") ErrDLockNotFound = errors.New("dlock not found") + ErrCannotReleaseLock = errors.New("can not release account") + ErrAccountLockNotExist = errors.New("account not exist") ErrDeleteAccountFailed = errors.New("failed to delete account") ErrQueryAccountFailed = errors.New("failed to query account") ErrAccountNotExist = errors.New("account not exist") ErrRoleBindingExist = errors.New("role is bind to account") ) +const ( + StatusBanned = "banned" +) + // AccountManager contains the RBAC CRUD type AccountManager interface { CreateAccount(ctx context.Context, a *rbac.Account) error @@ -43,3 +49,15 @@ type AccountManager interface { DeleteAccount(ctx context.Context, names []string) (bool, error) UpdateAccount(ctx context.Context, name string, account *rbac.Account) error } + +// AccountLockManager saves login failure status +type AccountLockManager interface { + GetLock(ctx context.Context, key string) (*AccountLock, error) + DeleteLock(ctx context.Context, key string) error + Ban(ctx context.Context, key string) error +} +type AccountLock struct { + Key string `json:"key,omitempty"` + Status string `json:"status,omitempty"` + ReleaseAt int64 `json:"releaseAt,omitempty"` +} diff --git a/datasource/datasource.go b/datasource/datasource.go index d229bb4..7909178 100644 --- a/datasource/datasource.go +++ b/datasource/datasource.go @@ -21,6 +21,7 @@ package datasource type DataSource interface { SystemManager() SystemManager AccountManager() AccountManager + AccountLockManager() AccountLockManager RoleManager() RoleManager DependencyManager() DependencyManager MetadataManager() MetadataManager diff --git a/datasource/etcd/account_lock.go b/datasource/etcd/account_lock.go new file mode 100644 index 0000000..24cfd5c --- /dev/null +++ b/datasource/etcd/account_lock.go @@ -0,0 +1,87 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package etcd + +import ( + "context" + "encoding/json" + "fmt" + "time" + + "github.com/apache/servicecomb-service-center/datasource" + "github.com/apache/servicecomb-service-center/datasource/etcd/client" + "github.com/apache/servicecomb-service-center/datasource/etcd/path" + "github.com/apache/servicecomb-service-center/pkg/log" +) + +type AccountLockManager struct { + releaseAfter time.Duration +} + +func (al AccountLockManager) GetLock(ctx context.Context, key string) (*datasource.AccountLock, error) { + resp, err := client.Instance().Do(ctx, client.GET, + client.WithStrKey(path.GenerateAccountLockKey(key))) + if err != nil { + return nil, err + } + if resp.Count == 0 { + return nil, datasource.ErrAccountLockNotExist + } + lock := &datasource.AccountLock{} + err = json.Unmarshal(resp.Kvs[0].Value, lock) + if err != nil { + log.Errorf(err, "format invalid") + return nil, err + } + return lock, nil +} + +func (al AccountLockManager) DeleteLock(ctx context.Context, key string) error { + ok, err := client.Delete(ctx, key) + if err != nil { + log.Errorf(err, "remove lock failed") + return datasource.ErrCannotReleaseLock + } + if ok { + log.Info(fmt.Sprintf("%s is released", key)) + return nil + } + return datasource.ErrCannotReleaseLock +} + +func NewAccountLockManager(ReleaseAfter time.Duration) datasource.AccountLockManager { + return &AccountLockManager{releaseAfter: ReleaseAfter} +} + +func (al AccountLockManager) Ban(ctx context.Context, key string) error { + l := &datasource.AccountLock{} + l.Key = key + l.Status = datasource.StatusBanned + l.ReleaseAt = time.Now().Add(al.releaseAfter).Unix() + value, err := json.Marshal(l) + if err != nil { + log.Errorf(err, "account lock is invalid") + return err + } + etcdKey := path.GenerateAccountLockKey(key) + err = client.PutBytes(ctx, etcdKey, value) + if err != nil { + log.Errorf(err, "can not save account lock") + return err + } + log.Info(fmt.Sprintf("%s is locked, release at %d", key, l.ReleaseAt)) + return nil +} diff --git a/datasource/etcd/etcd.go b/datasource/etcd/etcd.go index d7223c0..d3975e0 100644 --- a/datasource/etcd/etcd.go +++ b/datasource/etcd/etcd.go @@ -42,12 +42,17 @@ func init() { } type DataSource struct { - accountManager datasource.AccountManager - metadataManager datasource.MetadataManager - roleManager datasource.RoleManager - sysManager datasource.SystemManager - depManager datasource.DependencyManager - scManager datasource.SCManager + accountLockManager datasource.AccountLockManager + accountManager datasource.AccountManager + metadataManager datasource.MetadataManager + roleManager datasource.RoleManager + sysManager datasource.SystemManager + depManager datasource.DependencyManager + scManager datasource.SCManager +} + +func (ds *DataSource) AccountLockManager() datasource.AccountLockManager { + return ds.accountLockManager } func (ds *DataSource) SystemManager() datasource.SystemManager { @@ -86,8 +91,11 @@ func NewDataSource(opts datasource.Options) (datasource.DataSource, error) { if err := inst.initialize(opts); err != nil { return nil, err } - + if opts.ReleaseAccountAfter == 0 { + opts.ReleaseAccountAfter = 15 * time.Minute + } inst.accountManager = &AccountManager{} + inst.accountLockManager = NewAccountLockManager(opts.ReleaseAccountAfter) inst.roleManager = &RoleManager{} inst.metadataManager = newMetadataManager(opts.SchemaEditable, opts.InstanceTTL) inst.sysManager = newSysManager() diff --git a/datasource/etcd/path/key_generator.go b/datasource/etcd/path/key_generator.go index f010a1a..228f808 100644 --- a/datasource/etcd/path/key_generator.go +++ b/datasource/etcd/path/key_generator.go @@ -373,6 +373,13 @@ func GenerateAccountKey(name string) string { name, }, SPLIT) } +func GenerateAccountLockKey(key string) string { + return util.StringJoin([]string{ + GetRootKey(), + "account-locks", + key, + }, SPLIT) +} func GenerateRBACSecretKey() string { return util.StringJoin([]string{ GetRootKey(), diff --git a/datasource/manager.go b/datasource/manager.go index 3f39f45..9cd2f23 100644 --- a/datasource/manager.go +++ b/datasource/manager.go @@ -81,6 +81,9 @@ func GetRoleManager() RoleManager { func GetAccountManager() AccountManager { return dataSourceInst.AccountManager() } +func GetAccountLockManager() AccountLockManager { + return dataSourceInst.AccountLockManager() +} func GetDependencyManager() DependencyManager { return dataSourceInst.DependencyManager() } diff --git a/datasource/mongo/account_lock.go b/datasource/mongo/account_lock.go new file mode 100644 index 0000000..7d21333 --- /dev/null +++ b/datasource/mongo/account_lock.go @@ -0,0 +1,60 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package mongo + +import ( + "context" + "fmt" + "sync" + "time" + + "github.com/apache/servicecomb-service-center/datasource" + "github.com/apache/servicecomb-service-center/pkg/log" +) + +type AccountLockManager struct { + releaseAfter time.Duration + locks sync.Map +} + +func (al *AccountLockManager) GetLock(ctx context.Context, key string) (*datasource.AccountLock, error) { + l, ok := al.locks.Load(key) + if !ok { + log.Debug(fmt.Sprintf("%s is not locked", key)) + return nil, datasource.ErrAccountLockNotExist + } + return l.(*datasource.AccountLock), nil +} + +func (al *AccountLockManager) DeleteLock(ctx context.Context, key string) error { + al.locks.Delete(key) + log.Warn(fmt.Sprintf("%s is released", key)) + return nil +} + +func NewAccountLockManager(ReleaseAfter time.Duration) datasource.AccountLockManager { + return &AccountLockManager{releaseAfter: ReleaseAfter} +} + +func (al *AccountLockManager) Ban(ctx context.Context, key string) error { + l := &datasource.AccountLock{} + l.Key = key + l.Status = datasource.StatusBanned + l.ReleaseAt = time.Now().Add(al.releaseAfter).Unix() + al.locks.Store(key, l) + log.Warn(fmt.Sprintf("%s is locked, release at %d", key, l.ReleaseAt)) + return nil +} diff --git a/datasource/mongo/mongo.go b/datasource/mongo/mongo.go index ca16277..6d167da 100644 --- a/datasource/mongo/mongo.go +++ b/datasource/mongo/mongo.go @@ -43,12 +43,17 @@ func init() { } type DataSource struct { - accountManager datasource.AccountManager - metadataManager datasource.MetadataManager - roleManager datasource.RoleManager - sysManager datasource.SystemManager - depManager datasource.DependencyManager - scManager datasource.SCManager + accountLockManager datasource.AccountLockManager + accountManager datasource.AccountManager + metadataManager datasource.MetadataManager + roleManager datasource.RoleManager + sysManager datasource.SystemManager + depManager datasource.DependencyManager + scManager datasource.SCManager +} + +func (ds *DataSource) AccountLockManager() datasource.AccountLockManager { + return ds.accountLockManager } func (ds *DataSource) SystemManager() datasource.SystemManager { @@ -88,6 +93,7 @@ func NewDataSource(opts datasource.Options) (datasource.DataSource, error) { inst.roleManager = &RoleManager{} inst.metadataManager = &MetadataManager{SchemaEditable: opts.SchemaEditable, InstanceTTL: opts.InstanceTTL} inst.accountManager = &AccountManager{} + inst.accountLockManager = NewAccountLockManager(opts.ReleaseAccountAfter) return inst, nil } diff --git a/datasource/options.go b/datasource/options.go index b8022b4..0e97b5c 100644 --- a/datasource/options.go +++ b/datasource/options.go @@ -17,12 +17,15 @@ package datasource +import "time" + //Options contains configuration for plugins type Options struct { Kind Kind SslEnabled bool SchemaEditable bool // InstanceTTL: the default ttl of instance lease - InstanceTTL int64 + InstanceTTL int64 + ReleaseAccountAfter time.Duration // TODO: pay attention to more net config like TLSConfig when coding } diff --git a/etc/conf/app.yaml b/etc/conf/app.yaml index 72de775..eb5a36e 100644 --- a/etc/conf/app.yaml +++ b/etc/conf/app.yaml @@ -158,7 +158,7 @@ rbac: enable: false privateKeyFile: ./private.key publicKeyFile: ./public.key - + releaseLockAfter: 15m # failure login attempt causes account blocking, that is block duration metrics: # enable to start metrics gather enable: true diff --git a/server/server.go b/server/server.go index e81729e..7f3954f 100644 --- a/server/server.go +++ b/server/server.go @@ -111,11 +111,17 @@ func (s *ServiceCenterServer) initEndpoints() { func (s *ServiceCenterServer) initDatasource() { // init datasource kind := datasource.Kind(config.GetString("registry.kind", "", config.WithStandby("registry_plugin"))) + ReleaseAccountAfter := config.GetString("rbac.releaseLockAfter", "15m") + d, err := time.ParseDuration(ReleaseAccountAfter) + if err != nil { + log.Warn("releaseAfter is invalid, use default config") + } if err := datasource.Init(datasource.Options{ - Kind: kind, - SslEnabled: config.GetSSL().SslEnabled, - InstanceTTL: config.GetRegistry().InstanceTTL, - SchemaEditable: config.GetRegistry().SchemaEditable, + Kind: kind, + SslEnabled: config.GetSSL().SslEnabled, + InstanceTTL: config.GetRegistry().InstanceTTL, + SchemaEditable: config.GetRegistry().SchemaEditable, + ReleaseAccountAfter: d, }); err != nil { log.Fatal("init datasource failed", err) } diff --git a/server/service/account/account.go b/server/service/account/account.go new file mode 100644 index 0000000..306b72b --- /dev/null +++ b/server/service/account/account.go @@ -0,0 +1,37 @@ +package account + +import ( + "context" + "fmt" + "time" + + "github.com/apache/servicecomb-service-center/pkg/log" + + "github.com/apache/servicecomb-service-center/datasource" +) + +func IsBanned(ctx context.Context, key string) (bool, error) { + lock, err := datasource.GetAccountLockManager().GetLock(ctx, key) + if err != nil { + if err == datasource.ErrAccountLockNotExist { + return false, nil + } + return false, err + } + if lock.ReleaseAt < time.Now().Unix() { + err := datasource.GetAccountLockManager().DeleteLock(ctx, key) + if err != nil { + log.Errorf(err, "remove lock failed") + return false, datasource.ErrCannotReleaseLock + } + log.Info(fmt.Sprintf("release lock for %s", key)) + return false, nil + } + if lock.Status == datasource.StatusBanned { + return true, nil + } + return false, nil +} +func Ban(ctx context.Context, key string) error { + return datasource.GetAccountLockManager().Ban(ctx, key) +} diff --git a/server/service/account/account_test.go b/server/service/account/account_test.go new file mode 100644 index 0000000..eacbaab --- /dev/null +++ b/server/service/account/account_test.go @@ -0,0 +1,34 @@ +package account_test + +import ( + "context" + + "testing" + "time" + + _ "github.com/apache/servicecomb-service-center/test" + + "github.com/apache/servicecomb-service-center/server/service/account" + "github.com/stretchr/testify/assert" +) + +func TestIsBanned(t *testing.T) { + t.Run("ban a key and check status, it should be banned, check other key should not be banned", + func(t *testing.T) { + err := account.Ban(context.TODO(), "dev_guy::127.0.0.1") + assert.NoError(t, err) + + ok, err := account.IsBanned(context.TODO(), "dev_guy::127.0.0.1") + assert.NoError(t, err) + assert.True(t, ok) + + ok, err = account.IsBanned(context.TODO(), "test_guy::127.0.0.1") + assert.NoError(t, err) + assert.False(t, ok) + + time.Sleep(4 * time.Second) + ok, err = account.IsBanned(context.TODO(), "dev_guy::127.0.0.1") + assert.NoError(t, err) + assert.False(t, ok) + }) +} diff --git a/server/service/rbac/authr_plugin.go b/server/service/rbac/authr_plugin.go index f22de7e..cfd8df5 100644 --- a/server/service/rbac/authr_plugin.go +++ b/server/service/rbac/authr_plugin.go @@ -59,14 +59,14 @@ func (a *EmbeddedAuthenticator) Login(ctx context.Context, user string, password account, err := GetAccount(ctx, user) if err != nil { if errsvc.IsErrEqualCode(err, rbac.ErrAccountNotExist) { - CountFailure(MakeBanKey(user, ip)) + TryLockAccount(MakeBanKey(user, ip)) return "", rbac.NewError(rbac.ErrUserOrPwdWrong, "") } return "", err } same := privacy.SamePassword(account.Password, password) if !same { - CountFailure(MakeBanKey(user, ip)) + TryLockAccount(MakeBanKey(user, ip)) return "", rbac.NewError(rbac.ErrUserOrPwdWrong, "") } diff --git a/server/service/rbac/blocker.go b/server/service/rbac/blocker.go index dce5053..a8766dd 100644 --- a/server/service/rbac/blocker.go +++ b/server/service/rbac/blocker.go @@ -18,9 +18,14 @@ package rbac import ( + "context" + "fmt" "sync" "time" + "github.com/apache/servicecomb-service-center/pkg/log" + accountsvc "github.com/apache/servicecomb-service-center/server/service/account" + "golang.org/x/time/rate" ) @@ -32,53 +37,36 @@ const ( var BanTime = 1 * time.Hour -type Client struct { - limiter *rate.Limiter - Key string - Banned bool - ReleaseAt time.Time //at this time client can be allow to attempt to do something +type LoginFailureLimiter struct { + limiter *rate.Limiter + Key string } var clients sync.Map -func BannedList() []*Client { - cs := make([]*Client, 0) - clients.Range(func(key, value interface{}) bool { - client := value.(*Client) - if client.Banned && time.Now().After(client.ReleaseAt) { - client.Banned = false - client.ReleaseAt = time.Time{} - return true - } - cs = append(cs, client) - return true - }) - return cs -} - -//CountFailure can cause a client banned +//TryLockAccount try to lock the account login attempt // it use time/rate to allow certainty failure, -//but will ban client if rate limiter can not accept failures -func CountFailure(key string) { +//it will ban client if rate limiter can not accept failures +func TryLockAccount(key string) { var c interface{} - var client *Client + var l *LoginFailureLimiter var ok bool - now := time.Now() if c, ok = clients.Load(key); !ok { - client = &Client{ - Key: key, - limiter: rate.NewLimiter(rate.Every(BlockInterval), MaxAttempts), - ReleaseAt: time.Time{}, + l = &LoginFailureLimiter{ + Key: key, + limiter: rate.NewLimiter(rate.Every(BlockInterval), MaxAttempts), } - clients.Store(key, client) + clients.Store(key, l) } else { - client = c.(*Client) + l = c.(*LoginFailureLimiter) } - allow := client.limiter.AllowN(time.Now(), 1) + allow := l.limiter.AllowN(time.Now(), 1) if !allow { - client.Banned = true - client.ReleaseAt = now.Add(BanTime) + err := accountsvc.Ban(context.TODO(), key) + if err != nil { + log.Error(fmt.Sprintf("can not ban account %s", key), err) + } } } @@ -86,16 +74,10 @@ func CountFailure(key string) { //it will release the client from banned status //use account name plus ip as key will maximum reduce the client conflicts func IsBanned(key string) bool { - var c interface{} - var client *Client - var ok bool - if c, ok = clients.Load(key); !ok { - return false - } - client = c.(*Client) - if client.Banned && time.Now().After(client.ReleaseAt) { - client.Banned = false - client.ReleaseAt = time.Time{} + IsBanned, err := accountsvc.IsBanned(context.TODO(), key) + if err != nil { + log.Error("can not check lock list, so return banned for security concern", err) + return true } - return client.Banned + return IsBanned } diff --git a/server/service/rbac/blocker_test.go b/server/service/rbac/blocker_test.go index 7a92c89..59624b7 100644 --- a/server/service/rbac/blocker_test.go +++ b/server/service/rbac/blocker_test.go @@ -33,28 +33,25 @@ func TestCountFailure(t *testing.T) { key1 := v4.MakeBanKey("root", "127.0.0.1") key2 := v4.MakeBanKey("root", "10.0.0.1") t.Run("ban root@IP, will not affect other root@another_IP", func(t *testing.T) { - rbac.CountFailure(key1) + rbac.TryLockAccount(key1) assert.False(t, rbac.IsBanned(key1)) - rbac.CountFailure(key1) + rbac.TryLockAccount(key1) assert.False(t, rbac.IsBanned(key1)) - rbac.CountFailure(key1) + rbac.TryLockAccount(key1) assert.True(t, rbac.IsBanned(key1)) - rbac.CountFailure(key2) + rbac.TryLockAccount(key2) assert.False(t, rbac.IsBanned(key2)) - rbac.CountFailure(key2) + rbac.TryLockAccount(key2) assert.False(t, rbac.IsBanned(key2)) - rbac.CountFailure(key2) + rbac.TryLockAccount(key2) assert.True(t, rbac.IsBanned(key2)) }) - t.Log(rbac.BannedList()[0].ReleaseAt) - assert.Equal(t, 2, len(rbac.BannedList())) time.Sleep(4 * time.Second) - assert.Equal(t, 0, len(rbac.BannedList())) assert.False(t, rbac.IsBanned(key1)) assert.False(t, rbac.IsBanned(key2)) diff --git a/server/service/rbac/password.go b/server/service/rbac/password.go index 856058f..d751057 100644 --- a/server/service/rbac/password.go +++ b/server/service/rbac/password.go @@ -88,7 +88,7 @@ func changePassword(ctx context.Context, name, currentPassword, pwd string) erro same := privacy.SamePassword(old.Password, currentPassword) if !same { log.Error("current password is wrong", nil) - CountFailure(MakeBanKey(name, ip)) + TryLockAccount(MakeBanKey(name, ip)) return rbac.NewError(rbac.ErrOldPwdWrong, "") } return doChangePassword(ctx, old, pwd) diff --git a/test/test.go b/test/test.go index caf37f2..4e2e9be 100644 --- a/test/test.go +++ b/test/test.go @@ -19,6 +19,8 @@ package test import ( + "time" + _ "github.com/apache/servicecomb-service-center/server/init" "github.com/apache/servicecomb-service-center/server/service/disco" @@ -34,6 +36,7 @@ func init() { if t == nil { t = "etcd" } + archaius.Set("rbac.releaseLockAfter", "3s") if t == "etcd" { archaius.Set("registry.cache.mode", 0) archaius.Set("discovery.kind", "etcd") @@ -41,6 +44,7 @@ func init() { } else { archaius.Set("registry.heartbeat.kind", "checker") } - datasource.Init(datasource.Options{Kind: datasource.Kind(t.(string))}) + datasource.Init(datasource.Options{Kind: datasource.Kind(t.(string)), + ReleaseAccountAfter: 3 * time.Second}) core.ServiceAPI = disco.AssembleResources() }