From 8bd9f96da937852b23878c0af14d2f949a1f2e07 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 04:09:55 +0100 Subject: [PATCH 1/2] fix(configure-azure): stop printing the new service principal's client secret The configure-azure wizard printed the freshly created Azure service principal's client secret to stdout with a bare fmt.Printf, then asked the operator to retype it. Under script/tee, CI logs or terminal scrollback that leaks a live credential holding the Reservations Administrator role. The wizard now hands the created credentials (tenant, appId, secret and the Step 2 subscription) straight to the Secrets Manager store step. Only the non-secret identifiers are printed, along with the name of the secret the credentials are stored in. If loading AWS config or storing fails after creation, the error explains how to mint a replacement secret ('az ad app credential reset') and re-run with --skip-setup. Closes #2049 (A10-028). Co-Authored-By: Claude Opus 5.5 --- cmd/configure_azure.go | 115 +++++++++++----- cmd/configure_azure_secret_test.go | 206 +++++++++++++++++++++++++++++ cmd/configure_azure_sp.go | 2 +- docs/cli/cloud-setup.md | 2 + 4 files changed, 290 insertions(+), 35 deletions(-) create mode 100644 cmd/configure_azure_secret_test.go diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 900fb345c..4a6caebcc 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -176,19 +176,23 @@ func runConfigureAzure(cmd *cobra.Command, args []string) error { fmt.Println("========================================================") fmt.Println() - // Run Azure CLI setup if not skipped + // Run Azure CLI setup if not skipped. When the wizard creates a service + // principal, its credentials are carried straight into the store step so + // the client secret never round-trips through the terminal. + var created *AzureCredentials if !azureOpts.SkipSetup { - if err := runAzureSetupCommands(ctx, reader); err != nil { + var err error + if created, err = runAzureSetupCommands(ctx, reader); err != nil { return err } } cfg, err := loadAWSConfigForAzure(ctx) if err != nil { - return err + return withAzureSPRecoveryHint(err, created) } - creds, err := collectAzureCredentials(reader) + creds, err := collectAzureCredentials(reader, created) if err != nil { return err } @@ -197,12 +201,15 @@ func runConfigureAzure(cmd *cobra.Command, args []string) error { store := NewAWSSecretsStore(smClient) if err := storeAzureCredentials(ctx, store, azureOpts.StackName, creds); err != nil { - return err + return withAzureSPRecoveryHint(err, created) } // Zero out sensitive data from memory azureOpts.ClientSecret = "" creds.ClientSecret = "" + if created != nil { + created.ClientSecret = "" + } log.Printf("Azure credentials stored successfully in Secrets Manager") fmt.Println("\nAzure configuration complete!") @@ -227,7 +234,15 @@ func loadAWSConfigForAzure(ctx context.Context) (aws.Config, error) { } // collectAzureCredentials collects Azure credentials interactively or from flags. -func collectAzureCredentials(reader *bufio.Reader) (AzureCredentials, error) { +// When created is non-nil (the wizard just created a service principal), its +// credentials are used as-is and nothing is prompted: the freshly generated +// client secret is never displayed, so the operator could not retype it anyway. +func collectAzureCredentials(reader *bufio.Reader, created *AzureCredentials) (AzureCredentials, error) { + if created != nil { + fmt.Println("\nUsing the service principal created in Step 3.") + return *created, nil + } + creds := AzureCredentials{ TenantID: azureOpts.TenantID, ClientID: azureOpts.ClientID, @@ -369,14 +384,17 @@ func listAzureSubscriptions(ctx context.Context) error { // (resolve the "Reservations Administrator" role definition and assign it at // subscription scope). This is the create-for-rbac equivalent and fails loud // on any SDK error. -func runAzureSetupCommands(ctx context.Context, reader *bufio.Reader) error { +// +// It returns the created service principal's credentials (nil when Step 3 was +// skipped) so the caller can store them without echoing the secret. +func runAzureSetupCommands(ctx context.Context, reader *bufio.Reader) (*AzureCredentials, error) { if err := azureStepLogin(reader); err != nil { - return err + return nil, err } subscriptionID, err := azureStepListSubscriptions(ctx, reader) if err != nil { - return err + return nil, err } return azureStepCreateServicePrincipal(ctx, reader, subscriptionID) @@ -459,11 +477,22 @@ func resolveAzureTenantID(ctx context.Context, subscriptionID string) (string, e return *resp.TenantID, nil } +// resolveAzureTenantIDFn and newAzureSPProvisionerFn are the cloud-facing +// constructors used by azureStepCreateServicePrincipal. They are variables so +// unit tests can substitute fakes and never reach Azure. +var ( + resolveAzureTenantIDFn = resolveAzureTenantID + newAzureSPProvisionerFn = func(subscriptionID string) (azureSPProvisioner, error) { + return newGraphSPProvisioner(subscriptionID) + } +) + // azureStepCreateServicePrincipal creates the service principal via the // Microsoft Graph + armauthorization SDKs (the create-for-rbac equivalent) and -// prints the resulting credential material. It fails loud: any SDK error is +// returns its credentials for direct storage (nil when the operator skips the +// step). The client secret is never printed. It fails loud: any SDK error is // returned rather than silently falling back to the CLI. -func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, subscriptionID string) error { +func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, subscriptionID string) (*AzureCredentials, error) { fmt.Println() fmt.Println("Step 3: Create Service Principal") fmt.Println("---------------------------------") @@ -475,7 +504,7 @@ func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, choice, err := readTrimmedLine(reader) if err != nil { - return fmt.Errorf("failed to read service-principal choice: %w", err) + return nil, fmt.Errorf("failed to read service-principal choice: %w", err) } choice = strings.ToLower(choice) if choice != "r" && choice != "run" && choice != "" { @@ -484,7 +513,7 @@ func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, fmt.Println("Provide the appId, client secret and tenant ID for an existing") fmt.Println("service principal in the next step.") fmt.Println() - return nil + return nil, nil } // The step budget must exceed roleAssignRetryBudget (3 min) plus the @@ -497,14 +526,14 @@ func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, spCtx, cancel := context.WithTimeout(ctx, 6*time.Minute) defer cancel() - tenantID, err := resolveAzureTenantID(spCtx, subscriptionID) + tenantID, err := resolveAzureTenantIDFn(spCtx, subscriptionID) if err != nil { - return err + return nil, err } - provisioner, err := newGraphSPProvisioner(subscriptionID) + provisioner, err := newAzureSPProvisionerFn(subscriptionID) if err != nil { - return fmt.Errorf("failed to initialize Azure SDK clients: %w\n"+ + return nil, fmt.Errorf("failed to initialize Azure SDK clients: %w\n"+ "Ensure you are authenticated: run 'az login' (Step 1) before continuing", err) } @@ -512,34 +541,52 @@ func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, fmt.Println(strings.Repeat("-", 60)) result, err := createAzureServicePrincipal(spCtx, provisioner, subscriptionID, tenantID) if err != nil { - return err + return nil, err } fmt.Println(strings.Repeat("-", 60)) - printAzureSPResult(result, subscriptionID) - return nil + printAzureSPResult(result, subscriptionID, azureOpts.StackName) + return &AzureCredentials{ + TenantID: result.TenantID, + ClientID: result.AppID, + ClientSecret: result.ClientSecret, + SubscriptionID: subscriptionID, + }, nil } -// printAzureSPResult prints the credential material in the same shape that -// "az ad sp create-for-rbac" prints, so the operator can feed it into the -// credential collection step that follows. -func printAzureSPResult(result azureSPResult, subscriptionID string) { +// printAzureSPResult prints the non-secret identifiers of the created service +// principal. The client secret is deliberately NOT printed (not even +// partially): stdout is routinely captured by script/tee, CI logs and terminal +// scrollback, and the wizard hands the secret straight to the Secrets Manager +// store step, so the operator never needs to see or retype it. +func printAzureSPResult(result azureSPResult, subscriptionID, stackName string) { fmt.Println() - fmt.Println("Service principal created. Credential material:") + fmt.Println("Service principal created:") fmt.Println() - fmt.Printf(" appId (Client ID): %s\n", result.AppID) - fmt.Printf(" password (Client Secret): %s\n", result.ClientSecret) - fmt.Printf(" tenant (Tenant ID): %s\n", result.TenantID) + fmt.Printf(" appId (Client ID): %s\n", result.AppID) + fmt.Println(" password (Client Secret): [hidden]") + fmt.Printf(" tenant (Tenant ID): %s\n", result.TenantID) + fmt.Printf(" Subscription ID: %s\n", subscriptionID) fmt.Println() - fmt.Println("IMPORTANT: copy the client secret now -- it cannot be retrieved later.") - fmt.Println("You'll enter these values in the next step:") - fmt.Println(" - appId -> Client ID") - fmt.Println(" - password -> Client Secret") - fmt.Println(" - tenant -> Tenant ID") - fmt.Printf(" - Subscription ID: %s\n", subscriptionID) + fmt.Printf("The client secret is not displayed. It will be stored directly in the\n") + fmt.Printf("AWS Secrets Manager secret %q (field client_secret).\n", stackName+"-AzureCredentials") fmt.Println() } +// withAzureSPRecoveryHint annotates a failure that happens after the wizard +// created a service principal but before its secret was stored. The secret was +// never displayed, so the operator is told how to mint a replacement instead. +func withAzureSPRecoveryHint(err error, created *AzureCredentials) error { + if created == nil { + return err + } + return fmt.Errorf("%w\nThe service principal (appId %s) was created but its client secret was NOT stored "+ + "and was never displayed. Fix the error above, then generate a new secret with "+ + "'az ad app credential reset --id %s' and re-run 'cudly configure-azure --skip-setup', "+ + "entering it at the hidden Client Secret prompt", + err, created.ClientID, created.ClientID) +} + // promptAndRunExplicitCommand shows a command and asks to run or skip. // Takes explicit program and args to avoid command injection via string splitting. func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd, program string, args ...string) error { diff --git a/cmd/configure_azure_secret_test.go b/cmd/configure_azure_secret_test.go new file mode 100644 index 000000000..eb71eac51 --- /dev/null +++ b/cmd/configure_azure_secret_test.go @@ -0,0 +1,206 @@ +package main + +import ( + "bufio" + "bytes" + "context" + "encoding/json" + "errors" + "io" + "log" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testSPSecret is the fake client secret returned by the mocked provisioner. +// It is long and distinctive so any partial leak is detectable. +const ( + testSPSecret = "Zq8~fakeSPsecret.DoNotPrint_7f3a9c2e1b4d6082" + testSPAppID = "99999999-8888-7777-6666-555555555555" +) + +// captureAllOutput runs fn with os.Stdout, os.Stderr and the standard logger +// redirected, and returns everything written to any of them. +func captureAllOutput(t *testing.T, fn func()) string { + t.Helper() + + origStdout, origStderr := os.Stdout, os.Stderr + origLogOut := log.Writer() + + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stdout, os.Stderr = w, w + log.SetOutput(w) + + done := make(chan string) + go func() { + var buf bytes.Buffer + _, _ = io.Copy(&buf, r) + done <- buf.String() + }() + + defer func() { + os.Stdout, os.Stderr = origStdout, origStderr + log.SetOutput(origLogOut) + }() + + fn() + + require.NoError(t, w.Close()) + out := <-done + require.NoError(t, r.Close()) + return out +} + +// assertSecretNotLeaked fails if the secret, or any 8-character window of it, +// appears in out. +func assertSecretNotLeaked(t *testing.T, out string) { + t.Helper() + require.NotContains(t, out, testSPSecret, "full client secret leaked to output") + for i := 0; i+8 <= len(testSPSecret); i++ { + require.NotContains(t, out, testSPSecret[i:i+8], "partial client secret leaked to output") + } +} + +// stubAzureSPConstructors replaces the cloud-facing constructors with fakes so +// no test reaches Azure, restoring them on cleanup. +func stubAzureSPConstructors(t *testing.T, p azureSPProvisioner) { + t.Helper() + origTenant, origProv := resolveAzureTenantIDFn, newAzureSPProvisionerFn + t.Cleanup(func() { + resolveAzureTenantIDFn, newAzureSPProvisionerFn = origTenant, origProv + }) + resolveAzureTenantIDFn = func(_ context.Context, _ string) (string, error) { + return testTenantID, nil + } + newAzureSPProvisionerFn = func(_ string) (azureSPProvisioner, error) { + return p, nil + } +} + +func newSecretTestProvisioner() *mockSPProvisioner { + return &mockSPProvisioner{ + appObjectID: "obj-1", + appID: testSPAppID, + secret: testSPSecret, + principalID: "principal-1", + roleDefID: "roledef-1", + } +} + +// TestAzureWizard_ClientSecretNeverPrinted drives the wizard's create-SP step, +// the credential collection step and the store step with a mocked Azure +// provisioner and a mock Secrets Manager. The secret must reach the store +// intact while never appearing on stdout, stderr or the logger. +func TestAzureWizard_ClientSecretNeverPrinted(t *testing.T) { + prov := newSecretTestProvisioner() + stubAzureSPConstructors(t, prov) + + origOpts := azureOpts + t.Cleanup(func() { azureOpts = origOpts }) + azureOpts = AzureConfigOptions{StackName: "my-cudly"} + + store := NewMockSecretsStore() + var created *AzureCredentials + var stepErr, collectErr, storeErr error + + out := captureAllOutput(t, func() { + created, stepErr = azureStepCreateServicePrincipal(context.Background(), + bufio.NewReader(strings.NewReader("r\n")), testSubID) + if stepErr != nil { + return + } + // Empty reader: any prompt would fail, proving nothing is re-asked. + var creds AzureCredentials + creds, collectErr = collectAzureCredentials(bufio.NewReader(strings.NewReader("")), created) + if collectErr != nil { + return + } + storeErr = storeAzureCredentials(context.Background(), store, azureOpts.StackName, creds) + }) + + require.NoError(t, stepErr) + require.NoError(t, collectErr) + require.NoError(t, storeErr) + require.NotNil(t, created) + + assertSecretNotLeaked(t, out) + assert.Contains(t, out, testSPAppID, "non-secret appId should still be shown") + assert.Contains(t, out, "[hidden]") + assert.Contains(t, out, "my-cudly-AzureCredentials", "operator should be told where the secret is stored") + + stored, ok := store.updatedSecrets["my-cudly-AzureCredentials"] + require.True(t, ok, "credentials must be written to Secrets Manager") + var got AzureCredentials + require.NoError(t, json.Unmarshal([]byte(stored), &got)) + assert.Equal(t, AzureCredentials{ + TenantID: testTenantID, + ClientID: testSPAppID, + ClientSecret: testSPSecret, + SubscriptionID: testSubID, + }, got) +} + +// TestPrintAzureSPResult_OmitsSecret pins the printer itself. +func TestPrintAzureSPResult_OmitsSecret(t *testing.T) { + out := captureAllOutput(t, func() { + printAzureSPResult(azureSPResult{ + AppID: testSPAppID, + ClientSecret: testSPSecret, + TenantID: testTenantID, + }, testSubID, "stack") + }) + assertSecretNotLeaked(t, out) + assert.Contains(t, out, testSPAppID) + assert.Contains(t, out, testTenantID) + assert.Contains(t, out, testSubID) +} + +// TestAzureWizard_SkipCreateSP_ReturnsNil checks that skipping Step 3 makes no +// Azure calls and leaves collection to the normal prompt/flag path. +func TestAzureWizard_SkipCreateSP_ReturnsNil(t *testing.T) { + stubAzureSPConstructors(t, nil) + newAzureSPProvisionerFn = func(_ string) (azureSPProvisioner, error) { + t.Fatal("provisioner must not be built when the step is skipped") + return nil, nil + } + + var created *AzureCredentials + var err error + captureAllOutput(t, func() { + created, err = azureStepCreateServicePrincipal(context.Background(), + bufio.NewReader(strings.NewReader("s\n")), testSubID) + }) + require.NoError(t, err) + assert.Nil(t, created) +} + +// TestAzureWizard_FailureAfterCreate_NoSecretInError covers the rollback path +// and the post-create recovery hint: neither error text nor output may carry +// the secret. +func TestAzureWizard_FailureAfterCreate_NoSecretInError(t *testing.T) { + prov := newSecretTestProvisioner() + prov.assignErr = errors.New("boom") + stubAzureSPConstructors(t, prov) + + var err error + out := captureAllOutput(t, func() { + _, err = azureStepCreateServicePrincipal(context.Background(), + bufio.NewReader(strings.NewReader("r\n")), testSubID) + }) + require.Error(t, err) + assertSecretNotLeaked(t, out+err.Error()) + + created := &AzureCredentials{ClientID: testSPAppID, ClientSecret: testSPSecret} + hinted := withAzureSPRecoveryHint(errors.New("store failed"), created) + assertSecretNotLeaked(t, hinted.Error()) + assert.Contains(t, hinted.Error(), "az ad app credential reset --id "+testSPAppID) + assert.ErrorContains(t, hinted, "store failed") + + plain := errors.New("plain") + assert.Same(t, plain, withAzureSPRecoveryHint(plain, nil)) +} diff --git a/cmd/configure_azure_sp.go b/cmd/configure_azure_sp.go index 6005911c1..9c45ce57f 100644 --- a/cmd/configure_azure_sp.go +++ b/cmd/configure_azure_sp.go @@ -77,7 +77,7 @@ const azureSPRoleName = "Reservations Administrator" // subsequent configure step. type azureSPResult struct { AppID string // application (client) ID - ClientSecret string // #nosec G117 -- generated SP password (client secret): SDK-produced, surfaced once to the interactive operator on stdout to copy (mirrors az ad sp create-for-rbac); never a hardcoded secret and never persisted to logs (verified) + ClientSecret string // #nosec G117 -- generated SP password (client secret): SDK-produced, handed directly to the Secrets Manager store step and never printed to stdout/stderr or logs (verified by TestAzureWizard_ClientSecretNeverPrinted); never a hardcoded secret TenantID string // Azure AD tenant ID } diff --git a/docs/cli/cloud-setup.md b/docs/cli/cloud-setup.md index 3ff847d1c..5415c89af 100644 --- a/docs/cli/cloud-setup.md +++ b/docs/cli/cloud-setup.md @@ -46,6 +46,8 @@ When `--skip-setup` is not set, the command runs an interactive guided flow: After the guided steps (or immediately with `--skip-setup`), the command prompts for or accepts any missing credential fields and writes them as JSON to the `-AzureCredentials` secret. +If step 3 creates the Service Principal, its credentials (including the subscription chosen in step 2) are written straight to the secret without prompting, and any credential flags are ignored. The client secret is never printed: the command shows only the appId, tenant ID and subscription ID, and names the secret it is stored in. If storing fails after the Service Principal was created, generate a new secret with `az ad app credential reset --id ` and re-run with `--skip-setup`. + ### Non-interactive usage ```bash From 7cf76afad2f6e54be52d8a7983f7d398c758e175 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 11:57:43 +0100 Subject: [PATCH 2/2] fix(configure-azure): check AWS config and credentials before creating the service principal Open the Secrets Manager store (load AWS config and resolve AWS credentials) before the wizard can create a service principal, so the most common failures happen before any Azure side effect and never orphan an SP whose undisplayed secret is lost. The recovery hint stays on the store step for failures that can still happen after creation. Extract configureAzure(ctx, reader) and make the store constructor a package variable so the command flow is unit-tested with mocks: AWS config failure makes zero Azure calls, a store failure after creation carries the appId recovery hint without the secret, and the end-to-end create-and-store path never prints the secret. Reword the manual-entry prompt, which referred to Service Principal output that is no longer shown. Co-Authored-By: Claude Opus 5.5 --- cmd/configure_azure.go | 57 +++++++++--- cmd/configure_azure_secret_test.go | 136 +++++++++++++++++++++++++++++ docs/cli/cloud-setup.md | 10 ++- 3 files changed, 186 insertions(+), 17 deletions(-) diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 4a6caebcc..f21a2f152 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -168,38 +168,41 @@ func storeAzureCredentials(ctx context.Context, store SecretsStore, stackName st return nil } -func runConfigureAzure(cmd *cobra.Command, args []string) error { - ctx := context.Background() - reader := bufio.NewReader(os.Stdin) +func runConfigureAzure(_ *cobra.Command, _ []string) error { + return configureAzure(context.Background(), bufio.NewReader(os.Stdin)) +} +// configureAzure runs the configure-azure flow, reading operator input from +// reader. +func configureAzure(ctx context.Context, reader *bufio.Reader) error { fmt.Println("Configure Azure Service Principal credentials for CUDly") fmt.Println("========================================================") fmt.Println() + // Open the Secrets Manager store (AWS config + credentials) BEFORE the + // wizard can create a service principal: these are the likeliest + // failures and have no side effects, so failing here never leaves an + // orphaned service principal whose undisplayed secret is lost. + store, err := openAzureSecretsStoreFn(ctx) + if err != nil { + return err + } + // Run Azure CLI setup if not skipped. When the wizard creates a service // principal, its credentials are carried straight into the store step so // the client secret never round-trips through the terminal. var created *AzureCredentials if !azureOpts.SkipSetup { - var err error if created, err = runAzureSetupCommands(ctx, reader); err != nil { return err } } - cfg, err := loadAWSConfigForAzure(ctx) - if err != nil { - return withAzureSPRecoveryHint(err, created) - } - creds, err := collectAzureCredentials(reader, created) if err != nil { return err } - smClient := secretsmanager.NewFromConfig(cfg) - store := NewAWSSecretsStore(smClient) - if err := storeAzureCredentials(ctx, store, azureOpts.StackName, creds); err != nil { return withAzureSPRecoveryHint(err, created) } @@ -218,6 +221,30 @@ func runConfigureAzure(cmd *cobra.Command, args []string) error { return nil } +// openAzureSecretsStoreFn builds the Secrets Manager store used by +// configureAzure. It is a variable so unit tests can substitute a mock and +// never reach AWS. +var openAzureSecretsStoreFn = openAzureSecretsStore + +// openAzureSecretsStore loads the AWS config and resolves AWS credentials, then +// returns a Secrets Manager store. Resolving credentials up front makes a +// missing or expired AWS login fail before any Azure resource is created. +func openAzureSecretsStore(ctx context.Context) (SecretsStore, error) { + cfg, err := loadAWSConfigForAzure(ctx) + if err != nil { + return nil, err + } + if cfg.Credentials == nil { + return nil, fmt.Errorf("failed to resolve AWS credentials: no credential provider configured") + } + credCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + if _, err := cfg.Credentials.Retrieve(credCtx); err != nil { + return nil, fmt.Errorf("failed to resolve AWS credentials: %w", err) + } + return NewAWSSecretsStore(secretsmanager.NewFromConfig(cfg)), nil +} + // loadAWSConfigForAzure loads AWS configuration with optional profile. func loadAWSConfigForAzure(ctx context.Context) (aws.Config, error) { var opts []func(*awsconfig.LoadOptions) error @@ -255,7 +282,7 @@ func collectAzureCredentials(reader *bufio.Reader, created *AzureCredentials) (A return creds, nil } - fmt.Println("\nEnter the credentials from the Service Principal output above:") + fmt.Println("\nEnter the credentials of an existing Azure Service Principal:") fmt.Println() if err := promptForAzureCredentialFields(reader, &creds); err != nil { @@ -574,7 +601,9 @@ func printAzureSPResult(result azureSPResult, subscriptionID, stackName string) } // withAzureSPRecoveryHint annotates a failure that happens after the wizard -// created a service principal but before its secret was stored. The secret was +// created a service principal but before its secret was stored (AWS config +// and credentials are checked before creation, so this covers the remaining +// store-time failures such as ListSecrets/UpdateSecret errors). The secret was // never displayed, so the operator is told how to mint a replacement instead. func withAzureSPRecoveryHint(err error, created *AzureCredentials) error { if created == nil { diff --git a/cmd/configure_azure_secret_test.go b/cmd/configure_azure_secret_test.go index eb71eac51..4888e464a 100644 --- a/cmd/configure_azure_secret_test.go +++ b/cmd/configure_azure_secret_test.go @@ -204,3 +204,139 @@ func TestAzureWizard_FailureAfterCreate_NoSecretInError(t *testing.T) { plain := errors.New("plain") assert.Same(t, plain, withAzureSPRecoveryHint(plain, nil)) } + +// wizardCreateSPInput drives configureAzure through the wizard: skip az login, +// skip the subscription listing, enter the subscription, run Step 3. +const wizardCreateSPInput = "s\ns\n" + testSubID + "\nr\n" + +// stubAzureSecretsStore replaces the AWS-facing store constructor, restoring +// it on cleanup. It returns a pointer to the number of times it was called. +func stubAzureSecretsStore(t *testing.T, store SecretsStore, err error) *int { + t.Helper() + orig := openAzureSecretsStoreFn + t.Cleanup(func() { openAzureSecretsStoreFn = orig }) + calls := 0 + openAzureSecretsStoreFn = func(_ context.Context) (SecretsStore, error) { + calls++ + if err != nil { + return nil, err + } + return store, nil + } + return &calls +} + +// stubCountingAzureSPConstructors is stubAzureSPConstructors plus counters for +// the tenant lookup and provisioner construction. +func stubCountingAzureSPConstructors(t *testing.T, p azureSPProvisioner) (tenantCalls, provCalls *int) { + t.Helper() + stubAzureSPConstructors(t, p) + var tc, pc int + resolveAzureTenantIDFn = func(_ context.Context, _ string) (string, error) { + tc++ + return testTenantID, nil + } + newAzureSPProvisionerFn = func(_ string) (azureSPProvisioner, error) { + pc++ + return p, nil + } + return &tc, &pc +} + +func setAzureOptsForTest(t *testing.T, opts AzureConfigOptions) { + t.Helper() + orig := azureOpts + t.Cleanup(func() { azureOpts = orig }) + azureOpts = opts +} + +// TestConfigureAzure_AWSConfigFailure_BeforeSPCreation pins the ordering fix: +// an AWS config/credentials failure must stop the command before any Azure +// call, so no service principal is created and no recovery hint is needed. +func TestConfigureAzure_AWSConfigFailure_BeforeSPCreation(t *testing.T) { + prov := newSecretTestProvisioner() + tenantCalls, provCalls := stubCountingAzureSPConstructors(t, prov) + storeCalls := stubAzureSecretsStore(t, nil, errors.New("failed to load AWS config: fake missing profile")) + setAzureOptsForTest(t, AzureConfigOptions{StackName: "my-cudly"}) + + var err error + out := captureAllOutput(t, func() { + err = configureAzure(context.Background(), bufio.NewReader(strings.NewReader(wizardCreateSPInput))) + }) + + require.Error(t, err) + assert.ErrorContains(t, err, "failed to load AWS config") + assert.Equal(t, 1, *storeCalls) + assert.Zero(t, *tenantCalls, "tenant must not be resolved before AWS config loads") + assert.Zero(t, *provCalls, "Graph provisioner must not be built before AWS config loads") + assert.Empty(t, prov.createAppName, "no application may be created") + assert.Empty(t, prov.addPasswordObjID, "no secret may be minted") + assert.NotContains(t, err.Error(), "az ad app credential reset", "nothing was created, so no recovery hint") + assert.NotContains(t, out, "Step 1", "the wizard must not start") +} + +// TestConfigureAzure_StoreFailureAfterCreate_HasRecoveryHint pins the wiring +// of withAzureSPRecoveryHint in configureAzure: a store failure after the SP +// was created must name the appId and the reset command, never the secret. +func TestConfigureAzure_StoreFailureAfterCreate_HasRecoveryHint(t *testing.T) { + prov := newSecretTestProvisioner() + stubCountingAzureSPConstructors(t, prov) + store := NewMockSecretsStore() + store.updateSecretFunc = func(_ context.Context, _, _ string) error { + return errors.New("AccessDeniedException: fake denial") + } + stubAzureSecretsStore(t, store, nil) + setAzureOptsForTest(t, AzureConfigOptions{StackName: "my-cudly"}) + + var err error + out := captureAllOutput(t, func() { + err = configureAzure(context.Background(), bufio.NewReader(strings.NewReader(wizardCreateSPInput))) + }) + + require.Error(t, err) + require.Equal(t, azureSPName, prov.createAppName, "the SP must have been created for this scenario") + assert.ErrorContains(t, err, "failed to store credentials in Secrets Manager") + assert.ErrorContains(t, err, "AccessDeniedException: fake denial") + assert.ErrorContains(t, err, "appId "+testSPAppID) + assert.ErrorContains(t, err, "az ad app credential reset --id "+testSPAppID) + assertSecretNotLeaked(t, out+err.Error()) +} + +// TestConfigureAzure_CreateAndStore_EndToEnd drives the whole command with +// mocks: the created SP's credentials are stored and never printed. +func TestConfigureAzure_CreateAndStore_EndToEnd(t *testing.T) { + prov := newSecretTestProvisioner() + stubCountingAzureSPConstructors(t, prov) + store := NewMockSecretsStore() + stubAzureSecretsStore(t, store, nil) + setAzureOptsForTest(t, AzureConfigOptions{StackName: "my-cudly"}) + + var err error + out := captureAllOutput(t, func() { + err = configureAzure(context.Background(), bufio.NewReader(strings.NewReader(wizardCreateSPInput))) + }) + + require.NoError(t, err) + assertSecretNotLeaked(t, out) + var got AzureCredentials + require.NoError(t, json.Unmarshal([]byte(store.updatedSecrets["my-cudly-AzureCredentials"]), &got)) + assert.Equal(t, testSPAppID, got.ClientID) + assert.Equal(t, testSPSecret, got.ClientSecret) + assert.Equal(t, testSubID, got.SubscriptionID) +} + +// TestCollectAzureCredentials_PromptWording checks that the manual-entry +// prompt does not refer to Service Principal output that was never shown. +func TestCollectAzureCredentials_PromptWording(t *testing.T) { + setAzureOptsForTest(t, AzureConfigOptions{StackName: "my-cudly", ClientSecret: "placeholder"}) + + var err error + out := captureAllOutput(t, func() { + _, err = collectAzureCredentials(bufio.NewReader(strings.NewReader( + testTenantID+"\n"+testSPAppID+"\n"+testSubID+"\n")), nil) + }) + + require.NoError(t, err) + assert.Contains(t, out, "Enter the credentials of an existing Azure Service Principal:") + assert.NotContains(t, out, "output above") +} diff --git a/docs/cli/cloud-setup.md b/docs/cli/cloud-setup.md index 5415c89af..710dc5e70 100644 --- a/docs/cli/cloud-setup.md +++ b/docs/cli/cloud-setup.md @@ -38,7 +38,9 @@ cudly configure-azure [flags] ### What it does -When `--skip-setup` is not set, the command runs an interactive guided flow: +The command first loads the AWS config and resolves AWS credentials, and stops there if either fails, before any Azure resource is created. + +When `--skip-setup` is not set, the command then runs an interactive guided flow: 1. **`az login`** - opens a browser window for Azure authentication (can be skipped at the prompt). 2. **`az account list --output table`** - lists subscriptions so you can identify the Subscription ID. @@ -46,7 +48,7 @@ When `--skip-setup` is not set, the command runs an interactive guided flow: After the guided steps (or immediately with `--skip-setup`), the command prompts for or accepts any missing credential fields and writes them as JSON to the `-AzureCredentials` secret. -If step 3 creates the Service Principal, its credentials (including the subscription chosen in step 2) are written straight to the secret without prompting, and any credential flags are ignored. The client secret is never printed: the command shows only the appId, tenant ID and subscription ID, and names the secret it is stored in. If storing fails after the Service Principal was created, generate a new secret with `az ad app credential reset --id ` and re-run with `--skip-setup`. +If step 3 creates the Service Principal, its credentials (including the subscription chosen in step 2) are written straight to the secret without prompting, and any credential flags are ignored. The client secret is never printed: the command shows only the appId, tenant ID and subscription ID, and names the secret it is stored in. If storing still fails after the Service Principal was created (for example, Secrets Manager rejects the write), generate a new secret with `az ad app credential reset --id ` and re-run with `--skip-setup`. ### Non-interactive usage @@ -103,7 +105,9 @@ cudly configure-gcp [flags] ### What it does -When `--skip-setup` is not set, the command runs an interactive guided flow: +The command first loads the AWS config and resolves AWS credentials, and stops there if either fails, before any Azure resource is created. + +When `--skip-setup` is not set, the command then runs an interactive guided flow: 1. **`gcloud auth login`** - opens a browser window for GCP authentication. 2. **`gcloud projects list`** - lists projects so you can identify your Project ID.