Skip to content

Commit

Permalink
Add additional fields for ServiceRole, issue: #11516 (#11712) (#12299)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pitlv2109 @istio-testing
pitlv2109 authored and istio-testing committed Mar 13, 2019
1 parent 4d9d3fd commit a9fe655
Show file tree
Hide file tree
Showing 8 changed files with 410 additions and 51 deletions.
21 changes: 21 additions & 0 deletions pilot/pkg/model/validation.go
View file
Expand Up @@ -107,6 +107,16 @@ func ValidatePort(port int) error {
return fmt.Errorf("port number %d must be in the range 1..65535", port)
}

// ValidatePort checks if all ports are in range [0, 65535]
func ValidatePorts(ports []int32) bool {
for _, port := range ports {
if ValidatePort(int(port)) != nil {
return false
}
}
return true
}

// Validate checks that each name conforms to the spec and has a ProtoMessage
func (descriptor ConfigDescriptor) Validate() error {
var errs error
Expand Down Expand Up @@ -1448,6 +1458,17 @@ func ValidateServiceRole(name, namespace string, msg proto.Message) error {
if len(rule.Services) == 0 {
errs = appendErrors(errs, fmt.Errorf("at least 1 service must be specified for rule %d", i))
}
// Regular rules and not rules (e.g. methods and not_methods should not be defined together).
sameAttributeKindError := "cannot have both regular and *not* attributes for the same kind (%s) for rule %d"
if len(rule.Methods) > 0 && len(rule.NotMethods) > 0 {
errs = appendErrors(errs, fmt.Errorf(sameAttributeKindError, "i.e. methods and not_methods", i))
}
if len(rule.Ports) > 0 && len(rule.NotPorts) > 0 {
errs = appendErrors(errs, fmt.Errorf(sameAttributeKindError, "i.e. ports and not_ports", i))
}
if !ValidatePorts(rule.Ports) || !ValidatePorts(rule.NotPorts) {
errs = appendErrors(errs, fmt.Errorf("at least one port is not in the range of [0, 65535]"))
}
for j, constraint := range rule.Constraints {
if len(constraint.Key) == 0 {
errs = appendErrors(errs, fmt.Errorf("key cannot be empty for constraint %d in rule %d", j, i))
Expand Down
33 changes: 33 additions & 0 deletions pilot/pkg/model/validation_test.go
View file
Expand Up @@ -3543,6 +3543,38 @@ func TestValidateServiceRole(t *testing.T) {
}},
expectErrMsg: "at least 1 service must be specified for rule 1",
},
{
name: "has both methods and not_methods",
in: &rbac.ServiceRole{Rules: []*rbac.AccessRule{
{
Services: []string{"service0"},
Methods: []string{"GET", "POST"},
NotMethods: []string{"DELETE"},
},
}},
expectErrMsg: "cannot have both regular and *not* attributes for the same kind (i.e. methods and not_methods) for rule 0",
},
{
name: "has both ports and not_ports",
in: &rbac.ServiceRole{Rules: []*rbac.AccessRule{
{
Services: []string{"service0"},
Ports: []int32{9080},
NotPorts: []int32{443},
},
}},
expectErrMsg: "cannot have both regular and *not* attributes for the same kind (i.e. ports and not_ports) for rule 0",
},
{
name: "has out of range port",
in: &rbac.ServiceRole{Rules: []*rbac.AccessRule{
{
Services: []string{"service0"},
Ports: []int32{9080, -80},
},
}},
expectErrMsg: "at least one port is not in the range of [0, 65535]",
},
{
name: "no key in constraint",
in: &rbac.ServiceRole{Rules: []*rbac.AccessRule{
Expand Down Expand Up @@ -3593,6 +3625,7 @@ func TestValidateServiceRole(t *testing.T) {
{
Services: []string{"service0"},
Methods: []string{"GET", "POST"},
NotHosts: []string{"finances.google.com"},
Constraints: []*rbac.AccessRule_Constraint{
{Key: "key", Values: []string{"value"}},
{Key: "key", Values: []string{"value"}},
Expand Down
79 changes: 57 additions & 22 deletions pilot/pkg/networking/plugin/authz/rbac.go
View file
Expand Up @@ -80,8 +80,11 @@ const (
attrDestUser = "destination.user" // service account, e.g. "bookinfo-productpage".
attrConnSNI = "connection.sni" // server name indication, e.g. "www.example.com".

// Envoy config attributes for ServiceRole rules.
methodHeader = ":method"
pathHeader = ":path"
hostHeader = ":authority"
portKey = "port"
)

// serviceMetadata is a collection of different kind of information about a service.
Expand Down Expand Up @@ -555,6 +558,26 @@ func convertRbacRulesToFilterConfig(service *serviceMetadata, option rbacOption)
return &http_config.RBAC{Rules: rbac}
}

// appendRule appends a |rule| to |rules| if |rule| is not nil.
func appendRule(rules *policyproto.Permission_AndRules, rule *policyproto.Permission) {
if rule == nil {
return
}
rules.AndRules.Rules = append(rules.AndRules.Rules, rule)
}

// appendNotRule appends a |notRule| to AndRules of |rules| if |notRule| is not nil.
func appendNotRule(rules *policyproto.Permission_AndRules, notRule *policyproto.Permission) {
if notRule == nil {
return
}
rules.AndRules.Rules = append(rules.AndRules.Rules,
&policyproto.Permission{Rule: &policyproto.Permission_NotRule{
NotRule: notRule,
},
})
}

// convertToPermission converts a single AccessRule to a Permission.
func convertToPermission(rule *rbacproto.AccessRule) *policyproto.Permission {
rules := &policyproto.Permission_AndRules{
Expand All @@ -563,46 +586,58 @@ func convertToPermission(rule *rbacproto.AccessRule) *policyproto.Permission {
},
}

if len(rule.Hosts) > 0 {
hostRule := permissionForKeyValues(hostHeader, rule.Hosts)
appendRule(rules, hostRule)
}

if len(rule.NotHosts) > 0 {
notHostRule := permissionForKeyValues(hostHeader, rule.NotHosts)
appendNotRule(rules, notHostRule)
}

if len(rule.Methods) > 0 {
methodRule := permissionForKeyValues(methodHeader, rule.Methods)
if methodRule != nil {
rules.AndRules.Rules = append(rules.AndRules.Rules, methodRule)
}
appendRule(rules, methodRule)
}

if len(rule.NotMethods) > 0 {
notMethodRule := permissionForKeyValues(methodHeader, rule.NotMethods)
if notMethodRule != nil {
rules.AndRules.Rules = append(rules.AndRules.Rules,
&policyproto.Permission{Rule: &policyproto.Permission_NotRule{
NotRule: notMethodRule,
},
})
}
appendNotRule(rules, notMethodRule)
}

if len(rule.Paths) > 0 {
pathRule := permissionForKeyValues(pathHeader, rule.Paths)
if pathRule != nil {
rules.AndRules.Rules = append(rules.AndRules.Rules, pathRule)
}
appendRule(rules, pathRule)
}

if len(rule.NotPaths) > 0 {
notPathRule := permissionForKeyValues(pathHeader, rule.NotPaths)
appendNotRule(rules, notPathRule)
}

if len(rule.Ports) > 0 {
portRule := permissionForKeyValues(portKey, convertPortsToString(rule.Ports))
appendRule(rules, portRule)
}

if len(rule.NotPorts) > 0 {
notPortRule := permissionForKeyValues(portKey, convertPortsToString(rule.NotPorts))
appendNotRule(rules, notPortRule)
}

if len(rule.Constraints) > 0 {
// Constraint rule is matched with AND semantics, it's invalid if 2 constraints have the same
// key and this should already be caught in validation stage.
for _, constraint := range rule.Constraints {
p := permissionForKeyValues(constraint.Key, constraint.Values)
if p != nil {
rules.AndRules.Rules = append(rules.AndRules.Rules, p)
}
appendRule(rules, p)
}
}

if len(rules.AndRules.Rules) == 0 {
// None of above rule satisfied means the permission applies to all paths/methods/constraints.
rules.AndRules.Rules = append(rules.AndRules.Rules,
&policyproto.Permission{Rule: &policyproto.Permission_Any{Any: true}})
appendRule(rules, &policyproto.Permission{Rule: &policyproto.Permission_Any{Any: true}})
}

return &policyproto.Permission{Rule: rules}
Expand Down Expand Up @@ -725,17 +760,17 @@ func permissionForKeyValues(key string, values []string) *policyproto.Permission
Rule: &policyproto.Permission_DestinationIp{DestinationIp: cidr},
}, nil
}
case key == attrDestPort:
case key == attrDestPort || key == portKey:
converter = func(v string) (*policyproto.Permission, error) {
port, err := convertToPort(v)
portValue, err := convertToPort(v)
if err != nil {
return nil, err
}
return &policyproto.Permission{
Rule: &policyproto.Permission_DestinationPort{DestinationPort: port},
Rule: &policyproto.Permission_DestinationPort{DestinationPort: portValue},
}, nil
}
case key == pathHeader || key == methodHeader:
case key == pathHeader || key == methodHeader || key == hostHeader:
converter = func(v string) (*policyproto.Permission, error) {
return &policyproto.Permission{
Rule: &policyproto.Permission_Header{
Expand Down

0 comments on commit a9fe655

Please sign in to comment.