feat(cleanup-sweeper): purge aged soft-deleted directory objects backing role assignments (AROSLSRE-2023)

docs/ci/cleanup.md

@@ -94,6 +94,8 @@ The `cleanup-sweeper` `shared-leftovers` workflow runs per environment. Alongsid
 
 To resolve the principals behind orphaned role assignments, `shared-leftovers` reads the Microsoft Graph directory, which requires `Directory.Read.All`. A role assignment is deleted only when its principal is absent from both the active directory and `directory/deletedItems`; assignments for soft-deleted principals remain intact during Entra's restore window. If either directory lookup fails, discovery fails closed without deleting assignments. The per-environment ARM identity (`VAULT_SECRET_PROFILE`) usually lacks that tenant-wide grant, so the step exports a dedicated Graph identity from `GRAPH_SECRET_PROFILE` (the dev bot, which holds `Directory.Read.All`) whenever its mounted profile differs from the ARM profile. When the two resolve to the same profile, no separate Graph credential is used and the ARM identity serves both. The sweeper binary reads that dedicated identity from `GRAPH_AZURE_CLIENT_ID` / `GRAPH_AZURE_TENANT_ID` / `GRAPH_AZURE_CLIENT_SECRET`, which the `aro-hcp-deprovision-cleanup-sweeper` step in `openshift/release` exports from the mounted Graph profile.
 
+Because `directory/deletedItems` protects a principal for the full 30-day Entra restore window, a subscription that keeps recreating short-lived service principals with the same role assignments (for example, e2e-test tooling) can build up a large backlog of role assignments pinned to soft-deleted principals well before that window closes. `shared-leftovers` can optionally purge those principals from `deletedItems` itself, once they are older than a grace period, so their role assignments become eligible for deletion in the same run. This is a separate, higher-privilege, opt-in step: it needs `Directory.ReadWrite.All` / `Application.ReadWrite.All`, well beyond the read-only Graph identity used for discovery, and it only ever acts on service principals or applications that already hold a role assignment in the target subscription (it never scans the tenant's `deletedItems` at large). It is enabled by setting `DIRECTORY_WRITE_AZURE_CLIENT_ID` / `DIRECTORY_WRITE_AZURE_TENANT_ID` / `DIRECTORY_WRITE_AZURE_CLIENT_SECRET`; when unset (the default), this step is skipped entirely and behavior is unchanged. The default grace period is 7 days from the object's `deletedDateTime`, configurable in code via `PurgeAgedDeletedStepConfig.MinAge`.
+
 For `cleanup-sweeper` `rg-ordered`, candidate resource groups are chosen using `tooling/cleanup-sweeper/resourcegroups.policy.yaml`. Discovery treats the `createdAt` tag (RFC3339 timestamp on the resource group) as required for any `action: delete` rule: groups without a parseable tag are not candidates.
 
 The policy excludes long-lived slot-managed identity pools whose resource-group

tooling/cleanup-sweeper/cmd/root/options.go

@@ -101,7 +101,13 @@ type completedOptions struct {
 	// dedicated Graph identity is supplied via the GRAPH_AZURE_* environment
 	// variables.
 	GraphCredential azcore.TokenCredential
-	Policy          *policy.Policy
+	// DirectoryWriteCredential backs the aged-deleted-directory-object purge
+	// step's Graph client. Unlike GraphCredential, it is opt-in: it is only
+	// set when a dedicated DIRECTORY_WRITE_AZURE_* identity is configured, and
+	// never falls back to AzureCredential or GraphCredential, since this
+	// credential needs materially higher (directory-write) privilege.
+	DirectoryWriteCredential azcore.TokenCredential
+	Policy                   *policy.Policy
 
 	Workflow WorkflowMode
 
@@ -185,26 +191,32 @@ func (o *ValidatedOptions) Complete(_ context.Context) (*Options, error) {
 	// This keeps a partial GRAPH_AZURE_* configuration from failing workflows
 	// that never touch Graph (for example, rg-ordered).
 	var graphCred azcore.TokenCredential = cred
+	var directoryWriteCred azcore.TokenCredential
 	if o.workflow == WorkflowSharedLeftovers {
 		graphCred, err = newGraphCredential(cred)
 		if err != nil {
 			return nil, err
 		}
+		directoryWriteCred, err = newDirectoryWriteCredential()
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	return &Options{
 		completedOptions: &completedOptions{
-			AzureCredential: cred,
-			GraphCredential: graphCred,
-			Policy:          o.policy,
-			Workflow:        o.workflow,
-			SubscriptionID:  subscriptionID,
-			PolicyFile:      policyFile,
-			ReferenceTime:   referenceTime,
-			DryRun:          o.DryRun,
-			Wait:            o.Wait,
-			Parallelism:     o.Parallelism,
-			ResourceGroups:  resourceGroups,
+			AzureCredential:          cred,
+			GraphCredential:          graphCred,
+			DirectoryWriteCredential: directoryWriteCred,
+			Policy:                   o.policy,
+			Workflow:                 o.workflow,
+			SubscriptionID:           subscriptionID,
+			PolicyFile:               policyFile,
+			ReferenceTime:            referenceTime,
+			DryRun:                   o.DryRun,
+			Wait:                     o.Wait,
+			Parallelism:              o.Parallelism,
+			ResourceGroups:           resourceGroups,
 		},
 	}, nil
 }
@@ -240,12 +252,13 @@ func (o *Options) Run(ctx context.Context) error {
 		}
 	case WorkflowSharedLeftovers:
 		err := sharedworkflow.Run(ctx, sharedworkflow.RunOptions{
-			SubscriptionID:  o.SubscriptionID,
-			AzureCredential: o.AzureCredential,
-			GraphCredential: o.GraphCredential,
-			DryRun:          o.DryRun,
-			Wait:            o.Wait,
-			Parallelism:     o.Parallelism,
+			SubscriptionID:           o.SubscriptionID,
+			AzureCredential:          o.AzureCredential,
+			GraphCredential:          o.GraphCredential,
+			DirectoryWriteCredential: o.DirectoryWriteCredential,
+			DryRun:                   o.DryRun,
+			Wait:                     o.Wait,
+			Parallelism:              o.Parallelism,
 		})
 		if err != nil {
 			return err
@@ -283,6 +296,34 @@ func newGraphCredential(fallback azcore.TokenCredential) (azcore.TokenCredential
 	return cred, nil
 }
 
+// newDirectoryWriteCredential returns the credential used by the
+// aged-deleted-directory-object purge step, which needs directory-write
+// permission (Directory.ReadWrite.All / Application.ReadWrite.All). Unlike
+// newGraphCredential, it never falls back to another credential: the step is
+// materially more privileged (it permanently deletes directory objects), so
+// it is only enabled when a dedicated identity is explicitly configured via
+// the DIRECTORY_WRITE_AZURE_* environment variables. Returns a nil credential
+// (and nil error) when unset, which the shared-leftovers workflow treats as
+// "omit this step".
+func newDirectoryWriteCredential() (azcore.TokenCredential, error) {
+	tenantID := strings.TrimSpace(os.Getenv("DIRECTORY_WRITE_AZURE_TENANT_ID"))
+	clientID := strings.TrimSpace(os.Getenv("DIRECTORY_WRITE_AZURE_CLIENT_ID"))
+	clientSecret := strings.TrimSpace(os.Getenv("DIRECTORY_WRITE_AZURE_CLIENT_SECRET"))
+
+	if tenantID == "" && clientID == "" && clientSecret == "" {
+		return nil, nil
+	}
+	if tenantID == "" || clientID == "" || clientSecret == "" {
+		return nil, fmt.Errorf("DIRECTORY_WRITE_AZURE_TENANT_ID, DIRECTORY_WRITE_AZURE_CLIENT_ID and DIRECTORY_WRITE_AZURE_CLIENT_SECRET must all be set to enable the aged-deleted-directory-object purge step")
+	}
+
+	cred, err := azidentity.NewClientSecretCredential(tenantID, clientID, clientSecret, nil)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create directory-write credential: %w", err)
+	}
+	return cred, nil
+}
+
 func parseWorkflowMode(raw string) (WorkflowMode, error) {
 	switch WorkflowMode(raw) {
 	case WorkflowRGOrdered:

tooling/cleanup-sweeper/cmd/root/options_test.go

@@ -179,3 +179,47 @@ func TestNewGraphCredential(t *testing.T) {
 		}
 	})
 }
+
+func TestNewDirectoryWriteCredential(t *testing.T) {
+	t.Run("returns nil when no DIRECTORY_WRITE_AZURE_* variables are set", func(t *testing.T) {
+		t.Setenv("DIRECTORY_WRITE_AZURE_TENANT_ID", "")
+		t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_ID", "")
+		t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_SECRET", "")
+
+		got, err := newDirectoryWriteCredential()
+		if err != nil {
+			t.Fatalf("expected no error, got %v", err)
+		}
+		if got != nil {
+			t.Fatalf("expected nil credential when unset, got %T", got)
+		}
+	})
+
+	t.Run("errors when DIRECTORY_WRITE_AZURE_* variables are partially set", func(t *testing.T) {
+		t.Setenv("DIRECTORY_WRITE_AZURE_TENANT_ID", "00000000-0000-0000-0000-000000000000")
+		t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_ID", "")
+		t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_SECRET", "secret")
+
+		_, err := newDirectoryWriteCredential()
+		if err == nil {
+			t.Fatalf("expected error for partial configuration")
+		}
+		if !strings.Contains(err.Error(), "must all be set") {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+
+	t.Run("builds a dedicated credential when all DIRECTORY_WRITE_AZURE_* variables are set", func(t *testing.T) {
+		t.Setenv("DIRECTORY_WRITE_AZURE_TENANT_ID", "00000000-0000-0000-0000-000000000000")
+		t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_ID", "11111111-1111-1111-1111-111111111111")
+		t.Setenv("DIRECTORY_WRITE_AZURE_CLIENT_SECRET", "secret")
+
+		got, err := newDirectoryWriteCredential()
+		if err != nil {
+			t.Fatalf("expected no error, got %v", err)
+		}
+		if got == nil {
+			t.Fatalf("expected a credential")
+		}
+	})
+}

tooling/cleanup-sweeper/cmd/workflow/shared/run.go

@@ -32,6 +32,12 @@ type RunOptions struct {
 	// GraphCredential is used exclusively for Microsoft Graph directory reads.
 	// When nil it defaults to AzureCredential.
 	GraphCredential azcore.TokenCredential
+	// DirectoryWriteCredential backs a second Graph client used only by the
+	// aged-deleted-directory-object purge step, which requires
+	// Directory.ReadWrite.All / Application.ReadWrite.All - materially higher
+	// privilege than GraphCredential needs. When nil, that step is omitted
+	// entirely rather than reusing GraphCredential or AzureCredential.
+	DirectoryWriteCredential azcore.TokenCredential
 
 	DryRun      bool
 	Wait        bool
@@ -51,6 +57,7 @@ func Run(ctx context.Context, opts RunOptions) error {
 		opts.SubscriptionID,
 		opts.AzureCredential,
 		opts.GraphCredential,
+		opts.DirectoryWriteCredential,
 		cleanupengine.WorkflowOptions{
 			DryRun:      opts.DryRun,
 			Wait:        opts.Wait,

tooling/cleanup-sweeper/pkg/engine/role_assignments_sweeper.go

@@ -25,13 +25,15 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources"
 
 	"github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/runner"
+	directoryobjectsteps "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/steps/directoryobjects"
 	kvsteps "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/steps/keyvault"
 	roleassignmentsteps "github.com/Azure/ARO-HCP/tooling/cleanup-sweeper/pkg/engine/steps/roleassignments"
 )
 
 const (
 	orphanedRoleAssignmentStepRetries = 3
 	orphanedVaultStepRetries          = 3
+	agedDeletedObjectStepRetries      = 3
 )
 
 // RoleAssignmentsSweeperWorkflow builds the shared-leftovers cleanup workflow.
@@ -40,11 +42,20 @@ const (
 // groups). graphCredential is used exclusively for the Microsoft Graph
 // directory reads performed by the orphaned role-assignment step; when nil it
 // defaults to credential, preserving single-identity behavior.
+//
+// directoryWriteCredential, when non-nil, backs a second Graph client used
+// only by the aged-deleted-directory-object purge step. That step permanently
+// deletes directory objects (Directory.ReadWrite.All / Application.ReadWrite.All),
+// a materially higher privilege than the read-only access graphCredential
+// needs, so the two are kept separate rather than reusing graphCredential.
+// When nil, the aged-deleted-directory-object purge step is omitted entirely -
+// it is opt-in, since most callers won't have an identity holding that grant.
 func RoleAssignmentsSweeperWorkflow(
 	_ context.Context,
 	subscriptionID string,
 	credential azcore.TokenCredential,
 	graphCredential azcore.TokenCredential,
+	directoryWriteCredential azcore.TokenCredential,
 	opts WorkflowOptions,
 ) (*runner.Engine, error) {
 	if strings.TrimSpace(subscriptionID) == "" {
@@ -85,26 +96,49 @@ func RoleAssignmentsSweeperWorkflow(
 		return resp.Success, nil
 	}
 
+	steps := []runner.Step{}
+
+	if directoryWriteCredential != nil {
+		directoryWriteGraphClient, err := roleassignmentsteps.NewGraphClient(directoryWriteCredential)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create directory-write graph client: %w", err)
+		}
+		// Runs before the orphaned-role-assignment step: purging an aged
+		// deletedItems object here means its role assignment is picked up as
+		// orphaned by that step in this very same run instead of waiting for
+		// Entra's 30-day recycle-bin timer.
+		steps = append(steps, directoryobjectsteps.MustNewPurgeAgedDeletedStep(directoryobjectsteps.PurgeAgedDeletedStepConfig{
+			RoleAssignmentsClient: roleAssignmentsClient,
+			GraphClient:           directoryWriteGraphClient,
+			SubscriptionID:        subscriptionID,
+			Name:                  "Purge aged deleted directory objects",
+			Retries:               agedDeletedObjectStepRetries,
+			ContinueOnError:       true,
+		}))
+	}
+
+	steps = append(steps,
+		roleassignmentsteps.MustNewDeleteOrphanedStep(roleassignmentsteps.DeleteOrphanedStepConfig{
+			RoleAssignmentsClient:       roleAssignmentsClient,
+			GraphClient:                 graphClient,
+			SubscriptionID:              subscriptionID,
+			Name:                        "Delete orphaned role assignments",
+			Retries:                     orphanedRoleAssignmentStepRetries,
+			ContinueOnTargetDeleteError: true,
+		}),
+		kvsteps.MustNewPurgeOrphanedDeletedStep(kvsteps.PurgeOrphanedDeletedStepConfig{
+			VaultsClient:        vaultsClient,
+			ResourceGroupExists: resourceGroupExists,
+			Name:                "Purge orphaned soft-deleted Key Vaults",
+			Retries:             orphanedVaultStepRetries,
+			ContinueOnError:     true,
+		}),
+	)
+
 	return &runner.Engine{
 		Parallelism: opts.Parallelism,
 		DryRun:      opts.DryRun,
 		Wait:        opts.Wait,
-		Steps: []runner.Step{
-			roleassignmentsteps.MustNewDeleteOrphanedStep(roleassignmentsteps.DeleteOrphanedStepConfig{
-				RoleAssignmentsClient:       roleAssignmentsClient,
-				GraphClient:                 graphClient,
-				SubscriptionID:              subscriptionID,
-				Name:                        "Delete orphaned role assignments",
-				Retries:                     orphanedRoleAssignmentStepRetries,
-				ContinueOnTargetDeleteError: true,
-			}),
-			kvsteps.MustNewPurgeOrphanedDeletedStep(kvsteps.PurgeOrphanedDeletedStepConfig{
-				VaultsClient:        vaultsClient,
-				ResourceGroupExists: resourceGroupExists,
-				Name:                "Purge orphaned soft-deleted Key Vaults",
-				Retries:             orphanedVaultStepRetries,
-				ContinueOnError:     true,
-			}),
-		},
+		Steps:       steps,
 	}, nil
 }