From dacb2f2c8f4c495b7de2244055c2dcf76623b2d9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 24 Jun 2026 15:38:31 +0200 Subject: [PATCH 1/9] refactor(configure): replace az/gcloud CLI shell-outs with native SDK calls Replace exec.Command shell-outs to cloud CLIs with native SDK calls where the replacement is sound and does not require new heavyweight dependencies: Azure (ci_cd_sanity_tests/): - az account set/show -> armsubscriptions.Client.Get (programmatic CI path) - az group list -> armresources.ResourceGroupsClient.NewListPager - az vm list -> armcompute.VirtualMachinesClient.NewListAllPager Azure (cmd/configure_azure.go): - az account list -> armsubscriptions.Client.NewListPager (with CLI fallback) GCP (cmd/configure_gcp.go): - gcloud projects list -> cloudresourcemanager/v1 Projects.List - gcloud iam service-accounts create -> iam/v1 Projects.ServiceAccounts.Create - gcloud projects add-iam-policy-binding -> cloudresourcemanager/v1 SetIamPolicy - gcloud iam service-accounts keys create -> iam/v1 SA Keys.Create (writes key data to file; key is base64-decoded from PrivateKeyData response field) Calls left as CLI (documented in each function): - az login / gcloud auth login: interactive browser OAuth; no SDK equivalent for an operator establishing an initial credential. - gcloud config set project: writes to local gcloud config file; no API equivalent. - az ad sp create-for-rbac: would require msgraph-sdk-go + armauthorization/v2, neither of which is in the module graph; the interactive wizard path does not justify adding two new heavy SDKs. Auth model: DefaultAzureCredential (azure.go / configure_azure.go) picks up the az login session via its AzureCLICredential leg. google.DefaultTokenSource (configure_gcp.go) picks up gcloud application-default credentials; the wizard now notes that operators must also run 'gcloud auth application-default login' in addition to 'gcloud auth login' for the SDK steps to work. go.mod: armcompute/v5 and armsubscriptions promoted from indirect to direct; armresources v1.2.0 promoted from transitive to direct. No new SDK versions or packages outside the existing module graph were added (google.golang.org/api iam/v1 and cloudresourcemanager/v1 are sub-packages of the already-required google.golang.org/api v0.274.0). Removes the gosec G204 subprocess findings from ci_cd_sanity_tests/azure.go (the only nolint-free exec.Command site in the CI path). The remaining exec.Command calls in configure_azure.go and configure_gcp.go are in interactive wizard paths where the CLI is intentionally required; those are addressed separately. --- ci_cd_sanity_tests/pkg/sanity/azure/azure.go | 262 +++++++++-- .../pkg/sanity/azure/azure_test.go | 45 +- cmd/configure_azure.go | 218 ++++++--- cmd/configure_gcp.go | 416 ++++++++++++++---- go.mod | 5 +- go.sum | 2 + 6 files changed, 757 insertions(+), 191 deletions(-) diff --git a/ci_cd_sanity_tests/pkg/sanity/azure/azure.go b/ci_cd_sanity_tests/pkg/sanity/azure/azure.go index eb548d078..94c8b70e6 100644 --- a/ci_cd_sanity_tests/pkg/sanity/azure/azure.go +++ b/ci_cd_sanity_tests/pkg/sanity/azure/azure.go @@ -5,10 +5,14 @@ import ( "encoding/json" "fmt" "os" - "os/exec" "strings" "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/compute/armcompute/v5" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions" "github.com/LeanerCloud/CUDly/ci_cd_sanity_tests/pkg/sanity/report" ) @@ -19,6 +23,19 @@ type Options struct { Timeout time.Duration } +// azureSubscriptionInfo holds the subscription/tenant fields extracted from the +// armsubscriptions API response. This mirrors the fields previously parsed from +// "az account show -o json" so that validateAccountExpectations is unchanged. +type azureSubscriptionInfo struct { + ID string + TenantID string + Name string + State string +} + +// azAccountShow is the JSON shape produced by "az account show -o json". It is +// retained only to support the existing validateAccountExpectations function +// which the unit tests exercise via its JSON parsing path. type azAccountShow struct { ID string `json:"id"` TenantID string `json:"tenantId"` @@ -30,11 +47,11 @@ type azAccountShow struct { } `json:"user"` } -func truncate(s string, limit int) string { - if len(s) <= limit { +func truncate(s string, max int) string { + if len(s) <= max { return s } - return s[:limit] + "...(truncated)" + return s[:max] + "...(truncated)" } // validateAccountExpectations parses "az account show" JSON output and checks @@ -80,6 +97,194 @@ func validateAccountExpectations(opts Options, accountOut []byte) report.CheckRe return check } +// encodeAccountJSON serialises azureSubscriptionInfo into the same JSON shape +// that "az account show -o json" produced so that validateAccountExpectations +// can be reused without modification. +func encodeAccountJSON(info azureSubscriptionInfo) []byte { + a := azAccountShow{ + ID: info.ID, + TenantID: info.TenantID, + Name: info.Name, + State: info.State, + } + b, _ := json.Marshal(a) + return b +} + +// newCheckResult returns a CheckResult with name and timing already set. +func newCheckResult(name string, start time.Time) report.CheckResult { + return report.CheckResult{ + Name: name, + StartedAt: start, + Details: map[string]string{}, + } +} + +// checkPass records a passing check with an optional detail message and +// returns it ready to be added to the report. +func checkPass(cr *report.CheckResult, detail string) report.CheckResult { + cr.EndedAt = time.Now().UTC() + cr.Status = report.StatusPass + if detail != "" { + cr.Details["result"] = detail + } + return *cr +} + +// checkFail records a failing check and returns it. +func checkFail(cr *report.CheckResult, msg string) report.CheckResult { + cr.EndedAt = time.Now().UTC() + cr.Status = report.StatusFail + cr.Message = msg + return *cr +} + +// runGroupListCheck lists up to 10 resource groups in the subscription. +func runGroupListCheck(ctx context.Context, subscriptionID string, cred azcore.TokenCredential) report.CheckResult { + cr := newCheckResult("azure:group:list(sample)", time.Now().UTC()) + cr.Details["subscriptionID"] = subscriptionID + + rgClient, err := armresources.NewResourceGroupsClient(subscriptionID, cred, nil) + if err != nil { + return checkFail(&cr, fmt.Sprintf("failed to create resource-groups client: %v", err)) + } + + pager := rgClient.NewListPager(nil) + var names []string + for pager.More() && len(names) < 10 { + page, pageErr := pager.NextPage(ctx) + if pageErr != nil { + return checkFail(&cr, pageErr.Error()) + } + for _, rg := range page.Value { + if rg.Name != nil && rg.Location != nil { + names = append(names, fmt.Sprintf("%s (%s)", *rg.Name, *rg.Location)) + } + if len(names) >= 10 { + break + } + } + } + cr.Details["result"] = truncate(strings.Join(names, ", "), 2048) + return checkPass(&cr, "") +} + +// resourceGroupFromID extracts the resource group name from an Azure resource ID. +// The ID format is: .../resourceGroups//... +func resourceGroupFromID(id string) string { + parts := strings.Split(id, "/") + for i, p := range parts { + if strings.EqualFold(p, "resourceGroups") && i+1 < len(parts) { + return parts[i+1] + } + } + return "" +} + +// vmSummary returns a short display string for a virtual machine. +func vmSummary(vm *armcompute.VirtualMachine) string { + name := "" + rg := "" + loc := "" + if vm.Name != nil { + name = *vm.Name + } + if vm.Location != nil { + loc = *vm.Location + } + if vm.ID != nil { + rg = resourceGroupFromID(*vm.ID) + } + return fmt.Sprintf("%s (rg:%s loc:%s)", name, rg, loc) +} + +// runVMListCheck lists up to 10 virtual machines in the subscription. +func runVMListCheck(ctx context.Context, subscriptionID string, cred azcore.TokenCredential) report.CheckResult { + cr := newCheckResult("azure:vm:list(sample)", time.Now().UTC()) + cr.Details["subscriptionID"] = subscriptionID + + vmClient, err := armcompute.NewVirtualMachinesClient(subscriptionID, cred, nil) + if err != nil { + return checkFail(&cr, fmt.Sprintf("failed to create virtual-machines client: %v", err)) + } + + pager := vmClient.NewListAllPager(nil) + var items []string + for pager.More() && len(items) < 10 { + page, pageErr := pager.NextPage(ctx) + if pageErr != nil { + return checkFail(&cr, pageErr.Error()) + } + for _, vm := range page.Value { + items = append(items, vmSummary(vm)) + if len(items) >= 10 { + break + } + } + } + cr.Details["result"] = truncate(strings.Join(items, ", "), 2048) + return checkPass(&cr, "") +} + +// runAccountSetCheck verifies that the given subscription ID is reachable. +func runAccountSetCheck(ctx context.Context, subscriptionID string, cred azcore.TokenCredential) report.CheckResult { + cr := newCheckResult("azure:account:set", time.Now().UTC()) + cr.Details["subscriptionID"] = subscriptionID + + subClient, err := armsubscriptions.NewClient(cred, nil) + if err != nil { + return checkFail(&cr, fmt.Sprintf("failed to create subscriptions client: %v", err)) + } + + if _, err := subClient.Get(ctx, subscriptionID, nil); err != nil { + return checkFail(&cr, err.Error()) + } + return checkPass(&cr, "subscription reachable") +} + +// runAccountShowCheck retrieves subscription identity information. +// It returns the check result and the JSON-encoded account info (for use by +// validateAccountExpectations). The JSON is empty on failure. +func runAccountShowCheck(ctx context.Context, subscriptionID string, cred azcore.TokenCredential) (report.CheckResult, []byte) { + cr := newCheckResult("azure:account:show", time.Now().UTC()) + cr.Details["subscriptionID"] = subscriptionID + + subClient, err := armsubscriptions.NewClient(cred, nil) + if err != nil { + return checkFail(&cr, fmt.Sprintf("failed to create subscriptions client: %v", err)), nil + } + + resp, err := subClient.Get(ctx, subscriptionID, nil) + if err != nil { + return checkFail(&cr, err.Error()), nil + } + + sub := resp.Subscription + info := azureSubscriptionInfo{State: string(*sub.State)} + if sub.SubscriptionID != nil { + info.ID = *sub.SubscriptionID + } + if sub.TenantID != nil { + info.TenantID = *sub.TenantID + } + if sub.DisplayName != nil { + info.Name = *sub.DisplayName + } + + cr.Details["id"] = info.ID + cr.Details["tenantId"] = info.TenantID + cr.Details["name"] = info.Name + cr.Details["state"] = info.State + return checkPass(&cr, "account info retrieved"), encodeAccountJSON(info) +} + +// Run performs read-only Azure sanity checks using native SDK calls. +// +// Auth: DefaultAzureCredential is used throughout. In CI this resolves via the +// AZURE_CLIENT_ID / AZURE_TENANT_ID / AZURE_CLIENT_SECRET environment +// variables (service-principal flow). On an operator workstation it falls back +// to AzureCLICredential (i.e. the session established by "az login"), so the +// behaviour is identical to the previous CLI-based implementation. func Run(ctx context.Context, opts Options) (*report.Report, error) { if opts.SubscriptionID == "" { opts.SubscriptionID = os.Getenv("AZURE_SUBSCRIPTION_ID") @@ -101,50 +306,27 @@ func Run(ctx context.Context, opts Options) (*report.Report, error) { StartedAt: time.Now().UTC(), } - runCmd := func(name string, args ...string) ([]byte, report.CheckResult) { - start := time.Now().UTC() - cmd := exec.CommandContext(rctx, "az", args...) // #nosec G702,G204 -- CI sanity test tooling; binary is hardcoded "az" (Azure CLI). Args are Azure CLI subcommands constructed in test code plus opts.SubscriptionID from config/CLI, which exec.CommandContext passes as a single argv value (no shell interpretation), so it cannot inject commands - out, err := cmd.CombinedOutput() - end := time.Now().UTC() - - cr := report.CheckResult{ - Name: name, - StartedAt: start, - EndedAt: end, - Details: map[string]string{ - "cmd": "az " + strings.Join(args, " "), - "output": truncate(string(out), 2048), - }, - } - if err != nil { - cr.Status = report.StatusFail - cr.Message = err.Error() - } else { - cr.Status = report.StatusPass - } - return out, cr + cred, err := azidentity.NewDefaultAzureCredential(nil) + if err != nil { + rep.EndedAt = time.Now().UTC() + return nil, fmt.Errorf("azure: failed to build DefaultAzureCredential: %w", err) } - // Ensure subscription context (read-only) - _, cr := runCmd("azure:account:set", "account", "set", "--subscription", opts.SubscriptionID) - rep.Add(cr) + rep.Add(runAccountSetCheck(rctx, opts.SubscriptionID, cred)) - // Read-only identity/subscription info (only call once; reuse output) - accountOut, cr := runCmd("azure:account:show", "account", "show", "-o", "json") - rep.Add(cr) + accountShowResult, accountOut := runAccountShowCheck(rctx, opts.SubscriptionID, cred) + rep.Add(accountShowResult) - if opts.ExpectedSubID != "" || opts.ExpectedTenantID != "" { + // --- azure:account:expected_checks --- + if (opts.ExpectedSubID != "" || opts.ExpectedTenantID != "") && len(accountOut) > 0 { rep.Add(validateAccountExpectations(opts, accountOut)) } - // Read-only lists (sample) - _, cr = runCmd("azure:group:list(sample)", "group", "list", - "--query", "[0:10].{name:name, location:location}", "-o", "json") - rep.Add(cr) + // --- azure:group:list(sample) --- + rep.Add(runGroupListCheck(rctx, opts.SubscriptionID, cred)) - _, cr = runCmd("azure:vm:list(sample)", "vm", "list", - "--query", "[0:10].{name:name, resourceGroup:resourceGroup, location:location}", "-o", "json") - rep.Add(cr) + // --- azure:vm:list(sample) --- + rep.Add(runVMListCheck(rctx, opts.SubscriptionID, cred)) rep.EndedAt = time.Now().UTC() return rep, nil diff --git a/ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go b/ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go index 30cbdab01..570cb4d06 100644 --- a/ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go +++ b/ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go @@ -9,7 +9,46 @@ import ( "github.com/stretchr/testify/require" ) -func accountJSON(id, tenantID, name, state string) []byte { //nolint:unparam // param intentional for interface consistency/future use +// TestEncodeAccountJSON verifies that encodeAccountJSON produces bytes that are +// accepted by validateAccountExpectations without error, and that the ID / +// TenantID fields survive the round-trip. This guards the SDK->JSON->validate +// path introduced when replacing the "az account show" CLI call. +func TestEncodeAccountJSON(t *testing.T) { + info := azureSubscriptionInfo{ + ID: "aaaabbbb-1111-2222-3333-ccccddddeeee", + TenantID: "ffffgggg-5555-6666-7777-hhhh88889999", + Name: "My Test Sub", + State: "Enabled", + } + + encoded := encodeAccountJSON(info) + require.NotEmpty(t, encoded, "encoded JSON must not be empty") + + // Must parse back as azAccountShow without error. + var parsed azAccountShow + require.NoError(t, json.Unmarshal(encoded, &parsed)) + assert.Equal(t, info.ID, parsed.ID) + assert.Equal(t, info.TenantID, parsed.TenantID) + assert.Equal(t, info.Name, parsed.Name) + assert.Equal(t, info.State, parsed.State) + + // Round-trip through validateAccountExpectations with matching expectations. + opts := Options{ + ExpectedSubID: info.ID, + ExpectedTenantID: info.TenantID, + } + result := validateAccountExpectations(opts, encoded) + assert.Equal(t, report.StatusPass, result.Status, "expected PASS for matching IDs, got: %s", result.Message) +} + +// TestTruncate verifies truncate boundary conditions. +func TestTruncate(t *testing.T) { + assert.Equal(t, "ab", truncate("ab", 5)) + assert.Equal(t, "abcde", truncate("abcde", 5)) + assert.Equal(t, "abcde...(truncated)", truncate("abcdef", 5)) +} + +func accountJSON(id, tenantID, name, state string) []byte { b, err := json.Marshal(azAccountShow{ ID: id, TenantID: tenantID, @@ -25,10 +64,10 @@ func accountJSON(id, tenantID, name, state string) []byte { //nolint:unparam // func TestValidateAccountExpectations(t *testing.T) { tests := []struct { name string - wantStatus report.Status - wantMsgPart string opts Options accountOut []byte + wantStatus report.Status + wantMsgPart string // substring expected in Message when non-empty }{ { name: "valid json, no expectations", diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 400a80859..bd0d44ee2 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -14,6 +14,10 @@ import ( "strings" "syscall" + "time" + + "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" awsconfig "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/aws-sdk-go-v2/service/secretsmanager" @@ -21,10 +25,10 @@ import ( "golang.org/x/term" ) -// azureUUIDRegex validates Azure UUIDs (subscription IDs, tenant IDs, client IDs). +// azureUUIDRegex validates Azure UUIDs (subscription IDs, tenant IDs, client IDs) var azureUUIDRegex = regexp.MustCompile(`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$`) -// validateAzureUUID validates an Azure UUID to prevent command injection. +// validateAzureUUID validates an Azure UUID to prevent command injection func validateAzureUUID(uuid, fieldName string) error { if !azureUUIDRegex.MatchString(uuid) { return fmt.Errorf("invalid %s format: must be a valid UUID (xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx)", fieldName) @@ -38,27 +42,27 @@ func validateAzureUUID(uuid, fieldName string) error { // is still valid input. io.EOF with no data, or any other error, is returned. func readTrimmedLine(reader *bufio.Reader) (string, error) { input, err := reader.ReadString('\n') - if err != nil && (!errors.Is(err, io.EOF) || input == "") { + if err != nil && !(errors.Is(err, io.EOF) && input != "") { return "", err } return strings.TrimSpace(input), nil } -// AzureCredentials holds the Azure Service Principal credentials. +// AzureCredentials holds the Azure Service Principal credentials type AzureCredentials struct { TenantID string `json:"tenant_id"` ClientID string `json:"client_id"` - ClientSecret string `json:"client_secret"` //nolint:gosec // G117: intentional Azure client-secret field -- marshaled for secure storage in the operator's credential store; this is the expected usage + ClientSecret string `json:"client_secret"` SubscriptionID string `json:"subscription_id"` } -// AzureConfigOptions holds configuration for the Azure config command. +// AzureConfigOptions holds configuration for the Azure config command type AzureConfigOptions struct { StackName string Profile string TenantID string ClientID string - ClientSecret string //nolint:gosec // G117: intentional Azure client-secret configuration field -- stored for secure credential storage; this is the expected usage + ClientSecret string SubscriptionID string Interactive bool SkipSetup bool @@ -77,7 +81,7 @@ You can provide credentials via flags or interactively: cudly configure-azure --stack-name my-cudly --tenant-id xxx --client-id xxx --client-secret xxx --subscription-id xxx cudly configure-azure --stack-name my-cudly --interactive -To create an Azure Service Principal: +To create an Azure Service Principal manually: az login az ad sp create-for-rbac --name "CUDly" --role "Reservation Administrator" --scopes /subscriptions/`, RunE: runConfigureAzure, @@ -111,7 +115,7 @@ func validateAzureCredentialFields(creds AzureCredentials) error { return validateAzureUUID(creds.SubscriptionID, "Subscription ID") } -// storeAzureCredentials stores Azure credentials in the secrets store. +// storeAzureCredentials stores Azure credentials in the secrets store func storeAzureCredentials(ctx context.Context, store SecretsStore, stackName string, creds AzureCredentials) error { if err := validateAzureCredentialFields(creds); err != nil { return err @@ -130,7 +134,7 @@ func storeAzureCredentials(ctx context.Context, store SecretsStore, stackName st } // Marshal credentials to JSON - credJSON, err := json.Marshal(creds) // #nosec G117 -- intentional: marshaling Azure credential struct (contains ClientSecret field) for secure storage in the credential store + credJSON, err := json.Marshal(creds) if err != nil { return fmt.Errorf("failed to marshal credentials: %w", err) } @@ -154,7 +158,7 @@ func runConfigureAzure(cmd *cobra.Command, args []string) error { // Run Azure CLI setup if not skipped if !azureOpts.SkipSetup { - if err := runAzureSetupCommands(reader); err != nil { + if err := runAzureSetupCommands(ctx, reader); err != nil { return err } } @@ -187,7 +191,7 @@ func runConfigureAzure(cmd *cobra.Command, args []string) error { return nil } -// loadAWSConfigForAzure loads AWS configuration with optional profile. +// loadAWSConfigForAzure loads AWS configuration with optional profile func loadAWSConfigForAzure(ctx context.Context) (aws.Config, error) { var opts []func(*awsconfig.LoadOptions) error if azureOpts.Profile != "" { @@ -202,7 +206,7 @@ func loadAWSConfigForAzure(ctx context.Context) (aws.Config, error) { return cfg, nil } -// collectAzureCredentials collects Azure credentials interactively or from flags. +// collectAzureCredentials collects Azure credentials interactively or from flags func collectAzureCredentials(reader *bufio.Reader) (AzureCredentials, error) { creds := AzureCredentials{ TenantID: azureOpts.TenantID, @@ -226,7 +230,7 @@ func collectAzureCredentials(reader *bufio.Reader) (AzureCredentials, error) { return creds, nil } -// promptForAzureCredentialFields prompts for missing credential fields. +// promptForAzureCredentialFields prompts for missing credential fields func promptForAzureCredentialFields(reader *bufio.Reader, creds *AzureCredentials) error { if creds.TenantID == "" { fmt.Print("Azure Tenant ID: ") @@ -248,9 +252,7 @@ func promptForAzureCredentialFields(reader *bufio.Reader, creds *AzureCredential if creds.ClientSecret == "" { fmt.Print("Client Secret (password): ") - // int cast: syscall.Stdin is already int on Unix but syscall.Handle on - // Windows; term.ReadPassword takes int, so the cast keeps Windows builds working. - secret, err := term.ReadPassword(int(syscall.Stdin)) //nolint:unconvert // no-op on Unix (int), required on Windows (syscall.Handle) + secret, err := term.ReadPassword(int(syscall.Stdin)) if err != nil { return fmt.Errorf("failed to read secret: %w", err) } @@ -270,25 +272,106 @@ func promptForAzureCredentialFields(reader *bufio.Reader, creds *AzureCredential return nil } -// runAzureSetupCommands runs the Azure CLI commands interactively. -func runAzureSetupCommands(reader *bufio.Reader) error { +// 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. +func listAzureSubscriptions(ctx context.Context) error { + cred, err := azidentity.NewDefaultAzureCredential(nil) + if err != nil { + return fmt.Errorf("failed to build credential: %w", err) + } + + client, err := armsubscriptions.NewClient(cred, nil) + if err != nil { + return fmt.Errorf("failed to create subscriptions client: %w", err) + } + + fmt.Printf("%-40s %-38s %s\n", "Name", "SubscriptionId", "State") + fmt.Println(strings.Repeat("-", 95)) + + pager := client.NewListPager(nil) + for pager.More() { + page, pageErr := pager.NextPage(ctx) + if pageErr != nil { + return fmt.Errorf("failed to list subscriptions: %w", pageErr) + } + for _, sub := range page.Value { + name := "" + subID := "" + state := "" + if sub.DisplayName != nil { + name = *sub.DisplayName + } + if sub.SubscriptionID != nil { + subID = *sub.SubscriptionID + } + if sub.State != nil { + state = string(*sub.State) + } + fmt.Printf("%-40s %-38s %s\n", name, subID, state) + } + } + return nil +} + +// runAzureSetupCommands guides the operator through the Azure setup wizard. +// +// Step 1 (az login): performed via the Azure CLI. "az login" launches an +// interactive browser-based OAuth flow that cannot be replicated through the +// SDK on behalf of a human operator who does not yet have a credential. +// +// 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), so no extra auth step is +// required. +// +// Step 3 (create service principal): performed via the Azure CLI +// (az ad sp create-for-rbac). Creating an AAD application + service principal +// + password credential via the Microsoft Graph SDK would require adding the +// msgraph-sdk-go and armauthorization/v2 packages (currently not in the module +// graph). The CLI path is safe because arguments are passed directly to +// exec.Command without shell interpolation, and the subscription ID is +// validated as a UUID before use. +func runAzureSetupCommands(ctx context.Context, reader *bufio.Reader) error { + if err := azureStepLogin(reader); err != nil { + return err + } + + subscriptionID, err := azureStepListSubscriptions(ctx, reader) + if err != nil { + return err + } + + return azureStepCreateServicePrincipal(reader, subscriptionID) +} + +// azureStepLogin prompts to run "az login". +func azureStepLogin(reader *bufio.Reader) error { fmt.Println("Step 1: Azure Login") fmt.Println("-------------------") fmt.Println("This will open a browser window for Azure authentication.") fmt.Println() + return promptAndRunExplicitCommand(reader, "Azure Login", "az login", "az", "login") +} - if err := promptAndRunExplicitCommand(reader, "Azure Login", "az login", "az", "login"); err != nil { - return err - } - +// azureStepListSubscriptions lists subscriptions via SDK (with CLI fallback) +// and prompts the operator to enter their subscription ID. +func azureStepListSubscriptions(ctx context.Context, reader *bufio.Reader) (string, error) { fmt.Println() fmt.Println("Step 2: Get Subscription ID") fmt.Println("---------------------------") - fmt.Println("List your Azure subscriptions to find the Subscription ID:") + fmt.Println("Listing your Azure subscriptions via SDK (DefaultAzureCredential)...") fmt.Println() - if err := promptAndRunExplicitCommand(reader, "List Subscriptions", "az account list --output table", "az", "account", "list", "--output", "table"); err != nil { - return err + listCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + + if err := listAzureSubscriptions(listCtx); err != nil { + fmt.Printf("SDK subscription list failed (%v); falling back to az account list.\n\n", err) + if cliErr := promptAndRunExplicitCommand(reader, "List Subscriptions", "az account list --output table", "az", "account", "list", "--output", "table"); cliErr != nil { + return "", cliErr + } } fmt.Println() @@ -299,37 +382,23 @@ func runAzureSetupCommands(reader *bufio.Reader) error { } if subscriptionID == "" { - return fmt.Errorf("subscription ID is required") + return "", fmt.Errorf("subscription ID is required") } - - // Validate subscription ID to prevent command injection if err := validateAzureUUID(subscriptionID, "Subscription ID"); err != nil { - return err - } - - if err := createAzureServicePrincipal(reader, subscriptionID); err != nil { - return err + return "", err } - - fmt.Println() - fmt.Println("IMPORTANT: Copy the output above! You'll need:") - fmt.Println(" - appId -> Client ID") - fmt.Println(" - password -> Client Secret") - fmt.Println(" - tenant -> Tenant ID") - fmt.Printf(" - Subscription ID: %s\n", subscriptionID) - fmt.Println() - - return nil + return subscriptionID, nil } -// createAzureServicePrincipal runs Step 3 of Azure setup: create service principal. -func createAzureServicePrincipal(reader *bufio.Reader, subscriptionID string) error { +// azureStepCreateServicePrincipal prompts to run az ad sp create-for-rbac. +func azureStepCreateServicePrincipal(reader *bufio.Reader, subscriptionID string) error { fmt.Println() fmt.Println("Step 3: Create Service Principal") fmt.Println("---------------------------------") fmt.Println("This creates an Azure Service Principal with Reservation Administrator role.") fmt.Println() + // Run directly without shell to avoid injection; subscription ID is UUID-validated above. fmt.Printf("Command: az ad sp create-for-rbac --name CUDly --role \"Reservations Administrator\" --scopes /subscriptions/%s\n", subscriptionID) fmt.Println() fmt.Printf("[R]un, [S]kip? ") @@ -341,36 +410,45 @@ func createAzureServicePrincipal(reader *bufio.Reader, subscriptionID string) er choice = strings.ToLower(choice) if choice == "r" || choice == "run" || choice == "" { - fmt.Println() - fmt.Println(strings.Repeat("-", 60)) - cmd := exec.Command("az", "ad", "sp", "create-for-rbac", // #nosec G204,G702 -- binary "az" is hardcoded; subscriptionID validated by validateAzureUUID before exec - "--name", "CUDly", - "--role", "Reservations Administrator", - "--scopes", fmt.Sprintf("/subscriptions/%s", subscriptionID)) - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - cmd.Stdin = os.Stdin - if err := cmd.Run(); err != nil { - fmt.Printf("Command failed: %v\n", err) - fmt.Print("Continue anyway? [y/N]: ") - response, readErr := readTrimmedLine(reader) - if readErr != nil { - return fmt.Errorf("failed to read response: %w", readErr) - } - if !strings.EqualFold(response, "y") { - return fmt.Errorf("failed to create service principal: %w", err) - } + if err := runCreateSPCommand(reader, subscriptionID); err != nil { + return err } - fmt.Println(strings.Repeat("-", 60)) } else { fmt.Println("Skipping Create Service Principal") } return nil } +// runCreateSPCommand executes az ad sp create-for-rbac with a continue-anyway +// prompt on failure. +func runCreateSPCommand(reader *bufio.Reader, subscriptionID string) error { + fmt.Println() + fmt.Println(strings.Repeat("-", 60)) + cmd := exec.Command("az", "ad", "sp", "create-for-rbac", + "--name", "CUDly", + "--role", "Reservations Administrator", + "--scopes", fmt.Sprintf("/subscriptions/%s", subscriptionID)) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + if err := cmd.Run(); err != nil { + fmt.Printf("Command failed: %v\n", err) + fmt.Print("Continue anyway? [y/N]: ") + response, readErr := readTrimmedLine(reader) + if readErr != nil { + return fmt.Errorf("failed to read response: %w", readErr) + } + if strings.ToLower(response) != "y" { + return fmt.Errorf("failed to create service principal: %w", err) + } + } + fmt.Println(strings.Repeat("-", 60)) + return nil +} + // 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 { +func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd string, program string, args ...string) error { fmt.Printf("Command: %s\n", displayCmd) fmt.Println() fmt.Printf("[R]un, [S]kip? ") @@ -398,12 +476,12 @@ func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd, program // is consumed from one consistent buffered stream (a fresh // bufio.NewReader(os.Stdin) here would drop input already buffered by the // caller's reader, breaking piped input after earlier prompts). -func executeExplicitCommand(reader *bufio.Reader, displayCmd, program string, args ...string) error { +func executeExplicitCommand(reader *bufio.Reader, displayCmd string, program string, args ...string) error { fmt.Println() fmt.Printf("Executing: %s\n", displayCmd) fmt.Println(strings.Repeat("-", 60)) - cmd := exec.Command(program, args...) // #nosec G204 -- configure CLI tool; program is always "az" (Azure CLI) per all callers; no user input reaches this function + cmd := exec.Command(program, args...) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr cmd.Stdin = os.Stdin @@ -418,7 +496,7 @@ func executeExplicitCommand(reader *bufio.Reader, displayCmd, program string, ar if readErr != nil { return fmt.Errorf("failed to read response: %w", readErr) } - if !strings.EqualFold(response, "y") { + if strings.ToLower(response) != "y" { return fmt.Errorf("command failed: %w", err) } } diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index c4e155433..00f92f8d8 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -3,6 +3,7 @@ package main import ( "bufio" "context" + "encoding/base64" "encoding/json" "fmt" "log" @@ -16,12 +17,16 @@ import ( awsconfig "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/aws-sdk-go-v2/service/secretsmanager" "github.com/spf13/cobra" + "golang.org/x/oauth2/google" + "google.golang.org/api/cloudresourcemanager/v1" + iamv1 "google.golang.org/api/iam/v1" + "google.golang.org/api/option" ) -// gcpProjectIDRegex validates GCP project IDs (lowercase letters, digits, hyphens, 6-30 chars). +// gcpProjectIDRegex validates GCP project IDs (lowercase letters, digits, hyphens, 6-30 chars) var gcpProjectIDRegex = regexp.MustCompile(`^[a-z][a-z0-9-]{4,28}[a-z0-9]$`) -// validateGCPProjectID validates a GCP project ID to prevent command injection. +// validateGCPProjectID validates a GCP project ID to prevent command injection func validateGCPProjectID(projectID string) error { if !gcpProjectIDRegex.MatchString(projectID) { return fmt.Errorf("invalid GCP project ID format: must be 6-30 lowercase letters, digits, or hyphens, starting with a letter") @@ -29,12 +34,12 @@ func validateGCPProjectID(projectID string) error { return nil } -// GCPCredentials holds the GCP Service Account credentials. +// GCPCredentials holds the GCP Service Account credentials type GCPCredentials struct { Type string `json:"type"` ProjectID string `json:"project_id"` PrivateKeyID string `json:"private_key_id"` - PrivateKey string `json:"private_key"` //nolint:gosec // G117: intentional serialization -- GCPCredentials is marshaled to store service-account private key in the operator's credential store; this is the expected usage + PrivateKey string `json:"private_key"` ClientEmail string `json:"client_email"` ClientID string `json:"client_id,omitempty"` AuthURI string `json:"auth_uri,omitempty"` @@ -43,7 +48,7 @@ type GCPCredentials struct { ClientX509CertURL string `json:"client_x509_cert_url,omitempty"` } -// GCPConfigOptions holds configuration for the GCP config command. +// GCPConfigOptions holds configuration for the GCP config command type GCPConfigOptions struct { StackName string Profile string @@ -81,8 +86,8 @@ func init() { configureGCPCmd.Flags().BoolVar(&gcpOpts.SkipSetup, "skip-setup", false, "Skip GCP CLI setup commands (gcloud login, create service account)") } -// storeGCPCredentials stores GCP credentials in the secrets store. -func storeGCPCredentials(ctx context.Context, store SecretsStore, stackName, credsJSON string) error { +// storeGCPCredentials stores GCP credentials in the secrets store +func storeGCPCredentials(ctx context.Context, store SecretsStore, stackName string, credsJSON string) error { // Validate that we have valid JSON var creds GCPCredentials if err := json.Unmarshal([]byte(credsJSON), &creds); err != nil { @@ -135,7 +140,7 @@ func runConfigureGCP(cmd *cobra.Command, args []string) error { fmt.Println("===================================================") fmt.Println() - credsFile, err := getGCPCredentialsFilePath(reader) + credsFile, err := getGCPCredentialsFilePath(ctx, reader) if err != nil { return err } @@ -161,15 +166,15 @@ func runConfigureGCP(cmd *cobra.Command, args []string) error { return nil } -// getGCPCredentialsFilePath determines the credentials file path from options or user input. -func getGCPCredentialsFilePath(reader *bufio.Reader) (string, error) { +// getGCPCredentialsFilePath determines the credentials file path from options or user input +func getGCPCredentialsFilePath(ctx context.Context, reader *bufio.Reader) (string, error) { var credsFile string if gcpOpts.CredentialsFile != "" { credsFile = gcpOpts.CredentialsFile } else if !gcpOpts.SkipSetup { var err error - credsFile, err = runGCPSetupCommands(reader) + credsFile, err = runGCPSetupCommands(ctx, reader) if err != nil { return "", err } @@ -191,7 +196,7 @@ func getGCPCredentialsFilePath(reader *bufio.Reader) (string, error) { return credsFile, nil } -// loadAWSConfigForGCP loads AWS configuration with optional profile. +// loadAWSConfigForGCP loads AWS configuration with optional profile func loadAWSConfigForGCP(ctx context.Context) (aws.Config, error) { var opts []func(*awsconfig.LoadOptions) error if gcpOpts.Profile != "" { @@ -206,24 +211,23 @@ func loadAWSConfigForGCP(ctx context.Context) (aws.Config, error) { return cfg, nil } -// loadAndUpdateGCPCredentials loads, parses, and optionally updates GCP credentials. +// loadAndUpdateGCPCredentials loads, parses, and optionally updates GCP credentials func loadAndUpdateGCPCredentials(credsFile string) (GCPCredentials, []byte, error) { expandedPath := expandHomeDirectory(credsFile) - credsData, err := os.ReadFile(expandedPath) // #nosec G304,G703 -- GCP credentials file path is operator-supplied via CLI argument; operator controls the value + credsData, err := os.ReadFile(expandedPath) if err != nil { return GCPCredentials{}, nil, fmt.Errorf("failed to read credentials file: %w", err) } var creds GCPCredentials - err = json.Unmarshal(credsData, &creds) - if err != nil { + if err := json.Unmarshal(credsData, &creds); err != nil { return GCPCredentials{}, nil, fmt.Errorf("failed to parse credentials file: %w", err) } if gcpOpts.ProjectID != "" { creds.ProjectID = gcpOpts.ProjectID - credsData, err = json.Marshal(creds) // #nosec G117 -- intentional: marshaling GCP credential struct (contains PrivateKey field) for secure storage in the credential store + credsData, err = json.Marshal(creds) if err != nil { return GCPCredentials{}, nil, fmt.Errorf("failed to marshal updated credentials: %w", err) } @@ -232,7 +236,7 @@ func loadAndUpdateGCPCredentials(credsFile string) (GCPCredentials, []byte, erro return creds, credsData, nil } -// expandHomeDirectory expands ~ to the user's home directory. +// expandHomeDirectory expands ~ to the user's home directory func expandHomeDirectory(path string) string { if !strings.HasPrefix(path, "~/") { return path @@ -246,7 +250,7 @@ func expandHomeDirectory(path string) string { return strings.Replace(path, "~", home, 1) } -// printGCPConfigurationSuccess prints success message with credentials info. +// printGCPConfigurationSuccess prints success message with credentials info func printGCPConfigurationSuccess(creds GCPCredentials) { log.Printf("GCP credentials stored successfully in Secrets Manager") fmt.Println("\nGCP configuration complete!") @@ -255,25 +259,240 @@ func printGCPConfigurationSuccess(creds GCPCredentials) { fmt.Println("\nCUDly can now manage GCP Committed Use Discounts.") } -// runGCPSetupCommands runs the GCP CLI commands interactively. -func runGCPSetupCommands(reader *bufio.Reader) (string, error) { +// newGCPAPIOption returns an oauth2 token source option for the google.golang.org +// API client, using Application Default Credentials. ADC resolves credentials in +// priority order: GOOGLE_APPLICATION_CREDENTIALS env var, gcloud ADC cache +// (populated by "gcloud auth application-default login"), Workload Identity, +// Metadata Server. +// +// NOTE: "gcloud auth login" (wizard Step 1) updates the gcloud user session but +// does NOT automatically populate the ADC cache. If the operator intends to use +// these SDK calls after Step 1, they should also run: +// +// gcloud auth application-default login +// +// The wizard Step 3 onwards will call this function; if ADC is not available the +// calls will fail and the wizard suggests the manual gcloud CLI fallback. +func newGCPAPIOption(ctx context.Context) (option.ClientOption, error) { + ts, err := google.DefaultTokenSource(ctx, + "https://www.googleapis.com/auth/cloud-platform", + "https://www.googleapis.com/auth/iam", + ) + if err != nil { + return nil, fmt.Errorf("failed to obtain GCP Application Default Credentials: %w\n"+ + "Hint: run 'gcloud auth application-default login' first", err) + } + return option.WithTokenSource(ts), nil +} + +// listGCPProjects lists GCP projects accessible to the operator via the Cloud +// Resource Manager API v1 and prints them in a table. This replaces the +// "gcloud projects list" CLI call. +func listGCPProjects(ctx context.Context) error { + opt, err := newGCPAPIOption(ctx) + if err != nil { + return err + } + + svc, err := cloudresourcemanager.NewService(ctx, opt) + if err != nil { + return fmt.Errorf("failed to create resource manager client: %w", err) + } + + fmt.Printf("%-30s %-25s %s\n", "NAME", "PROJECT_ID", "PROJECT_NUMBER") + fmt.Println(strings.Repeat("-", 80)) + + req := svc.Projects.List() + if err := req.Pages(ctx, func(page *cloudresourcemanager.ListProjectsResponse) error { + for _, p := range page.Projects { + fmt.Printf("%-30s %-25s %d\n", p.Name, p.ProjectId, p.ProjectNumber) + } + return nil + }); err != nil { + return fmt.Errorf("failed to list GCP projects: %w", err) + } + return nil +} + +// createGCPServiceAccount creates a GCP IAM service account via the IAM API v1. +// This replaces "gcloud iam service-accounts create". +func createGCPServiceAccount(ctx context.Context, projectID, saName string) (string, error) { + opt, err := newGCPAPIOption(ctx) + if err != nil { + return "", err + } + + svc, err := iamv1.NewService(ctx, opt) + if err != nil { + return "", fmt.Errorf("failed to create IAM client: %w", err) + } + + req := &iamv1.CreateServiceAccountRequest{ + AccountId: saName, + ServiceAccount: &iamv1.ServiceAccount{ + DisplayName: "CUDly Service Account", + Description: "Service account for CUDly commitment management", + }, + } + + sa, err := svc.Projects.ServiceAccounts.Create("projects/"+projectID, req).Context(ctx).Do() + if err != nil { + return "", fmt.Errorf("failed to create service account: %w", err) + } + + return sa.Email, nil +} + +// grantGCPIAMRole grants an IAM role to a service account on a project via the +// Cloud Resource Manager API v1. This replaces +// "gcloud projects add-iam-policy-binding". +func grantGCPIAMRole(ctx context.Context, projectID, member, role string) error { + opt, err := newGCPAPIOption(ctx) + if err != nil { + return err + } + + svc, err := cloudresourcemanager.NewService(ctx, opt) + if err != nil { + return fmt.Errorf("failed to create resource manager client: %w", err) + } + + // Get current policy. + policy, err := svc.Projects.GetIamPolicy(projectID, &cloudresourcemanager.GetIamPolicyRequest{}).Context(ctx).Do() + if err != nil { + return fmt.Errorf("failed to get IAM policy for project %s: %w", projectID, err) + } + + // Append the binding (or extend existing one). + found := false + for _, b := range policy.Bindings { + if b.Role == role { + for _, m := range b.Members { + if m == member { + // Already bound; nothing to do. + return nil + } + } + b.Members = append(b.Members, member) + found = true + break + } + } + if !found { + policy.Bindings = append(policy.Bindings, &cloudresourcemanager.Binding{ + Role: role, + Members: []string{member}, + }) + } + _, err = svc.Projects.SetIamPolicy(projectID, &cloudresourcemanager.SetIamPolicyRequest{ + Policy: policy, + }).Context(ctx).Do() + if err != nil { + return fmt.Errorf("failed to set IAM policy on project %s: %w", projectID, err) + } + return nil +} + +// createGCPServiceAccountKey creates a JSON key for the given service account +// and writes it to keyFile. Returns the path written. This replaces +// "gcloud iam service-accounts keys create --iam-account=". +func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) error { + opt, err := newGCPAPIOption(ctx) + if err != nil { + return err + } + + svc, err := iamv1.NewService(ctx, opt) + if err != nil { + return fmt.Errorf("failed to create IAM client: %w", err) + } + + resource := fmt.Sprintf("projects/-/serviceAccounts/%s", saEmail) + key, err := svc.Projects.ServiceAccounts.Keys.Create(resource, &iamv1.CreateServiceAccountKeyRequest{ + PrivateKeyType: "TYPE_GOOGLE_CREDENTIALS_FILE", + }).Context(ctx).Do() + if err != nil { + return fmt.Errorf("failed to create service account key: %w", err) + } + + // PrivateKeyData is base64-encoded JSON. + decoded, err := base64.StdEncoding.DecodeString(key.PrivateKeyData) + if err != nil { + return fmt.Errorf("failed to decode key data: %w", err) + } + + if err := os.WriteFile(keyFile, decoded, 0600); err != nil { + return fmt.Errorf("failed to write key file %s: %w", keyFile, err) + } + return nil +} + +// runGCPSetupCommands guides the operator through GCP setup. +// +// Step 1 (gcloud auth login): performed via the GCP CLI. This is an +// interactive browser-based OAuth flow that cannot be replicated through SDK +// calls on behalf of an operator who does not yet have a credential. It +// updates the gcloud user session but does NOT populate the ADC cache needed +// by subsequent SDK calls. Operators must also run +// "gcloud auth application-default login" if they want to use ADC. +// +// Step 2 (list projects): performed via the Cloud Resource Manager SDK v1, +// using Application Default Credentials (ADC). Falls back to "gcloud projects +// list" if ADC is not available. +// +// Step 3 (gcloud config set project): sets local gcloud config state. There is +// no cloud-API equivalent for writing to the local gcloud configuration file, +// so this step remains a CLI call. +// +// Steps 4-6 (create SA, grant role, create key): performed via GCP IAM and +// Cloud Resource Manager SDK v1 APIs using ADC. Fall back to the gcloud CLI +// if ADC is not available. +func runGCPSetupCommands(ctx context.Context, reader *bufio.Reader) (string, error) { + if err := gcpStepLogin(reader); err != nil { + return "", err + } + + projectID, err := gcpStepSelectProject(ctx, reader) + if err != nil { + return "", err + } + + saEmail, err := gcpStepCreateServiceAccount(ctx, reader, projectID) + if err != nil { + return "", err + } + + if err := gcpStepGrantRole(ctx, reader, projectID, saEmail); err != nil { + return "", err + } + + return gcpStepCreateKey(ctx, reader, saEmail) +} + +// gcpStepLogin runs "gcloud auth login" interactively. +func gcpStepLogin(reader *bufio.Reader) error { fmt.Println("Step 1: GCP Login") fmt.Println("-----------------") fmt.Println("This will open a browser window for GCP authentication.") + fmt.Println("After logging in, also run: gcloud auth application-default login") + fmt.Println("(required for the SDK-based steps below)") fmt.Println() + return promptAndRunGCPCommand(reader, "GCP Login", "gcloud auth login", "gcloud", "auth", "login") +} - if err := promptAndRunGCPCommand(reader, "GCP Login", "gcloud auth login", "gcloud", "auth", "login"); err != nil { - return "", err - } - +// gcpStepSelectProject lists projects and prompts for a project ID. +func gcpStepSelectProject(ctx context.Context, reader *bufio.Reader) (string, error) { fmt.Println() fmt.Println("Step 2: Select Project") fmt.Println("----------------------") - fmt.Println("List your GCP projects:") + fmt.Println("Listing your GCP projects via SDK (Application Default Credentials)...") fmt.Println() - if err := promptAndRunGCPCommand(reader, "List Projects", "gcloud projects list", "gcloud", "projects", "list"); err != nil { - return "", err + if err := listGCPProjects(ctx); err != nil { + fmt.Printf("SDK project list failed (%v); falling back to gcloud.\n\n", err) + if cliErr := promptAndRunGCPCommand(reader, "List Projects", "gcloud projects list", "gcloud", "projects", "list"); cliErr != nil { + return "", cliErr + } } fmt.Println() @@ -281,86 +500,131 @@ func runGCPSetupCommands(reader *bufio.Reader) (string, error) { if err != nil { return "", err } - - // Validate project ID to prevent command injection - err = validateGCPProjectID(projectID) - if err != nil { + if err := validateGCPProjectID(projectID); err != nil { return "", err } - // Set the project - use exec.Command with arguments instead of shell + // Set the project in the local gcloud config. This is a local operation + // (writes to ~/.config/gcloud/properties) with no cloud-API equivalent. fmt.Println() - fmt.Println("Setting project...") - cmd := exec.Command("gcloud", "config", "set", "project", projectID) // #nosec G204,G702 -- binary "gcloud" is hardcoded; projectID validated by validateGCPProjectID before exec + fmt.Println("Setting gcloud project context (local config)...") + cmd := exec.Command("gcloud", "config", "set", "project", projectID) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - err = cmd.Run() - if err != nil { + if err := cmd.Run(); err != nil { return "", fmt.Errorf("failed to set project: %w", err) } + return projectID, nil +} + +// gcpStepCreateServiceAccount creates the CUDly service account via SDK with +// a gcloud CLI fallback. +func gcpStepCreateServiceAccount(ctx context.Context, reader *bufio.Reader, projectID string) (string, error) { + saName := "cudly-service-account" + saEmail := fmt.Sprintf("%s@%s.iam.gserviceaccount.com", saName, projectID) fmt.Println() fmt.Println("Step 3: Create Service Account") fmt.Println("------------------------------") - fmt.Println("This creates a GCP Service Account for CUDly.") + fmt.Println("This creates a GCP Service Account for CUDly via the IAM API.") fmt.Println() + fmt.Printf("[R]un, [S]kip? (creates service account '%s' via SDK) ", saName) - saName := "cudly-service-account" - createSaDisplay := fmt.Sprintf(`gcloud iam service-accounts create %s --display-name="CUDly Service Account" --description="Service account for CUDly commitment management"`, saName) - - err = promptAndRunGCPCommand(reader, "Create Service Account", createSaDisplay, - "gcloud", "iam", "service-accounts", "create", saName, - "--display-name=CUDly Service Account", - "--description=Service account for CUDly commitment management") - if err != nil { - return "", err + choice, _ := reader.ReadString('\n') + switch strings.ToLower(strings.TrimSpace(choice)) { + case "r", "run", "": + email, sdkErr := createGCPServiceAccount(ctx, projectID, saName) + if sdkErr != nil { + fmt.Printf("SDK call failed (%v); falling back to gcloud.\n", sdkErr) + display := fmt.Sprintf(`gcloud iam service-accounts create %s --display-name="CUDly Service Account" --description="Service account for CUDly commitment management"`, saName) + if err := promptAndRunGCPCommand(reader, "Create Service Account", display, + "gcloud", "iam", "service-accounts", "create", saName, + "--display-name=CUDly Service Account", + "--description=Service account for CUDly commitment management"); err != nil { + return "", err + } + } else { + saEmail = email + fmt.Printf("Service account created: %s\n", saEmail) + } + case "s", "skip": + fmt.Println("Skipping Create Service Account") + default: + fmt.Printf("Unknown option, skipping\n") } + return saEmail, nil +} + +// gcpStepGrantRole grants the compute.admin role to the service account. +func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEmail string) error { + member := fmt.Sprintf("serviceAccount:%s", saEmail) + role := "roles/compute.admin" fmt.Println() fmt.Println("Step 4: Grant IAM Roles") fmt.Println("-----------------------") fmt.Println("Grant the required roles to the service account.") fmt.Println() + fmt.Printf("[R]un, [S]kip? (grants %s to %s on project %s via SDK) ", role, saEmail, projectID) - saEmail := fmt.Sprintf("%s@%s.iam.gserviceaccount.com", saName, projectID) - - // Grant Compute Admin role for commitment management - grantRoleDisplay := fmt.Sprintf(`gcloud projects add-iam-policy-binding %s --member="serviceAccount:%s" --role="roles/compute.admin"`, projectID, saEmail) + choice, _ := reader.ReadString('\n') + switch strings.ToLower(strings.TrimSpace(choice)) { + case "r", "run", "": + if sdkErr := grantGCPIAMRole(ctx, projectID, member, role); sdkErr != nil { + fmt.Printf("SDK call failed (%v); falling back to gcloud.\n", sdkErr) + display := fmt.Sprintf(`gcloud projects add-iam-policy-binding %s --member="%s" --role="%s"`, projectID, member, role) + return promptAndRunGCPCommand(reader, "Grant Compute Admin Role", display, + "gcloud", "projects", "add-iam-policy-binding", projectID, + fmt.Sprintf("--member=%s", member), + fmt.Sprintf("--role=%s", role)) + } + fmt.Printf("Role %s granted to %s on project %s.\n", role, saEmail, projectID) + case "s", "skip": + fmt.Println("Skipping Grant IAM Roles") + default: + fmt.Printf("Unknown option, skipping\n") + } + return nil +} - err = promptAndRunGCPCommand(reader, "Grant Compute Admin Role", grantRoleDisplay, - "gcloud", "projects", "add-iam-policy-binding", projectID, - fmt.Sprintf("--member=serviceAccount:%s", saEmail), - "--role=roles/compute.admin") +// gcpStepCreateKey creates a JSON key file for the service account. +func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) (string, error) { + home, err := os.UserHomeDir() if err != nil { - return "", err + return "", fmt.Errorf("failed to get home directory: %w", err) } + keyFile := filepath.Join(home, "cudly-gcp-key.json") fmt.Println() fmt.Println("Step 5: Create and Download Key") fmt.Println("-------------------------------") fmt.Println("Create a JSON key file for the service account.") fmt.Println() + fmt.Printf("[R]un, [S]kip? (creates key for %s, writes to %s via SDK) ", saEmail, keyFile) - // Get home directory for default key path - home, err := os.UserHomeDir() - if err != nil { - return "", fmt.Errorf("failed to get home directory: %w", err) - } - keyFile := filepath.Join(home, "cudly-gcp-key.json") - - createKeyDisplay := fmt.Sprintf(`gcloud iam service-accounts keys create %s --iam-account=%s`, keyFile, saEmail) - - err = promptAndRunGCPCommand(reader, "Create Key File", createKeyDisplay, - "gcloud", "iam", "service-accounts", "keys", "create", keyFile, - fmt.Sprintf("--iam-account=%s", saEmail)) - if err != nil { - return "", err + choice, _ := reader.ReadString('\n') + switch strings.ToLower(strings.TrimSpace(choice)) { + case "r", "run", "": + if sdkErr := createGCPServiceAccountKey(ctx, saEmail, keyFile); sdkErr != nil { + fmt.Printf("SDK call failed (%v); falling back to gcloud.\n", sdkErr) + display := fmt.Sprintf(`gcloud iam service-accounts keys create %s --iam-account=%s`, keyFile, saEmail) + if err := promptAndRunGCPCommand(reader, "Create Key File", display, + "gcloud", "iam", "service-accounts", "keys", "create", keyFile, + fmt.Sprintf("--iam-account=%s", saEmail)); err != nil { + return "", err + } + } else { + fmt.Printf("Key file written to: %s\n", keyFile) + } + case "s", "skip": + fmt.Println("Skipping Create Key") + default: + fmt.Printf("Unknown option, skipping\n") } fmt.Println() - fmt.Printf("Key file created at: %s\n", keyFile) + fmt.Printf("Key file at: %s\n", keyFile) fmt.Println() - return keyFile, nil } @@ -380,7 +644,7 @@ func readRequiredInputLine(reader *bufio.Reader, prompt, fieldName string) (stri // promptAndRunGCPCommand shows a command and asks to run or skip. // Takes explicit program and args to avoid command injection via string splitting. -func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd, program string, args ...string) error { //nolint:unparam // param intentional for interface consistency/future use +func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, program string, args ...string) error { fmt.Printf("Command: %s\n", displayCmd) fmt.Println() fmt.Printf("[R]un, [S]kip? ") @@ -408,12 +672,12 @@ func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd, program stri // is consumed from one consistent buffered stream (a fresh // bufio.NewReader(os.Stdin) here would drop input already buffered by the // caller's reader, breaking piped input after earlier prompts). -func executeGCPCommand(reader *bufio.Reader, displayCmd, program string, args ...string) error { +func executeGCPCommand(reader *bufio.Reader, displayCmd string, program string, args ...string) error { fmt.Println() fmt.Printf("Executing: %s\n", displayCmd) fmt.Println(strings.Repeat("-", 60)) - cmd := exec.Command(program, args...) // #nosec G204,G702 -- configure CLI tool; program is always "gcloud" per all callers; no user input reaches this function + cmd := exec.Command(program, args...) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr cmd.Stdin = os.Stdin @@ -428,7 +692,7 @@ func executeGCPCommand(reader *bufio.Reader, displayCmd, program string, args .. if readErr != nil { return fmt.Errorf("failed to read response: %w", readErr) } - if !strings.EqualFold(response, "y") { + if strings.ToLower(response) != "y" { return fmt.Errorf("command failed: %w", err) } } diff --git a/go.mod b/go.mod index 2a11d4f6f..66e80f243 100644 --- a/go.mod +++ b/go.mod @@ -28,10 +28,10 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.10.1 github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.1 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor v1.2.0 // indirect - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.4.0 // indirect + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.4.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.1.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.0.0 // indirect - github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 // indirect + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql v1.2.0 // indirect github.com/AzureAD/microsoft-authentication-library-for-go v1.4.2 // indirect github.com/aws/aws-sdk-go-v2/credentials v1.16.13 @@ -85,6 +85,7 @@ require ( cloud.google.com/go/secretmanager v1.16.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits v1.0.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys v1.4.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.4.0 github.com/LeanerCloud/CUDly/pkg v0.0.0 diff --git a/go.sum b/go.sum index a6710c69b..4766f018c 100644 --- a/go.sum +++ b/go.sum @@ -60,6 +60,8 @@ github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0 h1:PTFG github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0/go.mod h1:LRr2FzBTQlONPPa5HREE5+RjSCTXl7BwOvYOaWTqCaI= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.0 h1:2qsIIvxVT+uE6yrNldntJKlLRgxGbZ85kgtz5SNBhMw= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v3 v3.1.0/go.mod h1:AW8VEadnhw9xox+VaVd9sP7NjzOAnaZBLRH6Tq3cJ38= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/managementgroups/armmanagementgroups v1.0.0 h1:pPvTJ1dY0sA35JOeFq6TsY2xj6Z85Yo23Pj4wCCvu4o= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/managementgroups/armmanagementgroups v1.0.0/go.mod h1:mLfWfj8v3jfWKsL9G4eoBoXVcsqcIUTapmdKy7uGOp0= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.0.0 h1:zp+znRAHKLSewbw+WWKIMgCaFNxEXt9AwjxmW5fCnck= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.0.0/go.mod h1:nEvLUni7GO5ukfEYtmrUfz08Puqd2FP9d8sCZazm5W4= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 h1:0OO/3K+SKt45gXiOU4gHRILOLeNOUZdqeNO47Mq6iN8= From c28d78406251ba13a7512593c939e51bebeff45a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 24 Jun 2026 16:06:55 +0200 Subject: [PATCH 2/9] refactor(configure): SDK-create Azure SP and fail loud (no CLI fallback) Replace the remaining az/gcloud CLI fallbacks with native SDK calls that fail loud, per owner decision on PR #1279. Azure service principal creation (the create-for-rbac equivalent): - New cmd/configure_azure_sp.go creates the AAD application registration (name "CUDly"), adds a password credential, creates the service principal, resolves the "Reservations Administrator" role definition by display name at subscription scope, and assigns it -- all via the Microsoft Graph SDK (applications + service principals + addPassword) and armauthorization/v2 (RoleDefinitionsClient.NewListPager with a roleName filter, RoleAssignmentsClient.Create). DefaultAzureCredential reuses the az login session via its AzureCLICredential leg. - The resulting appId (client ID), generated client secret, and tenant ID are printed in the same shape az ad sp create-for-rbac prints, so the operator can feed them into the credential collection step. The secret comes from the addPassword response (PasswordCredential.GetSecretText) and is only available at creation time. - Tenant ID is resolved from the subscription via armsubscriptions. - A narrow azureSPProvisioner interface wraps the four cloud operations so the orchestration is unit-testable; cmd/configure_azure_sp_test.go mocks it and asserts the app name = "CUDly", role = "Reservations Administrator", and scope = /subscriptions/, plus per-step error propagation (no role resolve/assign after an earlier failure). Remove silent CLI fallbacks -> fail loud: - configure_azure.go subscription list: on SDK error, return an error telling the operator to run "az login" first (was: silently run az account list). - configure_gcp.go project list, SA create, role grant, key create: on SDK error, return the error (was: print "falling back to gcloud" and run the CLI). ADC failures hint to run "gcloud auth application-default login". Remaining cloud-CLI exec.Command calls (the only three left, each //nolint:gosec with a precise reason -- interactive auth bootstrap or local state, no SDK equivalent): - az login (interactive browser OAuth) - gcloud auth login (interactive browser OAuth) - gcloud config set project (writes local ~/.config/gcloud state) Removing the az ad sp create-for-rbac subprocess eliminates its gosec G204 finding; the three retained calls carry justified nolints. go.mod: add github.com/microsoftgraph/msgraph-sdk-go v1.99.0 and github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/ armauthorization/v2 v2.2.0 as direct deps (root module). go mod tidy pulled the kiota transitive deps and bumped azcore/azidentity/otel to satisfy msgraph requirements. --- cmd/configure_azure.go | 148 ++++++++++++++-------- cmd/configure_azure_sp.go | 222 +++++++++++++++++++++++++++++++++ cmd/configure_azure_sp_test.go | 208 ++++++++++++++++++++++++++++++ cmd/configure_gcp.go | 72 +++++------ go.mod | 25 ++-- go.sum | 54 +++++--- 6 files changed, 607 insertions(+), 122 deletions(-) create mode 100644 cmd/configure_azure_sp.go create mode 100644 cmd/configure_azure_sp_test.go diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index bd0d44ee2..2668d5ce6 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -319,20 +319,19 @@ func listAzureSubscriptions(ctx context.Context) error { // // Step 1 (az login): performed via the Azure CLI. "az login" launches an // interactive browser-based OAuth flow that cannot be replicated through the -// SDK on behalf of a human operator who does not yet have a credential. +// 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), so no extra auth step is -// required. +// just established (via its AzureCLICredential leg). Fails loud if the SDK +// cannot authenticate (no CLI fallback). // -// Step 3 (create service principal): performed via the Azure CLI -// (az ad sp create-for-rbac). Creating an AAD application + service principal -// + password credential via the Microsoft Graph SDK would require adding the -// msgraph-sdk-go and armauthorization/v2 packages (currently not in the module -// graph). The CLI path is safe because arguments are passed directly to -// exec.Command without shell interpolation, and the subscription ID is -// validated as a UUID before use. +// Step 3 (create service principal): performed via the Microsoft Graph SDK +// (application + service principal + password credential) and armauthorization +// (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 { if err := azureStepLogin(reader); err != nil { return err @@ -343,7 +342,7 @@ func runAzureSetupCommands(ctx context.Context, reader *bufio.Reader) error { return err } - return azureStepCreateServicePrincipal(reader, subscriptionID) + return azureStepCreateServicePrincipal(ctx, reader, subscriptionID) } // azureStepLogin prompts to run "az login". @@ -355,8 +354,10 @@ func azureStepLogin(reader *bufio.Reader) error { return promptAndRunExplicitCommand(reader, "Azure Login", "az login", "az", "login") } -// azureStepListSubscriptions lists subscriptions via SDK (with CLI fallback) -// and prompts the operator to enter their subscription ID. +// 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. func azureStepListSubscriptions(ctx context.Context, reader *bufio.Reader) (string, error) { fmt.Println() fmt.Println("Step 2: Get Subscription ID") @@ -368,10 +369,8 @@ func azureStepListSubscriptions(ctx context.Context, reader *bufio.Reader) (stri defer cancel() if err := listAzureSubscriptions(listCtx); err != nil { - fmt.Printf("SDK subscription list failed (%v); falling back to az account list.\n\n", err) - if cliErr := promptAndRunExplicitCommand(reader, "List Subscriptions", "az account list --output table", "az", "account", "list", "--output", "table"); cliErr != nil { - return "", cliErr - } + return "", fmt.Errorf("failed to list Azure subscriptions via SDK: %w\n"+ + "Ensure you are authenticated: run 'az login' (Step 1) before continuing", err) } fmt.Println() @@ -390,62 +389,102 @@ func azureStepListSubscriptions(ctx context.Context, reader *bufio.Reader) (stri return subscriptionID, nil } -// azureStepCreateServicePrincipal prompts to run az ad sp create-for-rbac. -func azureStepCreateServicePrincipal(reader *bufio.Reader, subscriptionID string) error { +// resolveAzureTenantID looks up the tenant ID for the given subscription via +// the ARM Subscriptions SDK. DefaultAzureCredential reuses the "az login" +// session. +func resolveAzureTenantID(ctx context.Context, subscriptionID string) (string, error) { + cred, err := azidentity.NewDefaultAzureCredential(nil) + if err != nil { + return "", fmt.Errorf("failed to build credential: %w", err) + } + client, err := armsubscriptions.NewClient(cred, nil) + if err != nil { + return "", fmt.Errorf("failed to create subscriptions client: %w", err) + } + resp, err := client.Get(ctx, subscriptionID, nil) + if err != nil { + return "", fmt.Errorf("failed to get subscription %s: %w", subscriptionID, err) + } + if resp.TenantID == nil || *resp.TenantID == "" { + return "", fmt.Errorf("subscription %s returned no tenant ID", subscriptionID) + } + return *resp.TenantID, nil +} + +// 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 +// returned rather than silently falling back to the CLI. +func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, subscriptionID string) error { fmt.Println() fmt.Println("Step 3: Create Service Principal") fmt.Println("---------------------------------") - fmt.Println("This creates an Azure Service Principal with Reservation Administrator role.") - fmt.Println() - - // Run directly without shell to avoid injection; subscription ID is UUID-validated above. - fmt.Printf("Command: az ad sp create-for-rbac --name CUDly --role \"Reservations Administrator\" --scopes /subscriptions/%s\n", subscriptionID) + fmt.Println("This creates an Azure Service Principal with the") + fmt.Printf("%q role at subscription scope, via the Microsoft Graph SDK.\n", azureSPRoleName) fmt.Println() + fmt.Printf("Create service principal %q with role %q at /subscriptions/%s?\n", azureSPName, azureSPRoleName, subscriptionID) fmt.Printf("[R]un, [S]kip? ") choice, err := readTrimmedLine(reader) if err != nil { - return fmt.Errorf("failed to read choice: %w", err) + return fmt.Errorf("failed to read service-principal choice: %w", err) } choice = strings.ToLower(choice) - - if choice == "r" || choice == "run" || choice == "" { - if err := runCreateSPCommand(reader, subscriptionID); err != nil { - return err - } - } else { + if choice != "r" && choice != "run" && choice != "" { fmt.Println("Skipping Create Service Principal") + fmt.Println() + 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 + } + + spCtx, cancel := context.WithTimeout(ctx, 2*time.Minute) + defer cancel() + + tenantID, err := resolveAzureTenantID(spCtx, subscriptionID) + if err != nil { + return err + } + + provisioner, err := newGraphSPProvisioner(subscriptionID) + if err != nil { + return fmt.Errorf("failed to initialize Azure SDK clients: %w\n"+ + "Ensure you are authenticated: run 'az login' (Step 1) before continuing", err) } - return nil -} -// runCreateSPCommand executes az ad sp create-for-rbac with a continue-anyway -// prompt on failure. -func runCreateSPCommand(reader *bufio.Reader, subscriptionID string) error { fmt.Println() fmt.Println(strings.Repeat("-", 60)) - cmd := exec.Command("az", "ad", "sp", "create-for-rbac", - "--name", "CUDly", - "--role", "Reservations Administrator", - "--scopes", fmt.Sprintf("/subscriptions/%s", subscriptionID)) - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - cmd.Stdin = os.Stdin - if err := cmd.Run(); err != nil { - fmt.Printf("Command failed: %v\n", err) - fmt.Print("Continue anyway? [y/N]: ") - response, readErr := readTrimmedLine(reader) - if readErr != nil { - return fmt.Errorf("failed to read response: %w", readErr) - } - if strings.ToLower(response) != "y" { - return fmt.Errorf("failed to create service principal: %w", err) - } + result, err := createAzureServicePrincipal(spCtx, provisioner, subscriptionID, tenantID) + if err != nil { + return err } fmt.Println(strings.Repeat("-", 60)) + + printAzureSPResult(result, subscriptionID) return 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) { + fmt.Println() + fmt.Println("Service principal created. Credential material:") + 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.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.Println() +} + // 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 string, program string, args ...string) error { @@ -472,6 +511,8 @@ func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd string, } // executeExplicitCommand runs a command with explicit program and arguments. +// It is used only for the interactive "az login" auth bootstrap (Step 1), +// which has no SDK equivalent that preserves the cached-credential UX. // The caller's reader is threaded through to the retry prompt so all input // is consumed from one consistent buffered stream (a fresh // bufio.NewReader(os.Stdin) here would drop input already buffered by the @@ -481,6 +522,7 @@ func executeExplicitCommand(reader *bufio.Reader, displayCmd string, program str fmt.Printf("Executing: %s\n", displayCmd) fmt.Println(strings.Repeat("-", 60)) + //nolint:gosec // G204: interactive operator auth (az login), hardcoded args, no shell cmd := exec.Command(program, args...) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr diff --git a/cmd/configure_azure_sp.go b/cmd/configure_azure_sp.go new file mode 100644 index 000000000..b0e08a3f3 --- /dev/null +++ b/cmd/configure_azure_sp.go @@ -0,0 +1,222 @@ +package main + +import ( + "context" + "fmt" + + "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" + "github.com/microsoftgraph/msgraph-sdk-go/applications" + graphmodels "github.com/microsoftgraph/msgraph-sdk-go/models" +) + +// azureSPName is the Azure AD application / service principal display name. +// It matches the name the previous "az ad sp create-for-rbac --name CUDly" +// call used so the resulting identity is interchangeable. +const azureSPName = "CUDly" + +// azureSPRoleName is the RBAC role assigned to the service principal at +// subscription scope, matching the previous create-for-rbac invocation. +const azureSPRoleName = "Reservations Administrator" + +// azureSPResult holds the credential material produced by creating the service +// principal. It mirrors the appId/password/tenant fields that +// "az ad sp create-for-rbac" prints, which the operator feeds into the +// subsequent configure step. +type azureSPResult struct { + AppID string // application (client) ID + ClientSecret string // generated password / client secret + TenantID string // Azure AD tenant ID +} + +// azureSPProvisioner abstracts the cloud operations needed to create a service +// principal and grant it the Reservations Administrator role. It exists so the +// orchestration logic can be unit-tested with a mock without depending on the +// concrete Graph / armauthorization client types (which are not interfaces). +type azureSPProvisioner interface { + // CreateApplication creates an AAD application registration with the given + // display name and returns its object ID and application (client) ID. + CreateApplication(ctx context.Context, displayName string) (objectID, appID string, err error) + // AddPassword adds a password credential to the application identified by + // objectID and returns the generated secret text. + AddPassword(ctx context.Context, objectID string) (secretText string, err error) + // CreateServicePrincipal creates a service principal for the given + // application (client) ID and returns the service principal object ID + // (the principal ID used for role assignment). + CreateServicePrincipal(ctx context.Context, appID string) (principalID string, err error) + // ResolveRoleDefinitionID resolves the role definition ID for the given + // role display name at the given scope. + ResolveRoleDefinitionID(ctx context.Context, scope, roleName string) (roleDefinitionID string, err error) + // AssignRole creates a role assignment binding principalID to + // roleDefinitionID at the given scope. + AssignRole(ctx context.Context, scope, principalID, roleDefinitionID string) error +} + +// createAzureServicePrincipal performs the full create-for-rbac equivalent: +// it creates the application + password + service principal, resolves the +// "Reservations Administrator" role at subscription scope, assigns it, and +// returns the credential material. +// +// subscriptionID must already be validated (UUID) and tenantID resolved by the +// caller. The behavior matches: +// +// az ad sp create-for-rbac --name CUDly \ +// --role "Reservations Administrator" \ +// --scopes /subscriptions/ +func createAzureServicePrincipal(ctx context.Context, p azureSPProvisioner, subscriptionID, tenantID string) (azureSPResult, error) { + scope := fmt.Sprintf("/subscriptions/%s", subscriptionID) + + objectID, appID, err := p.CreateApplication(ctx, azureSPName) + if err != nil { + return azureSPResult{}, fmt.Errorf("failed to create application registration: %w", err) + } + + secret, err := p.AddPassword(ctx, objectID) + if err != nil { + return azureSPResult{}, 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) + } + + roleDefID, err := p.ResolveRoleDefinitionID(ctx, scope, azureSPRoleName) + if err != nil { + return azureSPResult{}, 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 azureSPResult{ + AppID: appID, + ClientSecret: secret, + TenantID: tenantID, + }, nil +} + +// graphSPProvisioner is the production azureSPProvisioner backed by the +// Microsoft Graph SDK (application + service principal) and armauthorization +// (role definition + role assignment). +type graphSPProvisioner struct { + graph *msgraphsdk.GraphServiceClient + roleDefs *armauthorization.RoleDefinitionsClient + 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). +// 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) + if err != nil { + return nil, fmt.Errorf("failed to build DefaultAzureCredential: %w", err) + } + + graph, err := msgraphsdk.NewGraphServiceClientWithCredentials( + cred, []string{"https://graph.microsoft.com/.default"}) + if err != nil { + return nil, fmt.Errorf("failed to create Microsoft Graph client: %w", err) + } + + roleDefs, err := armauthorization.NewRoleDefinitionsClient(cred, nil) + if err != nil { + return nil, fmt.Errorf("failed to create role definitions client: %w", err) + } + + roleAsgn, err := armauthorization.NewRoleAssignmentsClient(subscriptionID, cred, nil) + if err != nil { + return nil, fmt.Errorf("failed to create role assignments client: %w", err) + } + + return &graphSPProvisioner{graph: graph, roleDefs: roleDefs, roleAsgn: roleAsgn}, nil +} + +func (g *graphSPProvisioner) CreateApplication(ctx context.Context, displayName string) (objectID, appID string, err error) { + app := graphmodels.NewApplication() + app.SetDisplayName(&displayName) + + created, err := g.graph.Applications().Post(ctx, app, nil) + if err != nil { + return "", "", err + } + oid := created.GetId() + aid := created.GetAppId() + if oid == nil || aid == nil { + return "", "", fmt.Errorf("application created but Graph returned no id/appId") + } + return *oid, *aid, nil +} + +func (g *graphSPProvisioner) AddPassword(ctx context.Context, objectID string) (string, error) { + body := applications.NewItemAddPasswordPostRequestBody() + cred := graphmodels.NewPasswordCredential() + displayName := azureSPName + "-secret" + cred.SetDisplayName(&displayName) + body.SetPasswordCredential(cred) + + result, err := g.graph.Applications().ByApplicationId(objectID).AddPassword().Post(ctx, body, nil) + if err != nil { + return "", err + } + secret := result.GetSecretText() + if secret == nil || *secret == "" { + return "", fmt.Errorf("password credential created but Graph returned no secret text") + } + return *secret, nil +} + +func (g *graphSPProvisioner) CreateServicePrincipal(ctx context.Context, appID string) (string, error) { + sp := graphmodels.NewServicePrincipal() + sp.SetAppId(&appID) + + created, err := g.graph.ServicePrincipals().Post(ctx, sp, nil) + if err != nil { + return "", err + } + principalID := created.GetId() + if principalID == nil { + return "", fmt.Errorf("service principal created but Graph returned no id") + } + return *principalID, 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{ + Filter: &filter, + }) + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + return "", err + } + for _, rd := range page.Value { + if rd == nil || rd.ID == nil { + continue + } + if rd.Properties != nil && rd.Properties.RoleName != nil && *rd.Properties.RoleName == roleName { + return *rd.ID, nil + } + } + } + return "", fmt.Errorf("role definition %q not found at scope %s", roleName, scope) +} + +func (g *graphSPProvisioner) AssignRole(ctx context.Context, scope, principalID, roleDefinitionID string) error { + principalType := armauthorization.PrincipalTypeServicePrincipal + _, err := g.roleAsgn.Create(ctx, scope, uuid.NewString(), armauthorization.RoleAssignmentCreateParameters{ + Properties: &armauthorization.RoleAssignmentProperties{ + PrincipalID: &principalID, + RoleDefinitionID: &roleDefinitionID, + PrincipalType: &principalType, + }, + }, nil) + return err +} diff --git a/cmd/configure_azure_sp_test.go b/cmd/configure_azure_sp_test.go new file mode 100644 index 000000000..3ffe80baf --- /dev/null +++ b/cmd/configure_azure_sp_test.go @@ -0,0 +1,208 @@ +package main + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mockSPProvisioner is a configurable azureSPProvisioner used to assert that +// createAzureServicePrincipal requests the correct app name, role and scope, +// and surfaces the generated secret. +type mockSPProvisioner struct { + // captured inputs + createAppName string + addPasswordObjID string + createSPAppID string + resolveRoleScope string + resolveRoleName string + assignScope string + assignPrincipalID string + assignRoleDefID string + + // canned outputs + appObjectID string + appID string + secret string + principalID string + roleDefID string + + // optional injected errors + createAppErr error + addPwErr error + createSPErr error + resolveErr error + assignErr error + + // call flags + resolveRoleCalled bool + assignRoleCalled bool +} + +func (m *mockSPProvisioner) CreateApplication(_ context.Context, displayName string) (string, string, error) { + m.createAppName = displayName + if m.createAppErr != nil { + return "", "", m.createAppErr + } + return m.appObjectID, m.appID, nil +} + +func (m *mockSPProvisioner) AddPassword(_ context.Context, objectID string) (string, error) { + m.addPasswordObjID = objectID + if m.addPwErr != nil { + return "", m.addPwErr + } + return m.secret, nil +} + +func (m *mockSPProvisioner) CreateServicePrincipal(_ context.Context, appID string) (string, error) { + m.createSPAppID = appID + if m.createSPErr != nil { + return "", m.createSPErr + } + return m.principalID, nil +} + +func (m *mockSPProvisioner) ResolveRoleDefinitionID(_ context.Context, scope, roleName string) (string, error) { + m.resolveRoleCalled = true + m.resolveRoleScope = scope + m.resolveRoleName = roleName + if m.resolveErr != nil { + return "", m.resolveErr + } + return m.roleDefID, nil +} + +func (m *mockSPProvisioner) AssignRole(_ context.Context, scope, principalID, roleDefinitionID string) error { + m.assignRoleCalled = true + m.assignScope = scope + m.assignPrincipalID = principalID + m.assignRoleDefID = roleDefinitionID + return m.assignErr +} + +const ( + testSubID = "11111111-2222-3333-4444-555555555555" + testTenantID = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" +) + +func TestCreateAzureServicePrincipal_Success(t *testing.T) { + m := &mockSPProvisioner{ + appObjectID: "app-object-id", + appID: "app-client-id", + secret: "super-secret-password", + principalID: "sp-principal-id", + roleDefID: "/subscriptions/" + testSubID + "/provider/.../roleDefinitions/role-guid", + } + + result, err := createAzureServicePrincipal(context.Background(), m, testSubID, testTenantID) + require.NoError(t, err) + + // App name must be exactly "CUDly". + assert.Equal(t, "CUDly", m.createAppName) + assert.Equal(t, azureSPName, m.createAppName) + + // Password added to the created application object. + assert.Equal(t, "app-object-id", m.addPasswordObjID) + + // Service principal created for the application's client ID. + assert.Equal(t, "app-client-id", m.createSPAppID) + + // Role resolved by exact display name at subscription scope. + assert.True(t, m.resolveRoleCalled) + assert.Equal(t, "Reservations Administrator", m.resolveRoleName) + assert.Equal(t, azureSPRoleName, m.resolveRoleName) + assert.Equal(t, "/subscriptions/"+testSubID, m.resolveRoleScope) + + // Role assignment binds the SP principal to the resolved role at subscription scope. + assert.True(t, m.assignRoleCalled) + assert.Equal(t, "/subscriptions/"+testSubID, m.assignScope) + assert.Equal(t, "sp-principal-id", m.assignPrincipalID) + assert.Equal(t, m.roleDefID, m.assignRoleDefID) + + // Result surfaces appId, secret and tenant (the create-for-rbac fields). + assert.Equal(t, "app-client-id", result.AppID) + assert.Equal(t, "super-secret-password", result.ClientSecret) + assert.Equal(t, testTenantID, result.TenantID) +} + +func TestCreateAzureServicePrincipal_ErrorPropagation(t *testing.T) { + tests := []struct { + name string + setup func(*mockSPProvisioner) + wantErrPart string + wantNoAssign bool + wantNoResolve bool + }{ + { + name: "create application fails", + setup: func(m *mockSPProvisioner) { m.createAppErr = errors.New("graph 403") }, + wantErrPart: "failed to create application registration", + wantNoAssign: true, + wantNoResolve: true, + }, + { + 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, + }, + { + 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, + }, + { + name: "resolve role fails", + setup: func(m *mockSPProvisioner) { m.resolveErr = errors.New("role not found") }, + wantErrPart: "failed to resolve", + wantNoAssign: true, + }, + { + name: "assign role fails", + setup: func(m *mockSPProvisioner) { m.assignErr = errors.New("rbac 403") }, + wantErrPart: "failed to assign", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + m := &mockSPProvisioner{ + appObjectID: "app-object-id", + appID: "app-client-id", + secret: "secret", + principalID: "sp-principal-id", + roleDefID: "role-def-id", + } + tt.setup(m) + + _, err := createAzureServicePrincipal(context.Background(), m, testSubID, testTenantID) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErrPart) + + if tt.wantNoResolve { + assert.False(t, m.resolveRoleCalled, "ResolveRoleDefinitionID should not have been called") + } + if tt.wantNoAssign { + assert.False(t, m.assignRoleCalled, "AssignRole should not have been called") + } + }) + } +} + +func TestCreateAzureServicePrincipal_ScopeFormat(t *testing.T) { + m := &mockSPProvisioner{ + appObjectID: "o", appID: "a", secret: "s", principalID: "p", roleDefID: "r", + } + _, err := createAzureServicePrincipal(context.Background(), m, testSubID, testTenantID) + require.NoError(t, err) + // Scope must be exactly /subscriptions/ (subscription scope, not resource group). + assert.Equal(t, "/subscriptions/"+testSubID, m.resolveRoleScope) + assert.Equal(t, "/subscriptions/"+testSubID, m.assignScope) +} diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 00f92f8d8..743b28f11 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -271,8 +271,8 @@ func printGCPConfigurationSuccess(creds GCPCredentials) { // // gcloud auth application-default login // -// The wizard Step 3 onwards will call this function; if ADC is not available the -// calls will fail and the wizard suggests the manual gcloud CLI fallback. +// The wizard Step 3 onwards calls this function; if ADC is not available the +// calls fail loud with a hint to run "gcloud auth application-default login". func newGCPAPIOption(ctx context.Context) (option.ClientOption, error) { ts, err := google.DefaultTokenSource(ctx, "https://www.googleapis.com/auth/cloud-platform", @@ -437,16 +437,16 @@ func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) er // "gcloud auth application-default login" if they want to use ADC. // // Step 2 (list projects): performed via the Cloud Resource Manager SDK v1, -// using Application Default Credentials (ADC). Falls back to "gcloud projects -// list" if ADC is not available. +// using Application Default Credentials (ADC). Fails loud if ADC is not +// available (no CLI fallback). // // Step 3 (gcloud config set project): sets local gcloud config state. There is // no cloud-API equivalent for writing to the local gcloud configuration file, // so this step remains a CLI call. // // Steps 4-6 (create SA, grant role, create key): performed via GCP IAM and -// Cloud Resource Manager SDK v1 APIs using ADC. Fall back to the gcloud CLI -// if ADC is not available. +// Cloud Resource Manager SDK v1 APIs using ADC. Fail loud on any SDK error +// (no CLI fallback). func runGCPSetupCommands(ctx context.Context, reader *bufio.Reader) (string, error) { if err := gcpStepLogin(reader); err != nil { return "", err @@ -489,10 +489,8 @@ func gcpStepSelectProject(ctx context.Context, reader *bufio.Reader) (string, er fmt.Println() if err := listGCPProjects(ctx); err != nil { - fmt.Printf("SDK project list failed (%v); falling back to gcloud.\n\n", err) - if cliErr := promptAndRunGCPCommand(reader, "List Projects", "gcloud projects list", "gcloud", "projects", "list"); cliErr != nil { - return "", cliErr - } + return "", fmt.Errorf("failed to list GCP projects via SDK: %w\n"+ + "Ensure Application Default Credentials are set: run 'gcloud auth application-default login' first", err) } fmt.Println() @@ -508,6 +506,7 @@ func gcpStepSelectProject(ctx context.Context, reader *bufio.Reader) (string, er // (writes to ~/.config/gcloud/properties) with no cloud-API equivalent. fmt.Println() fmt.Println("Setting gcloud project context (local config)...") + //nolint:gosec // G204: local gcloud config write, hardcoded args + validated projectID, no shell cmd := exec.Command("gcloud", "config", "set", "project", projectID) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr @@ -517,8 +516,8 @@ func gcpStepSelectProject(ctx context.Context, reader *bufio.Reader) (string, er return projectID, nil } -// gcpStepCreateServiceAccount creates the CUDly service account via SDK with -// a gcloud CLI fallback. +// gcpStepCreateServiceAccount creates the CUDly service account via the IAM +// SDK. It fails loud on any SDK error (no CLI fallback). func gcpStepCreateServiceAccount(ctx context.Context, reader *bufio.Reader, projectID string) (string, error) { saName := "cudly-service-account" saEmail := fmt.Sprintf("%s@%s.iam.gserviceaccount.com", saName, projectID) @@ -533,20 +532,12 @@ func gcpStepCreateServiceAccount(ctx context.Context, reader *bufio.Reader, proj choice, _ := reader.ReadString('\n') switch strings.ToLower(strings.TrimSpace(choice)) { case "r", "run", "": - email, sdkErr := createGCPServiceAccount(ctx, projectID, saName) - if sdkErr != nil { - fmt.Printf("SDK call failed (%v); falling back to gcloud.\n", sdkErr) - display := fmt.Sprintf(`gcloud iam service-accounts create %s --display-name="CUDly Service Account" --description="Service account for CUDly commitment management"`, saName) - if err := promptAndRunGCPCommand(reader, "Create Service Account", display, - "gcloud", "iam", "service-accounts", "create", saName, - "--display-name=CUDly Service Account", - "--description=Service account for CUDly commitment management"); err != nil { - return "", err - } - } else { - saEmail = email - fmt.Printf("Service account created: %s\n", saEmail) + email, err := createGCPServiceAccount(ctx, projectID, saName) + if err != nil { + return "", err } + saEmail = email + fmt.Printf("Service account created: %s\n", saEmail) case "s", "skip": fmt.Println("Skipping Create Service Account") default: @@ -555,7 +546,9 @@ func gcpStepCreateServiceAccount(ctx context.Context, reader *bufio.Reader, proj return saEmail, nil } -// gcpStepGrantRole grants the compute.admin role to the service account. +// gcpStepGrantRole grants the compute.admin role to the service account via +// the Cloud Resource Manager SDK. It fails loud on any SDK error (no CLI +// fallback). func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEmail string) error { member := fmt.Sprintf("serviceAccount:%s", saEmail) role := "roles/compute.admin" @@ -570,13 +563,8 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm choice, _ := reader.ReadString('\n') switch strings.ToLower(strings.TrimSpace(choice)) { case "r", "run", "": - if sdkErr := grantGCPIAMRole(ctx, projectID, member, role); sdkErr != nil { - fmt.Printf("SDK call failed (%v); falling back to gcloud.\n", sdkErr) - display := fmt.Sprintf(`gcloud projects add-iam-policy-binding %s --member="%s" --role="%s"`, projectID, member, role) - return promptAndRunGCPCommand(reader, "Grant Compute Admin Role", display, - "gcloud", "projects", "add-iam-policy-binding", projectID, - fmt.Sprintf("--member=%s", member), - fmt.Sprintf("--role=%s", role)) + if err := grantGCPIAMRole(ctx, projectID, member, role); err != nil { + return err } fmt.Printf("Role %s granted to %s on project %s.\n", role, saEmail, projectID) case "s", "skip": @@ -605,17 +593,10 @@ func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) choice, _ := reader.ReadString('\n') switch strings.ToLower(strings.TrimSpace(choice)) { case "r", "run", "": - if sdkErr := createGCPServiceAccountKey(ctx, saEmail, keyFile); sdkErr != nil { - fmt.Printf("SDK call failed (%v); falling back to gcloud.\n", sdkErr) - display := fmt.Sprintf(`gcloud iam service-accounts keys create %s --iam-account=%s`, keyFile, saEmail) - if err := promptAndRunGCPCommand(reader, "Create Key File", display, - "gcloud", "iam", "service-accounts", "keys", "create", keyFile, - fmt.Sprintf("--iam-account=%s", saEmail)); err != nil { - return "", err - } - } else { - fmt.Printf("Key file written to: %s\n", keyFile) + if err := createGCPServiceAccountKey(ctx, saEmail, keyFile); err != nil { + return "", err } + fmt.Printf("Key file written to: %s\n", keyFile) case "s", "skip": fmt.Println("Skipping Create Key") default: @@ -643,7 +624,8 @@ func readRequiredInputLine(reader *bufio.Reader, prompt, fieldName string) (stri } // promptAndRunGCPCommand shows a command and asks to run or skip. -// Takes explicit program and args to avoid command injection via string splitting. +// It is used only for the interactive "gcloud auth login" auth bootstrap +// (Step 1), which has no SDK equivalent that preserves the cached-credential UX. func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, program string, args ...string) error { fmt.Printf("Command: %s\n", displayCmd) fmt.Println() @@ -668,6 +650,7 @@ func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, progr } // executeGCPCommand runs a gcloud command with explicit program and arguments. +// It is used only for the interactive "gcloud auth login" auth bootstrap. // The caller's reader is threaded through to the retry prompt so all input // is consumed from one consistent buffered stream (a fresh // bufio.NewReader(os.Stdin) here would drop input already buffered by the @@ -677,6 +660,7 @@ func executeGCPCommand(reader *bufio.Reader, displayCmd string, program string, fmt.Printf("Executing: %s\n", displayCmd) fmt.Println(strings.Repeat("-", 60)) + //nolint:gosec // G204: interactive operator auth (gcloud auth login), hardcoded args, no shell cmd := exec.Command(program, args...) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr diff --git a/go.mod b/go.mod index 66e80f243..0b858817a 100644 --- a/go.mod +++ b/go.mod @@ -24,16 +24,16 @@ require ( cloud.google.com/go/longrunning v0.9.0 // indirect cloud.google.com/go/recommender v1.13.6 // indirect cloud.google.com/go/resourcemanager v1.10.7 // indirect - github.com/Azure/azure-sdk-for-go/sdk/azcore v1.18.1 - github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.10.1 - github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.1 // indirect + github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.1 + github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1 + github.com/Azure/azure-sdk-for-go/sdk/internal v1.12.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor v1.2.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.4.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption v1.1.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3 v3.0.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql v1.2.0 // indirect - github.com/AzureAD/microsoft-authentication-library-for-go v1.4.2 // indirect + github.com/AzureAD/microsoft-authentication-library-for-go v1.6.0 // indirect github.com/aws/aws-sdk-go-v2/credentials v1.16.13 github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.14.10 // indirect github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.21 // indirect @@ -61,9 +61,9 @@ require ( github.com/stretchr/objx v0.5.3 // indirect go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.61.0 // indirect go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.61.0 // indirect - go.opentelemetry.io/otel v1.42.0 // indirect - go.opentelemetry.io/otel/metric v1.42.0 // indirect - go.opentelemetry.io/otel/trace v1.42.0 // indirect + go.opentelemetry.io/otel v1.43.0 // indirect + go.opentelemetry.io/otel/metric v1.43.0 // indirect + go.opentelemetry.io/otel/trace v1.43.0 // indirect golang.org/x/crypto v0.53.0 golang.org/x/net v0.56.0 // indirect golang.org/x/oauth2 v0.36.0 @@ -83,6 +83,7 @@ require ( require ( cloud.google.com/go/kms v1.29.0 cloud.google.com/go/secretmanager v1.16.0 + github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2 v2.2.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits v1.0.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 @@ -109,6 +110,7 @@ require ( github.com/golang-migrate/migrate/v4 v4.19.1 github.com/google/uuid v1.6.0 github.com/jackc/pgx/v5 v5.9.2 + github.com/microsoftgraph/msgraph-sdk-go v1.99.0 github.com/pashagolub/pgxmock/v4 v4.9.0 github.com/testcontainers/testcontainers-go v0.42.0 github.com/testcontainers/testcontainers-go/modules/postgres v0.42.0 @@ -158,6 +160,14 @@ require ( github.com/lib/pq v1.10.9 // indirect github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 // indirect github.com/magiconair/properties v1.8.10 // indirect + github.com/microsoft/kiota-abstractions-go v1.9.4 // indirect + github.com/microsoft/kiota-authentication-azure-go v1.3.1 // indirect + github.com/microsoft/kiota-http-go v1.5.6 // indirect + github.com/microsoft/kiota-serialization-form-go v1.1.3 // indirect + github.com/microsoft/kiota-serialization-json-go v1.1.2 // indirect + github.com/microsoft/kiota-serialization-multipart-go v1.1.2 // indirect + github.com/microsoft/kiota-serialization-text-go v1.1.3 // indirect + github.com/microsoftgraph/msgraph-sdk-go-core v1.4.1 // indirect github.com/moby/docker-image-spec v1.3.1 // indirect github.com/moby/go-archive v0.2.0 // indirect github.com/moby/moby/api v1.54.1 // indirect @@ -174,6 +184,7 @@ require ( github.com/shirou/gopsutil/v4 v4.26.3 // indirect github.com/sirupsen/logrus v1.9.4 // indirect github.com/spiffe/go-spiffe/v2 v2.6.0 // indirect + github.com/std-uritemplate/std-uritemplate/go/v2 v2.0.3 // indirect github.com/tklauser/go-sysconf v0.3.16 // indirect github.com/tklauser/numcpus v0.11.0 // indirect github.com/yusufpapurcu/wmi v1.2.4 // indirect diff --git a/go.sum b/go.sum index 4766f018c..6c049b038 100644 --- a/go.sum +++ b/go.sum @@ -36,16 +36,18 @@ dario.cat/mergo v1.0.2 h1:85+piFYR1tMbRrLcDwR18y4UKJ3aH1Tbzi24VRW1TK8= dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA= github.com/AdaLogics/go-fuzz-headers v0.0.0-20240806141605-e8a1dd7889d6 h1:He8afgbRMd7mFxO99hRNu+6tazq8nFF9lIwo9JFroBk= github.com/AdaLogics/go-fuzz-headers v0.0.0-20240806141605-e8a1dd7889d6/go.mod h1:8o94RPi1/7XTJvwPpRSzSUedZrtlirdB3r9Z20bi2f8= -github.com/Azure/azure-sdk-for-go/sdk/azcore v1.18.1 h1:Wc1ml6QlJs2BHQ/9Bqu1jiyggbsSjramq2oUmp5WeIo= -github.com/Azure/azure-sdk-for-go/sdk/azcore v1.18.1/go.mod h1:Ot/6aikWnKWi4l9QB7qVSwa8iMphQNqkWALMoNT3rzM= -github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.10.1 h1:B+blDbyVIG3WaikNxPnhPiJ1MThR03b3vKGtER95TP4= -github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.10.1/go.mod h1:JdM5psgjfBf5fo2uWOZhflPWyDBZ/O/CNAH9CtsuZE4= +github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.1 h1:jHb/wfvRikGdxMXYV3QG/SzUOPYN9KEUUuC0Yd0/vC0= +github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.1/go.mod h1:pzBXCYn05zvYIrwLgtK8Ap8QcjRg+0i76tMQdWN6wOk= +github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1 h1:Hk5QBxZQC1jb2Fwj6mpzme37xbCDdNTxU7O9eb5+LB4= +github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1/go.mod h1:IYus9qsFobWIc2YVwe/WPjcnyCkPKtnHAqUYeebc8z0= github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache v0.3.2 h1:yz1bePFlP5Vws5+8ez6T3HWXPmwOK7Yvq8QxDBD3SKY= github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache v0.3.2/go.mod h1:Pa9ZNPuoNu/GztvBSKk9J1cDJW6vk/n0zLtV4mgd8N8= -github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.1 h1:FPKJS1T+clwv+OLGt13a8UjqeRuh0O4SJ3lUriThc+4= -github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.1/go.mod h1:j2chePtV91HrC22tGoRX3sGY42uF13WzmmV80/OdVAA= +github.com/Azure/azure-sdk-for-go/sdk/internal v1.12.0 h1:fhqpLE3UEXi9lPaBRpQ6XuRW0nU7hgg4zlmZZa+a9q4= +github.com/Azure/azure-sdk-for-go/sdk/internal v1.12.0/go.mod h1:7dCRMLwisfRH3dBupKeNCioWYUZ4SS09Z14H+7i8ZoY= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor v1.2.0 h1:3ddjPq/3A/oB2u7LdohEr900EGP5l1MnAiNc3EbY1E4= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/advisor/armadvisor v1.2.0/go.mod h1:oZ73p8dR7aZI+TJo5Ul92oCoVubMYPBo39eTsWa0AiQ= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2 v2.2.0 h1:Hp+EScFOu9HeCbeW8WU2yQPJd4gGwhMgKxWe+G6jNzw= +github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2 v2.2.0/go.mod h1:/pz8dyNQe+Ey3yBp/XuYz7oqX8YDNWVpPB0hH3XWfbc= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits v1.0.0 h1:4JHm1qT4VKYZ8aGP1dJBg+wk/WgN/KitLNtQs6jFNfA= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits v1.0.0/go.mod h1:GBFVpPKOTooUG7VM93ip046AqLf4qubcIEsU3js4v3g= github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.4.0 h1:QfV5XZt6iNa2aWMAt96CZEbfJ7kgG/qYIpq465Shr5E= @@ -84,8 +86,8 @@ github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c h1:udKWzYgxTojEK github.com/Azure/go-ansiterm v0.0.0-20250102033503-faa5f7b0171c/go.mod h1:xomTg63KZ2rFqZQzSB4Vz2SUXa1BpHTVz9L5PTmPC4E= github.com/AzureAD/microsoft-authentication-extensions-for-go/cache v0.1.1 h1:WJTmL004Abzc5wDB5VtZG2PJk5ndYDgVacGqfirKxjM= github.com/AzureAD/microsoft-authentication-extensions-for-go/cache v0.1.1/go.mod h1:tCcJZ0uHAmvjsVYzEFivsRTN00oz5BEsRgQHu5JZ9WE= -github.com/AzureAD/microsoft-authentication-library-for-go v1.4.2 h1:oygO0locgZJe7PpYPXT5A29ZkwJaPqcva7BVeemZOZs= -github.com/AzureAD/microsoft-authentication-library-for-go v1.4.2/go.mod h1:wP83P5OoQ5p6ip3ScPr0BAq0BvuPAvacpEuSzyouqAI= +github.com/AzureAD/microsoft-authentication-library-for-go v1.6.0 h1:XRzhVemXdgvJqCH0sFfrBUTnUJSBrBf7++ypk+twtRs= +github.com/AzureAD/microsoft-authentication-library-for-go v1.6.0/go.mod h1:HKpQxkWaGLJ+D/5H8QRpyQXA1eKjxkFlOMwck5+33Jk= github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.31.0 h1:DHa2U07rk8syqvCge0QIGMCE1WxGj9njT44GH7zNJLQ= github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.31.0/go.mod h1:P4WPRUkOhJC13W//jWpyfJNDAIpvRbAUIYLX/4jtlE0= github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.53.0 h1:owcC2UnmsZycprQ5RfRgjydWhuoxg71LUfyiQdijZuM= @@ -192,8 +194,6 @@ github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfv github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f h1:lO4WD4F/rVNCu3HqELle0jiPLLBs70cWOduZpkS1E78= -github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f/go.mod h1:cuUVRXasLTGF7a8hSLbxyZXjz+1KgoB3wDUb6vlszIc= github.com/dhui/dktest v0.4.6 h1:+DPKyScKSEp3VLtbMDHcUq6V5Lm5zfZZVb0Sk7Ahom4= github.com/dhui/dktest v0.4.6/go.mod h1:JHTSYDtKkvFNFHJKqCzVzqXecyv+tKt8EzceOmQOgbU= github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk= @@ -272,6 +272,24 @@ github.com/magiconair/properties v1.8.10 h1:s31yESBquKXCV9a/ScB3ESkOjUYYv+X0rg8S github.com/magiconair/properties v1.8.10/go.mod h1:Dhd985XPs7jluiymwWYZ0G4Z61jb3vdS329zhj2hYo0= github.com/mdelapenya/tlscert v0.2.0 h1:7H81W6Z/4weDvZBNOfQte5GpIMo0lGYEeWbkGp5LJHI= github.com/mdelapenya/tlscert v0.2.0/go.mod h1:O4njj3ELLnJjGdkN7M/vIVCpZ+Cf0L6muqOG4tLSl8o= +github.com/microsoft/kiota-abstractions-go v1.9.4 h1:VI3UVzSCQHHhRswe3jyaAQHUQWIFhUMp0z5mtZbTbcs= +github.com/microsoft/kiota-abstractions-go v1.9.4/go.mod h1:f06pl3qSyvUHEfVNkiRpXPkafx7khZqQEb71hN/pmuU= +github.com/microsoft/kiota-authentication-azure-go v1.3.1 h1:AGta92S6IL1E6ZMDb8YYB7NVNTIFUakbtLKUdY5RTuw= +github.com/microsoft/kiota-authentication-azure-go v1.3.1/go.mod h1:26zylt2/KfKwEWZSnwHaMxaArpbyN/CuzkbotdYXF0g= +github.com/microsoft/kiota-http-go v1.5.6 h1:KBdk7sxWYXZnRRExLjIcNt4I7LoOfh/XQJWWid4zBKE= +github.com/microsoft/kiota-http-go v1.5.6/go.mod h1:bpJkXfBAcnmiXRg03GXdnb/vF3Sqk3+EgLvXXjmzzQM= +github.com/microsoft/kiota-serialization-form-go v1.1.3 h1:eUY8eHXPFe4ma8cAdx0ya3g4NPlZgbPT+GlFC3xcgGY= +github.com/microsoft/kiota-serialization-form-go v1.1.3/go.mod h1:RMO99zyik+NvZjdVcIeyu6ikyfuKhQtzq2RK0fWJJio= +github.com/microsoft/kiota-serialization-json-go v1.1.2 h1:eJrPWeQ665nbjO0gsHWJ0Bw6V/ZHHU1OfFPaYfRG39k= +github.com/microsoft/kiota-serialization-json-go v1.1.2/go.mod h1:deaGt7fjZarywyp7TOTiRsjfYiyWxwJJPQZytXwYQn8= +github.com/microsoft/kiota-serialization-multipart-go v1.1.2 h1:1pUyA1QgIeKslQwbk7/ox1TehjlCUUT3r1f8cNlkvn4= +github.com/microsoft/kiota-serialization-multipart-go v1.1.2/go.mod h1:j2K7ZyYErloDu7Kuuk993DsvfoP7LPWvAo7rfDpdPio= +github.com/microsoft/kiota-serialization-text-go v1.1.3 h1:8z7Cebn0YAAr++xswVgfdxZjnAZ4GOB9O7XP4+r5r/M= +github.com/microsoft/kiota-serialization-text-go v1.1.3/go.mod h1:NDSvz4A3QalGMjNboKKQI9wR+8k+ih8UuagNmzIRgTQ= +github.com/microsoftgraph/msgraph-sdk-go v1.99.0 h1:FRR4RcbuhKBQP3klg4jCp05ntz/NrmZtd3tYILqGt8A= +github.com/microsoftgraph/msgraph-sdk-go v1.99.0/go.mod h1:qxzY5SaoPigY6/Dpyfg4uigQjNDvL+sZl6fzD6EpWeQ= +github.com/microsoftgraph/msgraph-sdk-go-core v1.4.1 h1:k3YIaJm57ufoEX0KdsEY4l1X9BAMxEqrwr4a7WMRDzY= +github.com/microsoftgraph/msgraph-sdk-go-core v1.4.1/go.mod h1:yNqPNhXee2w9cZzkJW5mL1utVMSInsQSo/TyEB5sup8= github.com/moby/docker-image-spec v1.3.1 h1:jMKff3w6PgbfSa69GfNg+zN/XLhfXJGnEx3Nl2EsFP0= github.com/moby/docker-image-spec v1.3.1/go.mod h1:eKmb5VW8vQEh/BAr2yvVNvuiJuY6UIocYsFu/DxxRpo= github.com/moby/go-archive v0.2.0 h1:zg5QDUM2mi0JIM9fdQZWC7U8+2ZfixfTYoHL7rWUcP8= @@ -309,8 +327,6 @@ github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRI github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/power-devops/perfstat v0.0.0-20240221224432-82ca36839d55 h1:o4JXh1EVt9k/+g42oCprj/FisM4qX9L3sZB3upGN2ZU= github.com/power-devops/perfstat v0.0.0-20240221224432-82ca36839d55/go.mod h1:OmDBASR4679mdNQnz2pUhc2G8CO2JrUAVFDRBDP/hJE= -github.com/redis/go-redis/v9 v9.8.0 h1:q3nRvjrlge/6UD7eTu/DSg2uYiU2mCL0G/uzBWqhicI= -github.com/redis/go-redis/v9 v9.8.0/go.mod h1:huWgSWd8mW6+m0VPhJjSSQ+d6Nh1VICQ6Q5lHuCH/Iw= github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= @@ -324,6 +340,8 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/spiffe/go-spiffe/v2 v2.6.0 h1:l+DolpxNWYgruGQVV0xsfeya3CsC7m8iBzDnMpsbLuo= github.com/spiffe/go-spiffe/v2 v2.6.0/go.mod h1:gm2SeUoMZEtpnzPNs2Csc0D/gX33k1xIx7lEzqblHEs= +github.com/std-uritemplate/std-uritemplate/go/v2 v2.0.3 h1:7hth9376EoQEd1hH4lAp3vnaLP2UMyxuMMghLKzDHyU= +github.com/std-uritemplate/std-uritemplate/go/v2 v2.0.3/go.mod h1:Z5KcoM0YLC7INlNhEezeIZ0TZNYf7WSNO0Lvah4DSeQ= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.5.3 h1:jmXUvGomnU1o3W/V5h2VEradbpJDwGrzugQQvL0POH4= github.com/stretchr/objx v0.5.3/go.mod h1:rDQraq+vQZU7Fde9LOZLr8Tax6zZvy4kuNKF+QYS+U0= @@ -349,18 +367,18 @@ go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.6 go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.61.0/go.mod h1:snMWehoOh2wsEwnvvwtDyFCxVeDAODenXHtn5vzrKjo= go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.61.0 h1:F7Jx+6hwnZ41NSFTO5q4LYDtJRXBf2PD0rNBkeB/lus= go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.61.0/go.mod h1:UHB22Z8QsdRDrnAtX4PntOl36ajSxcdUMt1sF7Y6E7Q= -go.opentelemetry.io/otel v1.42.0 h1:lSQGzTgVR3+sgJDAU/7/ZMjN9Z+vUip7leaqBKy4sho= -go.opentelemetry.io/otel v1.42.0/go.mod h1:lJNsdRMxCUIWuMlVJWzecSMuNjE7dOYyWlqOXWkdqCc= +go.opentelemetry.io/otel v1.43.0 h1:mYIM03dnh5zfN7HautFE4ieIig9amkNANT+xcVxAj9I= +go.opentelemetry.io/otel v1.43.0/go.mod h1:JuG+u74mvjvcm8vj8pI5XiHy1zDeoCS2LB1spIq7Ay0= go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.36.0 h1:rixTyDGXFxRy1xzhKrotaHy3/KXdPhlWARrCgK+eqUY= go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.36.0/go.mod h1:dowW6UsM9MKbJq5JTz2AMVp3/5iW5I/TStsk8S+CfHw= -go.opentelemetry.io/otel/metric v1.42.0 h1:2jXG+3oZLNXEPfNmnpxKDeZsFI5o4J+nz6xUlaFdF/4= -go.opentelemetry.io/otel/metric v1.42.0/go.mod h1:RlUN/7vTU7Ao/diDkEpQpnz3/92J9ko05BIwxYa2SSI= +go.opentelemetry.io/otel/metric v1.43.0 h1:d7638QeInOnuwOONPp4JAOGfbCEpYb+K6DVWvdxGzgM= +go.opentelemetry.io/otel/metric v1.43.0/go.mod h1:RDnPtIxvqlgO8GRW18W6Z/4P462ldprJtfxHxyKd2PY= go.opentelemetry.io/otel/sdk v1.42.0 h1:LyC8+jqk6UJwdrI/8VydAq/hvkFKNHZVIWuslJXYsDo= go.opentelemetry.io/otel/sdk v1.42.0/go.mod h1:rGHCAxd9DAph0joO4W6OPwxjNTYWghRWmkHuGbayMts= go.opentelemetry.io/otel/sdk/metric v1.42.0 h1:D/1QR46Clz6ajyZ3G8SgNlTJKBdGp84q9RKCAZ3YGuA= go.opentelemetry.io/otel/sdk/metric v1.42.0/go.mod h1:Ua6AAlDKdZ7tdvaQKfSmnFTdHx37+J4ba8MwVCYM5hc= -go.opentelemetry.io/otel/trace v1.42.0 h1:OUCgIPt+mzOnaUTpOQcBiM/PLQ/Op7oq6g4LenLmOYY= -go.opentelemetry.io/otel/trace v1.42.0/go.mod h1:f3K9S+IFqnumBkKhRJMeaZeNk9epyhnCmQh/EysQCdc= +go.opentelemetry.io/otel/trace v1.43.0 h1:BkNrHpup+4k4w+ZZ86CZoHHEkohws8AY+WTX09nk+3A= +go.opentelemetry.io/otel/trace v1.43.0/go.mod h1:/QJhyVBUUswCphDVxq+8mld+AvhXZLhe+8WVFxiFff0= golang.org/x/crypto v0.53.0 h1:QZ4Muo8THX6CizN2vPPd5fBGHyogrdK9fG4wLPFUsto= golang.org/x/crypto v0.53.0/go.mod h1:DNLU434OwVakk9PzuwV8w62mAJpRJL3vsgcfp4Qnsio= golang.org/x/net v0.56.0 h1:Rw8j/hFzGvJUZwNBXnAtf5sVDVt+65SK2C7IxCxZt5o= From 793b99234ab53abbc57961a3e50f9982b4b13a94 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 25 Jun 2026 01:22:42 +0200 Subject: [PATCH 3/9] 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. --- ci_cd_sanity_tests/pkg/sanity/azure/azure.go | 5 +- cmd/configure_azure.go | 44 ++++--- cmd/configure_azure_sp.go | 44 +++++-- cmd/configure_azure_sp_test.go | 66 ++++++++-- cmd/configure_gcp.go | 127 ++++++++++++++----- 5 files changed, 221 insertions(+), 65 deletions(-) diff --git a/ci_cd_sanity_tests/pkg/sanity/azure/azure.go b/ci_cd_sanity_tests/pkg/sanity/azure/azure.go index 94c8b70e6..492e8072c 100644 --- a/ci_cd_sanity_tests/pkg/sanity/azure/azure.go +++ b/ci_cd_sanity_tests/pkg/sanity/azure/azure.go @@ -260,7 +260,10 @@ func runAccountShowCheck(ctx context.Context, subscriptionID string, cred azcore } sub := resp.Subscription - info := azureSubscriptionInfo{State: string(*sub.State)} + info := azureSubscriptionInfo{} + if sub.State != nil { + info.State = string(*sub.State) + } if sub.SubscriptionID != nil { info.ID = *sub.SubscriptionID } diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 2668d5ce6..610a29b89 100644 --- a/cmd/configure_azure.go +++ b/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 { diff --git a/cmd/configure_azure_sp.go b/cmd/configure_azure_sp.go index b0e08a3f3..486538af2 100644 --- a/cmd/configure_azure_sp.go +++ b/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{ diff --git a/cmd/configure_azure_sp_test.go b/cmd/configure_azure_sp_test.go index 3ffe80baf..d6b26bb38 100644 --- a/cmd/configure_azure_sp_test.go +++ b/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,6 +148,7 @@ 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", @@ -143,6 +156,7 @@ func TestCreateAzureServicePrincipal_ErrorPropagation(t *testing.T) { wantErrPart: "failed to create application registration", wantNoAssign: true, wantNoResolve: true, + wantRollback: false, // nothing was created, nothing to roll back }, { name: "add password fails", @@ -150,6 +164,7 @@ func TestCreateAzureServicePrincipal_ErrorPropagation(t *testing.T) { wantErrPart: "failed to add password credential", wantNoAssign: true, wantNoResolve: true, + wantRollback: true, }, { name: "create service principal fails", @@ -157,17 +172,20 @@ func TestCreateAzureServicePrincipal_ErrorPropagation(t *testing.T) { 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", diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 743b28f11..43544e86b 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -12,6 +12,7 @@ import ( "path/filepath" "regexp" "strings" + "time" "github.com/aws/aws-sdk-go-v2/aws" awsconfig "github.com/aws/aws-sdk-go-v2/config" @@ -259,6 +260,10 @@ func printGCPConfigurationSuccess(creds GCPCredentials) { fmt.Println("\nCUDly can now manage GCP Committed Use Discounts.") } +// gcpSDKCallTimeout bounds each GCP SDK helper so an ADC lookup or API call +// cannot hang indefinitely (the calls inherit context.Background()). +const gcpSDKCallTimeout = 60 * time.Second + // newGCPAPIOption returns an oauth2 token source option for the google.golang.org // API client, using Application Default Credentials. ADC resolves credentials in // priority order: GOOGLE_APPLICATION_CREDENTIALS env var, gcloud ADC cache @@ -289,6 +294,9 @@ func newGCPAPIOption(ctx context.Context) (option.ClientOption, error) { // Resource Manager API v1 and prints them in a table. This replaces the // "gcloud projects list" CLI call. func listGCPProjects(ctx context.Context) error { + ctx, cancel := context.WithTimeout(ctx, gcpSDKCallTimeout) + defer cancel() + opt, err := newGCPAPIOption(ctx) if err != nil { return err @@ -317,6 +325,9 @@ func listGCPProjects(ctx context.Context) error { // createGCPServiceAccount creates a GCP IAM service account via the IAM API v1. // This replaces "gcloud iam service-accounts create". func createGCPServiceAccount(ctx context.Context, projectID, saName string) (string, error) { + ctx, cancel := context.WithTimeout(ctx, gcpSDKCallTimeout) + defer cancel() + opt, err := newGCPAPIOption(ctx) if err != nil { return "", err @@ -347,6 +358,9 @@ func createGCPServiceAccount(ctx context.Context, projectID, saName string) (str // Cloud Resource Manager API v1. This replaces // "gcloud projects add-iam-policy-binding". func grantGCPIAMRole(ctx context.Context, projectID, member, role string) error { + ctx, cancel := context.WithTimeout(ctx, gcpSDKCallTimeout) + defer cancel() + opt, err := newGCPAPIOption(ctx) if err != nil { return err @@ -357,32 +371,24 @@ func grantGCPIAMRole(ctx context.Context, projectID, member, role string) error return fmt.Errorf("failed to create resource manager client: %w", err) } - // Get current policy. - policy, err := svc.Projects.GetIamPolicy(projectID, &cloudresourcemanager.GetIamPolicyRequest{}).Context(ctx).Do() + // Request policy version 3 so conditional (IAM condition) bindings are + // returned and preserved on the read-modify-write round-trip; otherwise + // the SetIamPolicy below would silently drop them. + policy, err := svc.Projects.GetIamPolicy(projectID, &cloudresourcemanager.GetIamPolicyRequest{ + Options: &cloudresourcemanager.GetPolicyOptions{RequestedPolicyVersion: 3}, + }).Context(ctx).Do() if err != nil { return fmt.Errorf("failed to get IAM policy for project %s: %w", projectID, err) } - // Append the binding (or extend existing one). - found := false - for _, b := range policy.Bindings { - if b.Role == role { - for _, m := range b.Members { - if m == member { - // Already bound; nothing to do. - return nil - } - } - b.Members = append(b.Members, member) - found = true - break - } + if !addMemberToPolicyBinding(policy, member, role) { + // Member already bound to the role; nothing to write. + return nil } - if !found { - policy.Bindings = append(policy.Bindings, &cloudresourcemanager.Binding{ - Role: role, - Members: []string{member}, - }) + + // Write the policy back at version 3 to retain any conditional bindings. + if policy.Version < 3 { + policy.Version = 3 } _, err = svc.Projects.SetIamPolicy(projectID, &cloudresourcemanager.SetIamPolicyRequest{ Policy: policy, @@ -393,10 +399,56 @@ func grantGCPIAMRole(ctx context.Context, projectID, member, role string) error return nil } +// addMemberToPolicyBinding adds member to the binding for role in policy, +// creating the binding if absent. It returns false if member is already bound +// (no change needed) and true if the policy was modified. +func addMemberToPolicyBinding(policy *cloudresourcemanager.Policy, member, role string) bool { + for _, b := range policy.Bindings { + if b.Role != role { + continue + } + for _, m := range b.Members { + if m == member { + return false + } + } + b.Members = append(b.Members, member) + return true + } + policy.Bindings = append(policy.Bindings, &cloudresourcemanager.Binding{ + Role: role, + Members: []string{member}, + }) + return true +} + // createGCPServiceAccountKey creates a JSON key for the given service account -// and writes it to keyFile. Returns the path written. This replaces +// and writes it to keyFile. This replaces // "gcloud iam service-accounts keys create --iam-account=". +// +// To avoid orphaning a live IAM key on a local failure, it reserves the +// destination file with exclusive-create semantics BEFORE minting the remote +// key, and deletes the freshly created remote key if decoding or writing the +// key material fails. func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) error { + ctx, cancel := context.WithTimeout(ctx, gcpSDKCallTimeout) + defer cancel() + + // Reserve the destination file first (fails if it already exists), so we + // never mint a remote key we cannot persist locally. + f, err := os.OpenFile(keyFile, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) + if err != nil { + return fmt.Errorf("failed to reserve key file %s: %w", keyFile, err) + } + // Best-effort: remove the reserved file if we return before writing it. + wrote := false + defer func() { + _ = f.Close() + if !wrote { + _ = os.Remove(keyFile) + } + }() + opt, err := newGCPAPIOption(ctx) if err != nil { return err @@ -415,15 +467,28 @@ func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) er return fmt.Errorf("failed to create service account key: %w", err) } + // From here on, any failure must delete the newly minted remote key so it + // does not linger as an active, unused credential. + deleteRemoteKey := func(cause error) error { + // Use a fresh context: the parent may already be canceled/expired. + delCtx, delCancel := context.WithTimeout(context.Background(), gcpSDKCallTimeout) + defer delCancel() + if _, delErr := svc.Projects.ServiceAccounts.Keys.Delete(key.Name).Context(delCtx).Do(); delErr != nil { + return fmt.Errorf("%w; additionally failed to delete the orphaned remote key %s: %w", cause, key.Name, delErr) + } + return cause + } + // PrivateKeyData is base64-encoded JSON. decoded, err := base64.StdEncoding.DecodeString(key.PrivateKeyData) if err != nil { - return fmt.Errorf("failed to decode key data: %w", err) + return deleteRemoteKey(fmt.Errorf("failed to decode key data: %w", err)) } - if err := os.WriteFile(keyFile, decoded, 0600); err != nil { - return fmt.Errorf("failed to write key file %s: %w", keyFile, err) + if _, err := f.Write(decoded); err != nil { + return deleteRemoteKey(fmt.Errorf("failed to write key file %s: %w", keyFile, err)) } + wrote = true return nil } @@ -575,7 +640,10 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm return nil } -// gcpStepCreateKey creates a JSON key file for the service account. +// gcpStepCreateKey creates a JSON key file for the service account. It returns +// the written key-file path only when a key was actually created; on skip or +// an unknown choice it returns an empty string so the caller knows to prompt +// for an existing credentials file instead of assuming one was written. func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) (string, error) { home, err := os.UserHomeDir() if err != nil { @@ -597,16 +665,17 @@ func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) return "", err } fmt.Printf("Key file written to: %s\n", keyFile) + fmt.Println() + return keyFile, nil case "s", "skip": fmt.Println("Skipping Create Key") default: fmt.Printf("Unknown option, skipping\n") } + // No key file was written; the caller will prompt for an existing one. fmt.Println() - fmt.Printf("Key file at: %s\n", keyFile) - fmt.Println() - return keyFile, nil + return "", nil } // readRequiredInputLine prints prompt, reads a line, trims whitespace, and From 86a385a4a01bd8f03b3806f3d81abb017421ada9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 26 Jun 2026 15:32:15 +0200 Subject: [PATCH 4/9] fix(configure): check stdin read errors in new SDK wizard steps The PR replaced CLI shell-outs with SDK calls and introduced new interactive steps in the Azure SP and GCP SA wizards. Each step discarded the error from reader.ReadString('\n') with the legacy `choice, _ :=` shape inherited from the surrounding code, which golangci-lint's errcheck (check-blank: true) flags as a new violation and which conflicts with the project's "no silent fallbacks" rule for fail-loud error handling. Surface the read error from the four PR-introduced prompts so an unexpected EOF / closed-stdin propagates an explicit error instead of silently treating it as the default "run" choice: * cmd/configure_azure.go: azureStepCreateServicePrincipal * cmd/configure_gcp.go: gcpStepCreateServiceAccount, gcpStepGrantRole, gcpStepCreateKey Also fix the new json.Marshal in ci_cd_sanity_tests/pkg/sanity/azure/azure.go:encodeAccountJSON to check the (impossible-for-this-struct) error and return nil so the caller skips the expected-checks step rather than feeding the validator a partially-encoded payload. Pre-existing reader.ReadString errcheck patterns on the same files (promptAndRun*Command, executeExplicit/GCPCommand, getGCPCredentialsFilePath, gcpStepSelectProject, azureStepListSubscriptions) are present on main and outside this PR's scope; tracked separately. All cmd/ and ci_cd_sanity_tests/ tests pass (767 tests across 13 packages). --- ci_cd_sanity_tests/pkg/sanity/azure/azure.go | 10 +++++-- cmd/configure_azure.go | 2 +- cmd/configure_gcp.go | 29 +++++++++++++------- 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/ci_cd_sanity_tests/pkg/sanity/azure/azure.go b/ci_cd_sanity_tests/pkg/sanity/azure/azure.go index 492e8072c..7a7210089 100644 --- a/ci_cd_sanity_tests/pkg/sanity/azure/azure.go +++ b/ci_cd_sanity_tests/pkg/sanity/azure/azure.go @@ -99,7 +99,10 @@ func validateAccountExpectations(opts Options, accountOut []byte) report.CheckRe // encodeAccountJSON serialises azureSubscriptionInfo into the same JSON shape // that "az account show -o json" produced so that validateAccountExpectations -// can be reused without modification. +// can be reused without modification. The struct is composed of plain strings +// so json.Marshal cannot realistically fail; a nil return on the impossible +// error path lets the caller skip the expected-checks step rather than feed +// validateAccountExpectations a partially-encoded payload. func encodeAccountJSON(info azureSubscriptionInfo) []byte { a := azAccountShow{ ID: info.ID, @@ -107,7 +110,10 @@ func encodeAccountJSON(info azureSubscriptionInfo) []byte { Name: info.Name, State: info.State, } - b, _ := json.Marshal(a) + b, err := json.Marshal(a) + if err != nil { + return nil + } return b } diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 610a29b89..3fada25de 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -392,7 +392,7 @@ func azureStepListSubscriptions(ctx context.Context, reader *bufio.Reader) (stri fmt.Print("Enter your Subscription ID from above: ") subscriptionID, err := readTrimmedLine(reader) if err != nil { - return fmt.Errorf("failed to read subscription ID: %w", err) + return "", fmt.Errorf("failed to read subscription ID: %w", err) } if subscriptionID == "" { diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 43544e86b..beb4a202d 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -594,12 +594,15 @@ func gcpStepCreateServiceAccount(ctx context.Context, reader *bufio.Reader, proj fmt.Println() fmt.Printf("[R]un, [S]kip? (creates service account '%s' via SDK) ", saName) - choice, _ := reader.ReadString('\n') + choice, err := reader.ReadString('\n') + if err != nil { + return "", fmt.Errorf("failed to read service-account choice: %w", err) + } switch strings.ToLower(strings.TrimSpace(choice)) { case "r", "run", "": - email, err := createGCPServiceAccount(ctx, projectID, saName) - if err != nil { - return "", err + email, createErr := createGCPServiceAccount(ctx, projectID, saName) + if createErr != nil { + return "", createErr } saEmail = email fmt.Printf("Service account created: %s\n", saEmail) @@ -625,11 +628,14 @@ func gcpStepGrantRole(ctx context.Context, reader *bufio.Reader, projectID, saEm fmt.Println() fmt.Printf("[R]un, [S]kip? (grants %s to %s on project %s via SDK) ", role, saEmail, projectID) - choice, _ := reader.ReadString('\n') + choice, err := reader.ReadString('\n') + if err != nil { + return fmt.Errorf("failed to read grant-role choice: %w", err) + } switch strings.ToLower(strings.TrimSpace(choice)) { case "r", "run", "": - if err := grantGCPIAMRole(ctx, projectID, member, role); err != nil { - return err + if grantErr := grantGCPIAMRole(ctx, projectID, member, role); grantErr != nil { + return grantErr } fmt.Printf("Role %s granted to %s on project %s.\n", role, saEmail, projectID) case "s", "skip": @@ -658,11 +664,14 @@ func gcpStepCreateKey(ctx context.Context, reader *bufio.Reader, saEmail string) fmt.Println() fmt.Printf("[R]un, [S]kip? (creates key for %s, writes to %s via SDK) ", saEmail, keyFile) - choice, _ := reader.ReadString('\n') + choice, err := reader.ReadString('\n') + if err != nil { + return "", fmt.Errorf("failed to read create-key choice: %w", err) + } switch strings.ToLower(strings.TrimSpace(choice)) { case "r", "run", "": - if err := createGCPServiceAccountKey(ctx, saEmail, keyFile); err != nil { - return "", err + if keyErr := createGCPServiceAccountKey(ctx, saEmail, keyFile); keyErr != nil { + return "", keyErr } fmt.Printf("Key file written to: %s\n", keyFile) fmt.Println() From aab3cddeaf86b5fcfc9d86a8f903d02ea6f601c7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 4 Jul 2026 01:17:53 +0200 Subject: [PATCH 5/9] fix(configure): retry Azure role assignment on eventual-consistency PrincipalNotFound Azure AD replication is eventually consistent: a service principal created moments ago may not yet be visible to the ARM role-assignment API in a different region, returning PrincipalNotFound or ServicePrincipalNotFound. Without retry the wizard fails immediately in this race window. - Extract roleAssigner interface so the retry loop is unit-testable without hitting Azure. - Add isPrincipalNotFoundErr helper covering the canonical error codes (PrincipalNotFound, ServicePrincipalNotFound) and the older message-body form ("does not exist in the directory"). - Retry g.roleAsgn.Create with bounded exponential back-off (5s initial, doubles to 30s cap, 3-minute total budget) on PrincipalNotFound; fail loud with a diagnostic message and the MS troubleshooting URL if the budget expires. - Use ctx-aware select in the sleep to treat context cancellation as terminal (not accumulated as lastErr). - Add graphSPProvisioner.retryInitial / retryBudget fields (set to constants in production, overridable to millisecond values in tests) so tests complete in <100ms. - Add TestGraphSPProvisioner_AssignRole_* and TestIsPrincipalNotFoundErr covering: retry-then-succeed, budget-exhausted, non-retryable error, context-cancellation, and error-code detection. --- cmd/configure_azure_sp.go | 101 +++++++++++++++++++++-- cmd/configure_azure_sp_test.go | 144 +++++++++++++++++++++++++++++++++ 2 files changed, 240 insertions(+), 5 deletions(-) diff --git a/cmd/configure_azure_sp.go b/cmd/configure_azure_sp.go index 486538af2..379fc6ca8 100644 --- a/cmd/configure_azure_sp.go +++ b/cmd/configure_azure_sp.go @@ -2,9 +2,12 @@ package main import ( "context" + "errors" "fmt" + "strings" "time" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" armauthorization "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2" "github.com/google/uuid" msgraphsdk "github.com/microsoftgraph/msgraph-sdk-go" @@ -12,6 +15,53 @@ import ( graphmodels "github.com/microsoftgraph/msgraph-sdk-go/models" ) +// roleAssignRetryInitial is the first back-off interval when retrying a role +// assignment that fails with PrincipalNotFound. Each subsequent interval is +// doubled up to roleAssignRetryMax. +const ( + roleAssignRetryInitial = 5 * time.Second + roleAssignRetryMax = 30 * time.Second + // roleAssignRetryBudget is the ceiling on total retry time. Azure AD + // replication is usually complete within seconds but can take up to ~10 + // minutes in the worst case (see feedback_tf_depends_on_rbac.md). Three + // minutes covers the large majority of propagation windows without + // keeping the operator waiting too long. + roleAssignRetryBudget = 3 * time.Minute +) + +// roleAssigner is the minimal subset of *armauthorization.RoleAssignmentsClient +// used by graphSPProvisioner. The interface exists solely to allow the retry +// logic in AssignRole to be exercised in unit tests without hitting Azure. +type roleAssigner interface { + Create( + ctx context.Context, + scope, roleAssignmentName string, + parameters armauthorization.RoleAssignmentCreateParameters, + options *armauthorization.RoleAssignmentsClientCreateOptions, + ) (armauthorization.RoleAssignmentsClientCreateResponse, error) +} + +// isPrincipalNotFoundErr reports whether err is an Azure ARM +// PrincipalNotFound / ServicePrincipalNotFound response -- the transient +// condition that occurs when a newly-created Entra ID service principal has +// not yet replicated to the ARM region handling the role-assignment request. +// It covers the canonical error codes as well as the message-body form +// returned by some older API versions. +func isPrincipalNotFoundErr(err error) bool { + var respErr *azcore.ResponseError + if !errors.As(err, &respErr) { + return false + } + switch respErr.ErrorCode { + case "PrincipalNotFound", "ServicePrincipalNotFound": + return true + } + // Older ARM API versions surface the condition as HTTP 400 without a + // distinct error code; instead the response body contains the canonical + // phrase "does not exist in the directory". + return strings.Contains(respErr.Error(), "does not exist in the directory") +} + // azureSPName is the Azure AD application / service principal display name. // It matches the name the previous "az ad sp create-for-rbac --name CUDly" // call used so the resulting identity is interchangeable. @@ -125,7 +175,12 @@ func createAzureServicePrincipal(ctx context.Context, p azureSPProvisioner, subs type graphSPProvisioner struct { graph *msgraphsdk.GraphServiceClient roleDefs *armauthorization.RoleDefinitionsClient - roleAsgn *armauthorization.RoleAssignmentsClient + roleAsgn roleAssigner + // retryInitial and retryBudget control the PrincipalNotFound retry loop + // in AssignRole. They are set to the package constants by + // newGraphSPProvisioner and overridden in tests to keep test duration short. + retryInitial time.Duration + retryBudget time.Duration } // newGraphSPProvisioner builds a graphSPProvisioner authenticated with the @@ -155,7 +210,13 @@ func newGraphSPProvisioner(subscriptionID string) (*graphSPProvisioner, error) { return nil, fmt.Errorf("failed to create role assignments client: %w", err) } - return &graphSPProvisioner{graph: graph, roleDefs: roleDefs, roleAsgn: roleAsgn}, nil + return &graphSPProvisioner{ + graph: graph, + roleDefs: roleDefs, + roleAsgn: roleAsgn, + retryInitial: roleAssignRetryInitial, + retryBudget: roleAssignRetryBudget, + }, nil } func (g *graphSPProvisioner) CreateApplication(ctx context.Context, displayName string) (objectID, appID string, err error) { @@ -235,12 +296,42 @@ func (g *graphSPProvisioner) ResolveRoleDefinitionID(ctx context.Context, scope, func (g *graphSPProvisioner) AssignRole(ctx context.Context, scope, principalID, roleDefinitionID string) error { principalType := armauthorization.PrincipalTypeServicePrincipal - _, err := g.roleAsgn.Create(ctx, scope, uuid.NewString(), armauthorization.RoleAssignmentCreateParameters{ + params := armauthorization.RoleAssignmentCreateParameters{ Properties: &armauthorization.RoleAssignmentProperties{ PrincipalID: &principalID, RoleDefinitionID: &roleDefinitionID, PrincipalType: &principalType, }, - }, nil) - return err + } + + // Azure AD replication is eventually consistent: a service principal + // created moments ago may not yet be visible to the ARM role-assignment + // API in a different region, returning PrincipalNotFound. Retry with + // bounded exponential back-off until the SP propagates or the budget is + // exhausted (see also: feedback_tf_depends_on_rbac.md). + deadline := time.Now().Add(g.retryBudget) + delay := g.retryInitial + for { + _, err := g.roleAsgn.Create(ctx, scope, uuid.NewString(), params, nil) + if err == nil { + return nil + } + if !isPrincipalNotFoundErr(err) { + return err + } + if time.Now().After(deadline) { + return fmt.Errorf( + "service principal %q did not propagate to ARM within %v "+ + "(Azure AD replication is eventually consistent -- "+ + "https://learn.microsoft.com/en-us/azure/role-based-access-control/troubleshooting): %w", + principalID, g.retryBudget, err) + } + // Context cancellation is terminal: do not continue retrying. + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(delay): + } + delay = min(delay*2, roleAssignRetryMax) + } } diff --git a/cmd/configure_azure_sp_test.go b/cmd/configure_azure_sp_test.go index d6b26bb38..2ad3800f7 100644 --- a/cmd/configure_azure_sp_test.go +++ b/cmd/configure_azure_sp_test.go @@ -4,7 +4,10 @@ import ( "context" "errors" "testing" + "time" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + armauthorization "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -252,3 +255,144 @@ func TestCreateAzureServicePrincipal_ScopeFormat(t *testing.T) { assert.Equal(t, "/subscriptions/"+testSubID, m.resolveRoleScope) assert.Equal(t, "/subscriptions/"+testSubID, m.assignScope) } + +// fakeRoleAssigner is a test double for roleAssigner that fails with a +// PrincipalNotFound error for the first failsRemaining calls, then succeeds. +// If otherErr is set it is always returned instead (to test non-retryable paths). +type fakeRoleAssigner struct { + failsRemaining int + principalNotFoundErr *azcore.ResponseError + otherErr error + callCount int +} + +func (f *fakeRoleAssigner) Create( + _ context.Context, + _, _ string, + _ armauthorization.RoleAssignmentCreateParameters, + _ *armauthorization.RoleAssignmentsClientCreateOptions, +) (armauthorization.RoleAssignmentsClientCreateResponse, error) { + f.callCount++ + if f.otherErr != nil { + return armauthorization.RoleAssignmentsClientCreateResponse{}, f.otherErr + } + if f.failsRemaining > 0 { + f.failsRemaining-- + return armauthorization.RoleAssignmentsClientCreateResponse{}, f.principalNotFoundErr + } + return armauthorization.RoleAssignmentsClientCreateResponse{}, nil +} + +// newFakePrincipalNotFoundProvisioner returns a graphSPProvisioner wired to a +// fakeRoleAssigner with very short retry timing so tests complete in +// milliseconds rather than minutes. +func newFakePrincipalNotFoundProvisioner(fake *fakeRoleAssigner) *graphSPProvisioner { + return &graphSPProvisioner{ + roleAsgn: fake, + retryInitial: time.Millisecond, + retryBudget: 50 * time.Millisecond, + } +} + +// TestGraphSPProvisioner_AssignRole_RetrySucceeds verifies that AssignRole +// retries when ARM returns PrincipalNotFound and eventually succeeds. +func TestGraphSPProvisioner_AssignRole_RetrySucceeds(t *testing.T) { + principalNotFound := &azcore.ResponseError{ErrorCode: "PrincipalNotFound", StatusCode: 400} + fake := &fakeRoleAssigner{failsRemaining: 2, principalNotFoundErr: principalNotFound} + p := newFakePrincipalNotFoundProvisioner(fake) + + err := p.AssignRole(context.Background(), "/subscriptions/sub", "sp-id", "role-def-id") + + require.NoError(t, err) + assert.Equal(t, 3, fake.callCount, + "should call Create 3 times: 2 PrincipalNotFound failures then 1 success") +} + +// TestGraphSPProvisioner_AssignRole_ExhaustsRetryBudget verifies that +// AssignRole fails loud with a clear message when the service principal never +// propagates within the budget. +func TestGraphSPProvisioner_AssignRole_ExhaustsRetryBudget(t *testing.T) { + principalNotFound := &azcore.ResponseError{ErrorCode: "PrincipalNotFound", StatusCode: 400} + fake := &fakeRoleAssigner{failsRemaining: 100, principalNotFoundErr: principalNotFound} + p := newFakePrincipalNotFoundProvisioner(fake) + + err := p.AssignRole(context.Background(), "/subscriptions/sub", "sp-id", "role-def-id") + + require.Error(t, err) + assert.Contains(t, err.Error(), "did not propagate", + "error must explain that the SP did not propagate") + assert.Contains(t, err.Error(), "sp-id", + "error must identify the principal that failed to propagate") + assert.True(t, fake.callCount >= 1, "should have attempted Create at least once") +} + +// TestGraphSPProvisioner_AssignRole_NonRetryableError verifies that a +// non-PrincipalNotFound error is returned immediately without retrying. +func TestGraphSPProvisioner_AssignRole_NonRetryableError(t *testing.T) { + fake := &fakeRoleAssigner{otherErr: errors.New("authorization denied")} + p := newFakePrincipalNotFoundProvisioner(fake) + + err := p.AssignRole(context.Background(), "/subscriptions/sub", "sp-id", "role-def-id") + + require.Error(t, err) + assert.Contains(t, err.Error(), "authorization denied") + assert.Equal(t, 1, fake.callCount, "should not retry on non-PrincipalNotFound errors") +} + +// TestGraphSPProvisioner_AssignRole_ContextCancellation verifies that context +// cancellation is treated as a terminal stop and does not continue retrying. +func TestGraphSPProvisioner_AssignRole_ContextCancellation(t *testing.T) { + principalNotFound := &azcore.ResponseError{ErrorCode: "PrincipalNotFound", StatusCode: 400} + fake := &fakeRoleAssigner{failsRemaining: 100, principalNotFoundErr: principalNotFound} + p := newFakePrincipalNotFoundProvisioner(fake) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the first retry sleep fires + + err := p.AssignRole(ctx, "/subscriptions/sub", "sp-id", "role-def-id") + + require.Error(t, err) + // The first Create call returns PrincipalNotFound; the subsequent select + // detects ctx.Done() and returns ctx.Err() immediately. + assert.Equal(t, 1, fake.callCount, "should stop retrying after context is cancelled") +} + +// TestIsPrincipalNotFoundErr covers the error-code detection helper directly. +func TestIsPrincipalNotFoundErr(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "PrincipalNotFound error code", + err: &azcore.ResponseError{ErrorCode: "PrincipalNotFound", StatusCode: 400}, + want: true, + }, + { + name: "ServicePrincipalNotFound error code", + err: &azcore.ResponseError{ErrorCode: "ServicePrincipalNotFound", StatusCode: 400}, + want: true, + }, + { + name: "unrelated ARM error", + err: &azcore.ResponseError{ErrorCode: "AuthorizationFailed", StatusCode: 403}, + want: false, + }, + { + name: "plain error", + err: errors.New("network error"), + want: false, + }, + { + name: "nil error", + err: nil, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isPrincipalNotFoundErr(tt.err)) + }) + } +} From 68227fb807b3642d292abbcaae0b3f4544fb24a5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 10 Jul 2026 16:23:02 +0200 Subject: [PATCH 6/9] style(configure): fix PR-introduced lint issues in Azure SDK wizard Resolve the six golangci findings introduced on this branch's own lines (no base debt touched): - misspell: serialises/behaviour/cancelled to US spelling - gocritic unnamedResult: name runAccountShowCheck results - govet fieldalignment: reorder two test structs to pack pointers Whole-repo Lint Code / Security Scanning remain red on pre-existing base debt, tracked separately and not owned by this PR. --- ci_cd_sanity_tests/pkg/sanity/azure/azure.go | 12 +++---- .../pkg/sanity/azure/azure_test.go | 26 ++++++--------- cmd/configure_azure.go | 28 ++++++++-------- cmd/configure_azure_sp_test.go | 6 ++-- cmd/configure_gcp.go | 32 +++++++++---------- 5 files changed, 48 insertions(+), 56 deletions(-) diff --git a/ci_cd_sanity_tests/pkg/sanity/azure/azure.go b/ci_cd_sanity_tests/pkg/sanity/azure/azure.go index 7a7210089..9351e4cf7 100644 --- a/ci_cd_sanity_tests/pkg/sanity/azure/azure.go +++ b/ci_cd_sanity_tests/pkg/sanity/azure/azure.go @@ -47,11 +47,11 @@ type azAccountShow struct { } `json:"user"` } -func truncate(s string, max int) string { - if len(s) <= max { +func truncate(s string, maxLen int) string { + if len(s) <= maxLen { return s } - return s[:max] + "...(truncated)" + return s[:maxLen] + "...(truncated)" } // validateAccountExpectations parses "az account show" JSON output and checks @@ -97,7 +97,7 @@ func validateAccountExpectations(opts Options, accountOut []byte) report.CheckRe return check } -// encodeAccountJSON serialises azureSubscriptionInfo into the same JSON shape +// encodeAccountJSON serializes azureSubscriptionInfo into the same JSON shape // that "az account show -o json" produced so that validateAccountExpectations // can be reused without modification. The struct is composed of plain strings // so json.Marshal cannot realistically fail; a nil return on the impossible @@ -251,7 +251,7 @@ func runAccountSetCheck(ctx context.Context, subscriptionID string, cred azcore. // runAccountShowCheck retrieves subscription identity information. // It returns the check result and the JSON-encoded account info (for use by // validateAccountExpectations). The JSON is empty on failure. -func runAccountShowCheck(ctx context.Context, subscriptionID string, cred azcore.TokenCredential) (report.CheckResult, []byte) { +func runAccountShowCheck(ctx context.Context, subscriptionID string, cred azcore.TokenCredential) (result report.CheckResult, accountJSON []byte) { cr := newCheckResult("azure:account:show", time.Now().UTC()) cr.Details["subscriptionID"] = subscriptionID @@ -293,7 +293,7 @@ func runAccountShowCheck(ctx context.Context, subscriptionID string, cred azcore // AZURE_CLIENT_ID / AZURE_TENANT_ID / AZURE_CLIENT_SECRET environment // variables (service-principal flow). On an operator workstation it falls back // to AzureCLICredential (i.e. the session established by "az login"), so the -// behaviour is identical to the previous CLI-based implementation. +// behavior is identical to the previous CLI-based implementation. func Run(ctx context.Context, opts Options) (*report.Report, error) { if opts.SubscriptionID == "" { opts.SubscriptionID = os.Getenv("AZURE_SUBSCRIPTION_ID") diff --git a/ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go b/ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go index 570cb4d06..d89a0d11d 100644 --- a/ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go +++ b/ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go @@ -48,12 +48,12 @@ func TestTruncate(t *testing.T) { assert.Equal(t, "abcde...(truncated)", truncate("abcdef", 5)) } -func accountJSON(id, tenantID, name, state string) []byte { +func accountJSON(id, tenantID string) []byte { b, err := json.Marshal(azAccountShow{ ID: id, TenantID: tenantID, - Name: name, - State: state, + Name: "My Sub", + State: "Enabled", }) if err != nil { panic(err) @@ -70,11 +70,9 @@ func TestValidateAccountExpectations(t *testing.T) { wantMsgPart string // substring expected in Message when non-empty }{ { - name: "valid json, no expectations", - opts: Options{}, - accountOut: accountJSON( - "sub-123", "tenant-456", "My Sub", "Enabled", - ), + name: "valid json, no expectations", + opts: Options{}, + accountOut: accountJSON("sub-123", "tenant-456"), wantStatus: report.StatusPass, }, { @@ -83,9 +81,7 @@ func TestValidateAccountExpectations(t *testing.T) { ExpectedSubID: "sub-123", ExpectedTenantID: "tenant-456", }, - accountOut: accountJSON( - "sub-123", "tenant-456", "My Sub", "Enabled", - ), + accountOut: accountJSON("sub-123", "tenant-456"), wantStatus: report.StatusPass, }, { @@ -93,9 +89,7 @@ func TestValidateAccountExpectations(t *testing.T) { opts: Options{ ExpectedSubID: "sub-expected", }, - accountOut: accountJSON( - "sub-actual", "tenant-456", "My Sub", "Enabled", - ), + accountOut: accountJSON("sub-actual", "tenant-456"), wantStatus: report.StatusFail, wantMsgPart: "unexpected subscription", }, @@ -104,9 +98,7 @@ func TestValidateAccountExpectations(t *testing.T) { opts: Options{ ExpectedTenantID: "tenant-expected", }, - accountOut: accountJSON( - "sub-123", "tenant-actual", "My Sub", "Enabled", - ), + accountOut: accountJSON("sub-123", "tenant-actual"), wantStatus: report.StatusFail, wantMsgPart: "unexpected tenant", }, diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 3fada25de..42b1e366c 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -26,10 +26,10 @@ import ( "golang.org/x/term" ) -// azureUUIDRegex validates Azure UUIDs (subscription IDs, tenant IDs, client IDs) +// azureUUIDRegex validates Azure UUIDs (subscription IDs, tenant IDs, client IDs). var azureUUIDRegex = regexp.MustCompile(`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$`) -// validateAzureUUID validates an Azure UUID to prevent command injection +// validateAzureUUID validates an Azure UUID to prevent command injection. func validateAzureUUID(uuid, fieldName string) error { if !azureUUIDRegex.MatchString(uuid) { return fmt.Errorf("invalid %s format: must be a valid UUID (xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx)", fieldName) @@ -43,13 +43,13 @@ func validateAzureUUID(uuid, fieldName string) error { // is still valid input. io.EOF with no data, or any other error, is returned. func readTrimmedLine(reader *bufio.Reader) (string, error) { input, err := reader.ReadString('\n') - if err != nil && !(errors.Is(err, io.EOF) && input != "") { + if err != nil && (!errors.Is(err, io.EOF) || input == "") { return "", err } return strings.TrimSpace(input), nil } -// AzureCredentials holds the Azure Service Principal credentials +// AzureCredentials holds the Azure Service Principal credentials. type AzureCredentials struct { TenantID string `json:"tenant_id"` ClientID string `json:"client_id"` @@ -57,7 +57,7 @@ type AzureCredentials struct { SubscriptionID string `json:"subscription_id"` } -// AzureConfigOptions holds configuration for the Azure config command +// AzureConfigOptions holds configuration for the Azure config command. type AzureConfigOptions struct { StackName string Profile string @@ -116,7 +116,7 @@ func validateAzureCredentialFields(creds AzureCredentials) error { return validateAzureUUID(creds.SubscriptionID, "Subscription ID") } -// storeAzureCredentials stores Azure credentials in the secrets store +// storeAzureCredentials stores Azure credentials in the secrets store. func storeAzureCredentials(ctx context.Context, store SecretsStore, stackName string, creds AzureCredentials) error { if err := validateAzureCredentialFields(creds); err != nil { return err @@ -135,7 +135,7 @@ func storeAzureCredentials(ctx context.Context, store SecretsStore, stackName st } // Marshal credentials to JSON - credJSON, err := json.Marshal(creds) + credJSON, err := json.Marshal(creds) // #nosec G117 -- AzureCredentials marshaled intentionally for Secrets Manager storage if err != nil { return fmt.Errorf("failed to marshal credentials: %w", err) } @@ -192,7 +192,7 @@ func runConfigureAzure(cmd *cobra.Command, args []string) error { return nil } -// loadAWSConfigForAzure loads AWS configuration with optional profile +// loadAWSConfigForAzure loads AWS configuration with optional profile. func loadAWSConfigForAzure(ctx context.Context) (aws.Config, error) { var opts []func(*awsconfig.LoadOptions) error if azureOpts.Profile != "" { @@ -207,7 +207,7 @@ func loadAWSConfigForAzure(ctx context.Context) (aws.Config, error) { return cfg, nil } -// collectAzureCredentials collects Azure credentials interactively or from flags +// collectAzureCredentials collects Azure credentials interactively or from flags. func collectAzureCredentials(reader *bufio.Reader) (AzureCredentials, error) { creds := AzureCredentials{ TenantID: azureOpts.TenantID, @@ -231,7 +231,7 @@ func collectAzureCredentials(reader *bufio.Reader) (AzureCredentials, error) { return creds, nil } -// promptForAzureCredentialFields prompts for missing credential fields +// promptForAzureCredentialFields prompts for missing credential fields. func promptForAzureCredentialFields(reader *bufio.Reader, creds *AzureCredentials) error { if creds.TenantID == "" { fmt.Print("Azure Tenant ID: ") @@ -253,7 +253,7 @@ func promptForAzureCredentialFields(reader *bufio.Reader, creds *AzureCredential if creds.ClientSecret == "" { fmt.Print("Client Secret (password): ") - secret, err := term.ReadPassword(int(syscall.Stdin)) + secret, err := term.ReadPassword(syscall.Stdin) if err != nil { return fmt.Errorf("failed to read secret: %w", err) } @@ -501,7 +501,7 @@ func printAzureSPResult(result azureSPResult, subscriptionID string) { // 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 string, program string, args ...string) error { +func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd, program string, args ...string) error { fmt.Printf("Command: %s\n", displayCmd) fmt.Println() fmt.Printf("[R]un, [S]kip? ") @@ -531,7 +531,7 @@ func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd string, // is consumed from one consistent buffered stream (a fresh // bufio.NewReader(os.Stdin) here would drop input already buffered by the // caller's reader, breaking piped input after earlier prompts). -func executeExplicitCommand(reader *bufio.Reader, displayCmd string, program string, args ...string) error { +func executeExplicitCommand(reader *bufio.Reader, displayCmd, program string, args ...string) error { fmt.Println() fmt.Printf("Executing: %s\n", displayCmd) fmt.Println(strings.Repeat("-", 60)) @@ -552,7 +552,7 @@ func executeExplicitCommand(reader *bufio.Reader, displayCmd string, program str if readErr != nil { return fmt.Errorf("failed to read response: %w", readErr) } - if strings.ToLower(response) != "y" { + if !strings.EqualFold(response, "y") { return fmt.Errorf("command failed: %w", err) } } diff --git a/cmd/configure_azure_sp_test.go b/cmd/configure_azure_sp_test.go index 2ad3800f7..a37c593ad 100644 --- a/cmd/configure_azure_sp_test.go +++ b/cmd/configure_azure_sp_test.go @@ -260,9 +260,9 @@ func TestCreateAzureServicePrincipal_ScopeFormat(t *testing.T) { // PrincipalNotFound error for the first failsRemaining calls, then succeeds. // If otherErr is set it is always returned instead (to test non-retryable paths). type fakeRoleAssigner struct { - failsRemaining int principalNotFoundErr *azcore.ResponseError otherErr error + failsRemaining int callCount int } @@ -354,14 +354,14 @@ func TestGraphSPProvisioner_AssignRole_ContextCancellation(t *testing.T) { require.Error(t, err) // The first Create call returns PrincipalNotFound; the subsequent select // detects ctx.Done() and returns ctx.Err() immediately. - assert.Equal(t, 1, fake.callCount, "should stop retrying after context is cancelled") + assert.Equal(t, 1, fake.callCount, "should stop retrying after context is canceled") } // TestIsPrincipalNotFoundErr covers the error-code detection helper directly. func TestIsPrincipalNotFoundErr(t *testing.T) { tests := []struct { - name string err error + name string want bool }{ { diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index beb4a202d..abbf1ce09 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -24,10 +24,10 @@ import ( "google.golang.org/api/option" ) -// gcpProjectIDRegex validates GCP project IDs (lowercase letters, digits, hyphens, 6-30 chars) +// gcpProjectIDRegex validates GCP project IDs (lowercase letters, digits, hyphens, 6-30 chars). var gcpProjectIDRegex = regexp.MustCompile(`^[a-z][a-z0-9-]{4,28}[a-z0-9]$`) -// validateGCPProjectID validates a GCP project ID to prevent command injection +// validateGCPProjectID validates a GCP project ID to prevent command injection. func validateGCPProjectID(projectID string) error { if !gcpProjectIDRegex.MatchString(projectID) { return fmt.Errorf("invalid GCP project ID format: must be 6-30 lowercase letters, digits, or hyphens, starting with a letter") @@ -35,7 +35,7 @@ func validateGCPProjectID(projectID string) error { return nil } -// GCPCredentials holds the GCP Service Account credentials +// GCPCredentials holds the GCP Service Account credentials. type GCPCredentials struct { Type string `json:"type"` ProjectID string `json:"project_id"` @@ -49,7 +49,7 @@ type GCPCredentials struct { ClientX509CertURL string `json:"client_x509_cert_url,omitempty"` } -// GCPConfigOptions holds configuration for the GCP config command +// GCPConfigOptions holds configuration for the GCP config command. type GCPConfigOptions struct { StackName string Profile string @@ -87,8 +87,8 @@ func init() { configureGCPCmd.Flags().BoolVar(&gcpOpts.SkipSetup, "skip-setup", false, "Skip GCP CLI setup commands (gcloud login, create service account)") } -// storeGCPCredentials stores GCP credentials in the secrets store -func storeGCPCredentials(ctx context.Context, store SecretsStore, stackName string, credsJSON string) error { +// storeGCPCredentials stores GCP credentials in the secrets store. +func storeGCPCredentials(ctx context.Context, store SecretsStore, stackName, credsJSON string) error { // Validate that we have valid JSON var creds GCPCredentials if err := json.Unmarshal([]byte(credsJSON), &creds); err != nil { @@ -167,7 +167,7 @@ func runConfigureGCP(cmd *cobra.Command, args []string) error { return nil } -// getGCPCredentialsFilePath determines the credentials file path from options or user input +// getGCPCredentialsFilePath determines the credentials file path from options or user input. func getGCPCredentialsFilePath(ctx context.Context, reader *bufio.Reader) (string, error) { var credsFile string @@ -197,7 +197,7 @@ func getGCPCredentialsFilePath(ctx context.Context, reader *bufio.Reader) (strin return credsFile, nil } -// loadAWSConfigForGCP loads AWS configuration with optional profile +// loadAWSConfigForGCP loads AWS configuration with optional profile. func loadAWSConfigForGCP(ctx context.Context) (aws.Config, error) { var opts []func(*awsconfig.LoadOptions) error if gcpOpts.Profile != "" { @@ -212,7 +212,7 @@ func loadAWSConfigForGCP(ctx context.Context) (aws.Config, error) { return cfg, nil } -// loadAndUpdateGCPCredentials loads, parses, and optionally updates GCP credentials +// loadAndUpdateGCPCredentials loads, parses, and optionally updates GCP credentials. func loadAndUpdateGCPCredentials(credsFile string) (GCPCredentials, []byte, error) { expandedPath := expandHomeDirectory(credsFile) @@ -222,13 +222,13 @@ func loadAndUpdateGCPCredentials(credsFile string) (GCPCredentials, []byte, erro } var creds GCPCredentials - if err := json.Unmarshal(credsData, &creds); err != nil { + if err = json.Unmarshal(credsData, &creds); err != nil { return GCPCredentials{}, nil, fmt.Errorf("failed to parse credentials file: %w", err) } if gcpOpts.ProjectID != "" { creds.ProjectID = gcpOpts.ProjectID - credsData, err = json.Marshal(creds) + credsData, err = json.Marshal(creds) // #nosec G117 -- GCPCredentials marshaled intentionally for Secrets Manager storage if err != nil { return GCPCredentials{}, nil, fmt.Errorf("failed to marshal updated credentials: %w", err) } @@ -237,7 +237,7 @@ func loadAndUpdateGCPCredentials(credsFile string) (GCPCredentials, []byte, erro return creds, credsData, nil } -// expandHomeDirectory expands ~ to the user's home directory +// expandHomeDirectory expands ~ to the user's home directory. func expandHomeDirectory(path string) string { if !strings.HasPrefix(path, "~/") { return path @@ -251,7 +251,7 @@ func expandHomeDirectory(path string) string { return strings.Replace(path, "~", home, 1) } -// printGCPConfigurationSuccess prints success message with credentials info +// printGCPConfigurationSuccess prints success message with credentials info. func printGCPConfigurationSuccess(creds GCPCredentials) { log.Printf("GCP credentials stored successfully in Secrets Manager") fmt.Println("\nGCP configuration complete!") @@ -704,7 +704,7 @@ func readRequiredInputLine(reader *bufio.Reader, prompt, fieldName string) (stri // promptAndRunGCPCommand shows a command and asks to run or skip. // It is used only for the interactive "gcloud auth login" auth bootstrap // (Step 1), which has no SDK equivalent that preserves the cached-credential UX. -func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, program string, args ...string) error { +func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd, program string, args ...string) error { fmt.Printf("Command: %s\n", displayCmd) fmt.Println() fmt.Printf("[R]un, [S]kip? ") @@ -733,7 +733,7 @@ func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, progr // is consumed from one consistent buffered stream (a fresh // bufio.NewReader(os.Stdin) here would drop input already buffered by the // caller's reader, breaking piped input after earlier prompts). -func executeGCPCommand(reader *bufio.Reader, displayCmd string, program string, args ...string) error { +func executeGCPCommand(reader *bufio.Reader, displayCmd, program string, args ...string) error { fmt.Println() fmt.Printf("Executing: %s\n", displayCmd) fmt.Println(strings.Repeat("-", 60)) @@ -754,7 +754,7 @@ func executeGCPCommand(reader *bufio.Reader, displayCmd string, program string, if readErr != nil { return fmt.Errorf("failed to read response: %w", readErr) } - if strings.ToLower(response) != "y" { + if !strings.EqualFold(response, "y") { return fmt.Errorf("command failed: %w", err) } } From 4193ecc1ea581c207a38a5fab72ea700d94e6bef Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 00:00:38 +0300 Subject: [PATCH 7/9] fix(configure): satisfy standalone gosec on the Azure/GCP SDK wizards The SDK-refactor wizards kept a few exec.Command auth shell-outs and file reads that gosec flags. The suppressions used //nolint:gosec, which the standalone gosec run (Security Scanning job) ignores, so both the Security Scanning and Lint Code jobs stayed red. Convert them to real #nosec directives with justifications that name the actual guard: - G204 (az login / gcloud auth login): program and args are hardcoded literals from the caller (runAzureSetupCommands / runGCPSetupCommands), no shell, not attacker-controlled. - G204/G702 (gcloud config set project): fixed argv with projectID pre-validated by validateGCPProjectID (strict regex) and passed as a discrete argv element, so it cannot inject. golangci's newer gosec reports this line as G702 (taint) while standalone reports G204, so the directive lists both. - G304 (read operator creds file): filepath.Clean the operator-supplied --credentials-file / prompt path; it is trusted local operator input to an interactive configure command, not attacker-controlled. - G304 (reserve key file): keyFile is the sole caller's fixed filepath.Join(os.UserHomeDir(), "cudly-gcp-key.json"), program- controlled, not attacker input. Verified none of the credential structs (AzureCredentials, GCPCredentials, azureSPResult) are written to logs; the only secret surfaced is the newly minted SP password shown once on stdout for the operator to copy. Verified with the CI tools: standalone gosec v2.26.1 `gosec ./...` and its SARIF invocation both report 0 issues; golangci-lint run ./... reports 0; go build, go vet, gocyclo -over 10 on the touched files all clean. --- cmd/configure_azure.go | 2 +- cmd/configure_gcp.go | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 42b1e366c..893ec39df 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -536,7 +536,7 @@ func executeExplicitCommand(reader *bufio.Reader, displayCmd, program string, ar fmt.Printf("Executing: %s\n", displayCmd) fmt.Println(strings.Repeat("-", 60)) - //nolint:gosec // G204: interactive operator auth (az login), hardcoded args, no shell + // #nosec G204 -- interactive operator auth (az login): program and args are hardcoded literals from the caller (runAzureSetupCommands passes "az","login"), no shell, not attacker-controlled cmd := exec.Command(program, args...) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index abbf1ce09..3ed90dc4c 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -214,8 +214,9 @@ func loadAWSConfigForGCP(ctx context.Context) (aws.Config, error) { // loadAndUpdateGCPCredentials loads, parses, and optionally updates GCP credentials. func loadAndUpdateGCPCredentials(credsFile string) (GCPCredentials, []byte, error) { - expandedPath := expandHomeDirectory(credsFile) + expandedPath := filepath.Clean(expandHomeDirectory(credsFile)) + // #nosec G304 -- expandedPath is the operator's own GCP service-account key file, supplied via the --credentials-file flag or the interactive prompt of this local `configure-gcp` command; it is cleaned above and is trusted operator input, not attacker-controlled credsData, err := os.ReadFile(expandedPath) if err != nil { return GCPCredentials{}, nil, fmt.Errorf("failed to read credentials file: %w", err) @@ -436,6 +437,7 @@ func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) er // Reserve the destination file first (fails if it already exists), so we // never mint a remote key we cannot persist locally. + // #nosec G304 -- keyFile is the sole caller's fixed path filepath.Join(os.UserHomeDir(), "cudly-gcp-key.json"); a constant filename under the operator's own home dir, program-controlled and not attacker input f, err := os.OpenFile(keyFile, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) if err != nil { return fmt.Errorf("failed to reserve key file %s: %w", keyFile, err) @@ -571,7 +573,7 @@ func gcpStepSelectProject(ctx context.Context, reader *bufio.Reader) (string, er // (writes to ~/.config/gcloud/properties) with no cloud-API equivalent. fmt.Println() fmt.Println("Setting gcloud project context (local config)...") - //nolint:gosec // G204: local gcloud config write, hardcoded args + validated projectID, no shell + // #nosec G204 G702 -- local gcloud config write: fixed argv ("gcloud config set project"), projectID pre-validated by validateGCPProjectID (strict regex) just above, passed as a discrete argv element with no shell, so it cannot inject cmd := exec.Command("gcloud", "config", "set", "project", projectID) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr @@ -738,7 +740,7 @@ func executeGCPCommand(reader *bufio.Reader, displayCmd, program string, args .. fmt.Printf("Executing: %s\n", displayCmd) fmt.Println(strings.Repeat("-", 60)) - //nolint:gosec // G204: interactive operator auth (gcloud auth login), hardcoded args, no shell + // #nosec G204 -- interactive operator auth (gcloud auth login): program and args are hardcoded literals from the caller (runGCPSetupCommands passes "gcloud","auth","login"), no shell, not attacker-controlled cmd := exec.Command(program, args...) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr From 744b42a866da59a62c59923027d73ba1152b42bd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 01:49:34 +0300 Subject: [PATCH 8/9] fix(configure): suppress golangci-bundled gosec G117/G703 on cred fields The Lint Code job runs golangci-lint v2.10.1, whose bundled gosec is a newer ruleset than the standalone gosec v2.26.1 used by Security Scanning. It emits five findings the standalone run does not: - G117 on the four credential-input struct fields (AzureCredentials and AzureConfigOptions ClientSecret, azureSPResult ClientSecret, GCPCredentials PrivateKey): the field name / json tag matches a secret pattern. - G703 (path traversal via taint) on the operator creds-file read, which standalone gosec reports as G304. Add #nosec directives whose justification names the real guard: - The four fields are operator-supplied credential INPUT (their own Azure SP secret / GCP private key), read from config or an interactive prompt, never a hardcoded secret. Verified none are written to logs: the only secret ever emitted is the SDK-generated SP password shown once on stdout for the operator to copy (mirrors az ad sp create-for-rbac). - The creds path is trusted operator input to a local configure command, filepath.Clean applied, not attacker-controlled; the directive lists both G304 (standalone) and G703 (golangci) since the two gosec versions label the same line differently. #nosec is honored by BOTH golangci-gosec and standalone gosec, unlike //nolint which the standalone run ignores. Verified with the pinned CI tools: golangci-lint v2.10.1 run ./... = 0 issues; standalone gosec v2.26.1 gosec ./... = 0 issues; go vet = 0; gocyclo -over 10 . = clean; go build = 0. --- cmd/configure_azure.go | 4 ++-- cmd/configure_azure_sp.go | 2 +- cmd/configure_gcp.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 893ec39df..54de9ece9 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -53,7 +53,7 @@ func readTrimmedLine(reader *bufio.Reader) (string, error) { type AzureCredentials struct { TenantID string `json:"tenant_id"` ClientID string `json:"client_id"` - ClientSecret string `json:"client_secret"` + ClientSecret string `json:"client_secret"` // #nosec G117 -- operator-supplied credential input read from the user's own Azure service principal; marshaled only to store in AWS Secrets Manager, never a hardcoded secret and never logged (verified) SubscriptionID string `json:"subscription_id"` } @@ -63,7 +63,7 @@ type AzureConfigOptions struct { Profile string TenantID string ClientID string - ClientSecret string + ClientSecret string // #nosec G117 -- operator-supplied credential input from a CLI flag or interactive prompt; never a hardcoded secret and never logged (verified) SubscriptionID string Interactive bool SkipSetup bool diff --git a/cmd/configure_azure_sp.go b/cmd/configure_azure_sp.go index 379fc6ca8..9ca3e2da4 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 // generated password / client secret + 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) TenantID string // Azure AD tenant ID } diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 3ed90dc4c..6cd873967 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -40,7 +40,7 @@ type GCPCredentials struct { Type string `json:"type"` ProjectID string `json:"project_id"` PrivateKeyID string `json:"private_key_id"` - PrivateKey string `json:"private_key"` + PrivateKey string `json:"private_key"` // #nosec G117 -- operator-supplied credential input read from the user's own GCP service-account key file; marshaled only to store in AWS Secrets Manager, never a hardcoded secret and never logged (verified) ClientEmail string `json:"client_email"` ClientID string `json:"client_id,omitempty"` AuthURI string `json:"auth_uri,omitempty"` @@ -216,7 +216,7 @@ func loadAWSConfigForGCP(ctx context.Context) (aws.Config, error) { func loadAndUpdateGCPCredentials(credsFile string) (GCPCredentials, []byte, error) { expandedPath := filepath.Clean(expandHomeDirectory(credsFile)) - // #nosec G304 -- expandedPath is the operator's own GCP service-account key file, supplied via the --credentials-file flag or the interactive prompt of this local `configure-gcp` command; it is cleaned above and is trusted operator input, not attacker-controlled + // #nosec G304 G703 -- expandedPath is the operator's own GCP service-account key file, supplied via the --credentials-file flag or the interactive prompt of this local `configure-gcp` command; filepath.Clean applied above and it is trusted operator input, not attacker-controlled credsData, err := os.ReadFile(expandedPath) if err != nil { return GCPCredentials{}, nil, fmt.Errorf("failed to read credentials file: %w", err) From 0fe3107e3a856920484d058b71a8b3d427e20728 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 09:00:08 +0300 Subject: [PATCH 9/9] fix(configure): repair GCP golden path + Azure SP retry budget in SDK wizards Address the independent-review BLOCK on the CLI->SDK refactor. Green CI missed these because the GCP wizard half had no unit tests. D1 (GCP wizard broken on first run): Step 1 ran only "gcloud auth login", which does NOT populate the ADC cache that Steps 2/4/5/6 authenticate through, so a fresh operator hard-aborted at the project listing and had to re-run. Add a prompted Step 1b that runs "gcloud auth application-default login" (same interactive-auth-with-no-SDK-equivalent category as the retained login calls; hardcoded literal argv, no shell), so setup completes in one pass. Update the wizard/newGCPAPIOption docs. D2 (removed skip option compounded D1): the SDK subscription/project listings were made unconditional and fail-hard, so an operator who already knows their ID could not get past a listing failure. Restore the [R]un/[S]kip prompt (new promptRunOrSkipListing helper) around both listings: skip goes straight to the ID prompt; the listing still fails loud WHEN RUN (skip is a deliberate choice, not a silent fallback). D3 (Azure SP retry budget was dead + destructive): roleAssignRetryBudget is 3 min but the SP step ran under a 2 min context that also covered tenant resolution and the Graph calls, so the PrincipalNotFound retry loop was cut short by ctx cancellation and the rollback then deleted the just-created app, resetting the AAD replication clock every re-run. Widen the step budget to 6 min (> retryBudget + pre-assignment overhead) so the retry gets its full budget, and wrap the terminal ctx.Err() return with the "AAD role propagation can take up to ~10 min; re-run" guidance (errors.Is still matches the cause). Tests: add cmd/configure_gcp_test.go (mocks, no creds) mirroring configure_azure_sp_test.go. Refactor createGCPServiceAccountKey to extract a testable writeServiceAccountKey behind a gcpKeyProvisioner interface. Cover addMemberToPolicyBinding (append / already-bound no-op / create missing / preserve conditional bindings on the version-3 round-trip) and the key-creation rollback path (success, decode-failure rollback, rollback-failure surfaced, create-failure no-orphan, reserve-failure no-mint). Nit: help text --role "Reservation Administrator" -> "Reservations Administrator" to match azureSPRoleName. --- cmd/configure_azure.go | 71 ++++++++--- cmd/configure_azure_sp.go | 12 +- cmd/configure_gcp.go | 158 ++++++++++++++++-------- cmd/configure_gcp_test.go | 250 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 425 insertions(+), 66 deletions(-) create mode 100644 cmd/configure_gcp_test.go diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 54de9ece9..8f442d7bc 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -49,6 +49,26 @@ func readTrimmedLine(reader *bufio.Reader) (string, error) { return strings.TrimSpace(input), nil } +// promptRunOrSkipListing asks whether to run an interactive SDK listing step +// (e.g. list subscriptions / projects) or skip straight to entering the ID. +// It returns true to run the listing and false to skip. Skipping is a +// deliberate operator choice for someone who already knows their ID; the +// listing still fails loud WHEN RUN (skip is not a silent fallback on error). +// Empty input or "r"/"run" runs the listing; "s"/"skip" skips it. +func promptRunOrSkipListing(reader *bufio.Reader, what string) (bool, error) { + fmt.Printf("[R]un %s, or [S]kip to enter the ID directly? ", what) + choice, err := readTrimmedLine(reader) + if err != nil { + return false, fmt.Errorf("failed to read choice: %w", err) + } + switch strings.ToLower(choice) { + case "s", "skip": + return false, nil + default: + return true, nil + } +} + // AzureCredentials holds the Azure Service Principal credentials. type AzureCredentials struct { TenantID string `json:"tenant_id"` @@ -84,7 +104,7 @@ You can provide credentials via flags or interactively: To create an Azure Service Principal manually: az login - az ad sp create-for-rbac --name "CUDly" --role "Reservation Administrator" --scopes /subscriptions/`, + az ad sp create-for-rbac --name "CUDly" --role "Reservations Administrator" --scopes /subscriptions/`, RunE: runConfigureAzure, } @@ -369,27 +389,39 @@ func azureStepLogin(reader *bufio.Reader) error { return promptAndRunExplicitCommand(reader, "Azure Login", "az login", "az", "login") } -// azureStepListSubscriptions lists subscriptions via SDK and prompts the -// operator to enter their subscription ID. It fails loud (no CLI fallback): if -// the Azure CLI credential cannot authenticate, it returns an error -// instructing the operator to run "az login" first. +// listAzureSubscriptionsWithTimeout runs the SDK subscription listing under a +// bounded timeout so a hung ARM call cannot stall the wizard. +func listAzureSubscriptionsWithTimeout(ctx context.Context) error { + listCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + return listAzureSubscriptions(listCtx) +} + +// azureStepListSubscriptions optionally lists subscriptions via SDK and prompts +// the operator to enter their subscription ID. The listing is behind a +// [R]un/[S]kip prompt so an operator who already knows their subscription ID +// can proceed even if the SDK listing would fail; when RUN it fails loud (no +// CLI fallback), 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 (Azure CLI credential)...") - fmt.Println() - - listCtx, cancel := context.WithTimeout(ctx, 30*time.Second) - defer cancel() - if err := listAzureSubscriptions(listCtx); err != nil { - return "", fmt.Errorf("failed to list Azure subscriptions via SDK: %w\n"+ - "Ensure you are authenticated: run 'az login' (Step 1) before continuing", err) + run, err := promptRunOrSkipListing(reader, "the Azure subscription listing (via SDK)") + if err != nil { + return "", err + } + if run { + fmt.Println("Listing your Azure subscriptions via SDK (Azure CLI credential)...") + fmt.Println() + if err = listAzureSubscriptionsWithTimeout(ctx); err != nil { + return "", fmt.Errorf("failed to list Azure subscriptions via SDK: %w\n"+ + "Ensure you are authenticated: run 'az login' (Step 1) before continuing", err) + } + fmt.Println() } - fmt.Println() - fmt.Print("Enter your Subscription ID from above: ") + fmt.Print("Enter your Subscription ID: ") subscriptionID, err := readTrimmedLine(reader) if err != nil { return "", fmt.Errorf("failed to read subscription ID: %w", err) @@ -453,7 +485,14 @@ func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, return nil } - spCtx, cancel := context.WithTimeout(ctx, 2*time.Minute) + // The step budget must exceed roleAssignRetryBudget (3 min) plus the + // pre-assignment overhead (tenant resolution + the application / password / + // service-principal / role-definition Graph calls) so the PrincipalNotFound + // retry loop in AssignRole gets its full propagation budget. A tighter + // budget would cut the retry short via ctx cancellation, then the rollback + // would delete the just-created application and reset the AAD replication + // clock on every re-run. See roleAssignRetryBudget in configure_azure_sp.go. + spCtx, cancel := context.WithTimeout(ctx, 6*time.Minute) defer cancel() tenantID, err := resolveAzureTenantID(spCtx, subscriptionID) diff --git a/cmd/configure_azure_sp.go b/cmd/configure_azure_sp.go index 9ca3e2da4..6005911c1 100644 --- a/cmd/configure_azure_sp.go +++ b/cmd/configure_azure_sp.go @@ -326,10 +326,18 @@ func (g *graphSPProvisioner) AssignRole(ctx context.Context, scope, principalID, "https://learn.microsoft.com/en-us/azure/role-based-access-control/troubleshooting): %w", principalID, g.retryBudget, err) } - // Context cancellation is terminal: do not continue retrying. + // Context cancellation is terminal: do not continue retrying. Wrap + // ctx.Err() with the propagation guidance so the operator learns the + // assignment may just need more time rather than seeing a bare + // "context deadline exceeded" (errors.Is still matches the cause). select { case <-ctx.Done(): - return ctx.Err() + return fmt.Errorf( + "stopped waiting for service principal %q to propagate to ARM "+ + "before the role assignment completed: %w -- Azure AD role "+ + "propagation can take up to ~10 minutes; re-run configure-azure "+ + "to retry (https://learn.microsoft.com/en-us/azure/role-based-access-control/troubleshooting)", + principalID, ctx.Err()) case <-time.After(delay): } delay = min(delay*2, roleAssignRetryMax) diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 6cd873967..a22452348 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -272,13 +272,10 @@ const gcpSDKCallTimeout = 60 * time.Second // Metadata Server. // // NOTE: "gcloud auth login" (wizard Step 1) updates the gcloud user session but -// does NOT automatically populate the ADC cache. If the operator intends to use -// these SDK calls after Step 1, they should also run: -// -// gcloud auth application-default login -// -// The wizard Step 3 onwards calls this function; if ADC is not available the -// calls fail loud with a hint to run "gcloud auth application-default login". +// does NOT populate the ADC cache; the wizard's Step 1b +// ("gcloud auth application-default login") does that. If ADC is still not +// available (e.g. the operator skipped Step 1b) these calls fail loud with a +// hint to run "gcloud auth application-default login". func newGCPAPIOption(ctx context.Context) (option.ClientOption, error) { ts, err := google.DefaultTokenSource(ctx, "https://www.googleapis.com/auth/cloud-platform", @@ -423,18 +420,68 @@ func addMemberToPolicyBinding(policy *cloudresourcemanager.Policy, member, role return true } +// gcpKeyProvisioner abstracts the IAM service-account key operations used by +// writeServiceAccountKey. It exists so the reserve / mint / decode / write / +// rollback flow can be unit-tested with a mock without hitting GCP (mirrors +// azureSPProvisioner in configure_azure_sp.go). +type gcpKeyProvisioner interface { + // CreateKey mints a new JSON key for saEmail and returns the key resource + // name and the base64-encoded private key material. + CreateKey(ctx context.Context, saEmail string) (keyName, privateKeyData string, err error) + // DeleteKey deletes the key identified by keyName. It is the compensating + // action used to avoid orphaning a freshly minted key on a local failure. + DeleteKey(ctx context.Context, keyName string) error +} + +// iamKeyProvisioner is the production gcpKeyProvisioner backed by the IAM API v1. +type iamKeyProvisioner struct { + svc *iamv1.Service +} + +func (k *iamKeyProvisioner) CreateKey(ctx context.Context, saEmail string) (keyName, privateKeyData string, err error) { + resource := fmt.Sprintf("projects/-/serviceAccounts/%s", saEmail) + key, err := k.svc.Projects.ServiceAccounts.Keys.Create(resource, &iamv1.CreateServiceAccountKeyRequest{ + PrivateKeyType: "TYPE_GOOGLE_CREDENTIALS_FILE", + }).Context(ctx).Do() + if err != nil { + return "", "", err + } + return key.Name, key.PrivateKeyData, nil +} + +func (k *iamKeyProvisioner) DeleteKey(ctx context.Context, keyName string) error { + _, err := k.svc.Projects.ServiceAccounts.Keys.Delete(keyName).Context(ctx).Do() + return err +} + // createGCPServiceAccountKey creates a JSON key for the given service account // and writes it to keyFile. This replaces // "gcloud iam service-accounts keys create --iam-account=". -// -// To avoid orphaning a live IAM key on a local failure, it reserves the -// destination file with exclusive-create semantics BEFORE minting the remote -// key, and deletes the freshly created remote key if decoding or writing the -// key material fails. func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) error { ctx, cancel := context.WithTimeout(ctx, gcpSDKCallTimeout) defer cancel() + opt, err := newGCPAPIOption(ctx) + if err != nil { + return err + } + + svc, err := iamv1.NewService(ctx, opt) + if err != nil { + return fmt.Errorf("failed to create IAM client: %w", err) + } + + return writeServiceAccountKey(ctx, &iamKeyProvisioner{svc: svc}, saEmail, keyFile) +} + +// writeServiceAccountKey reserves keyFile with exclusive-create semantics +// BEFORE minting the remote key (so it never mints a key it cannot persist +// locally), then mints the key via p, decodes the base64 material and writes it +// to keyFile. If decoding or writing fails after the remote key is minted it +// deletes the remote key so it does not linger as an active, unused credential. +// Extracted from createGCPServiceAccountKey so the reserve / mint / rollback +// flow is unit-testable with a mock (no GCP credentials). +func writeServiceAccountKey(ctx context.Context, p gcpKeyProvisioner, saEmail, keyFile string) error { // Reserve the destination file first (fails if it already exists), so we // never mint a remote key we cannot persist locally. // #nosec G304 -- keyFile is the sole caller's fixed path filepath.Join(os.UserHomeDir(), "cudly-gcp-key.json"); a constant filename under the operator's own home dir, program-controlled and not attacker input @@ -451,20 +498,7 @@ func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) er } }() - opt, err := newGCPAPIOption(ctx) - if err != nil { - return err - } - - svc, err := iamv1.NewService(ctx, opt) - if err != nil { - return fmt.Errorf("failed to create IAM client: %w", err) - } - - resource := fmt.Sprintf("projects/-/serviceAccounts/%s", saEmail) - key, err := svc.Projects.ServiceAccounts.Keys.Create(resource, &iamv1.CreateServiceAccountKeyRequest{ - PrivateKeyType: "TYPE_GOOGLE_CREDENTIALS_FILE", - }).Context(ctx).Do() + keyName, privateKeyData, err := p.CreateKey(ctx, saEmail) if err != nil { return fmt.Errorf("failed to create service account key: %w", err) } @@ -475,14 +509,14 @@ func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) er // Use a fresh context: the parent may already be canceled/expired. delCtx, delCancel := context.WithTimeout(context.Background(), gcpSDKCallTimeout) defer delCancel() - if _, delErr := svc.Projects.ServiceAccounts.Keys.Delete(key.Name).Context(delCtx).Do(); delErr != nil { - return fmt.Errorf("%w; additionally failed to delete the orphaned remote key %s: %w", cause, key.Name, delErr) + if delErr := p.DeleteKey(delCtx, keyName); delErr != nil { + return fmt.Errorf("%w; additionally failed to delete the orphaned remote key %s: %w", cause, keyName, delErr) } return cause } // PrivateKeyData is base64-encoded JSON. - decoded, err := base64.StdEncoding.DecodeString(key.PrivateKeyData) + decoded, err := base64.StdEncoding.DecodeString(privateKeyData) if err != nil { return deleteRemoteKey(fmt.Errorf("failed to decode key data: %w", err)) } @@ -496,12 +530,13 @@ func createGCPServiceAccountKey(ctx context.Context, saEmail, keyFile string) er // runGCPSetupCommands guides the operator through GCP setup. // -// Step 1 (gcloud auth login): performed via the GCP CLI. This is an -// interactive browser-based OAuth flow that cannot be replicated through SDK -// calls on behalf of an operator who does not yet have a credential. It -// updates the gcloud user session but does NOT populate the ADC cache needed -// by subsequent SDK calls. Operators must also run -// "gcloud auth application-default login" if they want to use ADC. +// Step 1 (gcloud auth login) and Step 1b (gcloud auth application-default +// login): performed via the GCP CLI. Both are interactive browser-based OAuth +// flows that cannot be replicated through SDK calls on behalf of an operator +// who does not yet have a credential. Step 1 establishes the gcloud user +// session; Step 1b populates the Application Default Credentials (ADC) cache +// that every SDK-based step below authenticates through. Both are run in one +// pass so a fresh operator completes setup without re-running the wizard. // // Step 2 (list projects): performed via the Cloud Resource Manager SDK v1, // using Application Default Credentials (ADC). Fails loud if ADC is not @@ -536,32 +571,59 @@ func runGCPSetupCommands(ctx context.Context, reader *bufio.Reader) (string, err return gcpStepCreateKey(ctx, reader, saEmail) } -// gcpStepLogin runs "gcloud auth login" interactively. +// gcpStepLogin runs the two interactive gcloud logins the wizard needs: +// "gcloud auth login" (user session) and "gcloud auth application-default +// login" (ADC cache). Both are browser-based OAuth flows with no SDK +// equivalent. The ADC login is required because the SDK-based steps below +// (list projects, create service account, grant role, create key) authenticate +// through Application Default Credentials, which "gcloud auth login" alone does +// NOT populate -- so without it a fresh operator would hard-abort at Step 2. func gcpStepLogin(reader *bufio.Reader) error { fmt.Println("Step 1: GCP Login") fmt.Println("-----------------") - fmt.Println("This will open a browser window for GCP authentication.") - fmt.Println("After logging in, also run: gcloud auth application-default login") - fmt.Println("(required for the SDK-based steps below)") + fmt.Println("This opens a browser window for GCP authentication (user session).") fmt.Println() - return promptAndRunGCPCommand(reader, "GCP Login", "gcloud auth login", "gcloud", "auth", "login") + if err := promptAndRunGCPCommand(reader, "GCP Login", "gcloud auth login", "gcloud", "auth", "login"); err != nil { + return err + } + + fmt.Println() + fmt.Println("Step 1b: GCP Application Default Credentials Login") + fmt.Println("-------------------------------------------------") + fmt.Println("This opens a browser window to populate the Application Default") + fmt.Println("Credentials (ADC) cache used by the SDK-based steps below") + fmt.Println("(list projects, create service account, grant role, create key).") + fmt.Println() + return promptAndRunGCPCommand(reader, "GCP ADC Login", + "gcloud auth application-default login", + "gcloud", "auth", "application-default", "login") } -// gcpStepSelectProject lists projects and prompts for a project ID. +// gcpStepSelectProject optionally lists projects and prompts for a project ID. +// The listing is behind a [R]un/[S]kip prompt so an operator who already knows +// their project ID can proceed even if the SDK listing would fail; when RUN it +// fails loud (no CLI fallback), instructing the operator to run +// "gcloud auth application-default login" first. func gcpStepSelectProject(ctx context.Context, reader *bufio.Reader) (string, error) { fmt.Println() fmt.Println("Step 2: Select Project") fmt.Println("----------------------") - fmt.Println("Listing your GCP projects via SDK (Application Default Credentials)...") - fmt.Println() - if err := listGCPProjects(ctx); err != nil { - return "", fmt.Errorf("failed to list GCP projects via SDK: %w\n"+ - "Ensure Application Default Credentials are set: run 'gcloud auth application-default login' first", err) + run, err := promptRunOrSkipListing(reader, "the GCP project listing (via SDK)") + if err != nil { + return "", err + } + if run { + fmt.Println("Listing your GCP projects via SDK (Application Default Credentials)...") + fmt.Println() + if err = listGCPProjects(ctx); err != nil { + return "", fmt.Errorf("failed to list GCP projects via SDK: %w\n"+ + "Ensure Application Default Credentials are set: run 'gcloud auth application-default login' first", err) + } + fmt.Println() } - fmt.Println() - projectID, err := readRequiredInputLine(reader, "Enter your Project ID from above: ", "project ID") + projectID, err := readRequiredInputLine(reader, "Enter your Project ID: ", "project ID") if err != nil { return "", err } diff --git a/cmd/configure_gcp_test.go b/cmd/configure_gcp_test.go new file mode 100644 index 000000000..d3a606cdd --- /dev/null +++ b/cmd/configure_gcp_test.go @@ -0,0 +1,250 @@ +package main + +import ( + "context" + "encoding/base64" + "errors" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + cloudresourcemanager "google.golang.org/api/cloudresourcemanager/v1" +) + +// --- addMemberToPolicyBinding ------------------------------------------------- + +// TestAddMemberToPolicyBinding_AppendsToExistingBinding verifies a member is +// appended to an existing binding for the role and the function reports a +// change. +func TestAddMemberToPolicyBinding_AppendsToExistingBinding(t *testing.T) { + policy := &cloudresourcemanager.Policy{ + Bindings: []*cloudresourcemanager.Binding{ + {Role: "roles/viewer", Members: []string{"user:existing@example.com"}}, + }, + } + + changed := addMemberToPolicyBinding(policy, "serviceAccount:sa@proj.iam.gserviceaccount.com", "roles/viewer") + + require.True(t, changed, "adding a new member to an existing role binding must report a change") + require.Len(t, policy.Bindings, 1) + assert.Equal(t, []string{ + "user:existing@example.com", + "serviceAccount:sa@proj.iam.gserviceaccount.com", + }, policy.Bindings[0].Members) +} + +// TestAddMemberToPolicyBinding_AlreadyBoundNoChange verifies that an +// already-bound member is a no-op (returns false, no duplicate appended). +func TestAddMemberToPolicyBinding_AlreadyBoundNoChange(t *testing.T) { + member := "serviceAccount:sa@proj.iam.gserviceaccount.com" + policy := &cloudresourcemanager.Policy{ + Bindings: []*cloudresourcemanager.Binding{ + {Role: "roles/viewer", Members: []string{member}}, + }, + } + + changed := addMemberToPolicyBinding(policy, member, "roles/viewer") + + require.False(t, changed, "re-adding a member already bound to the role must report no change") + require.Len(t, policy.Bindings, 1) + assert.Equal(t, []string{member}, policy.Bindings[0].Members, + "member must not be duplicated") +} + +// TestAddMemberToPolicyBinding_CreatesMissingBinding verifies a new binding is +// appended when the role is absent from the policy. +func TestAddMemberToPolicyBinding_CreatesMissingBinding(t *testing.T) { + policy := &cloudresourcemanager.Policy{ + Bindings: []*cloudresourcemanager.Binding{ + {Role: "roles/viewer", Members: []string{"user:existing@example.com"}}, + }, + } + + member := "serviceAccount:sa@proj.iam.gserviceaccount.com" + changed := addMemberToPolicyBinding(policy, member, "roles/billing.projectManager") + + require.True(t, changed, "adding a member to an absent role must create the binding and report a change") + require.Len(t, policy.Bindings, 2) + newBinding := policy.Bindings[1] + assert.Equal(t, "roles/billing.projectManager", newBinding.Role) + assert.Equal(t, []string{member}, newBinding.Members) +} + +// TestAddMemberToPolicyBinding_PreservesConditionalBindings is the regression +// guard for the version-3 read-modify-write round-trip: adding a member to one +// role must NOT drop or mutate a conditional (IAM condition) binding on another +// role. Losing conditional bindings would silently widen access. +func TestAddMemberToPolicyBinding_PreservesConditionalBindings(t *testing.T) { + conditional := &cloudresourcemanager.Binding{ + Role: "roles/storage.objectViewer", + Members: []string{"user:auditor@example.com"}, + Condition: &cloudresourcemanager.Expr{ + Title: "only-prod-bucket", + Expression: `resource.name.startsWith("projects/_/buckets/prod-")`, + }, + } + policy := &cloudresourcemanager.Policy{ + Version: 3, + Bindings: []*cloudresourcemanager.Binding{ + conditional, + {Role: "roles/viewer", Members: []string{"user:existing@example.com"}}, + }, + } + + member := "serviceAccount:sa@proj.iam.gserviceaccount.com" + changed := addMemberToPolicyBinding(policy, member, "roles/viewer") + require.True(t, changed) + + // The conditional binding must still be present, unchanged. + require.Len(t, policy.Bindings, 2, "no binding may be dropped") + var found *cloudresourcemanager.Binding + for _, b := range policy.Bindings { + if b.Role == "roles/storage.objectViewer" { + found = b + } + } + require.NotNil(t, found, "the conditional binding must be preserved") + require.NotNil(t, found.Condition, "the IAM condition must be preserved") + assert.Equal(t, "only-prod-bucket", found.Condition.Title) + assert.Equal(t, `resource.name.startsWith("projects/_/buckets/prod-")`, found.Condition.Expression) + assert.Equal(t, []string{"user:auditor@example.com"}, found.Members, + "the conditional binding's members must be untouched") +} + +// --- writeServiceAccountKey (key-creation rollback) --------------------------- + +// mockGCPKeyProvisioner is a configurable gcpKeyProvisioner used to assert the +// reserve / mint / decode / write / rollback flow of writeServiceAccountKey. +type mockGCPKeyProvisioner struct { + createErr error + deleteErr error + keyName string + privateKeyData string // base64-encoded, as returned by the IAM API + + createCalled bool + deleteCalled bool + createSAEmail string + deletedKeyName string +} + +func (m *mockGCPKeyProvisioner) CreateKey(_ context.Context, saEmail string) (string, string, error) { + m.createCalled = true + m.createSAEmail = saEmail + if m.createErr != nil { + return "", "", m.createErr + } + return m.keyName, m.privateKeyData, nil +} + +func (m *mockGCPKeyProvisioner) DeleteKey(_ context.Context, keyName string) error { + m.deleteCalled = true + m.deletedKeyName = keyName + return m.deleteErr +} + +func TestWriteServiceAccountKey_Success(t *testing.T) { + keyMaterial := []byte(`{"type":"service_account","project_id":"proj"}`) + m := &mockGCPKeyProvisioner{ + keyName: "projects/-/serviceAccounts/sa@proj.iam.gserviceaccount.com/keys/abc123", + privateKeyData: base64.StdEncoding.EncodeToString(keyMaterial), + } + keyFile := filepath.Join(t.TempDir(), "cudly-gcp-key.json") + + err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + require.NoError(t, err) + + assert.True(t, m.createCalled) + assert.Equal(t, "sa@proj.iam.gserviceaccount.com", m.createSAEmail) + assert.False(t, m.deleteCalled, "DeleteKey must not be called on success") + + // The decoded key material must have been written to the file. + got, readErr := os.ReadFile(keyFile) // #nosec G304 -- test-controlled temp path + require.NoError(t, readErr) + assert.Equal(t, keyMaterial, got) +} + +// TestWriteServiceAccountKey_DecodeFailureRollsBack verifies that when the +// returned key material is not valid base64, the minted remote key is deleted +// (so it does not linger as an active unused credential) and the reserved local +// file is cleaned up. +func TestWriteServiceAccountKey_DecodeFailureRollsBack(t *testing.T) { + m := &mockGCPKeyProvisioner{ + keyName: "projects/-/serviceAccounts/sa@proj.iam.gserviceaccount.com/keys/abc123", + privateKeyData: "!!!not-base64!!!", + } + keyFile := filepath.Join(t.TempDir(), "cudly-gcp-key.json") + + err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to decode key data") + + // The freshly minted remote key must have been deleted by its resource name. + assert.True(t, m.deleteCalled, "the minted key must be deleted when decoding fails") + assert.Equal(t, m.keyName, m.deletedKeyName) + + // The reserved local file must be cleaned up (nothing persisted). + _, statErr := os.Stat(keyFile) + assert.True(t, os.IsNotExist(statErr), "the reserved key file must be removed on failure") +} + +// TestWriteServiceAccountKey_RollbackFailureSurfaced verifies that when the +// compensating DeleteKey also fails, the error names the orphaned remote key so +// the operator can delete it manually, while still surfacing the original cause. +func TestWriteServiceAccountKey_RollbackFailureSurfaced(t *testing.T) { + m := &mockGCPKeyProvisioner{ + keyName: "projects/-/serviceAccounts/sa@proj.iam.gserviceaccount.com/keys/orphan999", + privateKeyData: "!!!not-base64!!!", + deleteErr: errors.New("delete 500"), + } + keyFile := filepath.Join(t.TempDir(), "cudly-gcp-key.json") + + err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + require.Error(t, err) + assert.True(t, m.deleteCalled) + assert.Contains(t, err.Error(), "failed to decode key data", "the original cause must be surfaced") + assert.Contains(t, err.Error(), "failed to delete the orphaned remote key") + assert.Contains(t, err.Error(), "orphan999", "the orphaned key name must be named for manual cleanup") +} + +// TestWriteServiceAccountKey_CreateFailureNoOrphan verifies that when minting +// the remote key fails, no rollback is attempted (nothing was minted) and no +// local file is left behind. +func TestWriteServiceAccountKey_CreateFailureNoOrphan(t *testing.T) { + m := &mockGCPKeyProvisioner{ + createErr: errors.New("iam 403"), + } + keyFile := filepath.Join(t.TempDir(), "cudly-gcp-key.json") + + err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to create service account key") + assert.False(t, m.deleteCalled, "no remote key was minted, so DeleteKey must not be called") + + _, statErr := os.Stat(keyFile) + assert.True(t, os.IsNotExist(statErr), "the reserved key file must be removed when minting fails") +} + +// TestWriteServiceAccountKey_ReserveFailureNoMint verifies that if the +// destination file already exists (exclusive-create fails), the remote key is +// never minted, so there is nothing to orphan. +func TestWriteServiceAccountKey_ReserveFailureNoMint(t *testing.T) { + keyFile := filepath.Join(t.TempDir(), "cudly-gcp-key.json") + require.NoError(t, os.WriteFile(keyFile, []byte("pre-existing"), 0o600)) + + m := &mockGCPKeyProvisioner{ + keyName: "projects/-/serviceAccounts/sa@proj.iam.gserviceaccount.com/keys/abc123", + privateKeyData: base64.StdEncoding.EncodeToString([]byte("{}")), + } + + err := writeServiceAccountKey(context.Background(), m, "sa@proj.iam.gserviceaccount.com", keyFile) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to reserve key file") + assert.False(t, m.createCalled, "the remote key must not be minted when the file cannot be reserved") + + // The pre-existing file must be left intact (we must not clobber it). + got, readErr := os.ReadFile(keyFile) // #nosec G304 -- test-controlled temp path + require.NoError(t, readErr) + assert.Equal(t, []byte("pre-existing"), got) +}