Skip to content

Commit 1f4c6f3

Browse files
nvxbugnvxbug
authored
xds/rbac: apply header matcher checks to nested and/or/not rules (#9258)
parseConfig only walks the top-level Permissions and Principals of each RBAC policy when it applies the A41 header-name rules, so a header matcher nested inside an and_rules, or_rules, or not_rule is never checked. A control plane can put a `:scheme` or `grpc-` prefixed matcher inside a nested rule to slip past the validation A41 says must reject it, and a nested `host` matcher never gets rewritten to `:authority`, so it silently fails to match the header grpc-go actually carries (a deny policy on a nested host matcher fails open). Walk the full permission and principal trees so both the :scheme/grpc- rejection and the host to :authority rewrite reach matchers at any depth. Doing it in parseConfig keeps the check in the one place that already owns A41 validation, and folds the two former top-level passes into a single recursive walk shared by permissions and principals. RELEASE NOTES: - xds/rbac: Fix a bug where nested `Principal` or `Permission` rules with `:scheme` or `grpc-` prefixed header matchers were not rejected, which could cause DENY rules to fail open. - xds/rbac: Fix a bug where the `host` header matcher was not being replaced with `:authority` in nested `Principal` or `Permission` rules.
1 parent 39156ac commit 1f4c6f3

2 files changed

Lines changed: 211 additions & 28 deletions

File tree

‎internal/xds/httpfilter/rbac/rbac.go‎

Lines changed: 84 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import (
3232
"google.golang.org/protobuf/types/known/anypb"
3333

3434
v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3"
35+
v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
3536
rpb "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/rbac/v3"
3637
)
3738

@@ -68,36 +69,25 @@ func parseConfig(rbacCfg *rpb.RBAC) (httpfilter.FilterConfig, error) {
6869
}
6970

7071
// "It is also a validation failure if Permission or Principal has a
71-
// header matcher for a grpc- prefixed header name or :scheme." - A41
72-
for _, principal := range policy.Principals {
73-
name := principal.GetHeader().GetName()
74-
if name == ":scheme" || strings.HasPrefix(name, "grpc-") {
75-
return nil, fmt.Errorf("rbac: principal header matcher for %v is :scheme or starts with grpc", name)
72+
// header matcher for a grpc- prefixed header name or :scheme." - A41.
73+
//
74+
// "Envoy aliases :authority and Host in its header map implementation,
75+
// so they should be treated equivalent for the RBAC matchers; there must
76+
// be no behavior change depending on which of the two header names is
77+
// used in the RBAC policy." - A41. Any header matcher with value "host"
78+
// is rewritten to ":authority", as that is what grpc-go shifts both
79+
// headers to in the transport layer.
80+
//
81+
// Both rules apply to header matchers nested inside and/or/not rules, so
82+
// the whole permission and principal trees are walked.
83+
for _, principal := range policy.GetPrincipals() {
84+
if err := normalizePrincipalHeaders(principal); err != nil {
85+
return nil, err
7686
}
7787
}
78-
for _, permission := range policy.Permissions {
79-
name := permission.GetHeader().GetName()
80-
if name == ":scheme" || strings.HasPrefix(name, "grpc-") {
81-
return nil, fmt.Errorf("rbac: permission header matcher for %v is :scheme or starts with grpc", name)
82-
}
83-
}
84-
}
85-
86-
// "Envoy aliases :authority and Host in its header map implementation, so
87-
// they should be treated equivalent for the RBAC matchers; there must be no
88-
// behavior change depending on which of the two header names is used in the
89-
// RBAC policy." - A41. Loop through config's principals and policies, change
90-
// any header matcher with value "host" to :authority", as that is what
91-
// grpc-go shifts both headers to in transport layer.
92-
for _, policy := range rbacCfg.GetRules().GetPolicies() {
93-
for _, principal := range policy.Principals {
94-
if principal.GetHeader().GetName() == "host" {
95-
principal.GetHeader().Name = ":authority"
96-
}
97-
}
98-
for _, permission := range policy.Permissions {
99-
if permission.GetHeader().GetName() == "host" {
100-
permission.GetHeader().Name = ":authority"
88+
for _, permission := range policy.GetPermissions() {
89+
if err := normalizePermissionHeaders(permission); err != nil {
90+
return nil, err
10191
}
10292
}
10393
}
@@ -126,6 +116,72 @@ func parseConfig(rbacCfg *rpb.RBAC) (httpfilter.FilterConfig, error) {
126116
return config{chainEngine: ce}, nil
127117
}
128118

119+
// normalizePermissionHeaders applies the A41 header-name rules to every header
120+
// matcher reachable from permission, including those nested inside and/or/not
121+
// rules.
122+
func normalizePermissionHeaders(permission *v3rbacpb.Permission) error {
123+
switch p := permission.GetRule().(type) {
124+
case *v3rbacpb.Permission_Header:
125+
return normalizeHeaderMatcher(p.Header)
126+
case *v3rbacpb.Permission_AndRules:
127+
for _, rule := range p.AndRules.GetRules() {
128+
if err := normalizePermissionHeaders(rule); err != nil {
129+
return err
130+
}
131+
}
132+
case *v3rbacpb.Permission_OrRules:
133+
for _, rule := range p.OrRules.GetRules() {
134+
if err := normalizePermissionHeaders(rule); err != nil {
135+
return err
136+
}
137+
}
138+
case *v3rbacpb.Permission_NotRule:
139+
return normalizePermissionHeaders(p.NotRule)
140+
}
141+
return nil
142+
}
143+
144+
// normalizePrincipalHeaders applies the A41 header-name rules to every header
145+
// matcher reachable from principal, including those nested inside and/or/not
146+
// ids.
147+
func normalizePrincipalHeaders(principal *v3rbacpb.Principal) error {
148+
switch p := principal.GetIdentifier().(type) {
149+
case *v3rbacpb.Principal_Header:
150+
return normalizeHeaderMatcher(p.Header)
151+
case *v3rbacpb.Principal_AndIds:
152+
for _, id := range p.AndIds.GetIds() {
153+
if err := normalizePrincipalHeaders(id); err != nil {
154+
return err
155+
}
156+
}
157+
case *v3rbacpb.Principal_OrIds:
158+
for _, id := range p.OrIds.GetIds() {
159+
if err := normalizePrincipalHeaders(id); err != nil {
160+
return err
161+
}
162+
}
163+
case *v3rbacpb.Principal_NotId:
164+
return normalizePrincipalHeaders(p.NotId)
165+
}
166+
return nil
167+
}
168+
169+
// normalizeHeaderMatcher rejects header matchers that A41 forbids (:scheme or a
170+
// grpc- prefixed name) and rewrites a "host" matcher to ":authority".
171+
func normalizeHeaderMatcher(header *v3routepb.HeaderMatcher) error {
172+
name := header.GetName()
173+
if name == ":scheme" {
174+
return fmt.Errorf("rbac: header matcher for %q is %q", name, ":scheme")
175+
}
176+
if strings.HasPrefix(name, "grpc-") {
177+
return fmt.Errorf("rbac: header matcher for %q starts with %q", name, "grpc-")
178+
}
179+
if name == "host" {
180+
header.Name = ":authority"
181+
}
182+
return nil
183+
}
184+
129185
func (builder) ParseFilterConfig(cfg proto.Message) (httpfilter.FilterConfig, error) {
130186
if cfg == nil {
131187
return nil, fmt.Errorf("rbac: nil configuration message provided")

‎internal/xds/httpfilter/rbac/rbac_test.go‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
/*
2+
*
3+
* Copyright 2026 gRPC authors.
4+
*
5+
* Licensed under the Apache License, Version 2.0 (the "License");
6+
* you may not use this file except in compliance with the License.
7+
* You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*
17+
*/
18+
19+
package rbac
20+
21+
import (
22+
"testing"
23+
24+
"google.golang.org/grpc/internal/grpctest"
25+
26+
v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3"
27+
v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
28+
rpb "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/rbac/v3"
29+
)
30+
31+
type s struct {
32+
grpctest.Tester
33+
}
34+
35+
func Test(t *testing.T) {
36+
grpctest.RunSubTests(t, s{})
37+
}
38+
39+
func headerPermission(name string) *v3rbacpb.Permission {
40+
return &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{
41+
Name: name,
42+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true},
43+
}}}
44+
}
45+
46+
func headerPrincipal(name string) *v3rbacpb.Principal {
47+
return &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{
48+
Name: name,
49+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true},
50+
}}}
51+
}
52+
53+
func rbacConfig(perm *v3rbacpb.Permission, principal *v3rbacpb.Principal) *rpb.RBAC {
54+
return &rpb.RBAC{Rules: &v3rbacpb.RBAC{
55+
Action: v3rbacpb.RBAC_ALLOW,
56+
Policies: map[string]*v3rbacpb.Policy{
57+
"test-policy": {
58+
Permissions: []*v3rbacpb.Permission{perm},
59+
Principals: []*v3rbacpb.Principal{principal},
60+
},
61+
},
62+
}}
63+
}
64+
65+
// TestNestedHeaderMatcherValidation checks that a header matcher for :scheme or
66+
// a grpc- prefixed name is rejected even when it is nested inside an and/or/not
67+
// rule, as A41 requires.
68+
func (s) TestNestedHeaderMatcherValidation(t *testing.T) {
69+
anyPermission := &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Any{Any: true}}
70+
anyPrincipal := &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Any{Any: true}}
71+
72+
tests := []struct {
73+
name string
74+
cfg *rpb.RBAC
75+
}{
76+
{
77+
name: "permission and_rules :scheme",
78+
cfg: rbacConfig(&v3rbacpb.Permission{Rule: &v3rbacpb.Permission_AndRules{AndRules: &v3rbacpb.Permission_Set{
79+
Rules: []*v3rbacpb.Permission{headerPermission(":scheme")},
80+
}}}, anyPrincipal),
81+
},
82+
{
83+
name: "permission not_rule grpc- prefix",
84+
cfg: rbacConfig(&v3rbacpb.Permission{Rule: &v3rbacpb.Permission_NotRule{NotRule: headerPermission("grpc-timeout")}}, anyPrincipal),
85+
},
86+
{
87+
name: "principal or_ids :scheme",
88+
cfg: rbacConfig(anyPermission, &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_OrIds{OrIds: &v3rbacpb.Principal_Set{
89+
Ids: []*v3rbacpb.Principal{headerPrincipal(":scheme")},
90+
}}}),
91+
},
92+
{
93+
name: "principal not_id grpc- prefix",
94+
cfg: rbacConfig(anyPermission, &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_NotId{NotId: headerPrincipal("grpc-encoding")}}),
95+
},
96+
}
97+
for _, test := range tests {
98+
t.Run(test.name, func(t *testing.T) {
99+
if _, err := parseConfig(test.cfg); err == nil {
100+
t.Fatalf("parseConfig() succeeded; want error rejecting a nested :scheme/grpc- header matcher")
101+
}
102+
})
103+
}
104+
}
105+
106+
// TestNestedHostHeaderAliasing checks that a "host" header matcher nested inside
107+
// an and/or/not rule is rewritten to ":authority", so it behaves the same as a
108+
// top-level host matcher (A41 host/:authority equivalence).
109+
func (s) TestNestedHostHeaderAliasing(t *testing.T) {
110+
perm := &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_NotRule{NotRule: headerPermission("host")}}
111+
principal := &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_AndIds{AndIds: &v3rbacpb.Principal_Set{
112+
Ids: []*v3rbacpb.Principal{headerPrincipal("host")},
113+
}}}
114+
115+
if _, err := parseConfig(rbacConfig(perm, principal)); err != nil {
116+
t.Fatalf("parseConfig() failed: %v", err)
117+
}
118+
119+
gotPerm := perm.GetRule().(*v3rbacpb.Permission_NotRule).NotRule.GetRule().(*v3rbacpb.Permission_Header).Header.GetName()
120+
if gotPerm != ":authority" {
121+
t.Errorf("Nested permission host matcher name = %q, want %q", gotPerm, ":authority")
122+
}
123+
gotPrincipal := principal.GetIdentifier().(*v3rbacpb.Principal_AndIds).AndIds.GetIds()[0].GetIdentifier().(*v3rbacpb.Principal_Header).Header.GetName()
124+
if gotPrincipal != ":authority" {
125+
t.Errorf("Nested principal host matcher name = %q, want %q", gotPrincipal, ":authority")
126+
}
127+
}

0 commit comments

Comments
 (0)