Skip to content

Commit

Permalink
[release-1.5] fix authz suffix matching in TCP (#30)
Browse files Browse the repository at this point in the history
  • Loading branch information
@yangminzhu
yangminzhu authored Jul 25, 2020
1 parent c0d01a0 commit 72d2e13
Show file tree
Hide file tree
Showing 8 changed files with 193 additions and 41 deletions.
16 changes: 8 additions & 8 deletions pilot/pkg/security/authz/builder/testdata/v1beta1/action-deny-HTTP-for-TCP-filter-in.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,13 @@ spec:
# rule[8] `from`: all fields, `to`: all fields, `when`: all fields.
- from:
- source:
principals: ["principal"]
principals: ["principal", "*principal-suffix", "principal-prefix*", "*"]
requestPrincipals: ["requestPrincipals"]
namespaces: ["ns"]
namespaces: ["ns", "*ns-suffix", "ns-prefix*", "*"]
ipBlocks: ["1.2.3.4"]
notPrincipals: ["not-principal"]
notPrincipals: ["not-principal", "*not-principal-suffix", "not-principal-prefix*", "*"]
notRequestPrincipals: ["not-requestPrincipals"]
notNamespaces: ["not-ns"]
notNamespaces: ["not-ns", "*not-ns-suffix", "not-ns-prefix*", "*"]
notIpBlocks: ["9.0.0.1"]
to:
- operation:
Expand All @@ -79,11 +79,11 @@ spec:
values: ["10.10.10.10"]
notValues: ["90.10.10.10"]
- key: "source.namespace"
values: ["ns"]
notValues: ["not-ns"]
values: ["ns", "*ns-suffix", "ns-prefix*", "*"]
notValues: ["not-ns", "*not-ns-suffix", "not-ns-prefix*", "*"]
- key: "source.principal"
values: ["principal"]
notValues: ["not-principal"]
values: ["principal", "*principal-suffix", "principal-prefix*", "*"]
notValues: ["not-principal", "*not-principal-suffix", "not-principal-prefix*", "*"]
- key: "request.auth.principal"
values: ["requestPrincipals"]
notValues: ["not-requestPrincipals"]
Expand Down
112 changes: 112 additions & 0 deletions pilot/pkg/security/authz/builder/testdata/v1beta1/action-deny-HTTP-for-TCP-filter-out.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,19 +156,60 @@ rules:
- authenticated:
principalName:
exact: spiffe://principal
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: spiffe://.*principal-suffix
- authenticated:
principalName:
prefix: spiffe://principal-prefix
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .+
- notId:
orIds:
ids:
- authenticated:
principalName:
exact: spiffe://not-principal
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: spiffe://.*not-principal-suffix
- authenticated:
principalName:
prefix: spiffe://not-principal-prefix
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .+
- orIds:
ids:
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/ns/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/.*ns-suffix/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/ns-prefix.*/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/.*/.*
- notId:
orIds:
ids:
Expand All @@ -177,6 +218,21 @@ rules:
safeRegex:
googleRe2: {}
regex: .*/ns/not-ns/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/.*not-ns-suffix/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/not-ns-prefix.*/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/.*/.*
- orIds:
ids:
- sourceIp:
Expand Down Expand Up @@ -206,6 +262,21 @@ rules:
safeRegex:
googleRe2: {}
regex: .*/ns/ns/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/.*ns-suffix/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/ns-prefix.*/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/.*/.*
- notId:
orIds:
ids:
Expand All @@ -214,14 +285,55 @@ rules:
safeRegex:
googleRe2: {}
regex: .*/ns/not-ns/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/.*not-ns-suffix/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/not-ns-prefix.*/.*
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .*/ns/.*/.*
- orIds:
ids:
- authenticated:
principalName:
exact: spiffe://principal
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: spiffe://.*principal-suffix
- authenticated:
principalName:
prefix: spiffe://principal-prefix
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .+
- notId:
orIds:
ids:
- authenticated:
principalName:
exact: spiffe://not-principal
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: spiffe://.*not-principal-suffix
- authenticated:
principalName:
prefix: spiffe://not-principal-prefix
- authenticated:
principalName:
safeRegex:
googleRe2: {}
regex: .+
11 changes: 7 additions & 4 deletions pilot/pkg/security/authz/model/matcher/string.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,14 @@ func StringMatcherWithPrefix(v, prefix string, treatWildcardAsRequired bool) *en
}
return StringMatcherRegex(".*")
case strings.HasPrefix(v, "*"):
return &envoy_matcher.StringMatcher{
MatchPattern: &envoy_matcher.StringMatcher_Suffix{
Suffix: prefix + strings.TrimPrefix(v, "*"),
},
if prefix == "" {
return &envoy_matcher.StringMatcher{
MatchPattern: &envoy_matcher.StringMatcher_Suffix{
Suffix: strings.TrimPrefix(v, "*"),
},
}
}
return StringMatcherRegex(prefix + ".*" + strings.TrimPrefix(v, "*"))
case strings.HasSuffix(v, "*"):
return &envoy_matcher.StringMatcher{
MatchPattern: &envoy_matcher.StringMatcher_Prefix{
Expand Down
35 changes: 23 additions & 12 deletions pilot/pkg/security/authz/model/matcher/string_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,42 +25,53 @@ func TestStringMatcherWithPrefix(t *testing.T) {
testCases := []struct {
name string
v string
prefix string
treatWildcardAsRequired bool
want *envoy_matcher.StringMatcher
}{
{
name: "wildcardAsRequired",
v: "*",
prefix: "abc",
treatWildcardAsRequired: true,
want: StringMatcherRegex(".+"),
},
{
name: "wildcard",
v: "*",
treatWildcardAsRequired: false,
want: StringMatcherRegex(".*"),
name: "wildcard",
v: "*",
prefix: "abc",
want: StringMatcherRegex(".*"),
},
{
name: "prefix",
v: "-prefix-*",
name: "prefix",
v: "-prefix-*",
prefix: "abc",
want: &envoy_matcher.StringMatcher{
MatchPattern: &envoy_matcher.StringMatcher_Prefix{
Prefix: "abc-prefix-",
},
},
},
{
name: "suffix",
v: "*-suffix",
name: "suffix-empty-prefix",
v: "*-suffix",
prefix: "",
want: &envoy_matcher.StringMatcher{
MatchPattern: &envoy_matcher.StringMatcher_Suffix{
Suffix: "abc-suffix",
Suffix: "-suffix",
},
},
},
{
name: "exact",
v: "-exact",
name: "suffix",
v: "*-suffix",
prefix: "abc",
want: StringMatcherRegex("abc.*-suffix"),
},
{
name: "exact",
v: "-exact",
prefix: "abc",
want: &envoy_matcher.StringMatcher{
MatchPattern: &envoy_matcher.StringMatcher_Exact{
Exact: "abc-exact",
Expand All @@ -71,7 +82,7 @@ func TestStringMatcherWithPrefix(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
actual := StringMatcherWithPrefix(tc.v, "abc", tc.treatWildcardAsRequired)
actual := StringMatcherWithPrefix(tc.v, tc.prefix, tc.treatWildcardAsRequired)
if !reflect.DeepEqual(*actual, *tc.want) {
t.Errorf("want %s but got %s", tc.want.String(), actual.String())
}
Expand Down
6 changes: 2 additions & 4 deletions pilot/pkg/security/authz/model/principal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,15 +253,13 @@ func (principal *Principal) forKeyValue(key, value string, forTCPFilter bool) *e
}
return principalSourceIP(cidr)
case attrSrcNamespace == key:
value = strings.Replace(value, "*", ".*", -1)
m := matcher.StringMatcherRegex(fmt.Sprintf(".*/ns/%s/.*", value))
if forTCPFilter {
regex := fmt.Sprintf(".*/ns/%s/.*", value)
m := matcher.StringMatcherRegex(regex)
return principalAuthenticated(m)
}
// Proxy doesn't have attrSrcNamespace directly, but the information is encoded in attrSrcPrincipal
// with format: cluster.local/ns/{NAMESPACE}/sa/{SERVICE-ACCOUNT}.
value = strings.Replace(value, "*", ".*", -1)
m := matcher.StringMatcherRegex(fmt.Sprintf(".*/ns/%s/.*", value))
metadata := matcher.MetadataStringMatcher(authn_model.AuthnFilterName, attrSrcPrincipal, m)
return principalMetadata(metadata)
case attrSrcPrincipal == key:
Expand Down
35 changes: 24 additions & 11 deletions tests/integration/security/rbac_v1beta1_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -684,10 +684,15 @@ func TestV1beta1_TCP(t *testing.T) {
InstancePort: 8091,
},
{
Name: "tcp",
Name: "tcp-8092",
Protocol: protocol.TCP,
InstancePort: 8092,
},
{
Name: "tcp-8093",
Protocol: protocol.TCP,
InstancePort: 8093,
},
}
echoboot.NewBuilderOrFail(t, ctx).
With(&x, util.EchoConfig("x", ns2, false, nil, g, p)).
Expand Down Expand Up @@ -752,38 +757,46 @@ func TestV1beta1_TCP(t *testing.T) {
// The policy on workload b denies request with path "/data" to port 8090:
// - request to port http-8090 should be denied because both path and port are matched.
// - request to port http-8091 should be allowed because the port is not matched.
// - request to port tcp should be allowed because the port is not matched.
// - request to port tcp-8092 should be allowed because the port is not matched.
newTestCase(a, b, "http-8090", false),
newTestCase(a, b, "http-8091", true),
newTestCase(a, b, "tcp", true),
newTestCase(a, b, "tcp-8092", true),

// The policy on workload c denies request to port 8090:
// - request to port http-8090 should be denied because the port is matched.
// - request to http port 8091 should be allowed because the port is not matched.
// - request to tcp port 8092 should be allowed because the port is not matched.
// - request from b to tcp port 8092 should be allowed by default.
// - request from b to tcp port 8093 should be denied because the principal is matched.
// - request from x to tcp port 8092 should be denied because the namespace is matched.
// - request from x to tcp port 8093 should be allowed by default.
newTestCase(a, c, "http-8090", false),
newTestCase(a, c, "http-8091", true),
newTestCase(a, c, "tcp", true),
newTestCase(a, c, "tcp-8092", true),
newTestCase(b, c, "tcp-8092", true),
newTestCase(b, c, "tcp-8093", false),
newTestCase(x, c, "tcp-8092", false),
newTestCase(x, c, "tcp-8093", true),

// The policy on workload d denies request from service account a and workloads in namespace 2:
// - request from a to d should be denied because it has service account a.
// - request from b to d should be allowed.
// - request from c to d should be allowed.
// - request from x to a should be allowed because there is no policy on a.
// - request from x to d should be denied because it's in namespace 2.
newTestCase(a, d, "tcp", false),
newTestCase(b, d, "tcp", true),
newTestCase(c, d, "tcp", true),
newTestCase(x, a, "tcp", true),
newTestCase(x, d, "tcp", false),
newTestCase(a, d, "tcp-8092", false),
newTestCase(b, d, "tcp-8092", true),
newTestCase(c, d, "tcp-8092", true),
newTestCase(x, a, "tcp-8092", true),
newTestCase(x, d, "tcp-8092", false),

// The policy on workload e denies request with path "/other":
// - request to port http-8090 should be allowed because the path is not matched.
// - request to port http-8091 should be allowed because the path is not matched.
// - request to port tcp should be denied because policy uses HTTP fields.
// - request to port tcp-8092 should be denied because policy uses HTTP fields.
newTestCase(a, e, "http-8090", true),
newTestCase(a, e, "http-8091", true),
newTestCase(a, e, "tcp", false),
newTestCase(a, e, "tcp-8092", false),
}

rbacUtil.RunRBACTest(t, cases)
Expand Down
Loading

0 comments on commit 72d2e13

Please sign in to comment.