From 06ae94ebe3e434a8148b5103c9fc5b9dcfd93674 Mon Sep 17 00:00:00 2001 From: Qiu Jian Date: Thu, 21 Mar 2019 01:03:51 +0800 Subject: [PATCH 1/4] fix: simplify rbac policy condition to match projects and roles --- cmd/climc/shell/policies.go | 7 +- pkg/cloudcommon/policy/policy.go | 34 ++++--- pkg/mcclient/modules/mod_policies.go | 10 ++ pkg/util/rbacutils/rabc.go | 94 +++++++++++++++--- pkg/util/rbacutils/rabc_test.go | 143 ++++++++++++++------------- 5 files changed, 193 insertions(+), 95 deletions(-) diff --git a/cmd/climc/shell/policies.go b/cmd/climc/shell/policies.go index e468e7f3c6..bf3f4fc574 100644 --- a/cmd/climc/shell/policies.go +++ b/cmd/climc/shell/policies.go @@ -253,7 +253,7 @@ func init() { } req.Add(jsonutils.NewArray(data...), key) } - fmt.Println(req.String()) + fmt.Println("Request:", req.String()) var token mcclient.TokenCredential if len(args.User) > 0 { @@ -273,6 +273,11 @@ func init() { } printObject(result) + fmt.Println("userCred:", token) + m := policy.PolicyManager.MatchedPolicies(false, token) + fmt.Println("matched policies:", m) + m = policy.PolicyManager.MatchedPolicies(true, token) + fmt.Println("matched admin policies:", m) return nil }) } diff --git a/pkg/cloudcommon/policy/policy.go b/pkg/cloudcommon/policy/policy.go index 86bb0dcef2..f2ac40ea1c 100644 --- a/pkg/cloudcommon/policy/policy.go +++ b/pkg/cloudcommon/policy/policy.go @@ -16,7 +16,7 @@ import ( "yunion.io/x/onecloud/pkg/mcclient" "yunion.io/x/onecloud/pkg/mcclient/auth" "yunion.io/x/onecloud/pkg/mcclient/modules" - "yunion.io/x/onecloud/pkg/util/conditionparser" + // "yunion.io/x/onecloud/pkg/util/conditionparser" "yunion.io/x/onecloud/pkg/util/hashcache" "yunion.io/x/onecloud/pkg/util/rbacutils" ) @@ -231,27 +231,21 @@ func (manager *SPolicyManager) allowWithoutCache(isAdmin bool, userCred mcclient log.Warningf("no policies fetched") return rbacutils.Deny } - var userCredJson jsonutils.JSONObject - if userCred != nil { - userCredJson = userCred.ToJson() - } else { - userCredJson = jsonutils.NewDict() - } currentPriv := rbacutils.Deny for _, p := range policies { - result := p.Allow(userCredJson, service, resource, action, extra...) + result := p.Allow(userCred, service, resource, action, extra...) if currentPriv.StricterThan(result) { currentPriv = result } } if !isAdmin && manager.defaultPolicy != nil { - result := manager.defaultPolicy.Allow(userCredJson, service, resource, action, extra...) + result := manager.defaultPolicy.Allow(userCred, service, resource, action, extra...) if currentPriv.StricterThan(result) { currentPriv = result } } if consts.IsRbacDebug() { - log.Debugf("[RBAC: %v] %s %s %s %#v permission %s userCred: %s", isAdmin, service, resource, action, extra, currentPriv, userCredJson) + log.Debugf("[RBAC: %v] %s %s %s %#v permission %s userCred: %s", isAdmin, service, resource, action, extra, currentPriv, userCred) } return unifyRbacResult(isAdmin, currentPriv) } @@ -365,12 +359,26 @@ func (manager *SPolicyManager) IsAdminCapable(userCred mcclient.TokenCredential) return true } - userCredJson := userCred.ToJson() for _, p := range manager.adminPolicies { - match, _ := conditionparser.Eval(p.Condition, userCredJson) - if match { + if p.Match(userCred) { return true } } return false } + +func (manager *SPolicyManager) MatchedPolicies(isAdmin bool, userCred mcclient.TokenCredential) []string { + var policies map[string]rbacutils.SRbacPolicy + if isAdmin { + policies = manager.adminPolicies + } else { + policies = manager.policies + } + ret := make([]string, 0) + for k, p := range policies { + if p.Match(userCred) { + ret = append(ret, k) + } + } + return ret +} diff --git a/pkg/mcclient/modules/mod_policies.go b/pkg/mcclient/modules/mod_policies.go index 0e0e81f9d0..ce0f125dcf 100644 --- a/pkg/mcclient/modules/mod_policies.go +++ b/pkg/mcclient/modules/mod_policies.go @@ -4,6 +4,7 @@ import ( "yunion.io/x/jsonutils" "yunion.io/x/onecloud/pkg/mcclient" + "yunion.io/x/onecloud/pkg/util/rbacutils" ) type SPolicyManager struct { @@ -17,7 +18,16 @@ func policyReadFilter(session *mcclient.ClientSession, s jsonutils.JSONObject, q ret := ss.CopyIncludes("id", "type") blobStr, _ := ss.GetString("blob") if len(blobStr) > 0 { + policy := rbacutils.SRbacPolicy{} blobJson, _ := jsonutils.ParseString(blobStr) + err := policy.Decode(blobJson) + if err != nil { + return nil, err + } + blobJson, err = policy.Encode() + if err != nil { + return nil, err + } var format string if query != nil { format, _ = query.GetString("format") diff --git a/pkg/util/rbacutils/rabc.go b/pkg/util/rbacutils/rabc.go index 3c11d60d45..7ef001dd65 100644 --- a/pkg/util/rbacutils/rabc.go +++ b/pkg/util/rbacutils/rabc.go @@ -2,11 +2,11 @@ package rbacutils import ( "fmt" + "regexp" "yunion.io/x/jsonutils" - "yunion.io/x/log" - "yunion.io/x/onecloud/pkg/util/conditionparser" + "yunion.io/x/onecloud/pkg/mcclient" ) type TRbacResult string @@ -45,6 +45,8 @@ type SRbacPolicy struct { Condition string IsAdmin bool Rules []SRbacRule + Projects []string + Roles []string } type SRbacRule struct { @@ -188,9 +190,48 @@ func CompactRules(rules []SRbacRule) []SRbacRule { return output } +var ( + tenantEqualsPattern = regexp.MustCompile(`tenant\s*==\s*['"]?(\w+)['"]?`) + roleContainsPattern = regexp.MustCompile(`roles.contains\(['"]?(\w+)['"]?\)`) +) + +func searchMatchStrings(pattern *regexp.Regexp, condstr string) []string { + ret := make([]string, 0) + matches := pattern.FindAllStringSubmatch(condstr, -1) + for _, match := range matches { + ret = append(ret, match[1]) + } + return ret +} + +func searchMatchTenants(condstr string) []string { + return searchMatchStrings(tenantEqualsPattern, condstr) +} + +func searchMatchRoles(condstr string) []string { + return searchMatchStrings(roleContainsPattern, condstr) +} + func (policy *SRbacPolicy) Decode(policyJson jsonutils.JSONObject) error { policy.Condition, _ = policyJson.GetString("condition") policy.IsAdmin = jsonutils.QueryBoolean(policyJson, "is_admin", false) + if policyJson.Contains("projects") { + projectJson, _ := policyJson.GetArray("projects") + policy.Projects = jsonutils.JSONArray2StringArray(projectJson) + } + if policyJson.Contains("roles") { + roleJson, _ := policyJson.GetArray("roles") + policy.Roles = jsonutils.JSONArray2StringArray(roleJson) + } + + if len(policy.Projects) == 0 && len(policy.Roles) == 0 && len(policy.Condition) > 0 { + // XXX hack + // for smooth transtion from condition to projects&roles + policy.Projects = searchMatchTenants(policy.Condition) + policy.Roles = searchMatchRoles(policy.Condition) + } + // empty condition, no longer use this field + policy.Condition = "" ruleJson, err := policyJson.Get("policy") if err != nil { @@ -288,7 +329,7 @@ func (rule *SRbacRule) toStringArray() []string { strArr = append(strArr, rule.Extra...) } i := len(strArr) - 1 - for i >= 0 && (len(strArr[i]) == 0 || strArr[i] == WILD_MATCH) { + for i > 0 && (len(strArr[i]) == 0 || strArr[i] == WILD_MATCH) { i -= 1 } return strArr[0 : i+1] @@ -346,12 +387,18 @@ func (policy *SRbacPolicy) Encode() (jsonutils.JSONObject, error) { } ret := jsonutils.NewDict() - ret.Add(jsonutils.NewString(policy.Condition), "condition") + // ret.Add(jsonutils.NewString(policy.Condition), "condition") if policy.IsAdmin { ret.Add(jsonutils.JSONTrue, "is_admin") } else { ret.Add(jsonutils.JSONFalse, "is_admin") } + if len(policy.Projects) > 0 { + ret.Add(jsonutils.NewStringArray(policy.Projects), "projects") + } + if len(policy.Roles) > 0 { + ret.Add(jsonutils.NewStringArray(policy.Roles), "roles") + } ret.Add(rules, "policy") return ret, nil } @@ -369,17 +416,40 @@ func (policy *SRbacPolicy) Explain(request [][]string) [][]string { return output } -func (policy *SRbacPolicy) Allow(userCred jsonutils.JSONObject, service, resource, action string, extra ...string) TRbacResult { - if len(policy.Condition) > 0 { - match, err := conditionparser.Eval(policy.Condition, userCred) - if err != nil { - log.Errorf("eval condition %s fail %s", policy.Condition, err) - return Deny +func contains(s1 []string, s string) bool { + for i := range s1 { + if s1[i] == s { + return true } - if !match { - return Deny + } + return false +} + +func intersect(s1 []string, s2 []string) bool { + for i := range s1 { + for j := range s2 { + if s1[i] == s2[j] { + return true + } } } + return false +} + +func (policy *SRbacPolicy) Match(userCred mcclient.TokenCredential) bool { + if len(policy.Projects) > 0 && userCred != nil && !contains(policy.Projects, userCred.GetProjectName()) { + return false + } + if len(policy.Roles) > 0 && userCred != nil && !intersect(policy.Roles, userCred.GetRoles()) { + return false + } + return true +} + +func (policy *SRbacPolicy) Allow(userCred mcclient.TokenCredential, service, resource, action string, extra ...string) TRbacResult { + if !policy.Match(userCred) { + return Deny + } rule := policy.GetMatchRule(service, resource, action, extra...) if rule == nil { return Deny diff --git a/pkg/util/rbacutils/rabc_test.go b/pkg/util/rbacutils/rabc_test.go index 56aeca8736..02c7755796 100644 --- a/pkg/util/rbacutils/rabc_test.go +++ b/pkg/util/rbacutils/rabc_test.go @@ -4,6 +4,8 @@ import ( "testing" "yunion.io/x/jsonutils" + + "yunion.io/x/onecloud/pkg/mcclient" ) func TestSRabcRule_Match(t *testing.T) { @@ -179,60 +181,6 @@ func TestSRabcPolicy_Encode(t *testing.T) { t.Logf("%s", policyStr1) } -func TestSRbacPolicy_Allow(t *testing.T) { - userCredStr := `{"domain":"Default","domain_id":"default","expires":"2018-10-28T05:33:54.000000Z","roles":"admin,teamleader","tenant":"system","tenant_id":"5d65667d112e47249ae66dbd7bc07030","token":"gAAAAABb0_jC2Qz2PpB00-pieLi4exKXq4O3QrvoqerpqoSxbp9pOLLdNWaAg0cPcd8eAjkiPhSo7VWQAVodoxnad95LdNbUf_1It8R_wXVDtO20caB7oLcas1oQt8b1cG0a7qagauP0iWVSW_dq_e92rD5Hd3SHn3Lw6ycrp_eHLskz_8EbPiI","user":"sysadmin","user_id":"dddf386b6ff24572b2e6a771d768495e"}` - - userCred, err := jsonutils.ParseString(userCredStr) - if err != nil { - t.Errorf("parse json fail %s", err) - return - } - - cases := []struct { - policy string - ops []string - want TRbacResult - }{ - { - `{ - "condition": "tenant==\"system\" && roles.contains(\"admin\")", - "is_admin": true, - "policy": { - "*": "allow" - } -}`, - []string{"compute", "servers", "list"}, - Allow, - }, - { - `{"is_admin":"false","policy":{"*":{"*":{"*":"allow","delete":"deny"}}}}`, - []string{"compute", "servers", "delete"}, - Deny, - }, - } - - for _, c := range cases { - policyJson, err := jsonutils.ParseString(c.policy) - if err != nil { - t.Errorf("fail to parse json string %s", err) - return - } - policy := SRbacPolicy{} - - err = policy.Decode(policyJson) - if err != nil { - t.Errorf("decode error %s", err) - return - } - - isAllow := policy.Allow(userCred, c.ops[0], c.ops[1], c.ops[2]) - if isAllow != c.want { - t.Errorf("%s %#v expect %#v, but get %#v", policyJson.String(), c.ops, c.want, isAllow) - return - } - } -} - func TestSRabcPolicy_Explain(t *testing.T) { policyStr := `{ "condition": "usercred.project != \"system\" && usercred.roles==\"projectowner\"", @@ -281,24 +229,81 @@ func TestSRabcPolicy_Explain(t *testing.T) { t.Logf("%#v", output) } -func TestTRbacResult_IsHigherPrivilege(t *testing.T) { - cases := []struct { - t1 TRbacResult - t2 TRbacResult - want bool - }{ - {Allow, Allow, false}, - {Allow, OwnerAllow, true}, - {Deny, Deny, false}, - {Deny, OwnerAllow, false}, - {Deny, Allow, false}, - {OwnerAllow, Allow, false}, - } +func TestConditionParser(t *testing.T) { + condition := `tenant=="system" && roles.contains("admin")` + tenants := searchMatchTenants(condition) + t.Logf("%s", tenants) + roles := searchMatchRoles(condition) + t.Logf("%s", roles) +} +func TestSRbacPolicyMatch(t *testing.T) { + cases := []struct { + policy SRbacPolicy + userCred mcclient.SSimpleToken + want bool + }{ + { + SRbacPolicy{}, + mcclient.SSimpleToken{}, + true, + }, + { + SRbacPolicy{ + Projects: []string{"system"}, + }, + mcclient.SSimpleToken{ + Project: "system", + }, + true, + }, + { + SRbacPolicy{ + Projects: []string{"system"}, + }, + mcclient.SSimpleToken{ + Project: "demo", + }, + false, + }, + { + SRbacPolicy{ + Projects: []string{"system"}, + Roles: []string{"admin"}, + }, + mcclient.SSimpleToken{ + Project: "system", + Roles: "admin", + }, + true, + }, + { + SRbacPolicy{ + Projects: []string{"system"}, + Roles: []string{"admin"}, + }, + mcclient.SSimpleToken{ + Project: "system", + Roles: "admin,_member_", + }, + true, + }, + { + SRbacPolicy{ + Projects: []string{"system"}, + Roles: []string{"admin"}, + }, + mcclient.SSimpleToken{ + Project: "system", + Roles: "_member_", + }, + false, + }, + } for _, c := range cases { - got := c.t1.IsHigherPrivilege(c.t2) + got := c.policy.Match(&c.userCred) if got != c.want { - t.Errorf("%s IsHigherPrivilege %s want %v got %v", c.t1, c.t2, c.want, got) + t.Errorf("%#v %#v got %v want %v", c.policy, c.userCred, got, c.want) } } } From 9d966e9959459730ba5786d78b25004afd2446c1 Mon Sep 17 00:00:00 2001 From: Qiu Jian Date: Thu, 21 Mar 2019 01:06:33 +0800 Subject: [PATCH 2/4] minor updates --- pkg/cloudcommon/policy/policy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cloudcommon/policy/policy.go b/pkg/cloudcommon/policy/policy.go index f2ac40ea1c..f9bed82332 100644 --- a/pkg/cloudcommon/policy/policy.go +++ b/pkg/cloudcommon/policy/policy.go @@ -16,7 +16,6 @@ import ( "yunion.io/x/onecloud/pkg/mcclient" "yunion.io/x/onecloud/pkg/mcclient/auth" "yunion.io/x/onecloud/pkg/mcclient/modules" - // "yunion.io/x/onecloud/pkg/util/conditionparser" "yunion.io/x/onecloud/pkg/util/hashcache" "yunion.io/x/onecloud/pkg/util/rbacutils" ) From cc30b585e058eae4920e96a3285ea2aa094a6f41 Mon Sep 17 00:00:00 2001 From: Qiu Jian Date: Thu, 21 Mar 2019 10:16:55 +0800 Subject: [PATCH 3/4] fix: refine policy match conditions --- pkg/util/rbacutils/rabc.go | 9 +++------ pkg/util/rbacutils/rabc_test.go | 29 +++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/pkg/util/rbacutils/rabc.go b/pkg/util/rbacutils/rabc.go index 7ef001dd65..e95b9663e0 100644 --- a/pkg/util/rbacutils/rabc.go +++ b/pkg/util/rbacutils/rabc.go @@ -437,13 +437,10 @@ func intersect(s1 []string, s2 []string) bool { } func (policy *SRbacPolicy) Match(userCred mcclient.TokenCredential) bool { - if len(policy.Projects) > 0 && userCred != nil && !contains(policy.Projects, userCred.GetProjectName()) { - return false + if (len(policy.Projects) == 0 || (userCred != nil && contains(policy.Projects, userCred.GetProjectName()))) && (len(policy.Roles) == 0 || (userCred != nil && intersect(policy.Roles, userCred.GetRoles()))) { + return true } - if len(policy.Roles) > 0 && userCred != nil && !intersect(policy.Roles, userCred.GetRoles()) { - return false - } - return true + return false } func (policy *SRbacPolicy) Allow(userCred mcclient.TokenCredential, service, resource, action string, extra ...string) TRbacResult { diff --git a/pkg/util/rbacutils/rabc_test.go b/pkg/util/rbacutils/rabc_test.go index 02c7755796..401f6c7e1a 100644 --- a/pkg/util/rbacutils/rabc_test.go +++ b/pkg/util/rbacutils/rabc_test.go @@ -240,19 +240,24 @@ func TestConditionParser(t *testing.T) { func TestSRbacPolicyMatch(t *testing.T) { cases := []struct { policy SRbacPolicy - userCred mcclient.SSimpleToken + userCred mcclient.TokenCredential want bool }{ { SRbacPolicy{}, - mcclient.SSimpleToken{}, + &mcclient.SSimpleToken{}, + true, + }, + { + SRbacPolicy{}, + nil, true, }, { SRbacPolicy{ Projects: []string{"system"}, }, - mcclient.SSimpleToken{ + &mcclient.SSimpleToken{ Project: "system", }, true, @@ -261,7 +266,7 @@ func TestSRbacPolicyMatch(t *testing.T) { SRbacPolicy{ Projects: []string{"system"}, }, - mcclient.SSimpleToken{ + &mcclient.SSimpleToken{ Project: "demo", }, false, @@ -271,7 +276,7 @@ func TestSRbacPolicyMatch(t *testing.T) { Projects: []string{"system"}, Roles: []string{"admin"}, }, - mcclient.SSimpleToken{ + &mcclient.SSimpleToken{ Project: "system", Roles: "admin", }, @@ -282,7 +287,7 @@ func TestSRbacPolicyMatch(t *testing.T) { Projects: []string{"system"}, Roles: []string{"admin"}, }, - mcclient.SSimpleToken{ + &mcclient.SSimpleToken{ Project: "system", Roles: "admin,_member_", }, @@ -293,15 +298,23 @@ func TestSRbacPolicyMatch(t *testing.T) { Projects: []string{"system"}, Roles: []string{"admin"}, }, - mcclient.SSimpleToken{ + &mcclient.SSimpleToken{ Project: "system", Roles: "_member_", }, false, }, + { + SRbacPolicy{ + Projects: []string{"system"}, + Roles: []string{"admin"}, + }, + nil, + false, + }, } for _, c := range cases { - got := c.policy.Match(&c.userCred) + got := c.policy.Match(c.userCred) if got != c.want { t.Errorf("%#v %#v got %v want %v", c.policy, c.userCred, got, c.want) } From 78faed91a4ee0ebda6608f55bc6ae924195df0e3 Mon Sep 17 00:00:00 2001 From: Qiu Jian Date: Thu, 21 Mar 2019 11:29:30 +0800 Subject: [PATCH 4/4] fix: SyncOnce should invalidate policy cache --- pkg/cloudcommon/policy/policy.go | 1 + pkg/util/hashcache/cache.go | 19 +++++++++++++++++++ pkg/util/hashcache/cache_test.go | 11 +++++++++++ 3 files changed, 31 insertions(+) diff --git a/pkg/cloudcommon/policy/policy.go b/pkg/cloudcommon/policy/policy.go index f9bed82332..bceebdb928 100644 --- a/pkg/cloudcommon/policy/policy.go +++ b/pkg/cloudcommon/policy/policy.go @@ -147,6 +147,7 @@ func (manager *SPolicyManager) SyncOnce() error { manager.adminPolicies = adminPolicies manager.lastSync = time.Now() + manager.cache.Invalidate() return nil } diff --git a/pkg/util/hashcache/cache.go b/pkg/util/hashcache/cache.go index fe053bddaf..03698f0380 100644 --- a/pkg/util/hashcache/cache.go +++ b/pkg/util/hashcache/cache.go @@ -8,12 +8,22 @@ import ( "time" ) +var initTime = time.Time{} + type cacheNode struct { key string expire time.Time value interface{} } +func (node *cacheNode) reset() { + if !node.expire.IsZero() { + node.key = "" + node.expire = initTime + node.value = nil + } +} + type Cache struct { table []cacheNode lock *sync.Mutex @@ -101,3 +111,12 @@ func (c *Cache) AtomicSet(key string, val interface{}) { defer c.lock.Unlock() c.Set(key, val) } + +func (c *Cache) Invalidate() { + c.lock.Lock() + defer c.lock.Unlock() + + for i := range c.table { + c.table[i].reset() + } +} diff --git a/pkg/util/hashcache/cache_test.go b/pkg/util/hashcache/cache_test.go index 3394e9ed14..44d57b849c 100644 --- a/pkg/util/hashcache/cache_test.go +++ b/pkg/util/hashcache/cache_test.go @@ -46,4 +46,15 @@ func TestCache(t *testing.T) { if v != nil { t.Errorf("key 123 shoud expire") } + + c.Set("123", 1234) + v = c.Get("123") + if v == nil || v.(int) != 1234 { + t.Error("Key 123 not found") + } + c.Invalidate() + v = c.Get("123") + if v != nil { + t.Error("Key 123 should not found") + } }