Skip to content

Commit 45f5920

Browse files
IAmJSDclaude
andauthored
fix(configure-azure): stop printing the new service principal's client secret (A10-028) (#2212)
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 62e08ea commit 45f5920

4 files changed

Lines changed: 472 additions & 48 deletions

File tree

‎cmd/configure_azure.go‎

Lines changed: 121 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -168,41 +168,51 @@ func storeAzureCredentials(ctx context.Context, store SecretsStore, stackName st
168168
return nil
169169
}
170170

171-
func runConfigureAzure(cmd *cobra.Command, args []string) error {
172-
ctx := context.Background()
173-
reader := bufio.NewReader(os.Stdin)
171+
func runConfigureAzure(_ *cobra.Command, _ []string) error {
172+
return configureAzure(context.Background(), bufio.NewReader(os.Stdin))
173+
}
174174

175+
// configureAzure runs the configure-azure flow, reading operator input from
176+
// reader.
177+
func configureAzure(ctx context.Context, reader *bufio.Reader) error {
175178
fmt.Println("Configure Azure Service Principal credentials for CUDly")
176179
fmt.Println("========================================================")
177180
fmt.Println()
178181

179-
// Run Azure CLI setup if not skipped
182+
// Open the Secrets Manager store (AWS config + credentials) BEFORE the
183+
// wizard can create a service principal: these are the likeliest
184+
// failures and have no side effects, so failing here never leaves an
185+
// orphaned service principal whose undisplayed secret is lost.
186+
store, err := openAzureSecretsStoreFn(ctx)
187+
if err != nil {
188+
return err
189+
}
190+
191+
// Run Azure CLI setup if not skipped. When the wizard creates a service
192+
// principal, its credentials are carried straight into the store step so
193+
// the client secret never round-trips through the terminal.
194+
var created *AzureCredentials
180195
if !azureOpts.SkipSetup {
181-
if err := runAzureSetupCommands(ctx, reader); err != nil {
196+
if created, err = runAzureSetupCommands(ctx, reader); err != nil {
182197
return err
183198
}
184199
}
185200

186-
cfg, err := loadAWSConfigForAzure(ctx)
187-
if err != nil {
188-
return err
189-
}
190-
191-
creds, err := collectAzureCredentials(reader)
201+
creds, err := collectAzureCredentials(reader, created)
192202
if err != nil {
193203
return err
194204
}
195205

196-
smClient := secretsmanager.NewFromConfig(cfg)
197-
store := NewAWSSecretsStore(smClient)
198-
199206
if err := storeAzureCredentials(ctx, store, azureOpts.StackName, creds); err != nil {
200-
return err
207+
return withAzureSPRecoveryHint(err, created)
201208
}
202209

203210
// Zero out sensitive data from memory
204211
azureOpts.ClientSecret = ""
205212
creds.ClientSecret = ""
213+
if created != nil {
214+
created.ClientSecret = ""
215+
}
206216

207217
log.Printf("Azure credentials stored successfully in Secrets Manager")
208218
fmt.Println("\nAzure configuration complete!")
@@ -211,6 +221,30 @@ func runConfigureAzure(cmd *cobra.Command, args []string) error {
211221
return nil
212222
}
213223

224+
// openAzureSecretsStoreFn builds the Secrets Manager store used by
225+
// configureAzure. It is a variable so unit tests can substitute a mock and
226+
// never reach AWS.
227+
var openAzureSecretsStoreFn = openAzureSecretsStore
228+
229+
// openAzureSecretsStore loads the AWS config and resolves AWS credentials, then
230+
// returns a Secrets Manager store. Resolving credentials up front makes a
231+
// missing or expired AWS login fail before any Azure resource is created.
232+
func openAzureSecretsStore(ctx context.Context) (SecretsStore, error) {
233+
cfg, err := loadAWSConfigForAzure(ctx)
234+
if err != nil {
235+
return nil, err
236+
}
237+
if cfg.Credentials == nil {
238+
return nil, fmt.Errorf("failed to resolve AWS credentials: no credential provider configured")
239+
}
240+
credCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
241+
defer cancel()
242+
if _, err := cfg.Credentials.Retrieve(credCtx); err != nil {
243+
return nil, fmt.Errorf("failed to resolve AWS credentials: %w", err)
244+
}
245+
return NewAWSSecretsStore(secretsmanager.NewFromConfig(cfg)), nil
246+
}
247+
214248
// loadAWSConfigForAzure loads AWS configuration with optional profile.
215249
func loadAWSConfigForAzure(ctx context.Context) (aws.Config, error) {
216250
var opts []func(*awsconfig.LoadOptions) error
@@ -227,7 +261,15 @@ func loadAWSConfigForAzure(ctx context.Context) (aws.Config, error) {
227261
}
228262

229263
// collectAzureCredentials collects Azure credentials interactively or from flags.
230-
func collectAzureCredentials(reader *bufio.Reader) (AzureCredentials, error) {
264+
// When created is non-nil (the wizard just created a service principal), its
265+
// credentials are used as-is and nothing is prompted: the freshly generated
266+
// client secret is never displayed, so the operator could not retype it anyway.
267+
func collectAzureCredentials(reader *bufio.Reader, created *AzureCredentials) (AzureCredentials, error) {
268+
if created != nil {
269+
fmt.Println("\nUsing the service principal created in Step 3.")
270+
return *created, nil
271+
}
272+
231273
creds := AzureCredentials{
232274
TenantID: azureOpts.TenantID,
233275
ClientID: azureOpts.ClientID,
@@ -240,7 +282,7 @@ func collectAzureCredentials(reader *bufio.Reader) (AzureCredentials, error) {
240282
return creds, nil
241283
}
242284

243-
fmt.Println("\nEnter the credentials from the Service Principal output above:")
285+
fmt.Println("\nEnter the credentials of an existing Azure Service Principal:")
244286
fmt.Println()
245287

246288
if err := promptForAzureCredentialFields(reader, &creds); err != nil {
@@ -369,14 +411,17 @@ func listAzureSubscriptions(ctx context.Context) error {
369411
// (resolve the "Reservations Administrator" role definition and assign it at
370412
// subscription scope). This is the create-for-rbac equivalent and fails loud
371413
// on any SDK error.
372-
func runAzureSetupCommands(ctx context.Context, reader *bufio.Reader) error {
414+
//
415+
// It returns the created service principal's credentials (nil when Step 3 was
416+
// skipped) so the caller can store them without echoing the secret.
417+
func runAzureSetupCommands(ctx context.Context, reader *bufio.Reader) (*AzureCredentials, error) {
373418
if err := azureStepLogin(reader); err != nil {
374-
return err
419+
return nil, err
375420
}
376421

377422
subscriptionID, err := azureStepListSubscriptions(ctx, reader)
378423
if err != nil {
379-
return err
424+
return nil, err
380425
}
381426

382427
return azureStepCreateServicePrincipal(ctx, reader, subscriptionID)
@@ -459,11 +504,22 @@ func resolveAzureTenantID(ctx context.Context, subscriptionID string) (string, e
459504
return *resp.TenantID, nil
460505
}
461506

507+
// resolveAzureTenantIDFn and newAzureSPProvisionerFn are the cloud-facing
508+
// constructors used by azureStepCreateServicePrincipal. They are variables so
509+
// unit tests can substitute fakes and never reach Azure.
510+
var (
511+
resolveAzureTenantIDFn = resolveAzureTenantID
512+
newAzureSPProvisionerFn = func(subscriptionID string) (azureSPProvisioner, error) {
513+
return newGraphSPProvisioner(subscriptionID)
514+
}
515+
)
516+
462517
// azureStepCreateServicePrincipal creates the service principal via the
463518
// Microsoft Graph + armauthorization SDKs (the create-for-rbac equivalent) and
464-
// prints the resulting credential material. It fails loud: any SDK error is
519+
// returns its credentials for direct storage (nil when the operator skips the
520+
// step). The client secret is never printed. It fails loud: any SDK error is
465521
// returned rather than silently falling back to the CLI.
466-
func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, subscriptionID string) error {
522+
func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, subscriptionID string) (*AzureCredentials, error) {
467523
fmt.Println()
468524
fmt.Println("Step 3: Create Service Principal")
469525
fmt.Println("---------------------------------")
@@ -475,7 +531,7 @@ func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader,
475531

476532
choice, err := readTrimmedLine(reader)
477533
if err != nil {
478-
return fmt.Errorf("failed to read service-principal choice: %w", err)
534+
return nil, fmt.Errorf("failed to read service-principal choice: %w", err)
479535
}
480536
choice = strings.ToLower(choice)
481537
if choice != "r" && choice != "run" && choice != "" {
@@ -484,7 +540,7 @@ func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader,
484540
fmt.Println("Provide the appId, client secret and tenant ID for an existing")
485541
fmt.Println("service principal in the next step.")
486542
fmt.Println()
487-
return nil
543+
return nil, nil
488544
}
489545

490546
// The step budget must exceed roleAssignRetryBudget (3 min) plus the
@@ -497,49 +553,69 @@ func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader,
497553
spCtx, cancel := context.WithTimeout(ctx, 6*time.Minute)
498554
defer cancel()
499555

500-
tenantID, err := resolveAzureTenantID(spCtx, subscriptionID)
556+
tenantID, err := resolveAzureTenantIDFn(spCtx, subscriptionID)
501557
if err != nil {
502-
return err
558+
return nil, err
503559
}
504560

505-
provisioner, err := newGraphSPProvisioner(subscriptionID)
561+
provisioner, err := newAzureSPProvisionerFn(subscriptionID)
506562
if err != nil {
507-
return fmt.Errorf("failed to initialize Azure SDK clients: %w\n"+
563+
return nil, fmt.Errorf("failed to initialize Azure SDK clients: %w\n"+
508564
"Ensure you are authenticated: run 'az login' (Step 1) before continuing", err)
509565
}
510566

511567
fmt.Println()
512568
fmt.Println(strings.Repeat("-", 60))
513569
result, err := createAzureServicePrincipal(spCtx, provisioner, subscriptionID, tenantID)
514570
if err != nil {
515-
return err
571+
return nil, err
516572
}
517573
fmt.Println(strings.Repeat("-", 60))
518574

519-
printAzureSPResult(result, subscriptionID)
520-
return nil
575+
printAzureSPResult(result, subscriptionID, azureOpts.StackName)
576+
return &AzureCredentials{
577+
TenantID: result.TenantID,
578+
ClientID: result.AppID,
579+
ClientSecret: result.ClientSecret,
580+
SubscriptionID: subscriptionID,
581+
}, nil
521582
}
522583

523-
// printAzureSPResult prints the credential material in the same shape that
524-
// "az ad sp create-for-rbac" prints, so the operator can feed it into the
525-
// credential collection step that follows.
526-
func printAzureSPResult(result azureSPResult, subscriptionID string) {
584+
// printAzureSPResult prints the non-secret identifiers of the created service
585+
// principal. The client secret is deliberately NOT printed (not even
586+
// partially): stdout is routinely captured by script/tee, CI logs and terminal
587+
// scrollback, and the wizard hands the secret straight to the Secrets Manager
588+
// store step, so the operator never needs to see or retype it.
589+
func printAzureSPResult(result azureSPResult, subscriptionID, stackName string) {
527590
fmt.Println()
528-
fmt.Println("Service principal created. Credential material:")
591+
fmt.Println("Service principal created:")
529592
fmt.Println()
530-
fmt.Printf(" appId (Client ID): %s\n", result.AppID)
531-
fmt.Printf(" password (Client Secret): %s\n", result.ClientSecret)
532-
fmt.Printf(" tenant (Tenant ID): %s\n", result.TenantID)
593+
fmt.Printf(" appId (Client ID): %s\n", result.AppID)
594+
fmt.Println(" password (Client Secret): [hidden]")
595+
fmt.Printf(" tenant (Tenant ID): %s\n", result.TenantID)
596+
fmt.Printf(" Subscription ID: %s\n", subscriptionID)
533597
fmt.Println()
534-
fmt.Println("IMPORTANT: copy the client secret now -- it cannot be retrieved later.")
535-
fmt.Println("You'll enter these values in the next step:")
536-
fmt.Println(" - appId -> Client ID")
537-
fmt.Println(" - password -> Client Secret")
538-
fmt.Println(" - tenant -> Tenant ID")
539-
fmt.Printf(" - Subscription ID: %s\n", subscriptionID)
598+
fmt.Printf("The client secret is not displayed. It will be stored directly in the\n")
599+
fmt.Printf("AWS Secrets Manager secret %q (field client_secret).\n", stackName+"-AzureCredentials")
540600
fmt.Println()
541601
}
542602

603+
// withAzureSPRecoveryHint annotates a failure that happens after the wizard
604+
// created a service principal but before its secret was stored (AWS config
605+
// and credentials are checked before creation, so this covers the remaining
606+
// store-time failures such as ListSecrets/UpdateSecret errors). The secret was
607+
// never displayed, so the operator is told how to mint a replacement instead.
608+
func withAzureSPRecoveryHint(err error, created *AzureCredentials) error {
609+
if created == nil {
610+
return err
611+
}
612+
return fmt.Errorf("%w\nThe service principal (appId %s) was created but its client secret was NOT stored "+
613+
"and was never displayed. Fix the error above, then generate a new secret with "+
614+
"'az ad app credential reset --id %s' and re-run 'cudly configure-azure --skip-setup', "+
615+
"entering it at the hidden Client Secret prompt",
616+
err, created.ClientID, created.ClientID)
617+
}
618+
543619
// promptAndRunExplicitCommand shows a command and asks to run or skip.
544620
// Takes explicit program and args to avoid command injection via string splitting.
545621
func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd, program string, args ...string) error {

0 commit comments

Comments
 (0)