Skip to content

Commit 57c9ff1

Browse files
pawbhardarjan-balgemini-code-assist[bot]
authored
xds: ensure full-string matching for RBAC Filter rules (#9148)
**Summary** This PR fixes a regression introduced in #9134 where xDS SafeRegexMatch configurations were performing partial matches instead of the full-string matches as earlier in RBAC. **Root cause** PullRequest #9134 removed grpcutil.FullMatchWithRegex in favor of regexp.MatchString. In Go, MatchString performs a partial match unless the regex is explicitly anchored with ^ and $. While #9134 introduced a CompileSafeRegex helper to add these anchors. This caused authorization policies and mutation rules to accept requests that only partially matched the defined regex (e.g., a rule for match-principal would incorrectly match a request with never-match-principal). **Changes** * RBAC: Updated internal/xds/rbac/matchers.go to use internalmatcher.CompileSafeRegex for SafeRegexMatch configurations. **Issue identification** The regression was caught by psm-interop tests where an RBAC DENY rule was bypassed due to a partial match in an preceding ALLOW rule. RBAC test started failing continuously after May 20 (Commit time) RELEASE NOTES: none --------- Co-authored-by: Arjan Singh Bal <46515553+arjan-bal@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Arjan Bal <arjansbal@google.com>
1 parent b58f32d commit 57c9ff1

5 files changed

Lines changed: 138 additions & 20 deletions

File tree

‎internal/xds/httpfilter/extconfig.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,14 +69,14 @@ func HeaderMutationRulesFromProto(mr *v3mutationpb.HeaderMutationRules) (HeaderM
6969
return rules, nil
7070
}
7171
if allowExpr := mr.GetAllowExpression(); allowExpr != nil {
72-
re, err := regexp.Compile(allowExpr.GetRegex())
72+
re, err := matcher.CompileSafeRegex(allowExpr.GetRegex())
7373
if err != nil {
7474
return rules, fmt.Errorf("httpfilter: %v", err)
7575
}
7676
rules.AllowExpr = re
7777
}
7878
if disallowExpr := mr.GetDisallowExpression(); disallowExpr != nil {
79-
re, err := regexp.Compile(disallowExpr.GetRegex())
79+
re, err := matcher.CompileSafeRegex(disallowExpr.GetRegex())
8080
if err != nil {
8181
return rules, fmt.Errorf("httpfilter: %v", err)
8282
}

‎internal/xds/httpfilter/extconfig_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,8 @@ func (s) TestHeaderMutationRulesFromProto_HappyPath(t *testing.T) {
4747
DisallowIsError: wrapperspb.Bool(false),
4848
}
4949
want := HeaderMutationRules{
50-
AllowExpr: regexp.MustCompile("^allow$"),
51-
DisallowExpr: regexp.MustCompile("^disallow$"),
50+
AllowExpr: regexp.MustCompile("^(?:^allow$)$"),
51+
DisallowExpr: regexp.MustCompile("^(?:^disallow$)$"),
5252
DisallowAll: true,
5353
DisallowIsError: false,
5454
}

‎internal/xds/httpfilter/extproc/config_test.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -206,8 +206,8 @@ func (s) TestParseFilterConfig_Success(t *testing.T) {
206206
},
207207
failureModeAllow: false,
208208
mutationRules: httpfilter.HeaderMutationRules{
209-
AllowExpr: regexp.MustCompile(".*"),
210-
DisallowExpr: regexp.MustCompile("a"),
209+
AllowExpr: regexp.MustCompile("^(?:.*)$"),
210+
DisallowExpr: regexp.MustCompile("^(?:a)$"),
211211
},
212212
deferredCloseTimeout: defaultDeferredCloseTimeout,
213213
},
@@ -560,8 +560,8 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
560560
Timeout: 5 * time.Second,
561561
},
562562
mutationRules: httpfilter.HeaderMutationRules{
563-
AllowExpr: regexp.MustCompile("allow-.*"),
564-
DisallowExpr: regexp.MustCompile("disallow-.*"),
563+
AllowExpr: regexp.MustCompile("^(?:allow-.*)$"),
564+
DisallowExpr: regexp.MustCompile("^(?:disallow-.*)$"),
565565
DisallowAll: true,
566566
DisallowIsError: true,
567567
},
@@ -572,8 +572,8 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
572572
requestAttributes: []string{"attr1"},
573573
responseAttributes: []string{"attr2"},
574574
mutationRules: httpfilter.HeaderMutationRules{
575-
AllowExpr: regexp.MustCompile("allow-.*"),
576-
DisallowExpr: regexp.MustCompile("disallow-.*"),
575+
AllowExpr: regexp.MustCompile("^(?:allow-.*)$"),
576+
DisallowExpr: regexp.MustCompile("^(?:disallow-.*)$"),
577577
DisallowAll: true,
578578
DisallowIsError: true,
579579
},
@@ -618,8 +618,8 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
618618
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
619619
},
620620
mutationRules: httpfilter.HeaderMutationRules{
621-
AllowExpr: regexp.MustCompile("allow-.*"),
622-
DisallowExpr: regexp.MustCompile("disallow-.*"),
621+
AllowExpr: regexp.MustCompile("^(?:allow-.*)$"),
622+
DisallowExpr: regexp.MustCompile("^(?:disallow-.*)$"),
623623
DisallowAll: true,
624624
DisallowIsError: true,
625625
},
@@ -647,8 +647,8 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
647647
requestAttributes: []string{"override-attr1"},
648648
responseAttributes: []string{"override-attr2"},
649649
mutationRules: httpfilter.HeaderMutationRules{
650-
AllowExpr: regexp.MustCompile("allow-.*"),
651-
DisallowExpr: regexp.MustCompile("disallow-.*"),
650+
AllowExpr: regexp.MustCompile("^(?:allow-.*)$"),
651+
DisallowExpr: regexp.MustCompile("^(?:disallow-.*)$"),
652652
DisallowAll: true,
653653
DisallowIsError: true,
654654
},
@@ -691,8 +691,8 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
691691
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
692692
},
693693
mutationRules: httpfilter.HeaderMutationRules{
694-
AllowExpr: regexp.MustCompile("allow-.*"),
695-
DisallowExpr: regexp.MustCompile("disallow-.*"),
694+
AllowExpr: regexp.MustCompile("^(?:allow-.*)$"),
695+
DisallowExpr: regexp.MustCompile("^(?:disallow-.*)$"),
696696
DisallowAll: true,
697697
DisallowIsError: true,
698698
},
@@ -707,8 +707,8 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
707707
requestAttributes: []string{"base-attr1"},
708708
responseAttributes: []string{"base-attr2"},
709709
mutationRules: httpfilter.HeaderMutationRules{
710-
AllowExpr: regexp.MustCompile("allow-.*"),
711-
DisallowExpr: regexp.MustCompile("disallow-.*"),
710+
AllowExpr: regexp.MustCompile("^(?:allow-.*)$"),
711+
DisallowExpr: regexp.MustCompile("^(?:disallow-.*)$"),
712712
DisallowAll: true,
713713
DisallowIsError: true,
714714
},

‎internal/xds/rbac/matchers.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"fmt"
2222
"net"
2323
"net/netip"
24-
"regexp"
2524

2625
v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
2726
v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3"
@@ -271,7 +270,7 @@ func newHeaderMatcher(headerMatcherConfig *v3route_componentspb.HeaderMatcher) (
271270
case *v3route_componentspb.HeaderMatcher_ExactMatch:
272271
m = internalmatcher.NewHeaderExactMatcher(headerMatcherConfig.Name, headerMatcherConfig.GetExactMatch(), headerMatcherConfig.InvertMatch)
273272
case *v3route_componentspb.HeaderMatcher_SafeRegexMatch:
274-
regex, err := regexp.Compile(headerMatcherConfig.GetSafeRegexMatch().Regex)
273+
regex, err := internalmatcher.CompileSafeRegex(headerMatcherConfig.GetSafeRegexMatch().GetRegex())
275274
if err != nil {
276275
return nil, err
277276
}

‎internal/xds/rbac/rbac_engine_test.go‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1236,6 +1236,125 @@ func (s) TestChainEngine(t *testing.T) {
12361236
},
12371237
},
12381238
},
1239+
// This test verifies that SafeRegex header matching is implicitly
1240+
// anchored at both ends (full string matching).
1241+
{
1242+
name: "SafeRegexHeaderMatcherImplicitAnchoring",
1243+
rbacConfigs: []*v3rbacpb.RBAC{
1244+
{
1245+
Action: v3rbacpb.RBAC_ALLOW,
1246+
Policies: map[string]*v3rbacpb.Policy{
1247+
"safe-regex-policy": {
1248+
Permissions: []*v3rbacpb.Permission{
1249+
{Rule: &v3rbacpb.Permission_Any{Any: true}},
1250+
},
1251+
Principals: []*v3rbacpb.Principal{
1252+
{
1253+
Identifier: &v3rbacpb.Principal_Header{
1254+
Header: &v3routepb.HeaderMatcher{
1255+
Name: "foo",
1256+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SafeRegexMatch{
1257+
SafeRegexMatch: &v3matcherpb.RegexMatcher{
1258+
Regex: "abc",
1259+
},
1260+
},
1261+
},
1262+
},
1263+
},
1264+
},
1265+
},
1266+
},
1267+
},
1268+
},
1269+
rbacQueries: []rbacQuery{
1270+
// This RPC has header "foo: abc", which matches "abc" exactly, so it should be allowed (OK).
1271+
{
1272+
rpcData: &rpcData{
1273+
md: metadata.MD{
1274+
"foo": []string{"abc"},
1275+
},
1276+
fullMethod: "some method",
1277+
peerInfo: &peer.Peer{
1278+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
1279+
},
1280+
},
1281+
wantStatusCode: codes.OK,
1282+
},
1283+
// This RPC has header "foo: 123abc456", which contains "abc" but is not an exact match.
1284+
// Since safe regex is implicitly anchored, it should not match and return PermissionDenied.
1285+
{
1286+
rpcData: &rpcData{
1287+
md: metadata.MD{
1288+
"foo": []string{"123abc456"},
1289+
},
1290+
fullMethod: "some method",
1291+
peerInfo: &peer.Peer{
1292+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
1293+
},
1294+
},
1295+
wantStatusCode: codes.PermissionDenied,
1296+
},
1297+
},
1298+
},
1299+
// This test verifies that SafeRegex header matching with wildcard matching
1300+
// (e.g. .*abc.*) matches substrings as expected.
1301+
{
1302+
name: "SafeRegexHeaderMatcherWildcard",
1303+
rbacConfigs: []*v3rbacpb.RBAC{
1304+
{
1305+
Action: v3rbacpb.RBAC_ALLOW,
1306+
Policies: map[string]*v3rbacpb.Policy{
1307+
"safe-regex-wildcard-policy": {
1308+
Permissions: []*v3rbacpb.Permission{
1309+
{Rule: &v3rbacpb.Permission_Any{Any: true}},
1310+
},
1311+
Principals: []*v3rbacpb.Principal{
1312+
{
1313+
Identifier: &v3rbacpb.Principal_Header{
1314+
Header: &v3routepb.HeaderMatcher{
1315+
Name: "foo",
1316+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_SafeRegexMatch{
1317+
SafeRegexMatch: &v3matcherpb.RegexMatcher{
1318+
Regex: ".*abc.*",
1319+
},
1320+
},
1321+
},
1322+
},
1323+
},
1324+
},
1325+
},
1326+
},
1327+
},
1328+
},
1329+
rbacQueries: []rbacQuery{
1330+
// This RPC has header "foo: 123abc456", which matches the wildcard safe regex, so it should be allowed (OK).
1331+
{
1332+
rpcData: &rpcData{
1333+
md: metadata.MD{
1334+
"foo": []string{"123abc456"},
1335+
},
1336+
fullMethod: "some method",
1337+
peerInfo: &peer.Peer{
1338+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
1339+
},
1340+
},
1341+
wantStatusCode: codes.OK,
1342+
},
1343+
// This RPC has header "foo: xyz", which does not contain "abc", so it should return PermissionDenied.
1344+
{
1345+
rpcData: &rpcData{
1346+
md: metadata.MD{
1347+
"foo": []string{"xyz"},
1348+
},
1349+
fullMethod: "some method",
1350+
peerInfo: &peer.Peer{
1351+
Addr: &addr{ipAddress: "0.0.0.0:8080"},
1352+
},
1353+
},
1354+
wantStatusCode: codes.PermissionDenied,
1355+
},
1356+
},
1357+
},
12391358
// AllowAndDenyPolicy tests a policy with an allow (on path) and
12401359
// deny (on port) policy chained together. This represents how a user
12411360
// configured interceptor would use this, and also is a potential

0 commit comments

Comments
 (0)