From a6a47225d9ba07c41a76aaedec77806c9d40fe6e Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Mon, 13 Sep 2021 20:55:46 -0400 Subject: [PATCH 1/6] Added ENV VAR Protection for RBAC, RBAC Test is failing because not being registered --- internal/xds/env/env.go | 4 ++ xds/internal/httpfilter/rbac/rbac.go | 5 +- xds/internal/server/listener_wrapper.go | 58 ++++++++++--------- xds/internal/server/listener_wrapper_test.go | 6 ++ .../test/xds_server_integration_test.go | 11 ++++ xds/internal/xdsclient/filter_chain.go | 55 +++++++++--------- xds/internal/xdsclient/filter_chain_test.go | 36 ++++++++++++ xds/internal/xdsclient/lds_test.go | 6 ++ xds/server.go | 13 +++-- 9 files changed, 136 insertions(+), 58 deletions(-) diff --git a/internal/xds/env/env.go b/internal/xds/env/env.go index 7f879b44da0..de76e8c9def 100644 --- a/internal/xds/env/env.go +++ b/internal/xds/env/env.go @@ -43,6 +43,7 @@ const ( clientSideSecuritySupportEnv = "GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT" aggregateAndDNSSupportEnv = "GRPC_XDS_EXPERIMENTAL_ENABLE_AGGREGATE_AND_LOGICAL_DNS_CLUSTER" retrySupportEnv = "GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY" + rbacSupportEnv = "GRPC_XDS_EXPERIMENTAL_ENABLE_RBAC" c2pResolverSupportEnv = "GRPC_EXPERIMENTAL_GOOGLE_C2P_RESOLVER" c2pResolverTestOnlyTrafficDirectorURIEnv = "GRPC_TEST_ONLY_GOOGLE_C2P_RESOLVER_TRAFFIC_DIRECTOR_URI" @@ -82,6 +83,9 @@ var ( // RetrySupport indicates whether xDS retry is enabled. RetrySupport = strings.EqualFold(os.Getenv(retrySupportEnv), "true") + // RBACSupport indicates whether xDS configured RBAC HTTP Filter is enabled. + RBACSupport = strings.EqualFold(os.Getenv(rbacSupportEnv), "true") + // C2PResolverSupport indicates whether support for C2P resolver is enabled. // This can be enabled by setting the environment variable // "GRPC_EXPERIMENTAL_GOOGLE_C2P_RESOLVER" to "true". diff --git a/xds/internal/httpfilter/rbac/rbac.go b/xds/internal/httpfilter/rbac/rbac.go index 969111b619a..fec60b2d3cd 100644 --- a/xds/internal/httpfilter/rbac/rbac.go +++ b/xds/internal/httpfilter/rbac/rbac.go @@ -28,6 +28,7 @@ import ( "github.com/golang/protobuf/proto" "github.com/golang/protobuf/ptypes" "google.golang.org/grpc/internal/resolver" + "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/internal/xds/rbac" "google.golang.org/grpc/xds/internal/httpfilter" "google.golang.org/protobuf/types/known/anypb" @@ -37,7 +38,9 @@ import ( ) func init() { - httpfilter.Register(builder{}) + if env.RBACSupport { + httpfilter.Register(builder{}) + } } type builder struct { diff --git a/xds/internal/server/listener_wrapper.go b/xds/internal/server/listener_wrapper.go index 0d1173324bb..b919a244579 100644 --- a/xds/internal/server/listener_wrapper.go +++ b/xds/internal/server/listener_wrapper.go @@ -35,6 +35,7 @@ import ( internalbackoff "google.golang.org/grpc/internal/backoff" internalgrpclog "google.golang.org/grpc/internal/grpclog" "google.golang.org/grpc/internal/grpcsync" + "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/xds/internal/xdsclient" "google.golang.org/grpc/xds/internal/xdsclient/bootstrap" ) @@ -272,34 +273,37 @@ func (l *listenerWrapper) Accept() (net.Conn, error) { conn.Close() continue } - var rc xdsclient.RouteConfigUpdate - if fc.InlineRouteConfig != nil { - rc = *fc.InlineRouteConfig - } else { - rcPtr := atomic.LoadPointer(&l.rdsUpdates) - rcuPtr := (*map[string]xdsclient.RouteConfigUpdate)(rcPtr) - // This shouldn't happen, but this error protects against a panic. - if rcuPtr == nil { - return nil, errors.New("route configuration pointer is nil") + var vhswi []xdsclient.VirtualHostWithInterceptors + if env.RBACSupport { + var rc xdsclient.RouteConfigUpdate + if fc.InlineRouteConfig != nil { + rc = *fc.InlineRouteConfig + } else { + rcPtr := atomic.LoadPointer(&l.rdsUpdates) + rcuPtr := (*map[string]xdsclient.RouteConfigUpdate)(rcPtr) + // This shouldn't happen, but this error protects against a panic. + if rcuPtr == nil { + return nil, errors.New("route configuration pointer is nil") + } + rcu := *rcuPtr + rc = rcu[fc.RouteConfigName] + } + // The filter chain will construct a usuable route table on each + // connection accept. This is done because preinstantiating every route + // table before it is needed for a connection would potentially lead to + // a lot of cpu time and memory allocated for route tables that will + // never be used. There was also a thought to cache this configuration, + // and reuse it for the next accepted connection. However, this would + // lead to a lot of code complexity (RDS Updates for a given route name + // can come it at any time), and connections aren't accepted too often, + // so this reinstantation of the Route Configuration is an acceptable + // tradeoff for simplicity. + vhswi, err = fc.ConstructUsableRouteConfiguration(rc) + if err != nil { + l.logger.Warningf("route configuration construction: %v", err) + conn.Close() + continue } - rcu := *rcuPtr - rc = rcu[fc.RouteConfigName] - } - // The filter chain will construct a usuable route table on each - // connection accept. This is done because preinstantiating every route - // table before it is needed for a connection would potentially lead to - // a lot of cpu time and memory allocated for route tables that will - // never be used. There was also a thought to cache this configuration, - // and reuse it for the next accepted connection. However, this would - // lead to a lot of code complexity (RDS Updates for a given route name - // can come it at any time), and connections aren't accepted too often, - // so this reinstantation of the Route Configuration is an acceptable - // tradeoff for simplicity. - vhswi, err := fc.ConstructUsableRouteConfiguration(rc) - if err != nil { - l.logger.Warningf("route configuration construction: %v", err) - conn.Close() - continue } return &connWrapper{Conn: conn, filterChain: fc, parent: l, virtualHosts: vhswi}, nil } diff --git a/xds/internal/server/listener_wrapper_test.go b/xds/internal/server/listener_wrapper_test.go index 010b2044d40..38372936366 100644 --- a/xds/internal/server/listener_wrapper_test.go +++ b/xds/internal/server/listener_wrapper_test.go @@ -34,6 +34,7 @@ import ( wrapperspb "github.com/golang/protobuf/ptypes/wrappers" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/internal/testutils" + "google.golang.org/grpc/internal/xds/env" _ "google.golang.org/grpc/xds/internal/httpfilter/router" "google.golang.org/grpc/xds/internal/testutils/e2e" "google.golang.org/grpc/xds/internal/testutils/fakeclient" @@ -325,6 +326,11 @@ func (s) TestNewListenerWrapper(t *testing.T) { // the update from the rds handler should it move the server to // ServingModeServing. func (s) TestNewListenerWrapperWithRouteUpdate(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() _, readyCh, xdsC, _, cleanup := newListenerWrapper(t) defer cleanup() diff --git a/xds/internal/test/xds_server_integration_test.go b/xds/internal/test/xds_server_integration_test.go index 89122e0647c..08df96a6b64 100644 --- a/xds/internal/test/xds_server_integration_test.go +++ b/xds/internal/test/xds_server_integration_test.go @@ -34,6 +34,7 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/internal/testutils" + "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/status" "google.golang.org/grpc/xds" _ "google.golang.org/grpc/xds/internal/httpfilter/rbac" @@ -374,6 +375,11 @@ func (s) TestServerSideXDS_SecurityConfigChange(t *testing.T) { // (NonForwardingAction), and the RPC's matching those routes should proceed as // normal. func (s) TestServerSideXDS_RouteConfiguration(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() managementServer, nodeID, bootstrapContents, resolver, cleanup1 := setupManagementServer(t) defer cleanup1() @@ -711,6 +717,11 @@ func serverListenerWithRBACHTTPFilters(host string, port uint32, rbacCfg *rpb.RB // as normal and certain RPC's are denied by the RBAC HTTP Filter which gets // called by hooked xds interceptors. func (s) TestRBACHTTPFilter(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() tests := []struct { name string rbacCfg *rpb.RBAC diff --git a/xds/internal/xdsclient/filter_chain.go b/xds/internal/xdsclient/filter_chain.go index 87aea9e46e1..d27e483a26c 100644 --- a/xds/internal/xdsclient/filter_chain.go +++ b/xds/internal/xdsclient/filter_chain.go @@ -29,6 +29,7 @@ import ( "github.com/golang/protobuf/proto" "github.com/golang/protobuf/ptypes" "google.golang.org/grpc/internal/resolver" + "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/xds/internal/httpfilter" "google.golang.org/grpc/xds/internal/version" ) @@ -598,33 +599,35 @@ func processNetworkFilters(filters []*v3listenerpb.Filter) (*FilterChain, error) // TODO: Implement terminal filter logic, as per A36. filterChain.HTTPFilters = filters seenHCM = true - switch hcm.RouteSpecifier.(type) { - case *v3httppb.HttpConnectionManager_Rds: - if hcm.GetRds().GetConfigSource().GetAds() == nil { - return nil, fmt.Errorf("ConfigSource is not ADS: %+v", hcm) + if env.RBACSupport { + switch hcm.RouteSpecifier.(type) { // Guard this routing parsing...what does this trigger in filter chain and more downstream (no, nil is completly valid all the way...)? Also guard routeAndProcess, this will prevent the logic of a nil route configuration) + case *v3httppb.HttpConnectionManager_Rds: + if hcm.GetRds().GetConfigSource().GetAds() == nil { + return nil, fmt.Errorf("ConfigSource is not ADS: %+v", hcm) + } + name := hcm.GetRds().GetRouteConfigName() + if name == "" { + return nil, fmt.Errorf("empty route_config_name: %+v", hcm) + } + filterChain.RouteConfigName = name + case *v3httppb.HttpConnectionManager_RouteConfig: + // "RouteConfiguration validation logic inherits all + // previous validations made for client-side usage as RDS + // does not distinguish between client-side and + // server-side." - A36 + // Can specify v3 here, as will never get to this function + // if v2. + routeU, err := generateRDSUpdateFromRouteConfiguration(hcm.GetRouteConfig(), nil, false) + if err != nil { + return nil, fmt.Errorf("failed to parse inline RDS resp: %v", err) + } + filterChain.InlineRouteConfig = &routeU + case nil: + // No-op, as no route specifier is a valid configuration on + // the server side. + default: + return nil, fmt.Errorf("unsupported type %T for RouteSpecifier", hcm.RouteSpecifier) } - name := hcm.GetRds().GetRouteConfigName() - if name == "" { - return nil, fmt.Errorf("empty route_config_name: %+v", hcm) - } - filterChain.RouteConfigName = name - case *v3httppb.HttpConnectionManager_RouteConfig: - // "RouteConfiguration validation logic inherits all - // previous validations made for client-side usage as RDS - // does not distinguish between client-side and - // server-side." - A36 - // Can specify v3 here, as will never get to this function - // if v2. - routeU, err := generateRDSUpdateFromRouteConfiguration(hcm.GetRouteConfig(), nil, false) - if err != nil { - return nil, fmt.Errorf("failed to parse inline RDS resp: %v", err) - } - filterChain.InlineRouteConfig = &routeU - case nil: - // No-op, as no route specifier is a valid configuration on - // the server side. - default: - return nil, fmt.Errorf("unsupported type %T for RouteSpecifier", hcm.RouteSpecifier) } } default: diff --git a/xds/internal/xdsclient/filter_chain_test.go b/xds/internal/xdsclient/filter_chain_test.go index 5219e34cb10..2cc73b0a511 100644 --- a/xds/internal/xdsclient/filter_chain_test.go +++ b/xds/internal/xdsclient/filter_chain_test.go @@ -40,6 +40,7 @@ import ( iresolver "google.golang.org/grpc/internal/resolver" "google.golang.org/grpc/internal/testutils" + "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/xds/internal/httpfilter" "google.golang.org/grpc/xds/internal/httpfilter/router" "google.golang.org/grpc/xds/internal/testutils/e2e" @@ -519,6 +520,11 @@ func TestNewFilterChainImpl_Failure_BadSecurityConfig(t *testing.T) { // TestNewFilterChainImpl_Success_RouteUpdate tests the construction of the // filter chain with valid HTTP Filters present. func TestNewFilterChainImpl_Success_RouteUpdate(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() tests := []struct { name string lis *v3listenerpb.Listener @@ -754,6 +760,11 @@ func TestNewFilterChainImpl_Success_RouteUpdate(t *testing.T) { // TestNewFilterChainImpl_Failure_BadRouteUpdate verifies cases where the Route // Update in the filter chain are invalid. func TestNewFilterChainImpl_Failure_BadRouteUpdate(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() tests := []struct { name string lis *v3listenerpb.Listener @@ -927,6 +938,11 @@ func TestNewFilterChainImpl_Failure_BadHTTPFilters(t *testing.T) { // TestNewFilterChainImpl_Success_HTTPFilters tests the construction of the // filter chain with valid HTTP Filters present. func TestNewFilterChainImpl_Success_HTTPFilters(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() tests := []struct { name string lis *v3listenerpb.Listener @@ -1245,6 +1261,11 @@ func TestNewFilterChainImpl_Success_HTTPFilters(t *testing.T) { // TestNewFilterChainImpl_Success_SecurityConfig verifies cases where the // security configuration in the filter chain contains valid data. func TestNewFilterChainImpl_Success_SecurityConfig(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() tests := []struct { desc string lis *v3listenerpb.Listener @@ -1472,6 +1493,11 @@ func TestNewFilterChainImpl_Success_SecurityConfig(t *testing.T) { // success at config validation time and the filter chains which contains // unsupported match fields will be skipped at lookup time. func TestNewFilterChainImpl_Success_UnsupportedMatchFields(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() unspecifiedEntry := &destPrefixEntry{ srcTypeArr: [3]*sourcePrefixes{ { @@ -1637,6 +1663,11 @@ func TestNewFilterChainImpl_Success_UnsupportedMatchFields(t *testing.T) { // TestNewFilterChainImpl_Success_AllCombinations verifies different // combinations of the supported match criteria. func TestNewFilterChainImpl_Success_AllCombinations(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() tests := []struct { desc string lis *v3listenerpb.Listener @@ -2283,6 +2314,11 @@ func TestLookup_Failures(t *testing.T) { } func TestLookup_Successes(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() lisWithDefaultChain := &v3listenerpb.Listener{ FilterChains: []*v3listenerpb.FilterChain{ { diff --git a/xds/internal/xdsclient/lds_test.go b/xds/internal/xdsclient/lds_test.go index d43cbaa8087..9045c0b9087 100644 --- a/xds/internal/xdsclient/lds_test.go +++ b/xds/internal/xdsclient/lds_test.go @@ -33,6 +33,7 @@ import ( "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/grpc/internal/testutils" + "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/xds/internal/httpfilter" _ "google.golang.org/grpc/xds/internal/httpfilter/router" "google.golang.org/grpc/xds/internal/testutils/e2e" @@ -601,6 +602,11 @@ func (s) TestUnmarshalListener_ClientSide(t *testing.T) { } func (s) TestUnmarshalListener_ServerSide(t *testing.T) { + oldRBAC := env.RBACSupport + env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() const ( v3LDSTarget = "grpc/server?xds.resource.listening_address=0.0.0.0:9999" testVersion = "test-version-lds-server" diff --git a/xds/server.go b/xds/server.go index 33a49095799..b36fa64b500 100644 --- a/xds/server.go +++ b/xds/server.go @@ -37,6 +37,7 @@ import ( "google.golang.org/grpc/internal/grpcsync" iresolver "google.golang.org/grpc/internal/resolver" "google.golang.org/grpc/internal/transport" + "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" "google.golang.org/grpc/xds/internal/server" @@ -381,8 +382,10 @@ func routeAndProcess(ctx context.Context) error { // xdsUnaryInterceptor is the unary interceptor added to the gRPC server to // perform any xDS specific functionality on unary RPCs. func xdsUnaryInterceptor(ctx context.Context, req interface{}, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) { - if err := routeAndProcess(ctx); err != nil { - return nil, err + if env.RBACSupport { + if err := routeAndProcess(ctx); err != nil { + return nil, err + } } return handler(ctx, req) } @@ -390,8 +393,10 @@ func xdsUnaryInterceptor(ctx context.Context, req interface{}, _ *grpc.UnaryServ // xdsStreamInterceptor is the stream interceptor added to the gRPC server to // perform any xDS specific functionality on streaming RPCs. func xdsStreamInterceptor(srv interface{}, ss grpc.ServerStream, _ *grpc.StreamServerInfo, handler grpc.StreamHandler) error { - if err := routeAndProcess(ss.Context()); err != nil { - return err + if env.RBACSupport { + if err := routeAndProcess(ss.Context()); err != nil { + return err + } } return handler(srv, ss) } From d8f85d8c7589231754ec05687afcb6f9d16cb623 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 14 Sep 2021 01:44:47 -0400 Subject: [PATCH 2/6] Fix for test --- xds/internal/httpfilter/httpfilter.go | 8 ++++++++ xds/internal/httpfilter/rbac/rbac.go | 5 +---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/xds/internal/httpfilter/httpfilter.go b/xds/internal/httpfilter/httpfilter.go index 3e10e4f3486..a9b49fc4775 100644 --- a/xds/internal/httpfilter/httpfilter.go +++ b/xds/internal/httpfilter/httpfilter.go @@ -23,6 +23,7 @@ package httpfilter import ( "github.com/golang/protobuf/proto" iresolver "google.golang.org/grpc/internal/resolver" + "google.golang.org/grpc/internal/xds/env" ) // FilterConfig represents an opaque data structure holding configuration for a @@ -98,5 +99,12 @@ func Register(b Filter) { // // If no filter is register with typeURL, nil will be returned. func Get(typeURL string) Filter { + // Hardcoded here vs. rbac http filter package because rbac HTTP Filter test + // is in integration test. + if !env.RBACSupport { + if typeURL == "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBAC" || typeURL == "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBACPerRoute" { + return nil + } + } return m[typeURL] } diff --git a/xds/internal/httpfilter/rbac/rbac.go b/xds/internal/httpfilter/rbac/rbac.go index fec60b2d3cd..969111b619a 100644 --- a/xds/internal/httpfilter/rbac/rbac.go +++ b/xds/internal/httpfilter/rbac/rbac.go @@ -28,7 +28,6 @@ import ( "github.com/golang/protobuf/proto" "github.com/golang/protobuf/ptypes" "google.golang.org/grpc/internal/resolver" - "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/internal/xds/rbac" "google.golang.org/grpc/xds/internal/httpfilter" "google.golang.org/protobuf/types/known/anypb" @@ -38,9 +37,7 @@ import ( ) func init() { - if env.RBACSupport { - httpfilter.Register(builder{}) - } + httpfilter.Register(builder{}) } type builder struct { From 1df3a91068d4a820fb675c8503688434c0990afc Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 15 Sep 2021 18:28:45 -0400 Subject: [PATCH 3/6] Added two toggling tests for RBAC Env var on to off and off to on --- .../test/xds_server_integration_test.go | 209 ++++++++++++++++++ xds/internal/xdsclient/filter_chain.go | 2 +- 2 files changed, 210 insertions(+), 1 deletion(-) diff --git a/xds/internal/test/xds_server_integration_test.go b/xds/internal/test/xds_server_integration_test.go index 08df96a6b64..8690900f7d6 100644 --- a/xds/internal/test/xds_server_integration_test.go +++ b/xds/internal/test/xds_server_integration_test.go @@ -834,7 +834,216 @@ func (s) TestRBACHTTPFilter(t *testing.T) { if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != test.wantStatusUnaryCall { t.Fatalf("UnaryCall() returned err with status: %v, wantStatusUnaryCall: %v", err, test.wantStatusUnaryCall) } + + // Toggle the RBAC Env variable off, this should disable RBAC and allow any RPC"s through (will not go through + // routing or processed by HTTP Filters and thus will never get denied by RBAC). + env.RBACSupport = false + if _, err := client.EmptyCall(ctx, &testpb.Empty{}); status.Code(err) != codes.OK { + t.Fatalf("EmptyCall() returned err with status: %v, once RBAC is disabled all RPC's should proceed as normal", status.Code(err)) + } + if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != codes.OK { + t.Fatalf("UnaryCall() returned err with status: %v, once RBAC is disabled all RPC's should proceed as normal", status.Code(err)) + } + // Toggle RBAC back on for next iterations. + env.RBACSupport = true }() }) } } + +// serverListenerWithBadRouteConfiguration returns an xds Listener resource with +// a Route Configuration that will never successfully match in order to test +// RBAC Environment variable being toggled on and off. +func serverListenerWithBadRouteConfiguration(host string, port uint32) *v3listenerpb.Listener { + return &v3listenerpb.Listener{ + Name: fmt.Sprintf(e2e.ServerListenerResourceNameTemplate, net.JoinHostPort(host, strconv.Itoa(int(port)))), + Address: &v3corepb.Address{ + Address: &v3corepb.Address_SocketAddress{ + SocketAddress: &v3corepb.SocketAddress{ + Address: host, + PortSpecifier: &v3corepb.SocketAddress_PortValue{ + PortValue: port, + }, + }, + }, + }, + FilterChains: []*v3listenerpb.FilterChain{ + { + Name: "v4-wildcard", + FilterChainMatch: &v3listenerpb.FilterChainMatch{ + PrefixRanges: []*v3corepb.CidrRange{ + { + AddressPrefix: "0.0.0.0", + PrefixLen: &wrapperspb.UInt32Value{ + Value: uint32(0), + }, + }, + }, + SourceType: v3listenerpb.FilterChainMatch_SAME_IP_OR_LOOPBACK, + SourcePrefixRanges: []*v3corepb.CidrRange{ + { + AddressPrefix: "0.0.0.0", + PrefixLen: &wrapperspb.UInt32Value{ + Value: uint32(0), + }, + }, + }, + }, + Filters: []*v3listenerpb.Filter{ + { + Name: "filter-1", + ConfigType: &v3listenerpb.Filter_TypedConfig{ + TypedConfig: testutils.MarshalAny(&v3httppb.HttpConnectionManager{ + RouteSpecifier: &v3httppb.HttpConnectionManager_RouteConfig{ + RouteConfig: &v3routepb.RouteConfiguration{ + Name: "routeName", + VirtualHosts: []*v3routepb.VirtualHost{{ + // Incoming RPC's will try and match to Virtual Hosts based on their :authority header. + // Thus, incoming RPC's will never match to a Virtual Host (server side requires matching + // to a VH/Route of type Non Forwarding Action to proceed normally), and all incoming RPC's + // with this route configuration will be denied. + Domains: []string{"will-never-match"}, + Routes: []*v3routepb.Route{{ + Match: &v3routepb.RouteMatch{ + PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/"}, + }, + Action: &v3routepb.Route_NonForwardingAction{}, + }}}}}, + }, + HttpFilters: []*v3httppb.HttpFilter{e2e.RouterHTTPFilter}, + }), + }, + }, + }, + }, + { + Name: "v6-wildcard", + FilterChainMatch: &v3listenerpb.FilterChainMatch{ + PrefixRanges: []*v3corepb.CidrRange{ + { + AddressPrefix: "::", + PrefixLen: &wrapperspb.UInt32Value{ + Value: uint32(0), + }, + }, + }, + SourceType: v3listenerpb.FilterChainMatch_SAME_IP_OR_LOOPBACK, + SourcePrefixRanges: []*v3corepb.CidrRange{ + { + AddressPrefix: "::", + PrefixLen: &wrapperspb.UInt32Value{ + Value: uint32(0), + }, + }, + }, + }, + Filters: []*v3listenerpb.Filter{ + { + Name: "filter-1", + ConfigType: &v3listenerpb.Filter_TypedConfig{ + TypedConfig: testutils.MarshalAny(&v3httppb.HttpConnectionManager{ + RouteSpecifier: &v3httppb.HttpConnectionManager_RouteConfig{ + RouteConfig: &v3routepb.RouteConfiguration{ + Name: "routeName", + VirtualHosts: []*v3routepb.VirtualHost{{ + // Incoming RPC's will try and match to Virtual Hosts based on their :authority header. + // Thus, incoming RPC's will never match to a Virtual Host (server side requires matching + // to a VH/Route of type Non Forwarding Action to proceed normally), and all incoming RPC's + // with this route configuration will be denied. + Domains: []string{"will-never-match"}, + Routes: []*v3routepb.Route{{ + Match: &v3routepb.RouteMatch{ + PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: "/"}, + }, + Action: &v3routepb.Route_NonForwardingAction{}, + }}}}}, + }, + HttpFilters: []*v3httppb.HttpFilter{e2e.RouterHTTPFilter}, + }), + }, + }, + }, + }, + }, + } +} + +// TestRBACToggledOffThenToggledOnWithBadRouteConfiguration tests a scenario +// where the server gets a listener configuration with a route table that is +// garbage, with incoming RPC's never matching to a VH/Route of type Non +// Forwarding Action, thus never proceeding as normal. In the default scenario +// (RBAC Env Var turned off, thus all logic related to Route Configuration +// protected), the RPC's should simply proceed as normal due to ignoring the +// route configuration. Once toggling the route configuration on, the RPC's +// should all fail after updating the Server. +func (s) TestRBACToggledOffThenToggledOnWithBadRouteConfiguration(t *testing.T) { + managementServer, nodeID, bootstrapContents, resolver, cleanup1 := setupManagementServer(t) + defer cleanup1() + + lis, cleanup2 := setupGRPCServer(t, bootstrapContents) + defer cleanup2() + + host, port, err := hostPortFromListener(lis) + if err != nil { + t.Fatalf("failed to retrieve host and port of server: %v", err) + } + const serviceName = "my-service-fallback" + + // The inbound listener needs a route table that will never match on a VH, + // and thus shouldn't allow incoming RPC's to proceed. + resources := e2e.DefaultClientResources(e2e.ResourceParams{ + DialTarget: serviceName, + NodeID: nodeID, + Host: host, + Port: port, + SecLevel: e2e.SecurityLevelNone, + }) + // This bad route configuration shouldn't affect incoming RPC's from + // proceeding as normal, as the configuration shouldn't be parsed due to the + // RBAC Environment variable not being set to true. + inboundLis := serverListenerWithBadRouteConfiguration(host, port) + resources.Listeners = append(resources.Listeners, inboundLis) + + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + // Setup the management server with client and server-side resources. + if err := managementServer.Update(ctx, resources); err != nil { + t.Fatal(err) + } + + cc, err := grpc.DialContext(ctx, fmt.Sprintf("xds:///%s", serviceName), grpc.WithInsecure(), grpc.WithResolvers(resolver)) + if err != nil { + t.Fatalf("failed to dial local test server: %v", err) + } + defer cc.Close() + + client := testpb.NewTestServiceClient(cc) + + // The default setting of RBAC being disabled should allow any RPC's to + // proceed as normal. + if _, err := client.EmptyCall(ctx, &testpb.Empty{}); status.Code(err) != codes.OK { + t.Fatalf("EmptyCall() returned err with status: %v, if RBAC is disabled all RPC's should proceed as normal", status.Code(err)) + } + if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != codes.OK { + t.Fatalf("UnaryCall() returned err with status: %v, if RBAC is disabled all RPC's should proceed as normal", status.Code(err)) + } + + // After toggling RBAC support on, all the RPC's should get denied with + // status code Unavailable due to not matching to a route of type Non + // Forwarding Action (Route Table not configured properly). + env.RBACSupport = true + // Update the server with the same configuration, this is blocking on server + // side so no raciness here. + if err := managementServer.Update(ctx, resources); err != nil { + t.Fatal(err) + } + + if _, err := client.EmptyCall(ctx, &testpb.Empty{}); status.Code(err) != codes.Unavailable { + t.Fatalf("EmptyCall() returned err with status: %v, if RBAC is disabled all RPC's should proceed as normal", status.Code(err)) + } + if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != codes.Unavailable { + t.Fatalf("UnaryCall() returned err with status: %v, if RBAC is disabled all RPC's should proceed as normal", status.Code(err)) + } + // Toggle RBACSupport off for next iteration. + env.RBACSupport = false +} diff --git a/xds/internal/xdsclient/filter_chain.go b/xds/internal/xdsclient/filter_chain.go index d27e483a26c..1a7a7f3b222 100644 --- a/xds/internal/xdsclient/filter_chain.go +++ b/xds/internal/xdsclient/filter_chain.go @@ -600,7 +600,7 @@ func processNetworkFilters(filters []*v3listenerpb.Filter) (*FilterChain, error) filterChain.HTTPFilters = filters seenHCM = true if env.RBACSupport { - switch hcm.RouteSpecifier.(type) { // Guard this routing parsing...what does this trigger in filter chain and more downstream (no, nil is completly valid all the way...)? Also guard routeAndProcess, this will prevent the logic of a nil route configuration) + switch hcm.RouteSpecifier.(type) { case *v3httppb.HttpConnectionManager_Rds: if hcm.GetRds().GetConfigSource().GetAds() == nil { return nil, fmt.Errorf("ConfigSource is not ADS: %+v", hcm) From fa9383dbe3fce6f0b4d595acad55802f436d6c15 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Thu, 16 Sep 2021 11:19:09 -0400 Subject: [PATCH 4/6] Responded to Doug's comment, added (Un)RegisterForTesting() --- xds/internal/httpfilter/httpfilter.go | 13 +++++-------- xds/internal/httpfilter/rbac/rbac.go | 19 +++++++++++++++++++ .../test/xds_server_integration_test.go | 4 +++- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/xds/internal/httpfilter/httpfilter.go b/xds/internal/httpfilter/httpfilter.go index a9b49fc4775..b4399f9faeb 100644 --- a/xds/internal/httpfilter/httpfilter.go +++ b/xds/internal/httpfilter/httpfilter.go @@ -23,7 +23,6 @@ package httpfilter import ( "github.com/golang/protobuf/proto" iresolver "google.golang.org/grpc/internal/resolver" - "google.golang.org/grpc/internal/xds/env" ) // FilterConfig represents an opaque data structure holding configuration for a @@ -95,16 +94,14 @@ func Register(b Filter) { } } +// UnregisterForTesting unregisters the HTTP Filter for testing purposes. +func UnregisterForTesting(typeURL string) { + delete(m, typeURL) +} + // Get returns the HTTPFilter registered with typeURL. // // If no filter is register with typeURL, nil will be returned. func Get(typeURL string) Filter { - // Hardcoded here vs. rbac http filter package because rbac HTTP Filter test - // is in integration test. - if !env.RBACSupport { - if typeURL == "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBAC" || typeURL == "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBACPerRoute" { - return nil - } - } return m[typeURL] } diff --git a/xds/internal/httpfilter/rbac/rbac.go b/xds/internal/httpfilter/rbac/rbac.go index 969111b619a..98bcd73ffa4 100644 --- a/xds/internal/httpfilter/rbac/rbac.go +++ b/xds/internal/httpfilter/rbac/rbac.go @@ -28,6 +28,7 @@ import ( "github.com/golang/protobuf/proto" "github.com/golang/protobuf/ptypes" "google.golang.org/grpc/internal/resolver" + "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/internal/xds/rbac" "google.golang.org/grpc/xds/internal/httpfilter" "google.golang.org/protobuf/types/known/anypb" @@ -37,9 +38,27 @@ import ( ) func init() { + if env.RBACSupport { + httpfilter.Register(builder{}) + } +} + +// RegisterForTesting registers the RBAC HTTP Filter for testing purposes, regardless +// of the RBAC environment variable. This is needed because there is no way to set the RBAC +// environment variable to true in a test before init() in this package is run. +func RegisterForTesting() { httpfilter.Register(builder{}) } +// UnregisterForTesting unregisters the RBAC HTTP Filter for testing purposes. This is needed because +// there is no way to unregister the HTTP Filter after registering it solely for testing purposes using +// rbac.RegisterForTesting() +func UnregisterForTesting() { + for _, typeURL := range builder.TypeURLs(builder{}) { + httpfilter.UnregisterForTesting(typeURL) + } +} + type builder struct { } diff --git a/xds/internal/test/xds_server_integration_test.go b/xds/internal/test/xds_server_integration_test.go index 8690900f7d6..a87b36d6c8e 100644 --- a/xds/internal/test/xds_server_integration_test.go +++ b/xds/internal/test/xds_server_integration_test.go @@ -37,7 +37,7 @@ import ( "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/status" "google.golang.org/grpc/xds" - _ "google.golang.org/grpc/xds/internal/httpfilter/rbac" + "google.golang.org/grpc/xds/internal/httpfilter/rbac" "google.golang.org/grpc/xds/internal/testutils/e2e" v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" @@ -722,6 +722,8 @@ func (s) TestRBACHTTPFilter(t *testing.T) { defer func() { env.RBACSupport = oldRBAC }() + rbac.RegisterForTesting() + defer rbac.UnregisterForTesting() tests := []struct { name string rbacCfg *rpb.RBAC From 9bc0d508c5422f0c018974af8b0445f0f2f688fe Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Thu, 16 Sep 2021 18:05:54 -0400 Subject: [PATCH 5/6] Responded to Doug's comment and protected draining --- xds/internal/server/listener_wrapper.go | 6 ++++-- xds/internal/test/xds_server_integration_test.go | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/xds/internal/server/listener_wrapper.go b/xds/internal/server/listener_wrapper.go index b919a244579..d5d04dd138d 100644 --- a/xds/internal/server/listener_wrapper.go +++ b/xds/internal/server/listener_wrapper.go @@ -414,8 +414,10 @@ func (l *listenerWrapper) handleLDSUpdate(update ldsUpdateWithError) { // Server's state to ServingModeNotServing. That prevents new connections // from being accepted, whereas here we simply want the clients to reconnect // to get the updated configuration. - if l.drainCallback != nil { - l.drainCallback(l.Listener.Addr()) + if env.RBACSupport { + if l.drainCallback != nil { + l.drainCallback(l.Listener.Addr()) + } } l.rdsHandler.updateRouteNamesToWatch(ilc.FilterChains.RouteConfigNames) // If there are no dynamic RDS Configurations still needed to be received diff --git a/xds/internal/test/xds_server_integration_test.go b/xds/internal/test/xds_server_integration_test.go index a87b36d6c8e..8baa3fbe2a3 100644 --- a/xds/internal/test/xds_server_integration_test.go +++ b/xds/internal/test/xds_server_integration_test.go @@ -1033,7 +1033,11 @@ func (s) TestRBACToggledOffThenToggledOnWithBadRouteConfiguration(t *testing.T) // After toggling RBAC support on, all the RPC's should get denied with // status code Unavailable due to not matching to a route of type Non // Forwarding Action (Route Table not configured properly). + oldRBAC := env.RBACSupport env.RBACSupport = true + defer func() { + env.RBACSupport = oldRBAC + }() // Update the server with the same configuration, this is blocking on server // side so no raciness here. if err := managementServer.Update(ctx, resources); err != nil { @@ -1046,6 +1050,4 @@ func (s) TestRBACToggledOffThenToggledOnWithBadRouteConfiguration(t *testing.T) if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != codes.Unavailable { t.Fatalf("UnaryCall() returned err with status: %v, if RBAC is disabled all RPC's should proceed as normal", status.Code(err)) } - // Toggle RBACSupport off for next iteration. - env.RBACSupport = false } From 6334e7b1f9c21c5dbf4e686fd4e9da18ef3544e2 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Thu, 16 Sep 2021 23:10:47 -0400 Subject: [PATCH 6/6] Final nit --- xds/internal/server/listener_wrapper.go | 60 ++++++++++++------------- xds/internal/xdsclient/filter_chain.go | 57 +++++++++++------------ 2 files changed, 59 insertions(+), 58 deletions(-) diff --git a/xds/internal/server/listener_wrapper.go b/xds/internal/server/listener_wrapper.go index d5d04dd138d..99c9a753230 100644 --- a/xds/internal/server/listener_wrapper.go +++ b/xds/internal/server/listener_wrapper.go @@ -273,37 +273,37 @@ func (l *listenerWrapper) Accept() (net.Conn, error) { conn.Close() continue } - var vhswi []xdsclient.VirtualHostWithInterceptors - if env.RBACSupport { - var rc xdsclient.RouteConfigUpdate - if fc.InlineRouteConfig != nil { - rc = *fc.InlineRouteConfig - } else { - rcPtr := atomic.LoadPointer(&l.rdsUpdates) - rcuPtr := (*map[string]xdsclient.RouteConfigUpdate)(rcPtr) - // This shouldn't happen, but this error protects against a panic. - if rcuPtr == nil { - return nil, errors.New("route configuration pointer is nil") - } - rcu := *rcuPtr - rc = rcu[fc.RouteConfigName] - } - // The filter chain will construct a usuable route table on each - // connection accept. This is done because preinstantiating every route - // table before it is needed for a connection would potentially lead to - // a lot of cpu time and memory allocated for route tables that will - // never be used. There was also a thought to cache this configuration, - // and reuse it for the next accepted connection. However, this would - // lead to a lot of code complexity (RDS Updates for a given route name - // can come it at any time), and connections aren't accepted too often, - // so this reinstantation of the Route Configuration is an acceptable - // tradeoff for simplicity. - vhswi, err = fc.ConstructUsableRouteConfiguration(rc) - if err != nil { - l.logger.Warningf("route configuration construction: %v", err) - conn.Close() - continue + if !env.RBACSupport { + return &connWrapper{Conn: conn, filterChain: fc, parent: l}, nil + } + var rc xdsclient.RouteConfigUpdate + if fc.InlineRouteConfig != nil { + rc = *fc.InlineRouteConfig + } else { + rcPtr := atomic.LoadPointer(&l.rdsUpdates) + rcuPtr := (*map[string]xdsclient.RouteConfigUpdate)(rcPtr) + // This shouldn't happen, but this error protects against a panic. + if rcuPtr == nil { + return nil, errors.New("route configuration pointer is nil") } + rcu := *rcuPtr + rc = rcu[fc.RouteConfigName] + } + // The filter chain will construct a usuable route table on each + // connection accept. This is done because preinstantiating every route + // table before it is needed for a connection would potentially lead to + // a lot of cpu time and memory allocated for route tables that will + // never be used. There was also a thought to cache this configuration, + // and reuse it for the next accepted connection. However, this would + // lead to a lot of code complexity (RDS Updates for a given route name + // can come it at any time), and connections aren't accepted too often, + // so this reinstantation of the Route Configuration is an acceptable + // tradeoff for simplicity. + vhswi, err := fc.ConstructUsableRouteConfiguration(rc) + if err != nil { + l.logger.Warningf("route configuration construction: %v", err) + conn.Close() + continue } return &connWrapper{Conn: conn, filterChain: fc, parent: l, virtualHosts: vhswi}, nil } diff --git a/xds/internal/xdsclient/filter_chain.go b/xds/internal/xdsclient/filter_chain.go index 1a7a7f3b222..3b010ebdb92 100644 --- a/xds/internal/xdsclient/filter_chain.go +++ b/xds/internal/xdsclient/filter_chain.go @@ -599,35 +599,36 @@ func processNetworkFilters(filters []*v3listenerpb.Filter) (*FilterChain, error) // TODO: Implement terminal filter logic, as per A36. filterChain.HTTPFilters = filters seenHCM = true - if env.RBACSupport { - switch hcm.RouteSpecifier.(type) { - case *v3httppb.HttpConnectionManager_Rds: - if hcm.GetRds().GetConfigSource().GetAds() == nil { - return nil, fmt.Errorf("ConfigSource is not ADS: %+v", hcm) - } - name := hcm.GetRds().GetRouteConfigName() - if name == "" { - return nil, fmt.Errorf("empty route_config_name: %+v", hcm) - } - filterChain.RouteConfigName = name - case *v3httppb.HttpConnectionManager_RouteConfig: - // "RouteConfiguration validation logic inherits all - // previous validations made for client-side usage as RDS - // does not distinguish between client-side and - // server-side." - A36 - // Can specify v3 here, as will never get to this function - // if v2. - routeU, err := generateRDSUpdateFromRouteConfiguration(hcm.GetRouteConfig(), nil, false) - if err != nil { - return nil, fmt.Errorf("failed to parse inline RDS resp: %v", err) - } - filterChain.InlineRouteConfig = &routeU - case nil: - // No-op, as no route specifier is a valid configuration on - // the server side. - default: - return nil, fmt.Errorf("unsupported type %T for RouteSpecifier", hcm.RouteSpecifier) + if !env.RBACSupport { + continue + } + switch hcm.RouteSpecifier.(type) { + case *v3httppb.HttpConnectionManager_Rds: + if hcm.GetRds().GetConfigSource().GetAds() == nil { + return nil, fmt.Errorf("ConfigSource is not ADS: %+v", hcm) + } + name := hcm.GetRds().GetRouteConfigName() + if name == "" { + return nil, fmt.Errorf("empty route_config_name: %+v", hcm) + } + filterChain.RouteConfigName = name + case *v3httppb.HttpConnectionManager_RouteConfig: + // "RouteConfiguration validation logic inherits all + // previous validations made for client-side usage as RDS + // does not distinguish between client-side and + // server-side." - A36 + // Can specify v3 here, as will never get to this function + // if v2. + routeU, err := generateRDSUpdateFromRouteConfiguration(hcm.GetRouteConfig(), nil, false) + if err != nil { + return nil, fmt.Errorf("failed to parse inline RDS resp: %v", err) } + filterChain.InlineRouteConfig = &routeU + case nil: + // No-op, as no route specifier is a valid configuration on + // the server side. + default: + return nil, fmt.Errorf("unsupported type %T for RouteSpecifier", hcm.RouteSpecifier) } } default: