fix(configure): address CodeRabbit findings on SDK migration

Resolve the 7 Major CodeRabbit findings on PR #1279 at root cause.

1. azure sanity: guard sub.State before dereferencing it. State is an
   optional pointer; build azureSubscriptionInfo with the same nil-check
   pattern used for the other optional fields, so an omitted state cannot
   panic.

2. azure wizard: bind to the Azure CLI session explicitly. The wizard now
   builds credentials via a shared newAzureWizardCredential() backed by
   azidentity.NewAzureCLICredential, instead of DefaultAzureCredential whose
   chain prioritizes environment / workload / managed identity and could pick
   a different principal than the operator's "az login". The CI sanity test
   keeps DefaultAzureCredential (it must resolve the CI service-principal env
   vars). listAzureSubscriptions, resolveAzureTenantID and the SP provisioner
   all use the CLI credential.

3. gcp: bound every SDK helper with a 60s timeout. listGCPProjects,
   createGCPServiceAccount, grantGCPIAMRole and createGCPServiceAccountKey
   inherited context.Background() and could hang; each now derives a
   context.WithTimeout(ctx, gcpSDKCallTimeout).

4. gcp: preserve conditional IAM bindings. GetIamPolicy now requests
   RequestedPolicyVersion 3 and the policy is written back at Version 3, so a
   read-modify-write no longer silently drops condition bindings. Binding
   mutation is extracted into addMemberToPolicyBinding to keep complexity in
   check.

5. gcp: prevent orphaned service-account keys. createGCPServiceAccountKey
   reserves the destination file with O_EXCL before minting the remote key,
   and deletes the newly created remote key if decode/write fails, so a local
   failure cannot leave an active unused credential behind.

6. gcp: return an empty key path when no key was written. gcpStepCreateKey
   returns the key file path only after a successful write; on skip or an
   unknown choice it returns "" so getGCPCredentialsFilePath prompts for an
   existing credentials file instead of loading a missing one.

7. azure SP: roll back partial creation. createAzureServicePrincipal now
   deletes the just-created application (cascading to its password credential
   and derived service principal) if any later step -- add-password, SP
   create, role resolve, or role assign -- fails. A new DeleteApplication
   provisioner method backs this; if the compensating delete also fails the
   error names the orphaned app so the operator can remove it by hand. Unit
   tests assert rollback fires for each post-create failure, not on success,
   and that a failed rollback is surfaced.

Author: cristim

cmd/configure_azure.go

@@ -16,6 +16,7 @@ import (
 
 	"time"
 
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions"
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -272,14 +273,29 @@ func promptForAzureCredentialFields(reader *bufio.Reader, creds *AzureCredential
 	return nil
 }
 
+// newAzureWizardCredential builds the credential used by the interactive Azure
+// setup wizard. It binds explicitly to the Azure CLI session (the "az login"
+// the operator runs in Step 1) via AzureCLICredential rather than
+// DefaultAzureCredential, whose chain prioritizes environment / workload /
+// managed-identity credentials and could otherwise resolve to a different
+// principal than the one the operator just signed in as.
+func newAzureWizardCredential() (azcore.TokenCredential, error) {
+	cred, err := azidentity.NewAzureCLICredential(nil)
+	if err != nil {
+		return nil, fmt.Errorf("failed to build Azure CLI credential: %w\n"+
+			"Ensure you are authenticated: run 'az login' (Step 1) before continuing", err)
+	}
+	return cred, nil
+}
+
 // listAzureSubscriptions retrieves the operator's subscriptions via the ARM
 // Subscriptions SDK and prints them in a table matching "az account list"
-// output. DefaultAzureCredential picks up the "az login" session via its
-// AzureCLICredential leg so the operator's active session is reused.
+// output. It uses the Azure CLI credential so the listing matches the
+// operator's active "az login" session.
 func listAzureSubscriptions(ctx context.Context) error {
-	cred, err := azidentity.NewDefaultAzureCredential(nil)
+	cred, err := newAzureWizardCredential()
 	if err != nil {
-		return fmt.Errorf("failed to build credential: %w", err)
+		return err
 	}
 
 	client, err := armsubscriptions.NewClient(cred, nil)
@@ -322,10 +338,9 @@ func listAzureSubscriptions(ctx context.Context) error {
 // SDK on behalf of a human operator who does not yet have a credential. This
 // is the only CLI call retained in this wizard.
 //
-// Step 2 (list subscriptions): performed via the ARM Subscriptions SDK.
-// DefaultAzureCredential automatically picks up the session that "az login"
-// just established (via its AzureCLICredential leg). Fails loud if the SDK
-// cannot authenticate (no CLI fallback).
+// Step 2 (list subscriptions): performed via the ARM Subscriptions SDK using
+// the Azure CLI credential, which reuses the session that "az login" just
+// established. Fails loud if the SDK cannot authenticate (no CLI fallback).
 //
 // Step 3 (create service principal): performed via the Microsoft Graph SDK
 // (application + service principal + password credential) and armauthorization
@@ -356,13 +371,13 @@ func azureStepLogin(reader *bufio.Reader) error {
 
 // azureStepListSubscriptions lists subscriptions via SDK and prompts the
 // operator to enter their subscription ID. It fails loud (no CLI fallback): if
-// DefaultAzureCredential cannot authenticate, it returns an error instructing
-// the operator to run "az login" first.
+// the Azure CLI credential cannot authenticate, it returns an error
+// instructing the operator to run "az login" first.
 func azureStepListSubscriptions(ctx context.Context, reader *bufio.Reader) (string, error) {
 	fmt.Println()
 	fmt.Println("Step 2: Get Subscription ID")
 	fmt.Println("---------------------------")
-	fmt.Println("Listing your Azure subscriptions via SDK (DefaultAzureCredential)...")
+	fmt.Println("Listing your Azure subscriptions via SDK (Azure CLI credential)...")
 	fmt.Println()
 
 	listCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
@@ -390,12 +405,11 @@ func azureStepListSubscriptions(ctx context.Context, reader *bufio.Reader) (stri
 }
 
 // resolveAzureTenantID looks up the tenant ID for the given subscription via
-// the ARM Subscriptions SDK. DefaultAzureCredential reuses the "az login"
-// session.
+// the ARM Subscriptions SDK, using the Azure CLI ("az login") credential.
 func resolveAzureTenantID(ctx context.Context, subscriptionID string) (string, error) {
-	cred, err := azidentity.NewDefaultAzureCredential(nil)
+	cred, err := newAzureWizardCredential()
 	if err != nil {
-		return "", fmt.Errorf("failed to build credential: %w", err)
+		return "", err
 	}
 	client, err := armsubscriptions.NewClient(cred, nil)
 	if err != nil {

cmd/configure_azure_sp.go

@@ -3,8 +3,8 @@ package main
 import (
 	"context"
 	"fmt"
+	"time"
 
-	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
 	armauthorization "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2"
 	"github.com/google/uuid"
 	msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go"
@@ -52,6 +52,11 @@ type azureSPProvisioner interface {
 	// AssignRole creates a role assignment binding principalID to
 	// roleDefinitionID at the given scope.
 	AssignRole(ctx context.Context, scope, principalID, roleDefinitionID string) error
+	// DeleteApplication deletes the application registration identified by
+	// objectID. Deleting the application also removes its password credentials
+	// and the service principal created from it, so it serves as the
+	// compensating action for a partially completed creation flow.
+	DeleteApplication(ctx context.Context, objectID string) error
 }
 
 // createAzureServicePrincipal performs the full create-for-rbac equivalent:
@@ -73,23 +78,38 @@ func createAzureServicePrincipal(ctx context.Context, p azureSPProvisioner, subs
 		return azureSPResult{}, fmt.Errorf("failed to create application registration: %w", err)
 	}
 
+	// rollback deletes the just-created application (which cascades to its
+	// password credentials and the derived service principal) so a failure in
+	// a later step does not orphan Azure AD objects. The cleanup runs on a
+	// fresh context in case the parent is already canceled/expired. If the
+	// cleanup itself fails, the operator is told exactly what to delete by hand.
+	rollback := func(cause error) (azureSPResult, error) {
+		cleanupCtx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
+		defer cancel()
+		if delErr := p.DeleteApplication(cleanupCtx, objectID); delErr != nil {
+			return azureSPResult{}, fmt.Errorf("%w; additionally failed to roll back application %q (appId %s) -- delete it manually: %w",
+				cause, objectID, appID, delErr)
+		}
+		return azureSPResult{}, fmt.Errorf("%w (rolled back: deleted application %q)", cause, objectID)
+	}
+
 	secret, err := p.AddPassword(ctx, objectID)
 	if err != nil {
-		return azureSPResult{}, fmt.Errorf("failed to add password credential: %w", err)
+		return rollback(fmt.Errorf("failed to add password credential: %w", err))
 	}
 
 	principalID, err := p.CreateServicePrincipal(ctx, appID)
 	if err != nil {
-		return azureSPResult{}, fmt.Errorf("failed to create service principal: %w", err)
+		return rollback(fmt.Errorf("failed to create service principal: %w", err))
 	}
 
 	roleDefID, err := p.ResolveRoleDefinitionID(ctx, scope, azureSPRoleName)
 	if err != nil {
-		return azureSPResult{}, fmt.Errorf("failed to resolve %q role definition: %w", azureSPRoleName, err)
+		return rollback(fmt.Errorf("failed to resolve %q role definition: %w", azureSPRoleName, err))
 	}
 
 	if err := p.AssignRole(ctx, scope, principalID, roleDefID); err != nil {
-		return azureSPResult{}, fmt.Errorf("failed to assign %q role at %s: %w", azureSPRoleName, scope, err)
+		return rollback(fmt.Errorf("failed to assign %q role at %s: %w", azureSPRoleName, scope, err))
 	}
 
 	return azureSPResult{
@@ -108,15 +128,15 @@ type graphSPProvisioner struct {
 	roleAsgn *armauthorization.RoleAssignmentsClient
 }
 
-// newGraphSPProvisioner builds a graphSPProvisioner authenticated with
-// DefaultAzureCredential. The credential chain includes AzureCLICredential, so
-// the session established by "az login" is reused (no separate auth step).
+// newGraphSPProvisioner builds a graphSPProvisioner authenticated with the
+// Azure CLI credential, so the session established by "az login" (wizard
+// Step 1) is reused -- matching the principal used by the rest of the wizard.
 // subscriptionID seeds the RoleAssignmentsClient; the actual scope is passed
 // per-call to its Create method.
 func newGraphSPProvisioner(subscriptionID string) (*graphSPProvisioner, error) {
-	cred, err := azidentity.NewDefaultAzureCredential(nil)
+	cred, err := newAzureWizardCredential()
 	if err != nil {
-		return nil, fmt.Errorf("failed to build DefaultAzureCredential: %w", err)
+		return nil, err
 	}
 
 	graph, err := msgraphsdk.NewGraphServiceClientWithCredentials(
@@ -187,6 +207,10 @@ func (g *graphSPProvisioner) CreateServicePrincipal(ctx context.Context, appID s
 	return *principalID, nil
 }
 
+func (g *graphSPProvisioner) DeleteApplication(ctx context.Context, objectID string) error {
+	return g.graph.Applications().ByApplicationId(objectID).Delete(ctx, nil)
+}
+
 func (g *graphSPProvisioner) ResolveRoleDefinitionID(ctx context.Context, scope, roleName string) (string, error) {
 	filter := fmt.Sprintf("roleName eq '%s'", roleName)
 	pager := g.roleDefs.NewListPager(scope, &armauthorization.RoleDefinitionsClientListOptions{

cmd/configure_azure_sp_test.go

@@ -13,6 +13,14 @@ import (
 // createAzureServicePrincipal requests the correct app name, role and scope,
 // and surfaces the generated secret.
 type mockSPProvisioner struct {
+	// optional injected errors
+	createAppErr error
+	addPwErr     error
+	createSPErr  error
+	resolveErr   error
+	assignErr    error
+	deleteAppErr error
+
 	// captured inputs
 	createAppName     string
 	addPasswordObjID  string
@@ -22,6 +30,7 @@ type mockSPProvisioner struct {
 	assignScope       string
 	assignPrincipalID string
 	assignRoleDefID   string
+	deleteAppObjID    string
 
 	// canned outputs
 	appObjectID string
@@ -30,16 +39,10 @@ type mockSPProvisioner struct {
 	principalID string
 	roleDefID   string
 
-	// optional injected errors
-	createAppErr error
-	addPwErr     error
-	createSPErr  error
-	resolveErr   error
-	assignErr    error
-
 	// call flags
 	resolveRoleCalled bool
 	assignRoleCalled  bool
+	deleteAppCalled   bool
 }
 
 func (m *mockSPProvisioner) CreateApplication(_ context.Context, displayName string) (string, string, error) {
@@ -84,6 +87,12 @@ func (m *mockSPProvisioner) AssignRole(_ context.Context, scope, principalID, ro
 	return m.assignErr
 }
 
+func (m *mockSPProvisioner) DeleteApplication(_ context.Context, objectID string) error {
+	m.deleteAppCalled = true
+	m.deleteAppObjID = objectID
+	return m.deleteAppErr
+}
+
 const (
 	testSubID    = "11111111-2222-3333-4444-555555555555"
 	testTenantID = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
@@ -127,6 +136,9 @@ func TestCreateAzureServicePrincipal_Success(t *testing.T) {
 	assert.Equal(t, "app-client-id", result.AppID)
 	assert.Equal(t, "super-secret-password", result.ClientSecret)
 	assert.Equal(t, testTenantID, result.TenantID)
+
+	// No rollback on success.
+	assert.False(t, m.deleteAppCalled, "DeleteApplication must not be called on success")
 }
 
 func TestCreateAzureServicePrincipal_ErrorPropagation(t *testing.T) {
@@ -136,38 +148,44 @@ func TestCreateAzureServicePrincipal_ErrorPropagation(t *testing.T) {
 		wantErrPart   string
 		wantNoAssign  bool
 		wantNoResolve bool
+		wantRollback  bool // DeleteApplication should be called to clean up
 	}{
 		{
 			name:          "create application fails",
 			setup:         func(m *mockSPProvisioner) { m.createAppErr = errors.New("graph 403") },
 			wantErrPart:   "failed to create application registration",
 			wantNoAssign:  true,
 			wantNoResolve: true,
+			wantRollback:  false, // nothing was created, nothing to roll back
 		},
 		{
 			name:          "add password fails",
 			setup:         func(m *mockSPProvisioner) { m.addPwErr = errors.New("graph addPassword 400") },
 			wantErrPart:   "failed to add password credential",
 			wantNoAssign:  true,
 			wantNoResolve: true,
+			wantRollback:  true,
 		},
 		{
 			name:          "create service principal fails",
 			setup:         func(m *mockSPProvisioner) { m.createSPErr = errors.New("graph sp 409") },
 			wantErrPart:   "failed to create service principal",
 			wantNoAssign:  true,
 			wantNoResolve: true,
+			wantRollback:  true,
 		},
 		{
 			name:         "resolve role fails",
 			setup:        func(m *mockSPProvisioner) { m.resolveErr = errors.New("role not found") },
 			wantErrPart:  "failed to resolve",
 			wantNoAssign: true,
+			wantRollback: true,
 		},
 		{
-			name:        "assign role fails",
-			setup:       func(m *mockSPProvisioner) { m.assignErr = errors.New("rbac 403") },
-			wantErrPart: "failed to assign",
+			name:         "assign role fails",
+			setup:        func(m *mockSPProvisioner) { m.assignErr = errors.New("rbac 403") },
+			wantErrPart:  "failed to assign",
+			wantRollback: true,
 		},
 	}
 
@@ -192,10 +210,38 @@ func TestCreateAzureServicePrincipal_ErrorPropagation(t *testing.T) {
 			if tt.wantNoAssign {
 				assert.False(t, m.assignRoleCalled, "AssignRole should not have been called")
 			}
+			assert.Equal(t, tt.wantRollback, m.deleteAppCalled,
+				"DeleteApplication call expectation mismatch")
+			if tt.wantRollback {
+				assert.Equal(t, "app-object-id", m.deleteAppObjID,
+					"rollback should delete the created application by object ID")
+			}
 		})
 	}
 }
 
+// TestCreateAzureServicePrincipal_RollbackFailureSurfaced verifies that when
+// the compensating delete also fails, the error names the orphaned application
+// so the operator can delete it manually.
+func TestCreateAzureServicePrincipal_RollbackFailureSurfaced(t *testing.T) {
+	m := &mockSPProvisioner{
+		appObjectID:  "app-object-id",
+		appID:        "app-client-id",
+		secret:       "secret",
+		principalID:  "sp-principal-id",
+		roleDefID:    "role-def-id",
+		assignErr:    errors.New("rbac 403"),
+		deleteAppErr: errors.New("delete 500"),
+	}
+
+	_, err := createAzureServicePrincipal(context.Background(), m, testSubID, testTenantID)
+	require.Error(t, err)
+	assert.True(t, m.deleteAppCalled)
+	assert.Contains(t, err.Error(), "failed to assign")
+	assert.Contains(t, err.Error(), "failed to roll back")
+	assert.Contains(t, err.Error(), "app-object-id")
+}
+
 func TestCreateAzureServicePrincipal_ScopeFormat(t *testing.T) {
 	m := &mockSPProvisioner{
 		appObjectID: "o", appID: "a", secret: "s", principalID: "p", roleDefID: "r",