[AWSIC] Tightened filter validation and application (#67234)

Fixes a couple of issues discovered the Identity Center resource filtering code:

 1. The regex-based filters compile their regexes on every search, rather
    than once up front. This is not as bad as it sounds due to Teleport's
    regex cache, but it is still not ideal.
 2. The filter validation and execution steps behave differently, so that
    it is possible for an invalid filter to make in past validation and
    only be discovered while the plugin is running.
 3. The volume log spam generated by a poorly-formed filter that managed
    to make it past validation would *astronomical*.

This patch addresses those issues by

 1. Compiling the wire-format, protobuf filter objects from the plugin
    spec into a known-good filter at construction time, and using that
    as the runtime definition of the filter.
 2. Compiling the filter regexes during this construction-time setup
    rather than just hoping that they are cached by Teleport
 2. Validating the filter stack once during this compilation phase,
    rather than forcing the runtime matching code to be super very
    defensive. This also removes inconsistent behaviour between the
    validation at construction time vs. run time.

lib/aws/identitycenter/filters/filters.go

@@ -17,49 +17,140 @@
 package filters
 
 import (
-	"context"
-	"log/slog"
-
 	"github.com/gravitational/trace"
 
 	"github.com/gravitational/teleport/api/types"
 	"github.com/gravitational/teleport/lib/utils"
-	logutils "github.com/gravitational/teleport/lib/utils/log"
 )
 
-// New creates a new Filters instance from the supplied [types.AWSICResourceFilter]s.
-func New(filters []*types.AWSICResourceFilter) (Filters, error) {
-	out := Filters(filters)
-	if err := out.validate(); err != nil {
+// filterItem collects values that the filters operate on so that we can use the
+// same matchers for multiple types without having to complicate things with
+// generics everywhere.
+type filterItem struct {
+	hasID bool
+	id    string
+
+	hasName bool
+	name    string
+}
+
+// matcherFn defines the signature of a predicate over a [filterItem]
+type matcherFn func(*filterItem) bool
+
+// newIDMatcher creates a new matcherFn for exact matching of IDs
+func newIDMatcher(id string) matcherFn {
+	return func(item *filterItem) bool {
+		return item.hasID && item.id == id
+	}
+}
+
+// newRegexNameMatcher creates new matcherFn for pattern-based matching of
+// item names.
+func newRegexNameMatcher(nameRegex string) (matcherFn, error) {
+	re, err := utils.CompileExpression(nameRegex)
+	if err != nil {
 		return nil, trace.Wrap(err)
 	}
+	return func(item *filterItem) bool {
+		return item.hasName && re.MatchString(item.name)
+	}, nil
+}
+
+// Filters is a set of subFilters that recognize which Identity Center resources
+// to manage.
+type Filters struct {
+	include []matcherFn
+	exclude []matcherFn
+}
+
+// Validate asserts the supplied collection protobuf `AWSICResourceFilter` objects
+// represent a valid filter.
+func Validate(inputFilters []*types.AWSICResourceFilter) error {
+	_, err := New(inputFilters)
+	return trace.Wrap(err)
+}
+
+// New compiles a new [Filters] instance from the supplied collection of
+// [types.AWSICResourceFilter] objects.
+func New(inputFilters []*types.AWSICResourceFilter) (Filters, error) {
+	// The protobuf definition of a filter is a bit odd, and takes some
+	// explaining before the validation makes sense.
+	//
+	// The YAML definition of a filter stack looks like this:
+	//
+	// filters:
+	// - nameRegex: "^admin-.*$"
+	// - id: "123"
+	// - excludeId: "567"
+	//
+	// From this, the protobuf parser creates a stack of AWSICResourceFilter
+	// objects, one for each predicate. Each AWSICResourceFilter has two
+	// predicate slots: `include` and `exclude`. A well-formed AWSICResourceFilter
+	// has only one filled predicate slot, with inclusive predicates in `Include`
+	// and exclusive predicates in `Exclude`.
+	//
+	// In rare cases (most likely caused by an unknown predicate in the filter's
+	// config text) the protobuf parser will give us an empty
+	// `AWSICResourceFilter`. This is considered an error because the unknown
+	// predicate could represent a mistyped exclude predicate, or a future-valid
+	// exclusion rule in a cluster downgraded from a future version of Teleport;
+	// just ignoring the unknown filter silently grant more access than the
+	// user intends.
+
+	var out Filters
+
+	for index, filter := range inputFilters {
+		if filter.Include == nil && filter.Exclude == nil {
+			return Filters{}, trace.BadParameter("malformed filter predicate at index %d", index)
+		}
+
+		switch include := filter.Include.(type) {
+		case *types.AWSICResourceFilter_NameRegex:
+			matcher, err := newRegexNameMatcher(include.NameRegex)
+			if err != nil {
+				return Filters{}, trace.Wrap(err)
+			}
+			out.include = append(out.include, matcher)
+
+		case *types.AWSICResourceFilter_Id:
+			out.include = append(out.include, newIDMatcher(include.Id))
+
+		case nil:
+			// this must be an exclude predicate, but we're collecting includes
+			// carry on
+
+		default:
+			// If we get to here, someone has added a new inclusion predicate
+			// type to the protobuf definition and we have not updated to match.
+			return Filters{}, trace.BadParameter("unsupported include predicate type: %T", include)
+		}
+
+		switch exclude := filter.Exclude.(type) {
+		case *types.AWSICResourceFilter_ExcludeNameRegex:
+			matcher, err := newRegexNameMatcher(exclude.ExcludeNameRegex)
+			if err != nil {
+				return Filters{}, trace.Wrap(err)
+			}
+			out.exclude = append(out.exclude, matcher)
+
+		case *types.AWSICResourceFilter_ExcludeId:
+			out.exclude = append(out.exclude, newIDMatcher(exclude.ExcludeId))
+
+		case nil:
+			// this must be an include predicate, but we're collecting excludes.
+			// cary on.
+
+		default:
+			// If we get to here, someone has added a new exclusion predicate
+			// type to the protobuf definition and we have not updated to match.
+			return Filters{}, trace.BadParameter("unsupported exclude predicate type: %T", exclude)
+		}
+	}
+
 	return out, nil
 }
 
-// Filters is a collection of filters.
-type Filters []*types.AWSICResourceFilter
-
-// validate validates the filters.
-func (f Filters) validate() error {
-	for _, v := range f {
-		switch v.Include.(type) {
-		case *types.AWSICResourceFilter_NameRegex:
-			if _, err := utils.CompileExpression(v.GetNameRegex()); err != nil {
-				return trace.Wrap(err)
-			}
-		}
-		switch v.Exclude.(type) {
-		case *types.AWSICResourceFilter_ExcludeNameRegex:
-			if _, err := utils.CompileExpression(v.GetExcludeNameRegex()); err != nil {
-				return trace.Wrap(err)
-			}
-		}
-	}
-	return nil
-}
-
-// Params is a collection of filter parameters.
-// It contains the items to filter, and functions to get the name and ID of an item.
+// Params holds extractor functions for the given item  type
 type Params[T any] struct {
 	// Items is the items to filter.
 	Items []T
@@ -69,63 +160,55 @@ type Params[T any] struct {
 	GetID func(T) string
 }
 
-// Filter filters items based on the filters and parameters.
+// Filter applies the supplied filter stack to a collection of items,
+// returning the filtered set. Note that Exclude rules are applied first
+// regardless of the order the filters were defined in the original protobuf
+// filter objects.
 func Filter[T any](filters Filters, params Params[T]) []T {
-	if len(filters) == 0 {
+	if len(filters.include) == 0 && len(filters.exclude) == 0 {
 		return params.Items
 	}
-	var out []T
+
+	if len(params.Items) == 0 {
+		return params.Items
+	}
+
+	filterValues := filterItem{
+		hasID:   params.GetID != nil,
+		hasName: params.GetName != nil,
+	}
+
+	out := make([]T, 0, len(params.Items))
 	for _, item := range params.Items {
-		if matchesFilters(item, filters, params) {
+		if filterValues.hasID {
+			filterValues.id = params.GetID(item)
+		}
+		if filterValues.hasName {
+			filterValues.name = params.GetName(item)
+		}
+
+		if filters.includeItem(&filterValues) {
 			out = append(out, item)
 		}
 	}
 	return out
 }
 
-func matchesFilters[T any](item T, filters Filters, params Params[T]) bool {
-	hasInclude := false
-	for _, filter := range filters {
-		if filter.Exclude == nil {
-			continue
-		}
-		switch v := filter.Exclude.(type) {
-		case *types.AWSICResourceFilter_ExcludeId:
-			if params.GetID != nil && params.GetID(item) == v.ExcludeId {
-				return false
-			}
-		case *types.AWSICResourceFilter_ExcludeNameRegex:
-			if params.GetName != nil {
-				compiledExclude, err := utils.CompileExpression(v.ExcludeNameRegex)
-				if err == nil && compiledExclude.MatchString(params.GetName(item)) {
-					return false
-				}
-			}
-		default:
-			slog.ErrorContext(context.Background(), "AWSSyncFilter skipping unsupported exclude filter type", "type", logutils.TypeAttr(v))
+func (fs *Filters) includeItem(item *filterItem) bool {
+	for _, exclude := range fs.exclude {
+		if exclude(item) {
+			return false
 		}
 	}
 
-	for _, filter := range filters {
-		if filter.Include == nil {
-			continue
-		}
-		hasInclude = true
-		switch v := filter.Include.(type) {
-		case *types.AWSICResourceFilter_Id:
-			if params.GetID != nil && params.GetID(item) == v.Id {
-				return true
-			}
-		case *types.AWSICResourceFilter_NameRegex:
-			if params.GetName != nil {
-				compiledFilter, err := utils.CompileExpression(v.NameRegex)
-				if err == nil && compiledFilter.MatchString(params.GetName(item)) {
-					return true
-				}
-			}
-		default:
-			slog.ErrorContext(context.Background(), "AWSSyncFilter unsupported filter type encountered. Filter will be skipped.", "type", logutils.TypeAttr(v))
+	if len(fs.include) == 0 {
+		return true
+	}
+
+	for _, include := range fs.include {
+		if include(item) {
+			return true
 		}
 	}
-	return !hasInclude
+	return false
 }

lib/aws/identitycenter/filters/filters_test.go

@@ -40,13 +40,13 @@ func TestFilterItems(t *testing.T) {
 
 	tests := []struct {
 		name     string
-		filters  Filters
+		filters  []*types.AWSICResourceFilter
 		params   Params[TestItem]
 		expected []TestItem
 	}{
 		{
 			name:    "Filter by ID",
-			filters: Filters{&types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_Id{Id: "2"}}},
+			filters: []*types.AWSICResourceFilter{{Include: &types.AWSICResourceFilter_Id{Id: "2"}}},
 			params: Params[TestItem]{
 				Items: items,
 				GetName: func(item TestItem) string {
@@ -60,7 +60,7 @@ func TestFilterItems(t *testing.T) {
 		},
 		{
 			name:    "Filter by Name",
-			filters: Filters{&types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "a*"}}},
+			filters: []*types.AWSICResourceFilter{{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "a*"}}},
 			params: Params[TestItem]{
 				Items: items,
 				GetName: func(item TestItem) string {
@@ -72,7 +72,7 @@ func TestFilterItems(t *testing.T) {
 		},
 		{
 			name:    "Exclude All",
-			filters: Filters{&types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "teleport.internal/exclude_all"}}},
+			filters: []*types.AWSICResourceFilter{{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "teleport.internal/exclude_all"}}},
 			params: Params[TestItem]{
 				Items: items,
 				GetName: func(item TestItem) string {
@@ -86,7 +86,7 @@ func TestFilterItems(t *testing.T) {
 		},
 		{
 			name:    "No Filters",
-			filters: Filters{},
+			filters: []*types.AWSICResourceFilter{},
 			params: Params[TestItem]{
 				Items: items,
 				GetName: func(item TestItem) string {
@@ -100,10 +100,10 @@ func TestFilterItems(t *testing.T) {
 		},
 		{
 			name: "Multiple Filters",
-			filters: Filters{
-				&types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_Id{Id: "2"}},
-				&types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "a*"}},
-				&types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_Id{Id: "4"}},
+			filters: []*types.AWSICResourceFilter{
+				{Include: &types.AWSICResourceFilter_Id{Id: "2"}},
+				{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "a*"}},
+				{Include: &types.AWSICResourceFilter_Id{Id: "4"}},
 			},
 			params: Params[TestItem]{
 				Items: items,
@@ -122,8 +122,8 @@ func TestFilterItems(t *testing.T) {
 		},
 		{
 			name: "Exclude by ID",
-			filters: Filters{
-				&types.AWSICResourceFilter{Exclude: &types.AWSICResourceFilter_ExcludeId{ExcludeId: "2"}},
+			filters: []*types.AWSICResourceFilter{
+				{Exclude: &types.AWSICResourceFilter_ExcludeId{ExcludeId: "2"}},
 			},
 			params: Params[TestItem]{
 				Items: items,
@@ -142,8 +142,8 @@ func TestFilterItems(t *testing.T) {
 		},
 		{
 			name: "Exclude by NameRegex",
-			filters: Filters{
-				&types.AWSICResourceFilter{Exclude: &types.AWSICResourceFilter_ExcludeNameRegex{ExcludeNameRegex: "a*"}},
+			filters: []*types.AWSICResourceFilter{
+				{Exclude: &types.AWSICResourceFilter_ExcludeNameRegex{ExcludeNameRegex: "a*"}},
 			},
 			params: Params[TestItem]{
 				Items: items,
@@ -161,9 +161,9 @@ func TestFilterItems(t *testing.T) {
 		},
 		{
 			name: "Include and Exclude - exclude wins",
-			filters: Filters{
-				&types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "*"}},
-				&types.AWSICResourceFilter{Exclude: &types.AWSICResourceFilter_ExcludeId{ExcludeId: "1"}},
+			filters: []*types.AWSICResourceFilter{
+				{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "*"}},
+				{Exclude: &types.AWSICResourceFilter_ExcludeId{ExcludeId: "1"}},
 			},
 			params: Params[TestItem]{
 				Items: items,
@@ -182,9 +182,9 @@ func TestFilterItems(t *testing.T) {
 		},
 		{
 			name: "Include and Exclude - exclude wins case 2",
-			filters: Filters{
-				&types.AWSICResourceFilter{Exclude: &types.AWSICResourceFilter_ExcludeNameRegex{ExcludeNameRegex: "*cado"}},
-				&types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "a*"}},
+			filters: []*types.AWSICResourceFilter{
+				{Exclude: &types.AWSICResourceFilter_ExcludeNameRegex{ExcludeNameRegex: "*cado"}},
+				{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "a*"}},
 			},
 			params: Params[TestItem]{
 				Items: items,
@@ -203,8 +203,11 @@ func TestFilterItems(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := Filter(tt.filters, tt.params)
-			assert.Equal(t, tt.expected, result)
+			filterUnderTest, err := New(tt.filters)
+			require.NoError(t, err)
+
+			result := Filter(filterUnderTest, tt.params)
+			assert.ElementsMatch(t, tt.expected, result)
 		})
 	}
 }
@@ -212,21 +215,29 @@ func TestFilterItems(t *testing.T) {
 func TestPoorlyFormedFiltersAreAnError(t *testing.T) {
 	testCases := []struct {
 		name           string
-		filters        Filters
+		filters        []*types.AWSICResourceFilter
 		errorAssertion require.ErrorAssertionFunc
 	}{
 		{
-			name: "Bad regex",
-			filters: Filters{
-				&types.AWSICResourceFilter{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "^[)$"}},
+			name: "Bad include regex",
+			filters: []*types.AWSICResourceFilter{
+				{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "^[)$"}},
 			},
 			errorAssertion: require.Error,
 		},
-
 		{
 			name: "Bad exclude regex",
-			filters: Filters{
-				&types.AWSICResourceFilter{Exclude: &types.AWSICResourceFilter_ExcludeNameRegex{ExcludeNameRegex: "^[)$"}},
+			filters: []*types.AWSICResourceFilter{
+				{Exclude: &types.AWSICResourceFilter_ExcludeNameRegex{ExcludeNameRegex: "^[)$"}},
+			},
+			errorAssertion: require.Error,
+		},
+		{
+			name: "Empty filter element",
+			filters: []*types.AWSICResourceFilter{
+				{Exclude: &types.AWSICResourceFilter_ExcludeNameRegex{ExcludeNameRegex: "*cado"}},
+				{Include: nil, Exclude: nil},
+				{Include: &types.AWSICResourceFilter_NameRegex{NameRegex: "a*"}},
 			},
 			errorAssertion: require.Error,
 		},
@@ -234,7 +245,9 @@ func TestPoorlyFormedFiltersAreAnError(t *testing.T) {
 
 	for _, test := range testCases {
 		t.Run(test.name, func(t *testing.T) {
-			test.errorAssertion(t, test.filters.validate())
+			test.errorAssertion(t, Validate(test.filters))
+			_, err := New(test.filters)
+			test.errorAssertion(t, err)
 		})
 	}
 }

tool/tctl/common/plugin/awsic.go

@@ -130,7 +130,7 @@ func (a *awsICInstallArgs) validateSCIMBaseURL(ctx context.Context, log *slog.Lo
 	return trace.Wrap(err)
 }
 
-func (a *awsICInstallArgs) parseGroupFilters() (icfilters.Filters, error) {
+func (a *awsICInstallArgs) parseGroupFilters() ([]*types.AWSICResourceFilter, error) {
 	filters := make([]*types.AWSICResourceFilter, 0, len(a.groupNameFilters)+len(a.excludeGroupNameFilters))
 	for _, n := range a.groupNameFilters {
 		filters = append(filters, &types.AWSICResourceFilter{
@@ -142,10 +142,15 @@ func (a *awsICInstallArgs) parseGroupFilters() (icfilters.Filters, error) {
 			Exclude: &types.AWSICResourceFilter_ExcludeNameRegex{ExcludeNameRegex: n},
 		})
 	}
-	return icfilters.New(filters)
+
+	if err := icfilters.Validate(filters); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	return filters, nil
 }
 
-func (a *awsICInstallArgs) parseAccountFilters() (icfilters.Filters, error) {
+func (a *awsICInstallArgs) parseAccountFilters() ([]*types.AWSICResourceFilter, error) {
 	filtersCap := len(a.accountNameFilters) + len(a.excludeAccountNameFilters) + len(a.accountIDFilters) + len(a.excludeAccountIDFilters)
 	filters := make([]*types.AWSICResourceFilter, 0, filtersCap)
 	for _, n := range a.accountNameFilters {
@@ -172,7 +177,11 @@ func (a *awsICInstallArgs) parseAccountFilters() (icfilters.Filters, error) {
 		})
 	}
 
-	return icfilters.New(filters)
+	if err := icfilters.Validate(filters); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	return filters, nil
 }
 
 func (a *awsICInstallArgs) parseUserFilters() ([]*types.AWSICUserSyncFilter, error) {