diff --git a/api/types/discoveryconfig/discoveryconfig.go b/api/types/discoveryconfig/discoveryconfig.go index 937a9da56efb0..45fb507899f93 100644 --- a/api/types/discoveryconfig/discoveryconfig.go +++ b/api/types/discoveryconfig/discoveryconfig.go @@ -253,12 +253,80 @@ func (a *DiscoveryConfig) MatchSearch(values []string) bool { } // IsMatchersEmpty returns true if all matchers are empty. +// +// Deprecated: check the Spec matcher fields directly, or use +// ReferencesOnlyIntegration to check if the config belongs to one integration. func (a *DiscoveryConfig) IsMatchersEmpty() bool { return len(a.Spec.AWS) == 0 && len(a.Spec.Azure) == 0 && len(a.Spec.GCP) == 0 && len(a.Spec.Kube) == 0 && - (a.Spec.AccessGraph == nil || len(a.Spec.AccessGraph.AWS) == 0) + (a.Spec.AccessGraph == nil || + (len(a.Spec.AccessGraph.AWS) == 0 && len(a.Spec.AccessGraph.Azure) == 0)) +} + +// ReferencesIntegration returns true if any matcher or Access Graph sync uses +// the named integration. +func (a *DiscoveryConfig) ReferencesIntegration(integration string) bool { + if integration == "" { + return false + } + + for _, matcher := range a.Spec.AWS { + if matcher.Integration == integration { + return true + } + } + for _, matcher := range a.Spec.Azure { + if matcher.Integration == integration { + return true + } + } + + if a.Spec.AccessGraph != nil { + for _, sync := range a.Spec.AccessGraph.AWS { + if sync != nil && sync.Integration == integration { + return true + } + } + for _, sync := range a.Spec.AccessGraph.Azure { + if sync != nil && sync.Integration == integration { + return true + } + } + } + + return false +} + +// ReferencesOnlyIntegration returns true if every matcher and Access Graph sync +// uses the named integration. GCP and Kubernetes matchers never do. +func (a *DiscoveryConfig) ReferencesOnlyIntegration(integration string) bool { + for _, matcher := range a.Spec.AWS { + if matcher.Integration != integration { + return false + } + } + for _, matcher := range a.Spec.Azure { + if matcher.Integration != integration { + return false + } + } + + if a.Spec.AccessGraph != nil { + for _, sync := range a.Spec.AccessGraph.AWS { + if sync == nil || sync.Integration != integration { + return false + } + } + for _, sync := range a.Spec.AccessGraph.Azure { + if sync == nil || sync.Integration != integration { + return false + } + } + } + + return len(a.Spec.GCP) == 0 && len(a.Spec.Kube) == 0 } // CloneResource returns a copy of the resource as types.ResourceWithLabels. diff --git a/api/types/discoveryconfig/discoveryconfig_test.go b/api/types/discoveryconfig/discoveryconfig_test.go index b8439dd0aa015..de2175b471ee2 100644 --- a/api/types/discoveryconfig/discoveryconfig_test.go +++ b/api/types/discoveryconfig/discoveryconfig_test.go @@ -452,7 +452,7 @@ func TestNewDiscoveryConfig(t *testing.T) { } } -func TestDiscoveryConfig_IsMatchersEmpty(t *testing.T) { +func TestIsMatchersEmpty(t *testing.T) { for _, tt := range []struct { name string config *DiscoveryConfig @@ -527,7 +527,7 @@ func TestDiscoveryConfig_IsMatchersEmpty(t *testing.T) { expected: false, }, { - name: "has AccessGraph but no AWS", + name: "has AccessGraph but no syncs", config: &DiscoveryConfig{ Spec: Spec{ AccessGraph: &types.AccessGraphSync{}, @@ -535,6 +535,20 @@ func TestDiscoveryConfig_IsMatchersEmpty(t *testing.T) { }, expected: true, }, + { + name: "has AccessGraph Azure sync", + config: &DiscoveryConfig{ + Spec: Spec{ + AccessGraph: &types.AccessGraphSync{ + Azure: []*types.AccessGraphAzureSync{{ + Integration: "integration1", + SubscriptionID: "sub-id", + }}, + }, + }, + }, + expected: false, + }, { name: "has multiple matcher types", config: &DiscoveryConfig{ @@ -558,3 +572,255 @@ func TestDiscoveryConfig_IsMatchersEmpty(t *testing.T) { }) } } + +func TestReferencesIntegration(t *testing.T) { + for _, tt := range []struct { + name string + config *DiscoveryConfig + expected bool + }{ + { + name: "empty config", + config: &DiscoveryConfig{Spec: Spec{}}, + expected: false, + }, + { + name: "AWS matcher on the integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{ + {Integration: "integration2"}, + {Integration: "integration1"}, + }, + }, + }, + expected: true, + }, + { + name: "Azure matcher on the integration", + config: &DiscoveryConfig{ + Spec: Spec{ + Azure: []types.AzureMatcher{{Integration: "integration1"}}, + }, + }, + expected: true, + }, + { + name: "AccessGraph AWS sync on the integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AccessGraph: &types.AccessGraphSync{ + AWS: []*types.AccessGraphAWSSync{{Integration: "integration1"}}, + }, + }, + }, + expected: true, + }, + { + name: "AccessGraph Azure sync on the integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AccessGraph: &types.AccessGraphSync{ + Azure: []*types.AccessGraphAzureSync{{Integration: "integration1"}}, + }, + }, + }, + expected: true, + }, + { + name: "only another integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{{Integration: "integration2"}}, + Azure: []types.AzureMatcher{{Integration: "integration2"}}, + AccessGraph: &types.AccessGraphSync{ + AWS: []*types.AccessGraphAWSSync{{Integration: "integration2"}}, + Azure: []*types.AccessGraphAzureSync{{Integration: "integration2"}}, + }, + }, + }, + expected: false, + }, + { + name: "nil AccessGraph sync entries", + config: &DiscoveryConfig{ + Spec: Spec{ + AccessGraph: &types.AccessGraphSync{ + AWS: []*types.AccessGraphAWSSync{nil}, + Azure: []*types.AccessGraphAzureSync{nil}, + }, + }, + }, + expected: false, + }, + { + name: "GCP and Kube matchers cannot reference an integration", + config: &DiscoveryConfig{ + Spec: Spec{ + GCP: []types.GCPMatcher{{Types: []string{"gce"}}}, + Kube: []types.KubernetesMatcher{{Types: []string{"app"}}}, + }, + }, + expected: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.expected, tt.config.ReferencesIntegration("integration1")) + }) + } + + t.Run("matchers using ambient credentials reference no integration", func(t *testing.T) { + config := &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{{Types: []string{"ec2"}}}, + Azure: []types.AzureMatcher{{Types: []string{"vm"}}}, + AccessGraph: &types.AccessGraphSync{ + AWS: []*types.AccessGraphAWSSync{{Regions: []string{"us-east-1"}}}, + Azure: []*types.AccessGraphAzureSync{{SubscriptionID: "sub-id"}}, + }, + }, + } + + require.False(t, config.ReferencesIntegration("")) + }) +} + +func TestReferencesOnlyIntegration(t *testing.T) { + for _, tt := range []struct { + name string + config *DiscoveryConfig + expected bool + }{ + { + name: "empty config", + config: &DiscoveryConfig{Spec: Spec{}}, + expected: true, + }, + { + name: "AWS matchers on the integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{ + {Integration: "integration1"}, + {Integration: "integration1"}, + }, + }, + }, + expected: true, + }, + { + name: "one AWS matcher on another integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{ + {Integration: "integration1"}, + {Integration: "integration2"}, + }, + }, + }, + expected: false, + }, + { + name: "AWS matcher with no integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{{Types: []string{"ec2"}}}, + }, + }, + expected: false, + }, + { + name: "Azure matcher on another integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{{Integration: "integration1"}}, + Azure: []types.AzureMatcher{{Integration: "integration2"}}, + }, + }, + expected: false, + }, + { + name: "AccessGraph syncs on the integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{{Integration: "integration1"}}, + AccessGraph: &types.AccessGraphSync{ + AWS: []*types.AccessGraphAWSSync{{Integration: "integration1"}}, + Azure: []*types.AccessGraphAzureSync{{Integration: "integration1"}}, + }, + }, + }, + expected: true, + }, + { + name: "AccessGraph AWS sync on another integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{{Integration: "integration1"}}, + AccessGraph: &types.AccessGraphSync{ + AWS: []*types.AccessGraphAWSSync{{Integration: "integration2"}}, + }, + }, + }, + expected: false, + }, + { + name: "AccessGraph Azure sync on another integration", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{{Integration: "integration1"}}, + AccessGraph: &types.AccessGraphSync{ + Azure: []*types.AccessGraphAzureSync{{Integration: "integration2"}}, + }, + }, + }, + expected: false, + }, + { + name: "nil AccessGraph AWS sync entry", + config: &DiscoveryConfig{ + Spec: Spec{ + AccessGraph: &types.AccessGraphSync{ + AWS: []*types.AccessGraphAWSSync{nil}, + }, + }, + }, + expected: false, + }, + { + name: "nil AccessGraph Azure sync entry", + config: &DiscoveryConfig{ + Spec: Spec{ + AccessGraph: &types.AccessGraphSync{ + Azure: []*types.AccessGraphAzureSync{nil}, + }, + }, + }, + expected: false, + }, + { + name: "GCP matcher", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{{Integration: "integration1"}}, + GCP: []types.GCPMatcher{{Types: []string{"gce"}}}, + }, + }, + expected: false, + }, + { + name: "Kube matcher", + config: &DiscoveryConfig{ + Spec: Spec{ + AWS: []types.AWSMatcher{{Integration: "integration1"}}, + Kube: []types.KubernetesMatcher{{Types: []string{"app"}}}, + }, + }, + expected: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.expected, tt.config.ReferencesOnlyIntegration("integration1")) + }) + } +} diff --git a/lib/auth/integration/integrationv1/awsoidc.go b/lib/auth/integration/integrationv1/awsoidc.go index fdabf82b13d72..6f1d2c4abd8f6 100644 --- a/lib/auth/integration/integrationv1/awsoidc.go +++ b/lib/auth/integration/integrationv1/awsoidc.go @@ -21,7 +21,6 @@ package integrationv1 import ( "context" "log/slog" - "slices" "strings" "github.com/google/uuid" @@ -99,6 +98,14 @@ func (s *Service) deleteAWSOIDCAssociatedResources(ctx context.Context, authCtx // TODO(alexhemard): follow up work needed to add explicit labels for // resources created by integration rather than rely on implicit rules + if err := authCtx.CheckAccessToKind(types.KindDiscoveryConfig, types.VerbDelete, types.VerbList); err != nil { + return trace.Wrap(err) + } + + if err := authCtx.CheckAccessToKind(types.KindAppServer, types.VerbDelete, types.VerbList); err != nil { + return trace.Wrap(err) + } + // Delete discovery_configs created by this integration var configsRequireCleanup []string var configsToDelete []string @@ -108,21 +115,15 @@ func (s *Service) deleteAWSOIDCAssociatedResources(ctx context.Context, authCtx return trace.Wrap(err) } - awsMatchers := config.Spec.AWS - - config.Spec.AWS = slices.DeleteFunc(config.Spec.AWS, func(matcher types.AWSMatcher) bool { - return matcher.Integration == ig.GetName() - }) - - if len(awsMatchers) == len(config.Spec.AWS) { + if !config.ReferencesIntegration(ig.GetName()) { continue } // discovery_configs can be assumed to be created by the integration // and deleted if - // 1. only has matchers referencing this integration + // 1. every matcher and Access Graph sync references this integration // 2. has valid uuid name - if config.IsMatchersEmpty() { + if config.ReferencesOnlyIntegration(ig.GetName()) { _, err := uuid.Parse(config.GetName()) if err == nil { @@ -131,20 +132,14 @@ func (s *Service) deleteAWSOIDCAssociatedResources(ctx context.Context, authCtx } } - configsRequireCleanup = append(configsRequireCleanup, config.GetName()) + configsRequireCleanup = append(configsRequireCleanup, "discovery_config/"+config.GetName()) } if len(configsRequireCleanup) > 0 { - var qualifiedConfigs []string - for _, config := range configsRequireCleanup { - qualifiedConfigs = append(qualifiedConfigs, "discovery_config/"+config) - } - - return trace.BadParameter("cannot delete integration, "+ - "Discovery Configs referencing this integration must be removed first: %s\n\n"+ - "Use `tsh rm %s` to remove them.", - strings.Join(configsRequireCleanup, ", "), - strings.Join(qualifiedConfigs, " ")) + return trace.BadParameter("cannot delete integration %q because these discovery configs reference it "+ + "and cannot be removed automatically: %s\n\n"+ + "Remove the reference from each one, or delete it with `tctl rm `, then try again.", + ig.GetName(), strings.Join(configsRequireCleanup, ", ")) } for _, configName := range configsToDelete { diff --git a/lib/auth/integration/integrationv1/service_test.go b/lib/auth/integration/integrationv1/service_test.go index e9795e153632a..1bad11181014d 100644 --- a/lib/auth/integration/integrationv1/service_test.go +++ b/lib/auth/integration/integrationv1/service_test.go @@ -61,6 +61,20 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } +// The AWS OIDC cascade delete lists and deletes the resources it cleans up. +var cascadeDeleteRole = types.RoleSpecV6{ + Allow: types.RoleConditions{Rules: []types.Rule{ + { + Resources: []string{types.KindIntegration}, + Verbs: []string{types.VerbDelete}, + }, + { + Resources: []string{types.KindDiscoveryConfig, types.KindAppServer}, + Verbs: []string{types.VerbDelete, types.VerbList}, + }, + }}, +} + func TestIntegrationCRUD(t *testing.T) { t.Parallel() clusterName := "test-cluster" @@ -719,6 +733,182 @@ func TestIntegrationCRUD(t *testing.T) { }, { Name: "cannot delete AWS OIDC integration with user-created discovery config", + Role: cascadeDeleteRole, + Setup: func(t *testing.T, igName string) { + t.Helper() + ig := sampleIntegrationFn(t, igName) + _, err := localClient.CreateIntegration(ctx, ig) + require.NoError(t, err) + _, err = localClient.CreateDiscoveryConfig(ctx, mustMakeDiscoveryConfig(t, ig)) + require.NoError(t, err) + problematicConfig := mustMakeDiscoveryConfig(t, ig) + problematicConfig.Metadata.Name = "problematicconfig" + _, err = localClient.CreateDiscoveryConfig(ctx, problematicConfig) + require.NoError(t, err) + }, + Test: func(ctx context.Context, resourceSvc *Service, igName string) error { + _, err := resourceSvc.DeleteIntegration(ctx, integrationpb.DeleteIntegrationRequest_builder{ + Name: igName, + DeleteAssociatedResources: true, + }.Build()) + return err + }, + Validate: func(t *testing.T, igName string) { + t.Helper() + _, err := localClient.GetIntegration(context.Background(), igName) + require.NoError(t, err) + _, err = localClient.GetDiscoveryConfig(context.Background(), igName) + require.NoError(t, err) + _, err = localClient.GetDiscoveryConfig(context.Background(), "problematicconfig") + require.NoError(t, err) + }, + ErrAssertion: trace.IsBadParameter, + }, + { + Name: "delete AWS OIDC integration with access graph sync on the same integration", + Role: cascadeDeleteRole, + Setup: func(t *testing.T, igName string) { + t.Helper() + ig := sampleIntegrationFn(t, igName) + _, err := localClient.CreateIntegration(ctx, ig) + require.NoError(t, err) + + config := mustMakeDiscoveryConfig(t, ig) + config.Spec.AccessGraph = &types.AccessGraphSync{ + AWS: []*types.AccessGraphAWSSync{{ + Regions: []string{"us-west-2"}, + Integration: igName, + }}, + } + _, err = localClient.CreateDiscoveryConfig(ctx, config) + require.NoError(t, err) + }, + Test: func(ctx context.Context, resourceSvc *Service, igName string) error { + _, err := resourceSvc.DeleteIntegration(ctx, integrationpb.DeleteIntegrationRequest_builder{ + Name: igName, + DeleteAssociatedResources: true, + }.Build()) + return err + }, + Validate: func(t *testing.T, igName string) { + t.Helper() + _, err := localClient.GetDiscoveryConfig(context.Background(), igName) + require.True(t, trace.IsNotFound(err)) + }, + ErrAssertion: noError, + }, + { + Name: "delete AWS OIDC integration referenced only by an access graph sync", + Role: cascadeDeleteRole, + Setup: func(t *testing.T, igName string) { + t.Helper() + ig := sampleIntegrationFn(t, igName) + _, err := localClient.CreateIntegration(ctx, ig) + require.NoError(t, err) + + config, err := discoveryconfig.NewDiscoveryConfig( + header.Metadata{Name: igName}, + discoveryconfig.Spec{ + DiscoveryGroup: igName, + AccessGraph: &types.AccessGraphSync{ + AWS: []*types.AccessGraphAWSSync{{ + Regions: []string{"us-west-2"}, + Integration: igName, + }}, + }, + }, + ) + require.NoError(t, err) + _, err = localClient.CreateDiscoveryConfig(ctx, config) + require.NoError(t, err) + }, + Test: func(ctx context.Context, resourceSvc *Service, igName string) error { + _, err := resourceSvc.DeleteIntegration(ctx, integrationpb.DeleteIntegrationRequest_builder{ + Name: igName, + DeleteAssociatedResources: true, + }.Build()) + return err + }, + Validate: func(t *testing.T, igName string) { + t.Helper() + _, err := localClient.GetDiscoveryConfig(context.Background(), igName) + require.True(t, trace.IsNotFound(err)) + }, + ErrAssertion: noError, + }, + { + Name: "cannot delete AWS OIDC integration with access graph azure sync on another integration", + Role: cascadeDeleteRole, + Setup: func(t *testing.T, igName string) { + t.Helper() + ig := sampleIntegrationFn(t, igName) + _, err := localClient.CreateIntegration(ctx, ig) + require.NoError(t, err) + + config := mustMakeDiscoveryConfig(t, ig) + config.Spec.AccessGraph = &types.AccessGraphSync{ + Azure: []*types.AccessGraphAzureSync{{ + SubscriptionID: "sub-id", + Integration: "azure-integration", + }}, + } + _, err = localClient.CreateDiscoveryConfig(ctx, config) + require.NoError(t, err) + }, + Test: func(ctx context.Context, resourceSvc *Service, igName string) error { + _, err := resourceSvc.DeleteIntegration(ctx, integrationpb.DeleteIntegrationRequest_builder{ + Name: igName, + DeleteAssociatedResources: true, + }.Build()) + return err + }, + Validate: func(t *testing.T, igName string) { + t.Helper() + _, err := localClient.GetIntegration(context.Background(), igName) + require.NoError(t, err) + _, err = localClient.GetDiscoveryConfig(context.Background(), igName) + require.NoError(t, err) + }, + ErrAssertion: trace.IsBadParameter, + }, + { + Name: "cannot delete AWS OIDC integration with azure matcher on another integration", + Role: cascadeDeleteRole, + Setup: func(t *testing.T, igName string) { + t.Helper() + ig := sampleIntegrationFn(t, igName) + _, err := localClient.CreateIntegration(ctx, ig) + require.NoError(t, err) + + config := mustMakeDiscoveryConfig(t, ig) + config.Spec.Azure = []types.AzureMatcher{{ + Types: []string{"vm"}, + Regions: []string{"eastus"}, + Subscriptions: []string{"sub-id"}, + ResourceGroups: []string{"rg"}, + Integration: "azure-integration", + }} + _, err = localClient.CreateDiscoveryConfig(ctx, config) + require.NoError(t, err) + }, + Test: func(ctx context.Context, resourceSvc *Service, igName string) error { + _, err := resourceSvc.DeleteIntegration(ctx, integrationpb.DeleteIntegrationRequest_builder{ + Name: igName, + DeleteAssociatedResources: true, + }.Build()) + return err + }, + Validate: func(t *testing.T, igName string) { + t.Helper() + _, err := localClient.GetIntegration(context.Background(), igName) + require.NoError(t, err) + _, err = localClient.GetDiscoveryConfig(context.Background(), igName) + require.NoError(t, err) + }, + ErrAssertion: trace.IsBadParameter, + }, + { + Name: "cannot delete AWS OIDC integration discovery config without permission", Role: types.RoleSpecV6{ Allow: types.RoleConditions{Rules: []types.Rule{ { @@ -734,10 +924,6 @@ func TestIntegrationCRUD(t *testing.T) { require.NoError(t, err) _, err = localClient.CreateDiscoveryConfig(ctx, mustMakeDiscoveryConfig(t, ig)) require.NoError(t, err) - problematicConfig := mustMakeDiscoveryConfig(t, ig) - problematicConfig.Metadata.Name = "problematicconfig" - _, err = localClient.CreateDiscoveryConfig(ctx, problematicConfig) - require.NoError(t, err) }, Test: func(ctx context.Context, resourceSvc *Service, igName string) error { _, err := resourceSvc.DeleteIntegration(ctx, integrationpb.DeleteIntegrationRequest_builder{ @@ -752,21 +938,52 @@ func TestIntegrationCRUD(t *testing.T) { require.NoError(t, err) _, err = localClient.GetDiscoveryConfig(context.Background(), igName) require.NoError(t, err) - _, err = localClient.GetDiscoveryConfig(context.Background(), "problematicconfig") - require.NoError(t, err) }, - ErrAssertion: trace.IsBadParameter, + ErrAssertion: trace.IsAccessDenied, }, { - Name: "delete AWS OIDC integration with associated resources", + Name: "cannot delete AWS OIDC integration app server without permission", Role: types.RoleSpecV6{ Allow: types.RoleConditions{Rules: []types.Rule{ { Resources: []string{types.KindIntegration}, Verbs: []string{types.VerbDelete}, }, + { + Resources: []string{types.KindDiscoveryConfig}, + Verbs: []string{types.VerbDelete, types.VerbList}, + }, }}, }, + Setup: func(t *testing.T, igName string) { + t.Helper() + ig := sampleIntegrationFn(t, igName) + _, err := localClient.CreateIntegration(ctx, ig) + require.NoError(t, err) + _, err = localClient.CreateDiscoveryConfig(ctx, mustMakeDiscoveryConfig(t, ig)) + require.NoError(t, err) + _, err = localClient.UpsertApplicationServer(ctx, mustMakeAppServer(t, ig)) + require.NoError(t, err) + }, + Test: func(ctx context.Context, resourceSvc *Service, igName string) error { + _, err := resourceSvc.DeleteIntegration(ctx, integrationpb.DeleteIntegrationRequest_builder{ + Name: igName, + DeleteAssociatedResources: true, + }.Build()) + return err + }, + Validate: func(t *testing.T, igName string) { + t.Helper() + _, err := localClient.GetIntegration(context.Background(), igName) + require.NoError(t, err) + _, err = localClient.GetDiscoveryConfig(context.Background(), igName) + require.NoError(t, err) + }, + ErrAssertion: trace.IsAccessDenied, + }, + { + Name: "delete AWS OIDC integration with associated resources", + Role: cascadeDeleteRole, Setup: func(t *testing.T, igName string) { t.Helper()