xds/rbac: lowercase header matcher names before matching (#9332)

The RBAC filter passes the name of a header matcher to the matching
engine unchanged. The metadata that the engine matches against always
has lowercase keys. A name that contains an uppercase character
therefore matches no header, and the rule that holds it never fires. The
policy parses, reports no error and looks active. A DENY policy written
this way fails open.

The A41 validation reads the same unnormalized name, so the rejection of
:scheme and grpc- prefixed matchers misses the name Grpc-Status. A Host
matcher also keeps its name, although A41 makes host and :authority
equivalent.

Lowercase the name in normalizeHeaderMatcher. That function already owns
the A41 rules and already rewrites the name in place, so the matching
engine, the :scheme and grpc- rejection, and the host alias all read one
normalized name. Envoy holds each header matcher name in a
LowerCaseString, grpc-java lowercases the name before it looks the
header up, and authz/rbac_translator.go lowercases the name on the
non-xDS path.

The new test in test/xds shows the effect on an end user. A DENY policy
on the header name User-Agent returns OK for every RPC before the change
and PermissionDenied after it. The unit tests cover the name that the
parse gives to the engine, the case of the :scheme and grpc- rejection,
and the host alias, at the top level and inside a nested rule.

RELEASE NOTES:

- 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: Markus Magnuson <331091+alimony@users.noreply.github.com>
(cherry picked from commit db94828)

Author: easwars

internal/xds/httpfilter/rbac/rbac.go

@@ -166,17 +166,27 @@ func normalizePrincipalHeaders(principal *v3rbacpb.Principal) error {
 	return nil
 }
 
-// normalizeHeaderMatcher rejects header matchers that A41 forbids (:scheme or a
-// grpc- prefixed name) and rewrites a "host" matcher to ":authority".
+// normalizeHeaderMatcher lowercases the name of a header matcher, rejects the
+// names that A41 forbids (:scheme or a grpc- prefixed name) and rewrites a
+// "host" matcher to ":authority".
 func normalizeHeaderMatcher(header *v3routepb.HeaderMatcher) error {
+	// The keys of the metadata the matchers run against are always lowercase,
+	// so a name that contains an uppercase character matches no header at all
+	// and the rule using it never fires. Lowercase the name, as Envoy and
+	// grpc-java do, both to make it match and to keep the checks below from
+	// being evaded by the case of the name.
 	name := header.GetName()
-	if name == ":scheme" {
+	lowerName := strings.ToLower(name)
+	if lowerName != name {
+		header.Name = lowerName
+	}
+	if lowerName == ":scheme" {
 		return fmt.Errorf("rbac: header matcher for %q is %q", name, ":scheme")
 	}
-	if strings.HasPrefix(name, "grpc-") {
+	if strings.HasPrefix(lowerName, "grpc-") {
 		return fmt.Errorf("rbac: header matcher for %q starts with %q", name, "grpc-")
 	}
-	if name == "host" {
+	if lowerName == "host" {
 		header.Name = ":authority"
 	}
 	return nil

internal/xds/httpfilter/rbac/rbac_test.go

@@ -36,18 +36,19 @@ 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{
+func headerMatcher(name string) *v3routepb.HeaderMatcher {
+	return &v3routepb.HeaderMatcher{
 		Name:                 name,
 		HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true},
-	}}}
+	}
+}
+
+func headerPermission(name string) *v3rbacpb.Permission {
+	return &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Header{Header: headerMatcher(name)}}
 }
 
 func headerPrincipal(name string) *v3rbacpb.Principal {
-	return &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Header{Header: &v3routepb.HeaderMatcher{
-		Name:                 name,
-		HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true},
-	}}}
+	return &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Header{Header: headerMatcher(name)}}
 }
 
 func rbacConfig(perm *v3rbacpb.Permission, principal *v3rbacpb.Principal) *rpb.RBAC {
@@ -125,3 +126,60 @@ func (s) TestNestedHostHeaderAliasing(t *testing.T) {
 		t.Errorf("Nested principal host matcher name = %q, want %q", gotPrincipal, ":authority")
 	}
 }
+
+// TestHeaderMatcherNameIsLowercased checks that a header matcher name is
+// lowercased before it reaches the matching engine. gRPC lowercases every
+// metadata key, so a name that carries an uppercase character matches no
+// header at all, and a DENY policy using one fails open.
+func (s) TestHeaderMatcherNameIsLowercased(t *testing.T) {
+	tests := []struct {
+		name     string
+		wantName string
+	}{
+		{name: "X-Role", wantName: "x-role"},
+		{name: "USER-AGENT", wantName: "user-agent"},
+		// The host to :authority alias must not depend on the case either.
+		{name: "Host", wantName: ":authority"},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			// The permission matcher is nested and the principal matcher is at
+			// the top level, so both the recursive walk and the top level are
+			// covered.
+			permHeader, principalHeader := headerMatcher(test.name), headerMatcher(test.name)
+			perm := &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_NotRule{
+				NotRule: &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Header{Header: permHeader}},
+			}}
+			principal := &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Header{Header: principalHeader}}
+
+			if _, err := parseConfig(rbacConfig(perm, principal)); err != nil {
+				t.Fatalf("parseConfig() failed: %v", err)
+			}
+			if got := permHeader.GetName(); got != test.wantName {
+				t.Errorf("Permission header matcher name = %q, want %q", got, test.wantName)
+			}
+			if got := principalHeader.GetName(); got != test.wantName {
+				t.Errorf("Principal header matcher name = %q, want %q", got, test.wantName)
+			}
+		})
+	}
+}
+
+// TestHeaderMatcherValidationIsCaseInsensitive checks that the A41 rejection of
+// :scheme and grpc- prefixed header matchers cannot be evaded by spelling the
+// name in another case.
+func (s) TestHeaderMatcherValidationIsCaseInsensitive(t *testing.T) {
+	anyPermission := &v3rbacpb.Permission{Rule: &v3rbacpb.Permission_Any{Any: true}}
+	anyPrincipal := &v3rbacpb.Principal{Identifier: &v3rbacpb.Principal_Any{Any: true}}
+
+	for _, name := range []string{":Scheme", "Grpc-Timeout", "GRPC-STATUS"} {
+		t.Run(name, func(t *testing.T) {
+			if _, err := parseConfig(rbacConfig(headerPermission(name), anyPrincipal)); err == nil {
+				t.Errorf("parseConfig() succeeded for permission header matcher %q; want error rejecting a :scheme/grpc- header matcher", name)
+			}
+			if _, err := parseConfig(rbacConfig(anyPermission, headerPrincipal(name))); err == nil {
+				t.Errorf("parseConfig() succeeded for principal header matcher %q; want error rejecting a :scheme/grpc- header matcher", name)
+			}
+		})
+	}
+}

test/xds/xds_server_rbac_test.go

@@ -584,6 +584,52 @@ func (s) TestRBACHTTPFilter(t *testing.T) {
 			wantStatusEmptyCall: codes.OK,
 			wantStatusUnaryCall: codes.OK,
 		},
+		// This test tests that a "Host" header matcher behaves the same as a
+		// "host" one. gRPC lowercases every metadata key, so the alias to
+		// :authority must not depend on how the control plane spells the name.
+		{
+			name: "match-on-host-canonical-case",
+			rbacCfg: &rpb.RBAC{
+				Rules: &v3rbacpb.RBAC{
+					Action: v3rbacpb.RBAC_ALLOW,
+					Policies: map[string]*v3rbacpb.Policy{
+						"match-on-authority": {
+							Permissions: []*v3rbacpb.Permission{
+								{Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: "Host", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PrefixMatch{PrefixMatch: "my-service-fallback"}}}},
+							},
+							Principals: []*v3rbacpb.Principal{
+								{Identifier: &v3rbacpb.Principal_Any{Any: true}},
+							},
+						},
+					},
+				},
+			},
+			wantStatusEmptyCall: codes.OK,
+			wantStatusUnaryCall: codes.OK,
+		},
+		// This test tests that a header matcher whose name is not lowercase
+		// still matches the header. Every RPC carries a user agent, so the
+		// RBAC Configuration below denies every RPC tried.
+		{
+			name: "deny-header-name-in-canonical-case",
+			rbacCfg: &rpb.RBAC{
+				Rules: &v3rbacpb.RBAC{
+					Action: v3rbacpb.RBAC_DENY,
+					Policies: map[string]*v3rbacpb.Policy{
+						"user-agent": {
+							Permissions: []*v3rbacpb.Permission{
+								{Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: "User-Agent", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_PresentMatch{PresentMatch: true}}}},
+							},
+							Principals: []*v3rbacpb.Principal{
+								{Identifier: &v3rbacpb.Principal_Any{Any: true}},
+							},
+						},
+					},
+				},
+			},
+			wantStatusEmptyCall: codes.PermissionDenied,
+			wantStatusUnaryCall: codes.PermissionDenied,
+		},
 		// This test tests that the RBAC HTTP Filter hard codes the :method
 		// header to POST. Since the RBAC Configuration says to deny every RPC
 		// with a method :POST, every RPC tried should be denied.