diff --git a/internal/xds/httpfilter/rbac/rbac.go b/internal/xds/httpfilter/rbac/rbac.go index eb42a7fb1ff1..3bb12b07ae4f 100644 --- a/internal/xds/httpfilter/rbac/rbac.go +++ b/internal/xds/httpfilter/rbac/rbac.go @@ -32,6 +32,7 @@ import ( "google.golang.org/protobuf/types/known/anypb" v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3" + v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" rpb "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/rbac/v3" ) @@ -68,36 +69,25 @@ func parseConfig(rbacCfg *rpb.RBAC) (httpfilter.FilterConfig, error) { } // "It is also a validation failure if Permission or Principal has a - // header matcher for a grpc- prefixed header name or :scheme." - A41 - for _, principal := range policy.Principals { - name := principal.GetHeader().GetName() - if name == ":scheme" || strings.HasPrefix(name, "grpc-") { - return nil, fmt.Errorf("rbac: principal header matcher for %v is :scheme or starts with grpc", name) + // header matcher for a grpc- prefixed header name or :scheme." - A41. + // + // "Envoy aliases :authority and Host in its header map implementation, + // so they should be treated equivalent for the RBAC matchers; there must + // be no behavior change depending on which of the two header names is + // used in the RBAC policy." - A41. Any header matcher with value "host" + // is rewritten to ":authority", as that is what grpc-go shifts both + // headers to in the transport layer. + // + // Both rules apply to header matchers nested inside and/or/not rules, so + // the whole permission and principal trees are walked. + for _, principal := range policy.GetPrincipals() { + if err := normalizePrincipalHeaders(principal); err != nil { + return nil, err } } - for _, permission := range policy.Permissions { - name := permission.GetHeader().GetName() - if name == ":scheme" || strings.HasPrefix(name, "grpc-") { - return nil, fmt.Errorf("rbac: permission header matcher for %v is :scheme or starts with grpc", name) - } - } - } - - // "Envoy aliases :authority and Host in its header map implementation, so - // they should be treated equivalent for the RBAC matchers; there must be no - // behavior change depending on which of the two header names is used in the - // RBAC policy." - A41. Loop through config's principals and policies, change - // any header matcher with value "host" to :authority", as that is what - // grpc-go shifts both headers to in transport layer. - for _, policy := range rbacCfg.GetRules().GetPolicies() { - for _, principal := range policy.Principals { - if principal.GetHeader().GetName() == "host" { - principal.GetHeader().Name = ":authority" - } - } - for _, permission := range policy.Permissions { - if permission.GetHeader().GetName() == "host" { - permission.GetHeader().Name = ":authority" + for _, permission := range policy.GetPermissions() { + if err := normalizePermissionHeaders(permission); err != nil { + return nil, err } } } @@ -126,6 +116,72 @@ func parseConfig(rbacCfg *rpb.RBAC) (httpfilter.FilterConfig, error) { return config{chainEngine: ce}, nil } +// normalizePermissionHeaders applies the A41 header-name rules to every header +// matcher reachable from permission, including those nested inside and/or/not +// rules. +func normalizePermissionHeaders(permission *v3rbacpb.Permission) error { + switch p := permission.GetRule().(type) { + case *v3rbacpb.Permission_Header: + return normalizeHeaderMatcher(p.Header) + case *v3rbacpb.Permission_AndRules: + for _, rule := range p.AndRules.GetRules() { + if err := normalizePermissionHeaders(rule); err != nil { + return err + } + } + case *v3rbacpb.Permission_OrRules: + for _, rule := range p.OrRules.GetRules() { + if err := normalizePermissionHeaders(rule); err != nil { + return err + } + } + case *v3rbacpb.Permission_NotRule: + return normalizePermissionHeaders(p.NotRule) + } + return nil +} + +// normalizePrincipalHeaders applies the A41 header-name rules to every header +// matcher reachable from principal, including those nested inside and/or/not +// ids. +func normalizePrincipalHeaders(principal *v3rbacpb.Principal) error { + switch p := principal.GetIdentifier().(type) { + case *v3rbacpb.Principal_Header: + return normalizeHeaderMatcher(p.Header) + case *v3rbacpb.Principal_AndIds: + for _, id := range p.AndIds.GetIds() { + if err := normalizePrincipalHeaders(id); err != nil { + return err + } + } + case *v3rbacpb.Principal_OrIds: + for _, id := range p.OrIds.GetIds() { + if err := normalizePrincipalHeaders(id); err != nil { + return err + } + } + case *v3rbacpb.Principal_NotId: + return normalizePrincipalHeaders(p.NotId) + } + return nil +} + +// normalizeHeaderMatcher rejects header matchers that A41 forbids (:scheme or a +// grpc- prefixed name) and rewrites a "host" matcher to ":authority". +func normalizeHeaderMatcher(header *v3routepb.HeaderMatcher) error { + name := header.GetName() + if name == ":scheme" { + return fmt.Errorf("rbac: header matcher for %q is %q", name, ":scheme") + } + if strings.HasPrefix(name, "grpc-") { + return fmt.Errorf("rbac: header matcher for %q starts with %q", name, "grpc-") + } + if name == "host" { + header.Name = ":authority" + } + return nil +} + func (builder) ParseFilterConfig(cfg proto.Message) (httpfilter.FilterConfig, error) { if cfg == nil { return nil, fmt.Errorf("rbac: nil configuration message provided") diff --git a/internal/xds/httpfilter/rbac/rbac_test.go b/internal/xds/httpfilter/rbac/rbac_test.go new file mode 100644 index 000000000000..3d714de897df --- /dev/null +++ b/internal/xds/httpfilter/rbac/rbac_test.go @@ -0,0 +1,127 @@ +/* + * + * Copyright 2026 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package rbac + +import ( + "testing" + + "google.golang.org/grpc/internal/grpctest" + + v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3" + v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" + rpb "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/rbac/v3" +) + +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { + grpctest.RunSubTests(t, s{}) +} + +func headerPermission(name string) *v3rbacpb.Permission { + return &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{ + Name: name, + HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true}, + }}} +} + +func headerPrincipal(name string) *v3rbacpb.Principal { + return &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{ + Name: name, + HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true}, + }}} +} + +func rbacConfig(perm *v3rbacpb.Permission, principal *v3rbacpb.Principal) *rpb.RBAC { + return &rpb.RBAC{Rules: &v3rbacpb.RBAC{ + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "test-policy": { + Permissions: []*v3rbacpb.Permission{perm}, + Principals: []*v3rbacpb.Principal{principal}, + }, + }, + }} +} + +// TestNestedHeaderMatcherValidation checks that a header matcher for :scheme or +// a grpc- prefixed name is rejected even when it is nested inside an and/or/not +// rule, as A41 requires. +func (s) TestNestedHeaderMatcherValidation(t *testing.T) { + anyPermission := &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Any{Any: true}} + anyPrincipal := &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Any{Any: true}} + + tests := []struct { + name string + cfg *rpb.RBAC + }{ + { + name: "permission and_rules :scheme", + cfg: rbacConfig(&v3rbacpb.Permission{Rule: &v3rbacpb.Permission_AndRules{AndRules: &v3rbacpb.Permission_Set{ + Rules: []*v3rbacpb.Permission{headerPermission(":scheme")}, + }}}, anyPrincipal), + }, + { + name: "permission not_rule grpc- prefix", + cfg: rbacConfig(&v3rbacpb.Permission{Rule: &v3rbacpb.Permission_NotRule{NotRule: headerPermission("grpc-timeout")}}, anyPrincipal), + }, + { + name: "principal or_ids :scheme", + cfg: rbacConfig(anyPermission, &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_OrIds{OrIds: &v3rbacpb.Principal_Set{ + Ids: []*v3rbacpb.Principal{headerPrincipal(":scheme")}, + }}}), + }, + { + name: "principal not_id grpc- prefix", + cfg: rbacConfig(anyPermission, &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_NotId{NotId: headerPrincipal("grpc-encoding")}}), + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if _, err := parseConfig(test.cfg); err == nil { + t.Fatalf("parseConfig() succeeded; want error rejecting a nested :scheme/grpc- header matcher") + } + }) + } +} + +// TestNestedHostHeaderAliasing checks that a "host" header matcher nested inside +// an and/or/not rule is rewritten to ":authority", so it behaves the same as a +// top-level host matcher (A41 host/:authority equivalence). +func (s) TestNestedHostHeaderAliasing(t *testing.T) { + perm := &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_NotRule{NotRule: headerPermission("host")}} + principal := &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_AndIds{AndIds: &v3rbacpb.Principal_Set{ + Ids: []*v3rbacpb.Principal{headerPrincipal("host")}, + }}} + + if _, err := parseConfig(rbacConfig(perm, principal)); err != nil { + t.Fatalf("parseConfig() failed: %v", err) + } + + gotPerm := perm.GetRule().(*v3rbacpb.Permission_NotRule).NotRule.GetRule().(*v3rbacpb.Permission_Header).Header.GetName() + if gotPerm != ":authority" { + t.Errorf("Nested permission host matcher name = %q, want %q", gotPerm, ":authority") + } + gotPrincipal := principal.GetIdentifier().(*v3rbacpb.Principal_AndIds).AndIds.GetIds()[0].GetIdentifier().(*v3rbacpb.Principal_Header).Header.GetName() + if gotPrincipal != ":authority" { + t.Errorf("Nested principal host matcher name = %q, want %q", gotPrincipal, ":authority") + } +}