xds: Small changes at xDS RBAC Layer (#4815)

* xds: Small changes at xDS RBAC Layer
grpc · Sep 27, 2021 · 4555155 · 4555155
1 parent 689f7b1
commit 4555155
Show file tree

Hide file tree

Showing 3 changed files with 132 additions and 1 deletion.
diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go
@@ -148,6 +148,11 @@ func newRPCData(ctx context.Context) (*rpcData, error) {
 	if !ok {
 		return nil, errors.New("missing metadata in incoming context")
 	}
+	// ":method can be hard-coded to POST if unavailable" - A41
+	md[":method"] = []string{"POST"}
+	// "If the transport exposes TE in Metadata, then RBAC must special-case the
+	// header to treat it as not present." - A41
+	delete(md, "TE")
 
 	pi, ok := peer.FromContext(ctx)
 	if !ok {

diff --git a/xds/internal/httpfilter/rbac/rbac.go b/xds/internal/httpfilter/rbac/rbac.go
@@ -106,7 +106,6 @@ func parseConfig(rbacCfg *rpb.RBAC) (httpfilter.FilterConfig, error) {
 			}
 		}
 	}
-
 	return config{config: rbacCfg}, nil
 }
 
@@ -174,6 +173,31 @@ func (builder) BuildServerInterceptor(cfg httpfilter.FilterConfig, override http
 		return nil, nil
 	}
 
+	// "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 icfg.Rules.GetPolicies() {
+		for _, principal := range policy.Principals {
+			if principal.GetHeader() != nil {
+				name := principal.GetHeader().GetName()
+				if name == "host" {
+					principal.GetHeader().Name = ":authority"
+				}
+			}
+		}
+		for _, permission := range policy.Permissions {
+			if permission.GetHeader() != nil {
+				name := permission.GetHeader().GetName()
+				if name == "host" {
+					permission.GetHeader().Name = ":authority"
+				}
+			}
+		}
+	}
+
 	ce, err := rbac.NewChainEngine([]*v3rbacpb.RBAC{icfg.Rules})
 	if err != nil {
 		return nil, fmt.Errorf("error constructing matching engine: %v", err)

diff --git a/xds/internal/test/xds_server_integration_test.go b/xds/internal/test/xds_server_integration_test.go
@@ -788,6 +788,108 @@ func (s) TestRBACHTTPFilter(t *testing.T) {
 			wantStatusEmptyCall: codes.OK,
 			wantStatusUnaryCall: codes.OK,
 		},
+		// The two tests below test that configuring the xDS RBAC HTTP Filter
+		// with :authority and host header matchers end up being logically
+		// equivalent. This represents functionality from this line in A41 -
+		// "As documented for HeaderMatcher, 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."
+
+		// This test tests an xDS RBAC Filter with an :authority header matcher.
+		{
+			name: "match-on-authority",
+			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: ":authority", 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 configuring an xDS RBAC Filter with a host
+		// header matcher has the same behavior as if it was configured with
+		// :authority. Since host and authority are aliased, this should still
+		// continue to match on incoming RPC's :authority, just as the test
+		// above.
+		{
+			name: "match-on-host",
+			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 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.
+		{
+			name: "deny-post",
+			rbacCfg: &rpb.RBAC{
+				Rules: &v3rbacpb.RBAC{
+					Action: v3rbacpb.RBAC_DENY,
+					Policies: map[string]*v3rbacpb.Policy{
+						"post-method": {
+							Permissions: []*v3rbacpb.Permission{
+								{Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: ":method", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "POST"}}}},
+							},
+							Principals: []*v3rbacpb.Principal{
+								{Identifier: &v3rbacpb.Principal_Any{Any: true}},
+							},
+						},
+					},
+				},
+			},
+			wantStatusEmptyCall: codes.PermissionDenied,
+			wantStatusUnaryCall: codes.PermissionDenied,
+		},
+		// This test tests that RBAC ignores the TE: trailers header (which is
+		// hardcoded in http2_client.go for every RPC). Since the RBAC
+		// Configuration says to only ALLOW RPC's with a TE: Trailers, every RPC
+		// tried should be denied.
+		{
+			name: "allow-only-te",
+			rbacCfg: &rpb.RBAC{
+				Rules: &v3rbacpb.RBAC{
+					Action: v3rbacpb.RBAC_ALLOW,
+					Policies: map[string]*v3rbacpb.Policy{
+						"post-method": {
+							Permissions: []*v3rbacpb.Permission{
+								{Rule: &v3rbacpb.Permission_Header{Header: &v3routepb.HeaderMatcher{Name: "TE", HeaderMatchSpecifier: &v3routepb.HeaderMatcher_ExactMatch{ExactMatch: "trailers"}}}},
+							},
+							Principals: []*v3rbacpb.Principal{
+								{Identifier: &v3rbacpb.Principal_Any{Any: true}},
+							},
+						},
+					},
+				},
+			},
+			wantStatusEmptyCall: codes.PermissionDenied,
+			wantStatusUnaryCall: codes.PermissionDenied,
+		},
 	}
 
 	for _, test := range tests {