diff --git a/internal/secrets/azure_credential_guard_test.go b/internal/secrets/azure_credential_guard_test.go new file mode 100644 index 000000000..a12be6dd3 --- /dev/null +++ b/internal/secrets/azure_credential_guard_test.go @@ -0,0 +1,35 @@ +package secrets + +import ( + "os" + "testing" +) + +// TestMain pins the Azure credential environment for every test in this +// package so no test can walk the live DefaultAzureCredential fallback chain +// (IMDS probe, `az account get-access-token`, `pwsh Get-AzAccessToken`). The +// pwsh leg reads the MSAL token cache from the macOS login keychain, which +// pops an interactive password prompt on developer machines whenever +// `go test ./...` runs. +// +// AZURE_TOKEN_CREDENTIALS restricts the chain to EnvironmentCredential only +// (supported since azidentity v1.10), and the dummy client-secret variables +// let that credential construct without contacting anything. A test that +// accidentally acquires a real token fails fast against the dummy tenant +// instead of harvesting local developer credentials. +func TestMain(m *testing.M) { + os.Setenv("AZURE_TOKEN_CREDENTIALS", "EnvironmentCredential") + os.Setenv("AZURE_TENANT_ID", "00000000-0000-0000-0000-000000000000") + os.Setenv("AZURE_CLIENT_ID", "00000000-0000-0000-0000-000000000000") + os.Setenv("AZURE_CLIENT_SECRET", "dummy-test-client-secret") + os.Exit(m.Run()) +} + +// TestAzureCredentialChainRestricted is the regression guard for the keychain +// prompt incident: it fails if TestMain stops pinning the restricted +// credential chain. +func TestAzureCredentialChainRestricted(t *testing.T) { + if got := os.Getenv("AZURE_TOKEN_CREDENTIALS"); got != "EnvironmentCredential" { + t.Fatalf("AZURE_TOKEN_CREDENTIALS = %q; TestMain must pin it to EnvironmentCredential so tests never invoke the az/pwsh/IMDS credential legs", got) + } +} diff --git a/internal/secrets/azure_resolver_coverage_test.go b/internal/secrets/azure_resolver_coverage_test.go index 2b4cdb6e0..55a352b60 100644 --- a/internal/secrets/azure_resolver_coverage_test.go +++ b/internal/secrets/azure_resolver_coverage_test.go @@ -2,6 +2,9 @@ package secrets import ( "context" + "encoding/json" + "net/http" + "sync/atomic" "testing" "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets" @@ -9,16 +12,23 @@ import ( "github.com/stretchr/testify/require" ) -// TestAzureResolver_DirectMethods tests the actual AzureResolver methods -// These tests exercise the real code paths but may skip if Azure credentials are unavailable. +// notFoundHandler simulates the Key Vault response for a missing secret. +func notFoundHandler(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _ = json.NewEncoder(w).Encode(map[string]any{ + "error": map[string]any{"code": "SecretNotFound", "message": "Secret not found"}, + }) +} + +// TestAzureResolver_DirectMethods tests the real constructor wiring. It makes +// no requests, and TestMain pins the credential chain to a dummy +// EnvironmentCredential, so construction is deterministic and offline. func TestAzureResolver_DirectMethods(t *testing.T) { ctx := context.Background() - // Try to create an Azure resolver resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } + require.NoError(t, err) defer resolver.Close() // Test that the resolver is properly configured @@ -28,89 +38,67 @@ func TestAzureResolver_DirectMethods(t *testing.T) { // TestAzureResolver_GetSecret_NonExistent tests getting a non-existent secret. func TestAzureResolver_GetSecret_NonExistent(t *testing.T) { - ctx := context.Background() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + _, err := resolver.GetSecret(context.Background(), "cudly-test-nonexistent-secret-12345-xyz") - // Try to get a secret that definitely doesn't exist - _, err = resolver.GetSecret(ctx, "cudly-test-nonexistent-secret-12345-xyz") - - // Should get an error (either not found or permission denied) assert.Error(t, err) assert.Contains(t, err.Error(), "failed to get secret") } // TestAzureResolver_GetSecretJSON_NonExistent tests getting a non-existent JSON secret. func TestAzureResolver_GetSecretJSON_NonExistent(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() - // Try to get a JSON secret that doesn't exist - result, err := resolver.GetSecretJSON(ctx, "cudly-test-nonexistent-json-secret-12345-xyz") + result, err := resolver.GetSecretJSON(context.Background(), "cudly-test-nonexistent-json-secret-12345-xyz") - // Should get an error assert.Error(t, err) assert.Nil(t, result) } // TestAzureResolver_ListSecrets tests listing secrets. func TestAzureResolver_ListSecrets(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"value": []map[string]any{}}) + }) + defer server.Close() - // List without filter - result, err := resolver.ListSecrets(ctx, "") + result, err := resolver.ListSecrets(context.Background(), "") - // Either succeeds or fails with permission error - if err == nil { - assert.NotNil(t, result) - } else { - assert.Contains(t, err.Error(), "failed to list secrets") - } + require.NoError(t, err) + assert.NotNil(t, result) } -// TestAzureResolver_ListSecrets_WithFilter_Coverage_Direct tests listing secrets with a filter using direct resolver. +// TestAzureResolver_ListSecrets_WithFilter_Coverage_Direct tests that the +// client-side prefix filter is applied while listing. func TestAzureResolver_ListSecrets_WithFilter_Coverage_Direct(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "value": []map[string]any{ + {"id": "https://myvault.vault.azure.net/secrets/prod-db-creds"}, + {"id": "https://myvault.vault.azure.net/secrets/dev-api-key"}, + }, + }) + }) + defer server.Close() - // List with filter - result, err := resolver.ListSecrets(ctx, "prod") + result, err := resolver.ListSecrets(context.Background(), "prod") - // Either succeeds or fails with permission error - if err == nil { - // Filter is applied client-side in Azure resolver - assert.NotNil(t, result) - } + require.NoError(t, err) + assert.Equal(t, []string{"prod-db-creds"}, result) } -// TestAzureResolver_Close_Idempotent tests that Close can be called multiple times. +// TestAzureResolver_Close_Idempotent tests that Close can be called multiple +// times. Constructor-only: no requests are made. func TestAzureResolver_Close_Idempotent(t *testing.T) { ctx := context.Background() resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } + require.NoError(t, err) // Close multiple times should not panic err1 := resolver.Close() @@ -133,9 +121,7 @@ func TestAzureResolver_DifferentVaultURLs(t *testing.T) { for _, vaultURL := range vaultURLs { t.Run(vaultURL, func(t *testing.T) { resolver, err := NewAzureResolver(ctx, vaultURL) - if err != nil { - t.Skipf("Skipping test: Azure config not available for vault %s: %v", vaultURL, err) - } + require.NoError(t, err) defer resolver.Close() assert.Equal(t, vaultURL, resolver.vaultURL) @@ -146,48 +132,35 @@ func TestAzureResolver_DifferentVaultURLs(t *testing.T) { // TestAzureResolver_ContextHandling tests context handling in Azure resolver. func TestAzureResolver_ContextHandling(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + var handlerCalls atomic.Int32 + resolver, server := newTestAzureResolver(t, func(w http.ResponseWriter, r *http.Request) { + handlerCalls.Add(1) + notFoundHandler(w, r) + }) + defer server.Close() - // Test with canceled context cancelledCtx, cancel := context.WithCancel(context.Background()) cancel() - // GetSecret with canceled context - _, err = resolver.GetSecret(cancelledCtx, "test-secret") - // Should fail - assert.Error(t, err) + // GetSecret with canceled context should fail before reaching the server. + _, err := resolver.GetSecret(cancelledCtx, "test-secret") + require.Error(t, err) + assert.Zero(t, handlerCalls.Load(), "canceled request must not reach the HTTP handler") } // TestAzureResolver_EmptySecretID tests getting a secret with empty ID. func TestAzureResolver_EmptySecretID(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() - // Empty secret ID - _, err = resolver.GetSecret(ctx, "") + _, err := resolver.GetSecret(context.Background(), "") assert.Error(t, err) } // TestAzureResolver_SpecialCharactersInSecretID tests secret IDs with special characters. func TestAzureResolver_SpecialCharactersInSecretID(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() // Azure Key Vault secret names can contain alphanumeric and dashes testIDs := []string{ @@ -198,8 +171,8 @@ func TestAzureResolver_SpecialCharactersInSecretID(t *testing.T) { for _, id := range testIDs { t.Run(id, func(t *testing.T) { - // These will fail since secrets don't exist - _, err := resolver.GetSecret(ctx, id) + // These fail because the mock vault returns 404 for every secret. + _, err := resolver.GetSecret(context.Background(), id) assert.Error(t, err) }) } @@ -207,16 +180,10 @@ func TestAzureResolver_SpecialCharactersInSecretID(t *testing.T) { // TestAzureResolver_GetSecretJSON_RealMethod tests the GetSecretJSON error propagation. func TestAzureResolver_GetSecretJSON_RealMethod(t *testing.T) { - ctx := context.Background() - - resolver, err := NewAzureResolver(ctx, "https://test-vault.vault.azure.net/") - if err != nil { - t.Skipf("Skipping test: Azure config not available: %v", err) - } - defer resolver.Close() + resolver, server := newTestAzureResolver(t, notFoundHandler) + defer server.Close() - // GetSecretJSON should fail because the secret doesn't exist - result, err := resolver.GetSecretJSON(ctx, "cudly-test-nonexistent-json-secret-xyz") + result, err := resolver.GetSecretJSON(context.Background(), "cudly-test-nonexistent-json-secret-xyz") require.Error(t, err) assert.Nil(t, result) diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index 8a13718d8..7c5d5a7b8 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -196,27 +196,73 @@ func TestRecommendationsClientAdapter_Fields(t *testing.T) { assert.Nil(t, adapter.cred) } +// overrideServiceClientFns swaps every per-service client constructor for the +// given fakes and restores the originals on cleanup, so adapter tests never +// build real ARM clients (which would issue live requests to +// management.azure.com). +func overrideServiceClientFns(t *testing.T, compute, database, cache, cosmos, sp serviceRecsGetter) { + t.Helper() + origCompute := newComputeClientFn + origDatabase := newDatabaseClientFn + origCache := newCacheClientFn + origCosmos := newCosmosDBClientFn + origSP := newSavingsPlansClientFn + t.Cleanup(func() { + newComputeClientFn = origCompute + newDatabaseClientFn = origDatabase + newCacheClientFn = origCache + newCosmosDBClientFn = origCosmos + newSavingsPlansClientFn = origSP + }) + newComputeClientFn = newFakeFn(compute) + newDatabaseClientFn = newFakeFn(database) + newCacheClientFn = newFakeFn(cache) + newCosmosDBClientFn = newFakeFn(cosmos) + newSavingsPlansClientFn = newFakeFn(sp) +} + func TestRecommendationsClientAdapter_GetRecommendationsForService(t *testing.T) { + computeRec := common.Recommendation{Provider: common.ProviderAzure, Service: common.ServiceCompute} + overrideServiceClientFns(t, + &fakeServiceClient{recs: []common.Recommendation{computeRec}}, + &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}, &fakeServiceClient{}) + adapter := &RecommendationsClientAdapter{ - cred: &mockAzureTokenCredential{}, - subscriptionID: "test-subscription", + cred: &mockAzureTokenCredential{}, + subscriptionID: "test-subscription", + getAdvisorRecsFn: noopAdvisorFn, } - // This will try to make API calls which will fail without real credentials, - // but we test the wiring is correct. Error is expected since we don't have - // real Azure credentials - the important thing is the function is wired correctly. - _, _ = adapter.GetRecommendationsForService(context.Background(), common.ServiceCompute) + recs, err := adapter.GetRecommendationsForService(context.Background(), common.ServiceCompute) + + require.NoError(t, err) + assert.Equal(t, []common.Recommendation{computeRec}, recs) } func TestRecommendationsClientAdapter_GetAllRecommendations(t *testing.T) { + rec := func(svc common.ServiceType) common.Recommendation { + return common.Recommendation{Provider: common.ProviderAzure, Service: svc} + } + overrideServiceClientFns(t, + &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceCompute)}}, + &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceRelationalDB)}}, + &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceCache)}}, + &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceNoSQL)}}, + &fakeServiceClient{}) + adapter := &RecommendationsClientAdapter{ - cred: &mockAzureTokenCredential{}, - subscriptionID: "test-subscription", + cred: &mockAzureTokenCredential{}, + subscriptionID: "test-subscription", + getAdvisorRecsFn: noopAdvisorFn, } - // This will try to make API calls which will fail without real credentials, - // but we test the wiring is correct. Error is expected - we just verify the function is callable. - _, _ = adapter.GetAllRecommendations(context.Background()) + recs, err := adapter.GetAllRecommendations(context.Background()) + + require.NoError(t, err) + assert.ElementsMatch(t, []common.Recommendation{ + rec(common.ServiceCompute), rec(common.ServiceRelationalDB), + rec(common.ServiceCache), rec(common.ServiceNoSQL), + }, recs) } // TestGetRecommendations_SavingsPlansServiceIncluded pins that shouldIncludeService