fix(configure-azure): stop printing the new service principal's client secret (A10-028)

[IAmJSD]

Summary

Closes #2049 (A10-028, the CLI part of this finding. A08-016 and A08b-026 were split out to LeanerCloud/cloud-commitments-go#64 and are not touched here.)

Root cause

printAzureSPResult in cmd/configure_azure.go wrote the newly created service principal's client secret to stdout with a plain fmt.Printf. It ran with no TTY check and no redaction. The wizard then asked the operator to retype the same value at the next prompt. Anything that captures stdout (script, tee, CI job logs, terminal scrollback) ended up holding a live credential with the Reservations Administrator role in plaintext.

Fix: hand the secret straight to the store step and never display it

I took the issue's primary suggestion: carry the wizard's result straight into collectAzureCredentials.

• azureStepCreateServicePrincipal / runAzureSetupCommands now return the created credentials as *AzureCredentials: tenant, appId, secret, and the subscription the SP was scoped to in Step 2. The return is nil when Step 3 is skipped.
• collectAzureCredentials(reader, created) uses those credentials as they are when non-nil and asks for nothing. storeAzureCredentials then writes them to <stack>-AzureCredentials. The skip path and --skip-setup path behave as before.
• printAzureSPResult prints only appId, tenant ID and subscription ID, plus password (Client Secret): [hidden] and the name of the Secrets Manager secret it is stored in. No part of the secret is shown, not even a prefix hint.
• How the operator gets the secret: they don't need to. It goes into <stack>-AzureCredentials (field client_secret), which is the only place CUDly reads it from. Anyone who really needs the value can read it from Secrets Manager with their AWS permissions, which is an audited path.
• Failure ordering: the command loads the AWS config and resolves AWS credentials before the wizard starts, so those common failures stop it before any SP is created.
• Failure after creation: if storing the secret still fails after the SP was created (e.g. UpdateSecret is denied), withAzureSPRecoveryHint adds recovery steps to the error. It gives the appId (not secret), says the secret was not stored and never displayed, and tells the operator to run az ad app credential reset --id <appId> and re-run cudly configure-azure --skip-setup, typing the new secret at the existing hidden (term.ReadPassword) prompt.

Alternatives I considered and rejected:

• A term.IsTerminal gate (the issue's fallback). It still leaks under script, terminal scrollback and screen sharing.
• A 0600 file. It leaves a plaintext credential on disk that someone has to remember to clean up.
• An opt-in --show-secret flag. Nothing needs it once the handoff is direct, and it adds new surface.

Behavior change: when the wizard creates an SP, the credentials from that SP replace any --tenant-id/--client-id/--client-secret/--subscription-id flags. This is deliberate: those values belong to the SP that was just created. docs/cli/cloud-setup.md now describes this.

To make Step 3 testable without Azure, resolveAzureTenantID and newGraphSPProvisioner are now called through the package variables resolveAzureTenantIDFn / newAzureSPProvisionerFn. The production values are unchanged. The command body is configureAzure(ctx, reader), and the Secrets Manager store is built through openAzureSecretsStoreFn, so the whole command flow is unit-tested with mocks (follow-up commit 7cf76af).

Verification

All evidence comes from unit tests with mocks. The Azure provisioner is the existing mockSPProvisioner and Secrets Manager is MockSecretsStore. No real Azure or AWS calls were made and no service principals were created.

New tests in cmd/configure_azure_secret_test.go. Each one sends os.Stdout, os.Stderr and the standard log output to one pipe. They assert that neither the full fake secret nor any 8-character window of it appears in that output:

• TestAzureWizard_ClientSecretNeverPrinted: runs the full handoff (Step 3 with input r → collectAzureCredentials with an empty reader, so any re-prompt would fail → storeAzureCredentials). Asserts no secret appears in the output, appId / [hidden] / my-cudly-AzureCredentials are shown, and the stored JSON has exactly the tenant, appId, secret and subscription.
• TestPrintAzureSPResult_OmitsSecret: tests the printer directly.
• TestAzureWizard_SkipCreateSP_ReturnsNil: the skip path returns nil and never builds a provisioner.
• TestConfigureAzure_AWSConfigFailure_BeforeSPCreation, TestConfigureAzure_StoreFailureAfterCreate_HasRecoveryHint, TestConfigureAzure_CreateAndStore_EndToEnd, TestCollectAzureCredentials_PromptWording (added in 7cf76af; see the follow-up PR comment).
• TestAzureWizard_FailureAfterCreate_NoSecretInError: the secret does not appear in the rollback error (AssignRole failure) or in the recovery-hint error, and the hint names az ad app credential reset --id <appId>.

Fail-before:

• On pristine origin/main (detached worktree), the printer test adapted to the old 2-argument signature fails:

  --- FAIL: TestPrintAzureSPResult_OmitsSecret
      "...password (Client Secret): Zq8~fakeSPsecret.DoNotPrint_7f3a9c2e1b4d6082\n..." should not contain "Zq8~fakeSPsecret.DoNotPrint_7f3a9c2e1b4d6082"
      Messages: full client secret leaked to output
  

• Mutation probe on this branch: putting back fmt.Printf(" password (Client Secret): %s\n", result.ClientSecret) in place of the [hidden] line makes both TestAzureWizard_ClientSecretNeverPrinted and TestPrintAzureSPResult_OmitsSecret fail with full client secret leaked to output.

Pass-after (commands run from the repo root):

go build -o /dev/null ./cmd                      # ok
go vet ./...                                     # ok
gofmt -l cmd                                     # clean
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test -short -race -count=1 ./...   # ok cmd 148.7s
golangci-lint v2.10.1 run --allow-parallel-runners ./...   # 0 issues
gocyclo -over 10 cmd/configure_azure.go cmd/configure_azure_sp.go   # silent
bash scripts/gosec-hook.sh cmd/configure_azure.go cmd/configure_azure_sp.go cmd/configure_azure_secret_test.go   # Issues: 0
npx markdownlint-cli@0.47.0 docs/cli/cloud-setup.md   # clean

I set AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null only because the local ~/.aws/config default profile on the verification machine is invalid. These tests don't depend on it.

Gaps: I did not run the wizard against a real Azure tenant or AWS account. In particular, I did not check by hand the full interactive cudly configure-azure run (az login → Graph SP creation → Secrets Manager write).

Out of scope

• --client-secret on the command line (chore(pkg/cmd): medium/low findings from the 2026-07-28 full review #1613).
• setup.sh printing the same kind of secret (chore(terraform/environments): medium/low findings from the 2026-07-28 full review cloud-commitments-platform#132 item 19).
• A08-016 / A08b-026 (sec(azure): accounts-cache generation guard and OData filter injection [go part] cloud-commitments-go#64).
• cmd/configure_azure.go was already over the 500-line guideline before this change (602 → 649 lines). I left the file split for a separate change.

Maintainers: please mirror the issue's triage labels onto this PR: triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/s, type/security.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

Or wait 4 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9e890671-d40f-4c0a-869b-b00d44b168f7

📥 Commits

Reviewing files that changed from the base of the PR and between 817ab4b and 7cf76af.

📒 Files selected for processing: 4

• cmd/configure_azure.go
• cmd/configure_azure_secret_test.go
• cmd/configure_azure_sp.go
• docs/cli/cloud-setup.md

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Merge-gate review verdict: CLEAN at 8bd9f96

Independent adversarial security review by a non-author agent (Sonnet high, cold brief, full diff 817ab4bc...8bd9f96d): no blocking findings. git merge-tree with current main is clean.

Secret path traced from creation to the end of the flow: Graph AddPassword → azureSPResult → AzureCredentials (created) → json.Marshal → store.UpdateSecret (AWS Secrets Manager), then zeroed. No Printf, log, %v of a struct, panic or error wrap carries the secret. Rollback errors carry only the object ID, appId and the cause. The recovery hint prints only the appId and an az ad app credential reset command.

Non-blocking notes, @IAmJSD (your call; none block this PR):

1. cmd/configure_azure.go (~193-205): loadAWSConfigForAzure runs after the service principal is created. If AWS config or credentials are missing, which is a common failure, or the store call fails, the SP keeps its subscription-scope role and its secret is gone. withAzureSPRecoveryHint and the docs cover recovery. Loading the AWS config before creating the SP would move the likely failure ahead of any side effect. Ctrl-C between create and store has the same effect.
2. The withAzureSPRecoveryHint call sites in runConfigureAzure are untested; only the helper is. Deleting the wiring would not fail any test.
3. cmd/configure_azure.go:258: "Enter the credentials from the Service Principal output above" is stale on paths where no SP was created.

Behaviour change: when the wizard creates the SP, the --tenant-id/--client-id/--client-secret/--subscription-id flags are ignored and nothing is prompted. Both are documented in docs/cli/cloud-setup.md.

Local verification on macOS at 8bd9f96 (Azure and AWS calls are mocked; no real cloud):

• go test ./... -count=1 (go1.26.9, AWS config and credentials set to /dev/null): ok cmd 69s. go build ./cmd OK; go vet ./... clean.
• Captured output of go test ./cmd -run Azure -v: the fixture secret appears 0 times.
• Mutation probes, each reverted:
  • Re-adding the secret print fails TestAzureWizard_ClientSecretNeverPrinted and TestPrintAzureSPResult_OmitsSecret. This serves as the behavioural fail-before, because the PR's tests do not compile against the base.
  • Printing a 10-character prefix fails the same two tests.
  • Putting the secret in the recovery hint fails TestAzureWizard_FailureAfterCreate_NoSecretInError.
  • Ignoring created in collectAzureCredentials fails TestAzureWizard_ClientSecretNeverPrinted.
• Not done: an end-to-end run of the wizard against real Azure and AWS (real-account coverage gap).

Merge blockers (not code):

1. Approve the fork CI runs 38844194726 and 38844194727 (maintainer).
2. Mark the PR ready (author).
3. Run CodeRabbit, or waive it on quota, once the PR is ready.

🤖 Generated with Claude Code

[IAmJSD]

Follow-up on the three non-blocking review notes: new head 7cf76af

One new commit on top of 8bd9f96 (no rebase or force-push). Files: cmd/configure_azure.go, cmd/configure_azure_secret_test.go, docs/cli/cloud-setup.md.

1. AWS config loaded after SP creation. Fixed. configureAzure now calls openAzureSecretsStoreFn before runAzureSetupCommands. It loads the AWS config and also calls cfg.Credentials.Retrieve (30s timeout), then builds the store. LoadDefaultConfig on its own does not resolve credentials, so without the Retrieve call a missing AWS login would still have failed only at store time, after the SP existed. Neither call has side effects. withAzureSPRecoveryHint stays on the store step, which can still fail after creation (for example UpdateSecret being denied). The config-load call no longer needs the hint because nothing exists yet at that point. Ctrl-C between create and store is unchanged. The docs now describe the order.

2. Hint call sites untested. The command body is now configureAzure(ctx, reader) (runConfigureAzure only wraps stdin), and the store constructor is a package variable. New tests drive the whole command through the mocks:

• TestConfigureAzure_StoreFailureAfterCreate_HasRecoveryHint: the SP is created through the mock, then the mock UpdateSecret fails. The error must contain the store cause, appId <appId> and az ad app credential reset --id <appId>. Neither the output nor the error text may contain the secret or any 8-character window of it.
• TestConfigureAzure_AWSConfigFailure_BeforeSPCreation: the store constructor fails. The test checks there were zero tenant lookups, zero provisioner constructions, no CreateApplication or AddPassword calls, no recovery hint, and that the wizard never started.
• TestConfigureAzure_CreateAndStore_EndToEnd: the full command with mocks. The credentials are stored and the secret is never printed.

3. Stale prompt wording. Changed to "Enter the credentials of an existing Azure Service Principal:". TestCollectAzureCredentials_PromptWording pins it.

Verification (mock/unit evidence only; no real Azure or AWS calls, no SP created)

Fail-before (mutation probes on 7cf76af, each reverted and confirmed identical to the committed file):

• Restoring the old order (store opened after the wizard): TestConfigureAzure_AWSConfigFailure_BeforeSPCreation fails. Tenant and provisioner counts are 1, CreateApplication was called with CUDly, and the hint appears.
• Replacing withAzureSPRecoveryHint(err, created) with err: TestConfigureAzure_StoreFailureAfterCreate_HasRecoveryHint fails because the error lacks appId … and az ad app credential reset --id ….
• Restoring "…Service Principal output above:": TestCollectAzureCredentials_PromptWording fails.

Pass-after on macOS, go1.26.9:

go build -o /dev/null ./cmd                                   # ok
go vet ./...                                                  # ok
gofmt -l cmd                                                  # clean
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./... -count=1   # ok cmd 70.0s
golangci-lint v2.10.1 run --allow-parallel-runners ./...      # 0 issues
gocyclo -over 10 cmd/configure_azure.go                       # silent
bash scripts/gosec-hook.sh cmd/configure_azure.go cmd/configure_azure_secret_test.go   # Issues: 0
git secrets --scan <changed files>                            # pass
npx markdownlint-cli@0.47.0 docs/cli/cloud-setup.md           # clean

Gaps: I have not run this against a real AWS account. In particular, I have not confirmed with real credentials that Retrieve fails early on an expired SSO session or with no credentials. cmd/configure_azure.go is now 678 lines, already over the 500-line guideline before this PR; splitting it is still deferred.

🤖 Generated with Claude Code