diff --git a/providers/azure/accounts_cache.go b/providers/azure/accounts_cache.go index 11bb33f1..523c0926 100644 --- a/providers/azure/accounts_cache.go +++ b/providers/azure/accounts_cache.go @@ -97,6 +97,11 @@ func accountsSFKey(gen uint64) string { return accountsCacheSFKeyPrefix + strconv.FormatUint(gen, 10) } +// errAccountsStale signals that the credential, subscriptions client or cache +// generation changed while a fetch was in flight (a SetCredential contract +// violation), so the fetched list belongs to the previous principal. +var errAccountsStale = errors.New("azure accounts cache: credential or subscriptions client changed during fetch") + // fetchAccountsShared collapses concurrent cold-cache callers onto a single // in-flight ARM subscriptions.List and returns the SHARED (un-cloned) slice. // Callers must clone before handing the result outside the package. @@ -122,14 +127,19 @@ func (p *Provider) fetchAccountsShared(ctx context.Context) ([]common.Account, e } p.accountsMu.Lock() - // Publish only when no InvalidateAccountsCache landed while the ARM - // call was in flight. A fetch that started before an invalidation - // carries a pre-invalidation snapshot; writing it now would silently - // resurrect exactly the data the caller asked to discard. - if p.accountsGen == gen { - p.cachedAccounts = accounts + defer p.accountsMu.Unlock() + // A generation bump while the ARM call was in flight (SetCredential, + // SetSubscriptionsClient or InvalidateAccountsCache) makes this list a + // snapshot of what the PREVIOUS principal could see. Publishing it would + // resurrect discarded data, and handing it to the callers that joined + // the singleflight would let GetRecommendationsClient fan out over the + // old credential's subscriptions using the new credential. Fail loud + // instead of re-fetching: a swap mid-fetch is a caller contract + // violation, and one retry loop here would hide it. + if p.accountsGen != gen { + return nil, errAccountsStale } - p.accountsMu.Unlock() + p.cachedAccounts = accounts return accounts, nil }) diff --git a/providers/azure/internal/pricing/odata.go b/providers/azure/internal/pricing/odata.go new file mode 100644 index 00000000..30bdd970 --- /dev/null +++ b/providers/azure/internal/pricing/odata.go @@ -0,0 +1,22 @@ +package pricing + +import ( + "fmt" + "regexp" + "strings" +) + +// odataTokenRE is the character set of every Azure SKU and region name the +// Retail Prices filters are built from. Anything else is rejected rather than +// escaped, so a value can never change the shape of the $filter. +var odataTokenRE = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`) + +// ODataString returns v as a single-quoted OData string literal for a Retail +// Prices $filter. It rejects values outside [A-Za-z0-9_.-]; the quote doubling +// is defense in depth should the allowed set ever grow. +func ODataString(v string) (string, error) { + if !odataTokenRE.MatchString(v) { + return "", fmt.Errorf("invalid value %q for Retail Prices filter: must match %s", v, odataTokenRE) + } + return "'" + strings.ReplaceAll(v, "'", "''") + "'", nil +} diff --git a/providers/azure/internal/pricing/odata_test.go b/providers/azure/internal/pricing/odata_test.go new file mode 100644 index 00000000..52f01eff --- /dev/null +++ b/providers/azure/internal/pricing/odata_test.go @@ -0,0 +1,32 @@ +package pricing + +import "testing" + +func TestODataString(t *testing.T) { + tests := []struct { + name string + in string + want string + wantErr bool + }{ + {"region", "eastus", "'eastus'", false}, + {"valid sku", "GP_Gen5_2", "'GP_Gen5_2'", false}, + {"dots and dashes", "Azure_Redis_Cache-E10.1", "'Azure_Redis_Cache-E10.1'", false}, + {"single quote", "a'b", "", true}, + {"injection attempt", "x' or '1' eq '1", "", true}, + {"space", "East US", "", true}, + {"empty", "", "", true}, + {"newline", "a\nb", "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ODataString(tt.in) + if (err != nil) != tt.wantErr { + t.Fatalf("err = %v, wantErr %v", err, tt.wantErr) + } + if got != tt.want { + t.Fatalf("got %q, want %q", got, tt.want) + } + }) + } +} diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index 25324452..092527b0 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -1559,11 +1559,13 @@ func TestAzureProvider_ConcurrentCredentialSwapAndFetch_NoDataRace(t *testing.T) // Both readers of the swapped fields: GetAccounts reaches // fetchAccounts, GetServiceClientForAccount builds a client // straight from the credential. - if _, err := p.GetAccounts(context.Background()); err != nil { + // The swappers invalidate constantly, so a fetch failing as + // stale is the expected outcome here, not a defect. + if _, err := p.GetAccounts(context.Background()); err != nil && !errors.Is(err, errAccountsStale) { errs <- err return } - if _, err := p.GetServiceClientForAccount(context.Background(), common.ServiceCompute, "eastus", "sub-1"); err != nil { + if _, err := p.GetServiceClientForAccount(context.Background(), common.ServiceCompute, "eastus", "sub-1"); err != nil && !errors.Is(err, errAccountsStale) { errs <- err return } @@ -1702,9 +1704,9 @@ func TestAzureProvider_InvalidateAccountsCache_DuringInFlightFetch(t *testing.T) close(gated.release) first := <-done - require.NoError(t, first.err) - require.Len(t, first.accounts, 1) - assert.Equal(t, "sub-gen-1", first.accounts[0].ID) + require.ErrorIs(t, first.err, errAccountsStale, + "a fetch invalidated mid-flight must fail, not hand back the stale snapshot") + assert.Empty(t, first.accounts) // The invalidated snapshot must not have been published: this read has to // re-hit ARM and observe the current subscription list. @@ -1717,6 +1719,39 @@ func TestAzureProvider_InvalidateAccountsCache_DuringInFlightFetch(t *testing.T) "read after invalidation must see fresh data, not the resurrected pre-invalidation snapshot") } +// A credential swap landing during an in-flight fetch must not hand the old +// credential's subscription list to the caller that joined the singleflight +// (GetRecommendationsClient would fan out over it using the new credential), +// and must not populate the cache. +func TestAzureProvider_SetCredential_DuringInFlightFetch_ReturnsError(t *testing.T) { + clearAzureSubscriptionEnv(t) + gated := &gatedGenerationSubscriptionsClient{ + firstInFlight: make(chan struct{}), + release: make(chan struct{}), + } + p := &Provider{cred: &mockTokenCredential{}} + p.SetSubscriptionsClient(gated) + + type fetchResult struct { + accounts []common.Account + err error + } + done := make(chan fetchResult, 1) + go func() { + accts, err := p.getOrFetchAccounts(context.Background()) + done <- fetchResult{accounts: accts, err: err} + }() + + <-gated.firstInFlight + p.SetCredential(&mockTokenCredential{}) + close(gated.release) + + res := <-done + require.ErrorIs(t, res.err, errAccountsStale) + assert.Nil(t, res.accounts) + assert.Nil(t, p.readCachedAccounts(), "stale fetch must not write the cache") +} + func TestAzureProvider_GetRecommendationsClient_MultiSubscriptionFanOut(t *testing.T) { clearAzureSubscriptionEnv(t) diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 2c577b20..e58031cb 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -572,8 +572,16 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe if err != nil { return nil, err } - filter := fmt.Sprintf("serviceName eq 'Redis Cache' and armRegionName eq '%s' and armSkuName eq '%s' and priceType eq 'Reservation'", - strings.ReplaceAll(region, "'", "''"), identity.ArmSKUName) + qRegion, err := pricing.ODataString(region) + if err != nil { + return nil, err + } + qSKU, err := pricing.ODataString(identity.ArmSKUName) + if err != nil { + return nil, err + } + filter := fmt.Sprintf("serviceName eq 'Redis Cache' and armRegionName eq %s and armSkuName eq %s and priceType eq 'Reservation'", + qRegion, qSKU) params := url.Values{} params.Add("$filter", filter) diff --git a/providers/azure/services/cache/retail_selection_test.go b/providers/azure/services/cache/retail_selection_test.go index 78831563..4d23d9e9 100644 --- a/providers/azure/services/cache/retail_selection_test.go +++ b/providers/azure/services/cache/retail_selection_test.go @@ -191,16 +191,15 @@ func TestRedisOffering_CapturedReservations(t *testing.T) { } } -func TestRedisOffering_FilterAndEscaping(t *testing.T) { +func TestRedisOffering_RejectsQuoteInRegion(t *testing.T) { for _, c := range redisClients() { t.Run(c.name, func(t *testing.T) { - r := redisCatalog()[0].row(1) - r["armRegionName"] = "east'us" - filter := []string{"serviceName eq 'Redis Cache'", "armRegionName eq 'east''us'", "armSkuName eq 'Azure_Redis_Cache_Premium_P1_Cache'", "priceType eq 'Reservation'"} - h, _ := redisHTTP(t, [][]map[string]any{{redisConsumption(), r}}, filter) + h, requests := redisHTTP(t, nil, nil) q, err := c.newClient("east'us", h).GetOfferingDetails(context.Background(), common.Recommendation{ResourceType: "Premium_P1", Term: "1yr", PaymentOption: "upfront"}) - require.NoError(t, err) - assert.Equal(t, 1553.0, q.TotalCost) + require.Error(t, err) + assert.Nil(t, q) + assert.Contains(t, err.Error(), "Retail Prices filter") + assert.Equal(t, int32(0), requests.Load()) }) } } diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index 220577e7..2bddd757 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -587,8 +587,16 @@ type SQLPricing struct { // getSQLPricing gets real pricing from Azure Retail Prices API. func (c *Client) getSQLPricing(ctx context.Context, sku, region string, termYears int) (*SQLPricing, error) { - filter := fmt.Sprintf("serviceName eq 'SQL Database' and armRegionName eq '%s' and armSkuName eq '%s' and priceType eq 'Reservation'", - strings.ReplaceAll(region, "'", "''"), strings.ReplaceAll(sku, "'", "''")) + qRegion, err := pricing.ODataString(region) + if err != nil { + return nil, err + } + qSKU, err := pricing.ODataString(sku) + if err != nil { + return nil, err + } + filter := fmt.Sprintf("serviceName eq 'SQL Database' and armRegionName eq %s and armSkuName eq %s and priceType eq 'Reservation'", + qRegion, qSKU) priceData, err := c.fetchAzurePricing(ctx, filter) if err != nil { diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index 00e430e2..5552a756 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -558,37 +558,16 @@ func TestDatabaseClient_GetOfferingDetails_SelectsSyntheticVCoreRow(t *testing.T } } -func TestDatabaseClient_GetOfferingDetails_ExactEscapedReservationFilter(t *testing.T) { - const region, sku = "east'us", "GeneralPurpose_Gen5_'2" - row := azpricing.RetailPriceItem{ - ServiceName: "SQL Database", ArmRegionName: region, ArmSKUName: sku, - Type: "Reservation", ReservationTerm: "1 Year", UnitOfMeasure: "1 Hour", - CurrencyCode: "EUR", RetailPrice: 1200, - } - body, err := json.Marshal(AzureRetailPrice{Items: []azpricing.RetailPriceItem{row}}) - require.NoError(t, err) - mockHTTP := &MockHTTPClient{} - response := createMockHTTPResponse(http.StatusOK, string(body)) - t.Cleanup(func() { - require.NoError(t, response.Body.Close()) +func TestDatabaseClient_GetOfferingDetails_RejectsQuoteInRegionAndSKU(t *testing.T) { + // Quote characters are rejected outright (not escaped); the mock has no + // expectations, so any request would panic. + client := NewClientWithHTTP(nil, "sub", "east'us", &MockHTTPClient{}) + details, err := client.GetOfferingDetails(context.Background(), common.Recommendation{ + ResourceType: "GeneralPurpose_Gen5_'2", Term: "1yr", PaymentOption: "upfront", }) - mockHTTP.On("Do", mock.Anything).Return(response, nil).Run(func(args mock.Arguments) { - req := args.Get(0).(*http.Request) - assert.Equal(t, http.MethodGet, req.Method) - assert.Equal(t, "https", req.URL.Scheme) - assert.Equal(t, "prices.azure.com", req.URL.Host) - assert.Equal(t, "/api/retail/prices", req.URL.Path) - assert.Equal(t, "2023-01-01-preview", req.URL.Query().Get("api-version")) - assert.Equal(t, "serviceName eq 'SQL Database' and armRegionName eq 'east''us' and armSkuName eq 'GeneralPurpose_Gen5_''2' and priceType eq 'Reservation'", req.URL.Query().Get("$filter")) - }).Once() - details, err := NewClientWithHTTP(nil, "sub", region, mockHTTP).GetOfferingDetails(context.Background(), common.Recommendation{ - ResourceType: sku, Term: "1yr", PaymentOption: "upfront", - }) - require.NoError(t, err) - require.NotNil(t, details) - assert.Equal(t, 1200.0, details.TotalCost) - assert.Equal(t, "EUR", details.Currency) - mockHTTP.AssertExpectations(t) + require.Error(t, err) + assert.Nil(t, details) + assert.Contains(t, err.Error(), "Retail Prices filter") } func TestDatabaseClient_GetOfferingDetails_ReservationAcrossPages(t *testing.T) { @@ -2119,6 +2098,50 @@ func TestDatabaseClient_ServerInfo_FetchedOnce(t *testing.T) { "servers pager NextPage not called when it has no pages") } +// An injected SKU or region must be rejected before any Retail Prices request +// is built; the mock has no expectations, so any HTTP call would panic. +func TestGetSQLPricing_RejectsInjectedFilterValues(t *testing.T) { + for _, tc := range []struct{ name, sku, region string }{ + {"sku", "x' or armSkuName ne '", "eastus"}, + {"region", "GP_Gen5_2", "eastus' or '1' eq '1"}, + } { + t.Run(tc.name, func(t *testing.T) { + client := NewClientWithHTTP(nil, "test-subscription", "eastus", &MockHTTPClient{}) + _, err := client.getSQLPricing(context.Background(), tc.sku, tc.region, 1) + require.Error(t, err) + assert.Contains(t, err.Error(), "Retail Prices filter") + }) + } +} + +func TestDatabaseClient_GetOfferingDetails_ExactReservationFilter(t *testing.T) { + const region, sku = "eastus", "GP_Gen5_2" + row := azpricing.RetailPriceItem{ + ServiceName: "SQL Database", ArmRegionName: region, ArmSKUName: sku, + Type: "Reservation", ReservationTerm: "1 Year", UnitOfMeasure: "1 Hour", + CurrencyCode: "EUR", RetailPrice: 1200, + } + body, err := json.Marshal(AzureRetailPrice{Items: []azpricing.RetailPriceItem{row}}) + require.NoError(t, err) + mockHTTP := &MockHTTPClient{} + response := createMockHTTPResponse(http.StatusOK, string(body)) + t.Cleanup(func() { + require.NoError(t, response.Body.Close()) + }) + mockHTTP.On("Do", mock.Anything).Return(response, nil).Run(func(args mock.Arguments) { + req := args.Get(0).(*http.Request) + assert.Equal(t, "2023-01-01-preview", req.URL.Query().Get("api-version")) + assert.Equal(t, "serviceName eq 'SQL Database' and armRegionName eq 'eastus' and armSkuName eq 'GP_Gen5_2' and priceType eq 'Reservation'", req.URL.Query().Get("$filter")) + }).Once() + details, err := NewClientWithHTTP(nil, "sub", region, mockHTTP).GetOfferingDetails(context.Background(), common.Recommendation{ + ResourceType: sku, Term: "1yr", PaymentOption: "upfront", + }) + require.NoError(t, err) + require.NotNil(t, details) + assert.Equal(t, 1200.0, details.TotalCost) + mockHTTP.AssertExpectations(t) +} + // TestNewClientWithHTTP_NilFallbackIsHardened verifies that passing nil as the // httpClient falls back to the SSRF-hardened httpclient.New(), which rejects // IMDS connections, instead of leaving a nil client that panics on first use. diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index def08b23..7d3d9bf6 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -518,8 +518,16 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe if err != nil { return nil, err } - filter := fmt.Sprintf("serviceName eq 'Redis Cache' and armRegionName eq '%s' and armSkuName eq '%s' and priceType eq 'Reservation'", - strings.ReplaceAll(region, "'", "''"), identity.ArmSKUName) + qRegion, err := pricing.ODataString(region) + if err != nil { + return nil, err + } + qSKU, err := pricing.ODataString(identity.ArmSKUName) + if err != nil { + return nil, err + } + filter := fmt.Sprintf("serviceName eq 'Redis Cache' and armRegionName eq %s and armSkuName eq %s and priceType eq 'Reservation'", + qRegion, qSKU) params := url.Values{} params.Add("$filter", filter) diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index dc3d91a7..3fedab73 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -464,8 +464,16 @@ type Pricing struct { // getSynapsePricing fetches pricing from the Azure Retail Prices API. func (c *Client) getSynapsePricing(ctx context.Context, sku, region string, termYears int) (*Pricing, error) { - filter := fmt.Sprintf("serviceName eq 'Azure Synapse Analytics' and productName eq 'Azure Synapse Analytics Dedicated SQL Pool' and armRegionName eq '%s' and skuName eq '%s' and priceType eq 'Reservation'", - strings.ReplaceAll(region, "'", "''"), strings.ReplaceAll(sku, "'", "''")) + qRegion, err := pricing.ODataString(region) + if err != nil { + return nil, err + } + qSKU, err := pricing.ODataString(sku) + if err != nil { + return nil, err + } + filter := fmt.Sprintf("serviceName eq 'Azure Synapse Analytics' and productName eq 'Azure Synapse Analytics Dedicated SQL Pool' and armRegionName eq %s and skuName eq %s and priceType eq 'Reservation'", + qRegion, qSKU) params := url.Values{} params.Add("$filter", filter) diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index 1124750b..6e910970 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -957,16 +957,13 @@ func TestGetOfferingDetails_Pagination(t *testing.T) { } } -func TestGetOfferingDetails_QueryEscapesRegionAndSKU(t *testing.T) { - client, requests := synapseQuoteClient(t, "east'us", synapsePricePage(t, []map[string]any{ - synapseCapturedDW100c(map[string]any{"armRegionName": "east'us", "skuName": "DW'100c"}), - }, "")) +func TestGetOfferingDetails_RejectsQuoteInRegionAndSKU(t *testing.T) { + client, requests := synapseQuoteClient(t, "east'us", synapsePricePage(t, nil, "")) quote, err := client.GetOfferingDetails(context.Background(), common.Recommendation{ResourceType: "DW'100c", Term: "1yr", PaymentOption: "upfront"}) - require.NoError(t, err) - assert.Equal(t, 8333.0, quote.TotalCost) - require.Len(t, *requests, 1) - assert.Equal(t, "serviceName eq 'Azure Synapse Analytics' and productName eq 'Azure Synapse Analytics Dedicated SQL Pool' and armRegionName eq 'east''us' and skuName eq 'DW''100c' and priceType eq 'Reservation'", (*requests)[0].URL.Query().Get("$filter")) - assert.Contains(t, (*requests)[0].URL.RawQuery, "%24filter=") + require.Error(t, err) + assert.Nil(t, quote) + assert.Contains(t, err.Error(), "Retail Prices filter") + assert.Empty(t, *requests) } func TestGetOfferingDetails_RejectsUnsupportedTermAndPayment(t *testing.T) {