fix: prevent policy violation

pkg/util/rbacutils/policyset.go

@@ -0,0 +1,90 @@
+// Copyright 2019 Yunion
+//
+// Licensed 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 rbacutils
+
+type TPolicySet []*SRbacPolicy
+
+func GetMatchedPolicies(policies map[string]*SRbacPolicy, userCred IRbacIdentity) (TPolicySet, []string) {
+	matchedPolicies := make([]*SRbacPolicy, 0)
+	matchedNames := make([]string, 0)
+	maxMatchWeight := 0
+	for k := range policies {
+		isMatched, matchWeight := policies[k].Match(userCred)
+		if !isMatched || matchWeight < maxMatchWeight {
+			continue
+		}
+		if maxMatchWeight <= matchWeight {
+			if maxMatchWeight < matchWeight {
+				maxMatchWeight = matchWeight
+				matchedPolicies = matchedPolicies[:0]
+				matchedNames = matchedNames[:0]
+			}
+			matchedPolicies = append(matchedPolicies, policies[k])
+			matchedNames = append(matchedNames, k)
+		}
+	}
+	return matchedPolicies, matchedNames
+}
+
+func (policies TPolicySet) GetMatchRules(service string, resource string, action string, extra ...string) []SRbacRule {
+	matchRules := make([]SRbacRule, 0)
+	for i := range policies {
+		rule := policies[i].GetMatchRule(service, resource, action, extra...)
+		if rule != nil {
+			matchRules = append(matchRules, *rule)
+		}
+	}
+	return matchRules
+}
+
+// ViolatedBy: policies中deny的权限，但是assign中却是allow
+// if any assign allow, but policies deny
+// OR
+// assign allow, if any policies deny
+func (policies TPolicySet) ViolatedBy(assign TPolicySet) bool {
+	if policies.violatedBySet(assign, Allow) {
+		return true
+	}
+	if assign.violatedBySet(policies, Deny) {
+		return true
+	}
+	return false
+}
+
+func (policies TPolicySet) violatedBySet(assign TPolicySet, expect TRbacResult) bool {
+	for i := range assign {
+		if policies.violatedByPolicy(assign[i], expect) {
+			return true
+		}
+	}
+	return false
+}
+
+func (policies TPolicySet) violatedByPolicy(policy *SRbacPolicy, expect TRbacResult) bool {
+	for i := range policy.Rules {
+		rule := policy.Rules[i]
+		if rule.Result != expect {
+			continue
+		}
+		matchRules := policies.GetMatchRules(rule.Service, rule.Resource, rule.Action, rule.Extra...)
+		matchRule := GetMatchRule(matchRules, rule.Service, rule.Resource, rule.Action, rule.Extra...)
+		if expect == Allow && (matchRule == nil || matchRule.Result == Deny) {
+			return true
+		} else if expect == Deny && matchRule != nil && matchRule.Result == Allow {
+			return true
+		}
+	}
+	return false
+}

pkg/util/rbacutils/policyset_test.go

@@ -0,0 +1,203 @@
+// Copyright 2019 Yunion
+//
+// Licensed 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 rbacutils
+
+import "testing"
+
+func TestTPolicySet_Violate(t *testing.T) {
+	cases := []struct {
+		name string
+		p1   TPolicySet
+		p2   TPolicySet
+		want bool
+	}{
+		{
+			name: "case1",
+			p1: TPolicySet{
+				&SRbacPolicy{
+					Rules: []SRbacRule{
+						{
+							Service:  "compute",
+							Resource: "servers",
+							Action:   "list",
+							Result:   Deny,
+						},
+						{
+							Service:  "compute",
+							Resource: "servers",
+							Action:   WILD_MATCH,
+							Result:   Allow,
+						},
+					},
+				},
+			},
+			p2: TPolicySet{
+				&SRbacPolicy{
+					Rules: []SRbacRule{
+						{
+							Service:  "compute",
+							Resource: "servers",
+							Action:   WILD_MATCH,
+							Result:   Allow,
+						},
+					},
+				},
+			},
+			want: true,
+		},
+		{
+			name: "case2",
+			p1: TPolicySet{
+				&SRbacPolicy{
+					Rules: []SRbacRule{
+						{
+							Service:  "comptue",
+							Resource: "servers",
+							Action:   "list",
+							Result:   Deny,
+						},
+						{
+							Service:  "compute",
+							Resource: "servers",
+							Action:   WILD_MATCH,
+							Result:   Allow,
+						},
+					},
+				},
+			},
+			p2: TPolicySet{
+				&SRbacPolicy{
+					Rules: []SRbacRule{
+						{
+							Service: WILD_MATCH,
+							Result:  Allow,
+						},
+					},
+				},
+				&SRbacPolicy{
+					Rules: []SRbacRule{
+						{
+							Service:  "compute",
+							Resource: "servers",
+							Action:   "list",
+							Result:   Deny,
+						},
+					},
+				},
+			},
+			want: true,
+		},
+		{
+			name: "case3",
+			p1: TPolicySet{
+				&SRbacPolicy{
+					Rules: []SRbacRule{
+						{
+							Service: WILD_MATCH,
+							Result:  Allow,
+						},
+						{
+							Service:  "compute",
+							Resource: "servers",
+							Action:   "create",
+							Result:   Deny,
+						},
+					},
+				},
+				&SRbacPolicy{
+					Rules: []SRbacRule{
+						{
+							Service:  "comptue",
+							Resource: "servers",
+							Action:   "list",
+							Result:   Deny,
+						},
+						{
+							Service:  "compute",
+							Resource: "servers",
+							Action:   WILD_MATCH,
+							Result:   Allow,
+						},
+					},
+				},
+			},
+			p2: TPolicySet{
+				&SRbacPolicy{
+					Rules: []SRbacRule{
+						{
+							Service: WILD_MATCH,
+							Result:  Deny,
+						},
+					},
+				},
+				&SRbacPolicy{
+					Rules: []SRbacRule{
+						{
+							Service:  "comptue",
+							Resource: "servers",
+							Action:   WILD_MATCH,
+							Result:   Deny,
+						},
+						{
+							Service:  "compute",
+							Resource: "servers",
+							Action:   "get",
+							Result:   Allow,
+						},
+					},
+				},
+			},
+			want: false,
+		},
+		{
+			name: "case4",
+			p2: TPolicySet{
+				&SRbacPolicy{
+					Scope: ScopeDomain,
+					Rules: []SRbacRule{
+						{
+							Service: WILD_MATCH,
+							Result:  Allow,
+						},
+					},
+				},
+			},
+			p1: TPolicySet{
+				&SRbacPolicy{
+					Scope: ScopeDomain,
+					Rules: []SRbacRule{
+						{
+							Service: WILD_MATCH,
+							Result:  Allow,
+						},
+						{
+							Service:  "compute",
+							Resource: "servers",
+							Action:   "list",
+							Result:   Deny,
+						},
+					},
+				},
+			},
+			want: true,
+		},
+	}
+	for _, c := range cases {
+		got := c.p1.ViolatedBy(c.p2)
+		if got != c.want {
+			t.Errorf("[%s] want %v got %v", c.name, c.want, got)
+		}
+	}
+}

pkg/util/rbacutils/rbac.go

@@ -18,11 +18,11 @@ import (
 	"regexp"
 	"strings"
 
-	"github.com/pkg/errors"
-
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
+	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/netutils"
+	"yunion.io/x/pkg/utils"
 )
 
 type TRbacResult string
@@ -90,11 +90,14 @@ func (s1 TRbacScope) HigherThan(s2 TRbacScope) bool {
 
 type SRbacPolicy struct {
 	// condition, when the policy takes effects
-	Condition string // deprecated
+	// Deprecated
+	Condition string
 
 	DomainId string
 
-	IsPublic bool
+	IsPublic        bool
+	PublicScope     TRbacScope
+	SharedDomainIds []string
 
 	Projects []string
 	Roles    []string
@@ -102,8 +105,10 @@ type SRbacPolicy struct {
 	Auth     bool // whether needs authentication
 
 	// scope, the scope of the policy, system/domain/project
-	Scope   TRbacScope
-	IsAdmin bool // deprecated, is_admin=true means system scope, is_admin=false means project scope
+	Scope TRbacScope
+	// Deprecated
+	// is_admin=true means scope=system, is_admin=false means scope=project
+	IsAdmin bool
 
 	// rules, the exact rules
 	Rules []SRbacRule
@@ -416,7 +421,7 @@ func decode(rules jsonutils.JSONObject, decodeRule SRbacRule, level int) ([]SRba
 		}
 		return rules, nil
 	default:
-		return nil, errors.WithMessage(ErrUnsuportRuleData, rules.String())
+		return nil, errors.Wrap(ErrUnsuportRuleData, rules.String())
 	}
 }
 
@@ -581,7 +586,45 @@ type IRbacIdentity interface {
 }
 
 func (policy *SRbacPolicy) IsSystemWidePolicy() bool {
-	return len(policy.Roles) == 0 && len(policy.Projects) == 0
+	return (len(policy.DomainId) == 0 || (policy.IsPublic && policy.PublicScope == ScopeSystem)) && len(policy.Roles) == 0 && len(policy.Projects) == 0
+}
+
+func (policy *SRbacPolicy) MatchDomain(domainId string) bool {
+	if len(policy.DomainId) == 0 {
+		return true
+	}
+	if policy.DomainId == domainId {
+		return true
+	}
+	if policy.IsPublic {
+		if policy.PublicScope == ScopeSystem {
+			return true
+		}
+		if utils.IsInStringArray(domainId, policy.SharedDomainIds) {
+			return true
+		}
+	}
+	return false
+}
+
+func (policy *SRbacPolicy) MatchProject(projectName string) bool {
+	if len(policy.Projects) == 0 {
+		return true
+	}
+	if contains(policy.Projects, projectName) {
+		return true
+	}
+	return false
+}
+
+func (policy *SRbacPolicy) MatchRoles(roleNames []string) bool {
+	if len(policy.Roles) == 0 {
+		return true
+	}
+	if intersect(policy.Roles, roleNames) {
+		return true
+	}
+	return false
 }
 
 // check whether policy maches a userCred
@@ -597,18 +640,20 @@ func (policy *SRbacPolicy) Match(userCred IRbacIdentity) (bool, int) {
 		return false, 0
 	}
 	weight := 0
-	if policy.IsPublic || len(policy.DomainId) == 0 || policy.DomainId == userCred.GetProjectDomainId() {
+	if policy.MatchDomain(userCred.GetProjectDomainId()) {
 		if len(policy.DomainId) > 0 {
 			weight += 10
 		}
 		if !policy.IsPublic {
-			weight += 10
-		}
-		if len(policy.Roles) == 0 || intersect(policy.Roles, userCred.GetRoles()) {
+			weight += 30 // exact domain match
+		} else if len(policy.SharedDomainIds) > 0 {
+			weight += 20 // shared domain match
+		} // else, system scope match
+		if policy.MatchRoles(userCred.GetRoles()) {
 			if len(policy.Roles) != 0 {
 				weight += 100
 			}
-			if len(policy.Projects) == 0 || contains(policy.Projects, userCred.GetProjectName()) {
+			if policy.MatchProject(userCred.GetProjectName()) {
 				if len(policy.Projects) > 0 {
 					weight += 1000
 				}
@@ -624,11 +669,34 @@ func (policy *SRbacPolicy) Match(userCred IRbacIdentity) (bool, int) {
 	return false, 0
 }
 
-func (policy *SRbacPolicy) MatchRole(roleName string) bool {
-	if len(policy.Roles) == 0 || contains(policy.Roles, roleName) {
-		return true
+type sSimpleRbacIdentity struct {
+	domainId    string
+	projectName string
+	roleNames   []string
+}
+
+func (id sSimpleRbacIdentity) GetProjectDomainId() string {
+	return id.domainId
+}
+
+func (id sSimpleRbacIdentity) GetRoles() []string {
+	return id.roleNames
+}
+
+func (id sSimpleRbacIdentity) GetProjectName() string {
+	return id.projectName
+}
+
+func (id sSimpleRbacIdentity) GetLoginIp() string {
+	return ""
+}
+
+func NewRbacIdentity(domainId, projectName string, roleNames []string) IRbacIdentity {
+	return sSimpleRbacIdentity{
+		domainId:    domainId,
+		projectName: projectName,
+		roleNames:   roleNames,
 	}
-	return false
 }
 
 func String2Scope(str string) TRbacScope {

pkg/util/shellutils/edit.go

@@ -49,5 +49,9 @@ func Edit(yaml string) (string, error) {
 		return "", errors.Wrap(err, "ioutil.ReadFile")
 	}
 
+	if yaml == string(policyBytes) {
+		return "", errors.Error("no change")
+	}
+
 	return string(policyBytes), nil
 }