You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@servicecomb.apache.org by li...@apache.org on 2021/05/30 08:13:22 UTC

[servicecomb-service-center] branch master updated: reduce client identity conflicts of ban list (#1024)

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 e065eea  reduce client identity conflicts of ban list (#1024)
e065eea is described below

commit e065eea0a80e5ebd5a4862883932b980991f8bf3
Author: Shawn <xi...@gmail.com>
AuthorDate: Sun May 30 16:13:13 2021 +0800

    reduce client identity conflicts of ban list (#1024)
---
 server/resource/v4/auth_resource.go | 24 +++++++++++++++---------
 server/service/rbac/blocker.go      |  1 +
 server/service/rbac/blocker_test.go | 35 +++++++++++++++++++----------------
 3 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/server/resource/v4/auth_resource.go b/server/resource/v4/auth_resource.go
index a34875d..2cbaab4 100644
--- a/server/resource/v4/auth_resource.go
+++ b/server/resource/v4/auth_resource.go
@@ -155,8 +155,9 @@ func (ar *AuthResource) GetAccount(w http.ResponseWriter, r *http.Request) {
 }
 
 func (ar *AuthResource) ChangePassword(w http.ResponseWriter, req *http.Request) {
+	name := req.URL.Query().Get(":name")
 	ip := util.GetRealIP(req)
-	if rbacsvc.IsBanned(ip) {
+	if rbacsvc.IsBanned(MakeBanKey(name, ip)) {
 		log.Warn(fmt.Sprintf("ip is banned:%s, account: %s", ip, req.URL.Query().Get(":name")))
 		rest.WriteError(w, discovery.ErrForbidden, "")
 		return
@@ -194,7 +195,7 @@ func (ar *AuthResource) ChangePassword(w http.ResponseWriter, req *http.Request)
 			return
 		}
 		if err == rbacsvc.ErrWrongPassword {
-			rbacsvc.CountFailure(ip)
+			rbacsvc.CountFailure(MakeBanKey(a.Name, ip))
 			rest.WriteError(w, discovery.ErrInvalidParams, err.Error())
 			return
 		}
@@ -204,13 +205,8 @@ func (ar *AuthResource) ChangePassword(w http.ResponseWriter, req *http.Request)
 	}
 	rest.WriteSuccess(w, req)
 }
+
 func (ar *AuthResource) Login(w http.ResponseWriter, r *http.Request) {
-	ip := util.GetRealIP(r)
-	if rbacsvc.IsBanned(ip) {
-		log.Warn(fmt.Sprintf("ip is banned:%s, account: %s", ip, r.URL.Query().Get(":name")))
-		rest.WriteError(w, discovery.ErrForbidden, "")
-		return
-	}
 	body, err := ioutil.ReadAll(r.Body)
 	if err != nil {
 		log.Error("read body err", err)
@@ -231,12 +227,18 @@ func (ar *AuthResource) Login(w http.ResponseWriter, r *http.Request) {
 		rest.WriteError(w, discovery.ErrInvalidParams, err.Error())
 		return
 	}
+	ip := util.GetRealIP(r)
+	if rbacsvc.IsBanned(MakeBanKey(a.Name, ip)) {
+		log.Warn(fmt.Sprintf("ip is banned:%s, account: %s", ip, a.Name))
+		rest.WriteError(w, discovery.ErrForbidden, "")
+		return
+	}
 	t, err := authr.Login(context.TODO(), a.Name, a.Password,
 		authr.ExpireAfter(a.TokenExpirationTime))
 	if err != nil {
 		if err == rbacsvc.ErrUnauthorized {
 			log.Error("not authorized", err)
-			rbacsvc.CountFailure(ip)
+			rbacsvc.CountFailure(MakeBanKey(a.Name, ip))
 			rest.WriteError(w, discovery.ErrUnauthorized, err.Error())
 			return
 		}
@@ -246,3 +248,7 @@ func (ar *AuthResource) Login(w http.ResponseWriter, r *http.Request) {
 	}
 	rest.WriteResponse(w, r, nil, &rbac.Token{TokenStr: t})
 }
+
+func MakeBanKey(name, ip string) string {
+	return name + "::" + ip
+}
diff --git a/server/service/rbac/blocker.go b/server/service/rbac/blocker.go
index 265e4dd..dce5053 100644
--- a/server/service/rbac/blocker.go
+++ b/server/service/rbac/blocker.go
@@ -84,6 +84,7 @@ func CountFailure(key string) {
 
 //IsBanned check if a client is banned, and if client ban time expire,
 //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
diff --git a/server/service/rbac/blocker_test.go b/server/service/rbac/blocker_test.go
index 82e21ce..24f27f6 100644
--- a/server/service/rbac/blocker_test.go
+++ b/server/service/rbac/blocker_test.go
@@ -18,6 +18,7 @@
 package rbac_test
 
 import (
+	v4 "github.com/apache/servicecomb-service-center/server/resource/v4"
 	"testing"
 	"time"
 
@@ -27,31 +28,33 @@ import (
 
 func TestCountFailure(t *testing.T) {
 	rbac.BanTime = 3 * time.Second
-	rbac.CountFailure("1")
-	assert.False(t, rbac.IsBanned("1"))
 
-	rbac.CountFailure("1")
-	assert.False(t, rbac.IsBanned("1"))
+	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)
+		assert.False(t, rbac.IsBanned(key1))
 
-	rbac.CountFailure("1")
-	assert.True(t, rbac.IsBanned("1"))
+		rbac.CountFailure(key1)
+		assert.False(t, rbac.IsBanned(key1))
 
-	t.Run("ban 1 more", func(t *testing.T) {
-		rbac.CountFailure("2")
-		assert.False(t, rbac.IsBanned("2"))
+		rbac.CountFailure(key1)
+		assert.True(t, rbac.IsBanned(key1))
 
-		rbac.CountFailure("2")
-		assert.False(t, rbac.IsBanned("2"))
+		rbac.CountFailure(key2)
+		assert.False(t, rbac.IsBanned(key2))
 
-		rbac.CountFailure("2")
-		assert.True(t, rbac.IsBanned("2"))
+		rbac.CountFailure(key2)
+		assert.False(t, rbac.IsBanned(key2))
+
+		rbac.CountFailure(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("1"))
-	assert.False(t, rbac.IsBanned("2"))
+	assert.False(t, rbac.IsBanned(key1))
+	assert.False(t, rbac.IsBanned(key2))
 
 }