Skip to content

Commit ebba6f3

Browse files
easwarsnvxbugalimony
authored
Cherry-pick #9258 and #9332 into v1.83.x (#9335)
Original PRs: #9258 and #9332 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. - xds/rbac: Fix a bug where a header matcher whose name was not lowercase, such as `X-Role`, matched no header, which could cause DENY rules to fail open. - xds/rbac: Fix a bug where a `:scheme` or `grpc-` prefixed header matcher was accepted when its name was not lowercase. - xds/rbac: Fix a bug where a `Host` header matcher was not replaced with `:authority`. --------- Co-authored-by: Naveed <naveed@bugqore.com> Co-authored-by: Markus Magnuson <331091+alimony@users.noreply.github.com>
1 parent 8cfeca0 commit ebba6f3

3 files changed

Lines changed: 325 additions & 28 deletions

File tree

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

Lines changed: 94 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,82 @@ 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 lowercases the name of a header matcher, rejects the
170+
// names that A41 forbids (:scheme or a grpc- prefixed name) and rewrites a
171+
// "host" matcher to ":authority".
172+
func normalizeHeaderMatcher(header *v3routepb.HeaderMatcher) error {
173+
// The keys of the metadata the matchers run against are always lowercase,
174+
// so a name that contains an uppercase character matches no header at all
175+
// and the rule using it never fires. Lowercase the name, as Envoy and
176+
// grpc-java do, both to make it match and to keep the checks below from
177+
// being evaded by the case of the name.
178+
name := header.GetName()
179+
lowerName := strings.ToLower(name)
180+
if lowerName != name {
181+
header.Name = lowerName
182+
}
183+
if lowerName == ":scheme" {
184+
return fmt.Errorf("rbac: header matcher for %q is %q", name, ":scheme")
185+
}
186+
if strings.HasPrefix(lowerName, "grpc-") {
187+
return fmt.Errorf("rbac: header matcher for %q starts with %q", name, "grpc-")
188+
}
189+
if lowerName == "host" {
190+
header.Name = ":authority"
191+
}
192+
return nil
193+
}
194+
129195
func (builder) ParseFilterConfig(cfg proto.Message) (httpfilter.FilterConfig, error) {
130196
if cfg == nil {
131197
return nil, fmt.Errorf("rbac: nil configuration message provided")

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

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
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 headerMatcher(name string) *v3routepb.HeaderMatcher {
40+
return &v3routepb.HeaderMatcher{
41+
Name: name,
42+
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true},
43+
}
44+
}
45+
46+
func headerPermission(name string) *v3rbacpb.Permission {
47+
return &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Header{Header: headerMatcher(name)}}
48+
}
49+
50+
func headerPrincipal(name string) *v3rbacpb.Principal {
51+
return &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Header{Header: headerMatcher(name)}}
52+
}
53+
54+
func rbacConfig(perm *v3rbacpb.Permission, principal *v3rbacpb.Principal) *rpb.RBAC {
55+
return &rpb.RBAC{Rules: &v3rbacpb.RBAC{
56+
Action: v3rbacpb.RBAC_ALLOW,
57+
Policies: map[string]*v3rbacpb.Policy{
58+
"test-policy": {
59+
Permissions: []*v3rbacpb.Permission{perm},
60+
Principals: []*v3rbacpb.Principal{principal},
61+
},
62+
},
63+
}}
64+
}
65+
66+
// TestNestedHeaderMatcherValidation checks that a header matcher for :scheme or
67+
// a grpc- prefixed name is rejected even when it is nested inside an and/or/not
68+
// rule, as A41 requires.
69+
func (s) TestNestedHeaderMatcherValidation(t *testing.T) {
70+
anyPermission := &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Any{Any: true}}
71+
anyPrincipal := &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Any{Any: true}}
72+
73+
tests := []struct {
74+
name string
75+
cfg *rpb.RBAC
76+
}{
77+
{
78+
name: "permission and_rules :scheme",
79+
cfg: rbacConfig(&v3rbacpb.Permission{Rule: &v3rbacpb.Permission_AndRules{AndRules: &v3rbacpb.Permission_Set{
80+
Rules: []*v3rbacpb.Permission{headerPermission(":scheme")},
81+
}}}, anyPrincipal),
82+
},
83+
{
84+
name: "permission not_rule grpc- prefix",
85+
cfg: rbacConfig(&v3rbacpb.Permission{Rule: &v3rbacpb.Permission_NotRule{NotRule: headerPermission("grpc-timeout")}}, anyPrincipal),
86+
},
87+
{
88+
name: "principal or_ids :scheme",
89+
cfg: rbacConfig(anyPermission, &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_OrIds{OrIds: &v3rbacpb.Principal_Set{
90+
Ids: []*v3rbacpb.Principal{headerPrincipal(":scheme")},
91+
}}}),
92+
},
93+
{
94+
name: "principal not_id grpc- prefix",
95+
cfg: rbacConfig(anyPermission, &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_NotId{NotId: headerPrincipal("grpc-encoding")}}),
96+
},
97+
}
98+
for _, test := range tests {
99+
t.Run(test.name, func(t *testing.T) {
100+
if _, err := parseConfig(test.cfg); err == nil {
101+
t.Fatalf("parseConfig() succeeded; want error rejecting a nested :scheme/grpc- header matcher")
102+
}
103+
})
104+
}
105+
}
106+
107+
// TestNestedHostHeaderAliasing checks that a "host" header matcher nested inside
108+
// an and/or/not rule is rewritten to ":authority", so it behaves the same as a
109+
// top-level host matcher (A41 host/:authority equivalence).
110+
func (s) TestNestedHostHeaderAliasing(t *testing.T) {
111+
perm := &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_NotRule{NotRule: headerPermission("host")}}
112+
principal := &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_AndIds{AndIds: &v3rbacpb.Principal_Set{
113+
Ids: []*v3rbacpb.Principal{headerPrincipal("host")},
114+
}}}
115+
116+
if _, err := parseConfig(rbacConfig(perm, principal)); err != nil {
117+
t.Fatalf("parseConfig() failed: %v", err)
118+
}
119+
120+
gotPerm := perm.GetRule().(*v3rbacpb.Permission_NotRule).NotRule.GetRule().(*v3rbacpb.Permission_Header).Header.GetName()
121+
if gotPerm != ":authority" {
122+
t.Errorf("Nested permission host matcher name = %q, want %q", gotPerm, ":authority")
123+
}
124+
gotPrincipal := principal.GetIdentifier().(*v3rbacpb.Principal_AndIds).AndIds.GetIds()[0].GetIdentifier().(*v3rbacpb.Principal_Header).Header.GetName()
125+
if gotPrincipal != ":authority" {
126+
t.Errorf("Nested principal host matcher name = %q, want %q", gotPrincipal, ":authority")
127+
}
128+
}
129+
130+
// TestHeaderMatcherNameIsLowercased checks that a header matcher name is
131+
// lowercased before it reaches the matching engine. gRPC lowercases every
132+
// metadata key, so a name that carries an uppercase character matches no
133+
// header at all, and a DENY policy using one fails open.
134+
func (s) TestHeaderMatcherNameIsLowercased(t *testing.T) {
135+
tests := []struct {
136+
name string
137+
wantName string
138+
}{
139+
{name: "X-Role", wantName: "x-role"},
140+
{name: "USER-AGENT", wantName: "user-agent"},
141+
// The host to :authority alias must not depend on the case either.
142+
{name: "Host", wantName: ":authority"},
143+
}
144+
for _, test := range tests {
145+
t.Run(test.name, func(t *testing.T) {
146+
// The permission matcher is nested and the principal matcher is at
147+
// the top level, so both the recursive walk and the top level are
148+
// covered.
149+
permHeader, principalHeader := headerMatcher(test.name), headerMatcher(test.name)
150+
perm := &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_NotRule{
151+
NotRule: &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Header{Header: permHeader}},
152+
}}
153+
principal := &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Header{Header: principalHeader}}
154+
155+
if _, err := parseConfig(rbacConfig(perm, principal)); err != nil {
156+
t.Fatalf("parseConfig() failed: %v", err)
157+
}
158+
if got := permHeader.GetName(); got != test.wantName {
159+
t.Errorf("Permission header matcher name = %q, want %q", got, test.wantName)
160+
}
161+
if got := principalHeader.GetName(); got != test.wantName {
162+
t.Errorf("Principal header matcher name = %q, want %q", got, test.wantName)
163+
}
164+
})
165+
}
166+
}
167+
168+
// TestHeaderMatcherValidationIsCaseInsensitive checks that the A41 rejection of
169+
// :scheme and grpc- prefixed header matchers cannot be evaded by spelling the
170+
// name in another case.
171+
func (s) TestHeaderMatcherValidationIsCaseInsensitive(t *testing.T) {
172+
anyPermission := &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Any{Any: true}}
173+
anyPrincipal := &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Any{Any: true}}
174+
175+
for _, name := range []string{":Scheme", "Grpc-Timeout", "GRPC-STATUS"} {
176+
t.Run(name, func(t *testing.T) {
177+
if _, err := parseConfig(rbacConfig(headerPermission(name), anyPrincipal)); err == nil {
178+
t.Errorf("parseConfig() succeeded for permission header matcher %q; want error rejecting a :scheme/grpc- header matcher", name)
179+
}
180+
if _, err := parseConfig(rbacConfig(anyPermission, headerPrincipal(name))); err == nil {
181+
t.Errorf("parseConfig() succeeded for principal header matcher %q; want error rejecting a :scheme/grpc- header matcher", name)
182+
}
183+
})
184+
}
185+
}

‎test/xds/xds_server_rbac_test.go‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -583,6 +583,52 @@ func (s) TestRBACHTTPFilter(t *testing.T) {
583583
wantStatusEmptyCall: codes.OK,
584584
wantStatusUnaryCall: codes.OK,
585585
},
586+
// This test tests that a "Host" header matcher behaves the same as a
587+
// "host" one. gRPC lowercases every metadata key, so the alias to
588+
// :authority must not depend on how the control plane spells the name.
589+
{
590+
name: "match-on-host-canonical-case",
591+
rbacCfg: &rpb.RBAC{
592+
Rules: &v3rbacpb.RBAC{
593+
Action: v3rbacpb.RBAC_ALLOW,
594+
Policies: map[string]*v3rbacpb.Policy{
595+
"match-on-authority": {
596+
Permissions: []*v3rbacpb.Permission{
597+
{Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: "Host", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PrefixMatch{PrefixMatch: "my-service-fallback"}}}},
598+
},
599+
Principals: []*v3rbacpb.Principal{
600+
{Identifier: &v3rbacpb.Principal_Any{Any: true}},
601+
},
602+
},
603+
},
604+
},
605+
},
606+
wantStatusEmptyCall: codes.OK,
607+
wantStatusUnaryCall: codes.OK,
608+
},
609+
// This test tests that a header matcher whose name is not lowercase
610+
// still matches the header. Every RPC carries a user agent, so the
611+
// RBAC Configuration below denies every RPC tried.
612+
{
613+
name: "deny-header-name-in-canonical-case",
614+
rbacCfg: &rpb.RBAC{
615+
Rules: &v3rbacpb.RBAC{
616+
Action: v3rbacpb.RBAC_DENY,
617+
Policies: map[string]*v3rbacpb.Policy{
618+
"user-agent": {
619+
Permissions: []*v3rbacpb.Permission{
620+
{Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: "User-Agent", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true}}}},
621+
},
622+
Principals: []*v3rbacpb.Principal{
623+
{Identifier: &v3rbacpb.Principal_Any{Any: true}},
624+
},
625+
},
626+
},
627+
},
628+
},
629+
wantStatusEmptyCall: codes.PermissionDenied,
630+
wantStatusUnaryCall: codes.PermissionDenied,
631+
},
586632
// This test tests that the RBAC HTTP Filter hard codes the :method
587633
// header to POST. Since the RBAC Configuration says to deny every RPC
588634
// with a method :POST, every RPC tried should be denied.

0 commit comments

Comments
 (0)