From dc029fcfd3d513935e785309278e3be91a853dda Mon Sep 17 00:00:00 2001 From: flyinghermit Date: Mon, 28 Oct 2024 23:30:12 -0400 Subject: [PATCH 1/4] prevent aws oidc integration from deletion if referenced by ic plugin --- .../integration/integrationv1/service_test.go | 53 +++++++++++++++ lib/services/local/integrations.go | 64 ++++++++++++++++++- 2 files changed, 114 insertions(+), 3 deletions(-) diff --git a/lib/auth/integration/integrationv1/service_test.go b/lib/auth/integration/integrationv1/service_test.go index 0fd6355ebb8f5..3316cf239d0e8 100644 --- a/lib/auth/integration/integrationv1/service_test.go +++ b/lib/auth/integration/integrationv1/service_test.go @@ -280,6 +280,29 @@ func TestIntegrationCRUD(t *testing.T) { }, ErrAssertion: trace.IsBadParameter, }, + { + Name: "can't delete integration referenced by AWS Identity Center plugin", + Role: types.RoleSpecV6{ + Allow: types.RoleConditions{Rules: []types.Rule{{ + Resources: []string{types.KindIntegration}, + Verbs: []string{types.VerbDelete}, + }}}, + }, + Setup: func(t *testing.T, igName string) { + _, err := localClient.CreateIntegration(ctx, sampleIntegrationFn(t, igName)) + require.NoError(t, err) + require.NoError(t, localClient.CreatePlugin(ctx, newPlugin(t, igName))) + }, + Test: func(ctx context.Context, resourceSvc *Service, igName string) error { + _, err := resourceSvc.DeleteIntegration(ctx, &integrationpb.DeleteIntegrationRequest{Name: igName}) + return err + }, + Cleanup: func(t *testing.T, igName string) { + err := localClient.DeletePlugin(ctx, newPlugin(t, igName).GetName()) + require.NoError(t, err) + }, + ErrAssertion: trace.IsBadParameter, + }, { Name: "access to delete integration", Role: types.RoleSpecV6{ @@ -369,6 +392,8 @@ type localClient interface { DeleteDraftExternalAuditStorage(ctx context.Context) error PromoteToClusterExternalAuditStorage(ctx context.Context) error DisableClusterExternalAuditStorage(ctx context.Context) error + CreatePlugin(ctx context.Context, plugin types.Plugin) error + DeletePlugin(ctx context.Context, name string) error } type testClient struct { @@ -390,6 +415,7 @@ func initSvc(t *testing.T, ca types.CertAuthority, clusterName string, proxyPubl userSvc, err := local.NewTestIdentityService(backend) require.NoError(t, err) easSvc := local.NewExternalAuditStorageService(backend) + pluginSvc := local.NewPluginsService(backend) _, err = clusterConfigSvc.UpsertAuthPreference(ctx, types.DefaultAuthPreference()) require.NoError(t, err) @@ -455,11 +481,13 @@ func initSvc(t *testing.T, ca types.CertAuthority, clusterName string, proxyPubl *local.IdentityService *local.ExternalAuditStorageService *local.IntegrationsService + *local.PluginsService }{ AccessService: roleSvc, IdentityService: userSvc, ExternalAuditStorageService: easSvc, IntegrationsService: localResourceService, + PluginsService: pluginSvc, }, resourceSvc } @@ -525,3 +553,28 @@ func newCertAuthority(t *testing.T, caType types.CertAuthType, domain string) ty return ca } + +func newPlugin(t *testing.T, integrationName string) *types.PluginV1 { + t.Helper() + return &types.PluginV1{ + Metadata: types.Metadata{ + Name: types.PluginTypeAWSIdentityCenter, + Labels: map[string]string{ + types.HostedPluginLabel: "true", + }, + }, + Spec: types.PluginSpecV1{ + Settings: &types.PluginSpecV1_AwsIc{ + AwsIc: &types.PluginAWSICSettings{ + IntegrationName: integrationName, + Region: "test-region", + Arn: "test-arn", + AccessListDefaultOwners: []string{"user1", "user2"}, + ProvisioningSpec: &types.AWSICProvisioningSpec{ + BaseUrl: "https://example.com", + }, + }, + }, + }, + } +} diff --git a/lib/services/local/integrations.go b/lib/services/local/integrations.go index b22baecaa0be6..3e5dcdd25ac78 100644 --- a/lib/services/local/integrations.go +++ b/lib/services/local/integrations.go @@ -127,7 +127,7 @@ func (s *IntegrationsService) DeleteIntegration(ctx context.Context, name string return trace.Wrap(err) } - conditionalActions, err := notReferencedByEAS(ctx, s.backend, name) + conditionalActions, err := integrationNotReferencedByOtherServices(ctx, s.backend, name) if err != nil { return trace.Wrap(err) } @@ -140,9 +140,33 @@ func (s *IntegrationsService) DeleteIntegration(ctx context.Context, name string return trace.Wrap(err) } -// notReferencedByEAS returns a slice of ConditionalActions to use with a backend.AtomicWrite to ensure that +// integrationNotReferencedByOtherServices checks if the AWS OIDC integration is s referenced by another +// Teleport service. It returns a slice of backend.ConditionalAction or an error. +// ConditionalAction ensures that current reference status remains unchanged (i.e. integration not referenced) +// until the integration is completely deleted. +// Error should immidiately prevent deletion of the integration. +func integrationNotReferencedByOtherServices(ctx context.Context, bk backend.Backend, name string) ([]backend.ConditionalAction, error) { + var conditionalActions []backend.ConditionalAction + easCOndition, err := integrationNotReferencedByEAS(ctx, bk, name) + if err != nil { + return nil, trace.Wrap(err) + } + conditionalActions = append(conditionalActions, easCOndition...) + + awsIcCOndition, err := integrationReferencedByAWSICPlugin(ctx, bk, name) + if err != nil { + return nil, trace.Wrap(err) + } + if awsIcCOndition != nil { + conditionalActions = append(conditionalActions, *awsIcCOndition) + } + + return conditionalActions, nil +} + +// integrationNotReferencedByEAS returns a slice of ConditionalActions to use with a backend.AtomicWrite to ensure that // integration [name] is not referenced by any EAS (External Audit Storage) integration. -func notReferencedByEAS(ctx context.Context, bk backend.Backend, name string) ([]backend.ConditionalAction, error) { +func integrationNotReferencedByEAS(ctx context.Context, bk backend.Backend, name string) ([]backend.ConditionalAction, error) { var conditionalActions []backend.ConditionalAction for _, key := range []backend.Key{draftExternalAuditStorageBackendKey, clusterExternalAuditStorageBackendKey} { condition := backend.ConditionalAction{ @@ -173,6 +197,40 @@ func notReferencedByEAS(ctx context.Context, bk backend.Backend, name string) ([ return conditionalActions, nil } +// integrationNotReferencedByAWSICPlugin returns an error if the integration name is referenced +// by an existing AWS Identity Center plugin. In case the AWS Identity Center plugin exists +// but does not reference this integration, a conditional action is returned with a revision +// of the plugin to ensure that plugin hasn't changed when deleting the AWS OIDC integration. +func integrationReferencedByAWSICPlugin(ctx context.Context, bk backend.Backend, name string) (*backend.ConditionalAction, error) { + pluginService := NewPluginsService(bk) + plugins, err := pluginService.GetPlugins(ctx, false) + if err != nil { + return nil, trace.Wrap(err) + } + + for _, p := range plugins { + pluginV1, ok := p.(*types.PluginV1) + if !ok { + continue + } + + if pluginV1.GetType() == types.PluginType(types.PluginTypeAWSIdentityCenter) { + switch pluginV1.Spec.GetAwsIc().IntegrationName { + case name: + return nil, trace.BadParameter("cannot delete AWS OIDC integration currently referenced by AWS Identity Center integration") + default: + return &backend.ConditionalAction{ + Key: backend.NewKey(pluginsPrefix, name), + Action: backend.Nop(), + Condition: backend.Revision(pluginV1.GetRevision()), + }, nil + } + } + } + + return nil, nil +} + // DeleteAllIntegrations removes all Integration resources. This should only be used in a cache. func (s *IntegrationsService) DeleteAllIntegrations(ctx context.Context) error { if !s.cacheMode { From 95ce892fea1de04a711ae7d0f83b06fbec22a7a4 Mon Sep 17 00:00:00 2001 From: flyinghermit Date: Mon, 28 Oct 2024 23:50:35 -0400 Subject: [PATCH 2/4] fixes typo and updated comment --- lib/services/local/integrations.go | 41 +++++++++++++++--------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/lib/services/local/integrations.go b/lib/services/local/integrations.go index 3e5dcdd25ac78..9f6e0eb6f7ed5 100644 --- a/lib/services/local/integrations.go +++ b/lib/services/local/integrations.go @@ -127,46 +127,46 @@ func (s *IntegrationsService) DeleteIntegration(ctx context.Context, name string return trace.Wrap(err) } - conditionalActions, err := integrationNotReferencedByOtherServices(ctx, s.backend, name) + deleteConditions, err := integrationConditionalDeletion(ctx, s.backend, name) if err != nil { return trace.Wrap(err) } - conditionalActions = append(conditionalActions, backend.ConditionalAction{ + deleteConditions = append(deleteConditions, backend.ConditionalAction{ Key: s.svc.MakeKey(backend.NewKey(name)), Condition: backend.Exists(), Action: backend.Delete(), }) - _, err = s.backend.AtomicWrite(ctx, conditionalActions) + _, err = s.backend.AtomicWrite(ctx, deleteConditions) return trace.Wrap(err) } -// integrationNotReferencedByOtherServices checks if the AWS OIDC integration is s referenced by another -// Teleport service. It returns a slice of backend.ConditionalAction or an error. -// ConditionalAction ensures that current reference status remains unchanged (i.e. integration not referenced) -// until the integration is completely deleted. -// Error should immidiately prevent deletion of the integration. -func integrationNotReferencedByOtherServices(ctx context.Context, bk backend.Backend, name string) ([]backend.ConditionalAction, error) { - var conditionalActions []backend.ConditionalAction - easCOndition, err := integrationNotReferencedByEAS(ctx, bk, name) +// integrationConditionalDeletion returns a BadParameter error if the integration is referenced by another +// Teleport service. If it does not find any direct reference, the backend.ConditionalAction is returned +// with the current state of reference, which should be added to AtomicWrite to ensure that the current +// reference state remains unchanged until the integration is completeley deleted. +// Service may have zero or multuple ConditionalActions returned. +func integrationConditionalDeletion(ctx context.Context, bk backend.Backend, name string) ([]backend.ConditionalAction, error) { + var deleteConditionalActions []backend.ConditionalAction + easDeleteConditions, err := integrationReferencedByEAS(ctx, bk, name) if err != nil { return nil, trace.Wrap(err) } - conditionalActions = append(conditionalActions, easCOndition...) + deleteConditionalActions = append(deleteConditionalActions, easDeleteConditions...) - awsIcCOndition, err := integrationReferencedByAWSICPlugin(ctx, bk, name) + awsIcDeleteCondition, err := integrationReferencedByAWSICPlugin(ctx, bk, name) if err != nil { return nil, trace.Wrap(err) } - if awsIcCOndition != nil { - conditionalActions = append(conditionalActions, *awsIcCOndition) + if awsIcDeleteCondition != nil { + deleteConditionalActions = append(deleteConditionalActions, *awsIcDeleteCondition) } - return conditionalActions, nil + return deleteConditionalActions, nil } -// integrationNotReferencedByEAS returns a slice of ConditionalActions to use with a backend.AtomicWrite to ensure that +// integrationReferencedByEAS returns a slice of ConditionalActions to use with a backend.AtomicWrite to ensure that // integration [name] is not referenced by any EAS (External Audit Storage) integration. -func integrationNotReferencedByEAS(ctx context.Context, bk backend.Backend, name string) ([]backend.ConditionalAction, error) { +func integrationReferencedByEAS(ctx context.Context, bk backend.Backend, name string) ([]backend.ConditionalAction, error) { var conditionalActions []backend.ConditionalAction for _, key := range []backend.Key{draftExternalAuditStorageBackendKey, clusterExternalAuditStorageBackendKey} { condition := backend.ConditionalAction{ @@ -197,10 +197,11 @@ func integrationNotReferencedByEAS(ctx context.Context, bk backend.Backend, name return conditionalActions, nil } -// integrationNotReferencedByAWSICPlugin returns an error if the integration name is referenced +// integrationReferencedByAWSICPlugin returns an error if the integration name is referenced // by an existing AWS Identity Center plugin. In case the AWS Identity Center plugin exists // but does not reference this integration, a conditional action is returned with a revision -// of the plugin to ensure that plugin hasn't changed when deleting the AWS OIDC integration. +// of the plugin to ensure that plugin hasn't changed during deletion of the AWS OIDC integration. +// Returns nil ConditionalAction if no AWS Identity Center plugin is found. func integrationReferencedByAWSICPlugin(ctx context.Context, bk backend.Backend, name string) (*backend.ConditionalAction, error) { pluginService := NewPluginsService(bk) plugins, err := pluginService.GetPlugins(ctx, false) From 4075d8a6fefd2675a418647feb3bbd7a85b87416 Mon Sep 17 00:00:00 2001 From: flyinghermit Date: Mon, 28 Oct 2024 23:54:18 -0400 Subject: [PATCH 3/4] fixes typo --- lib/services/local/integrations.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/services/local/integrations.go b/lib/services/local/integrations.go index 9f6e0eb6f7ed5..c962cb1e8c567 100644 --- a/lib/services/local/integrations.go +++ b/lib/services/local/integrations.go @@ -201,7 +201,7 @@ func integrationReferencedByEAS(ctx context.Context, bk backend.Backend, name st // by an existing AWS Identity Center plugin. In case the AWS Identity Center plugin exists // but does not reference this integration, a conditional action is returned with a revision // of the plugin to ensure that plugin hasn't changed during deletion of the AWS OIDC integration. -// Returns nil ConditionalAction if no AWS Identity Center plugin is found. +// Returns nil ConditionalAction if AWS Identity Center plugin is not found. func integrationReferencedByAWSICPlugin(ctx context.Context, bk backend.Backend, name string) (*backend.ConditionalAction, error) { pluginService := NewPluginsService(bk) plugins, err := pluginService.GetPlugins(ctx, false) From 7630a4d26b2951359c6cc94f6d1fb8f848382398 Mon Sep 17 00:00:00 2001 From: flyinghermit Date: Tue, 29 Oct 2024 09:01:40 -0400 Subject: [PATCH 4/4] review suggestions: - return slice instead of pointer - func rename - typo fixes --- lib/services/local/integrations.go | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/lib/services/local/integrations.go b/lib/services/local/integrations.go index c962cb1e8c567..3b9842ee79690 100644 --- a/lib/services/local/integrations.go +++ b/lib/services/local/integrations.go @@ -127,7 +127,7 @@ func (s *IntegrationsService) DeleteIntegration(ctx context.Context, name string return trace.Wrap(err) } - deleteConditions, err := integrationConditionalDeletion(ctx, s.backend, name) + deleteConditions, err := integrationDeletionConditions(ctx, s.backend, name) if err != nil { return trace.Wrap(err) } @@ -140,12 +140,12 @@ func (s *IntegrationsService) DeleteIntegration(ctx context.Context, name string return trace.Wrap(err) } -// integrationConditionalDeletion returns a BadParameter error if the integration is referenced by another +// integrationDeletionConditions returns a BadParameter error if the integration is referenced by another // Teleport service. If it does not find any direct reference, the backend.ConditionalAction is returned // with the current state of reference, which should be added to AtomicWrite to ensure that the current -// reference state remains unchanged until the integration is completeley deleted. -// Service may have zero or multuple ConditionalActions returned. -func integrationConditionalDeletion(ctx context.Context, bk backend.Backend, name string) ([]backend.ConditionalAction, error) { +// reference state remains unchanged until the integration is completely deleted. +// Service may have zero or multiple ConditionalActions returned. +func integrationDeletionConditions(ctx context.Context, bk backend.Backend, name string) ([]backend.ConditionalAction, error) { var deleteConditionalActions []backend.ConditionalAction easDeleteConditions, err := integrationReferencedByEAS(ctx, bk, name) if err != nil { @@ -157,9 +157,7 @@ func integrationConditionalDeletion(ctx context.Context, bk backend.Backend, nam if err != nil { return nil, trace.Wrap(err) } - if awsIcDeleteCondition != nil { - deleteConditionalActions = append(deleteConditionalActions, *awsIcDeleteCondition) - } + deleteConditionalActions = append(deleteConditionalActions, awsIcDeleteCondition...) return deleteConditionalActions, nil } @@ -201,8 +199,8 @@ func integrationReferencedByEAS(ctx context.Context, bk backend.Backend, name st // by an existing AWS Identity Center plugin. In case the AWS Identity Center plugin exists // but does not reference this integration, a conditional action is returned with a revision // of the plugin to ensure that plugin hasn't changed during deletion of the AWS OIDC integration. -// Returns nil ConditionalAction if AWS Identity Center plugin is not found. -func integrationReferencedByAWSICPlugin(ctx context.Context, bk backend.Backend, name string) (*backend.ConditionalAction, error) { +func integrationReferencedByAWSICPlugin(ctx context.Context, bk backend.Backend, name string) ([]backend.ConditionalAction, error) { + var conditionalActions []backend.ConditionalAction pluginService := NewPluginsService(bk) plugins, err := pluginService.GetPlugins(ctx, false) if err != nil { @@ -218,18 +216,19 @@ func integrationReferencedByAWSICPlugin(ctx context.Context, bk backend.Backend, if pluginV1.GetType() == types.PluginType(types.PluginTypeAWSIdentityCenter) { switch pluginV1.Spec.GetAwsIc().IntegrationName { case name: - return nil, trace.BadParameter("cannot delete AWS OIDC integration currently referenced by AWS Identity Center integration") + return nil, trace.BadParameter("cannot delete AWS OIDC integration currently referenced by AWS Identity Center integration %q", pluginV1.GetName()) default: - return &backend.ConditionalAction{ + conditionalActions = append(conditionalActions, backend.ConditionalAction{ Key: backend.NewKey(pluginsPrefix, name), Action: backend.Nop(), Condition: backend.Revision(pluginV1.GetRevision()), - }, nil + }) + return conditionalActions, nil } } } - return nil, nil + return conditionalActions, nil } // DeleteAllIntegrations removes all Integration resources. This should only be used in a cache.