diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 265859ea4..a298663d5 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -194,8 +194,10 @@ type azureExchangeClient interface { // wired OIDC signer so managed_identity / client_secret / WIF all work. // // Graceful empty-state rules (returns nil client, nil error): -// - subscriptionID is empty AND no Azure accounts are registered at all: -// Azure is not configured; the caller returns an empty reservations list. +// - subscriptionID is empty: GetCloudAccountByExternalID looks up an exact +// match on external_id, and no registered account has an empty +// external_id, so this unconditionally misses and the caller returns an +// empty reservations list. // - subscriptionID is provided but no matching CloudAccount is found: // treated as "Azure not configured for this subscription". // @@ -232,21 +234,67 @@ func (h *Handler) buildAzureExchangeClient(ctx context.Context, subscriptionID s return azurecompute.NewClient(cred, subscriptionID, ""), nil } -// listExchangeableAzureRIs returns all active Azure VM reservations that are +// listExchangeableAzureRIs returns active Azure VM reservations that are // eligible for the cross-SKU/cross-region exchange flow (InstanceFlexibility // == On, ProvisioningState == Succeeded). Requires "view:purchases" permission. // -// The optional ?subscription_id= query parameter scopes the credential lookup -// to the matching registered CloudAccount. When no Azure account is configured -// for the requested subscription (or no subscription is specified and none are -// registered), the handler returns an empty reservations list rather than a 500. +// ListExchangeableReservations is tenant-wide by design (see its doc +// comment: "the Azure Capacity exchange API operates on reservation order +// IDs which span subscriptions"), and that breadth does not change just +// because ?subscription_id= was supplied -- the parameter only picks which +// registered CloudAccount's credentials authenticate the call, not which +// rows come back. Left unfiltered, every caller with "view:purchases" saw +// every reservation the deployment's Azure credential could read, including +// subscriptions they are not scoped to (issue #1656). Both POST siblings on +// this resource deliberately withhold that same breadth (requireAzureSubscriptionScope +// + requireAzureSourceOwnership, both indistinguishable-denial), so this GET +// narrows the same way: +// +// - ?subscription_id= supplied: requireAzureSubscriptionScope gates +// whether the session may use that subscription at all (errNotFound, +// not 403, matching the POST siblings so a scoped caller cannot probe +// which subscriptions exist). The listing is then narrowed to that +// subscription's own rows via BillingScopeID -- the same discriminator +// requireAzureSourceOwnership uses -- since a reservation billed +// elsewhere could never be named as a valid exchange source against +// this subscription anyway. +// - No subscription_id: the listing is narrowed to whatever the session's +// allowed_accounts scope covers (filterAzureReservationsByScope). +// Unrestricted/admin sessions see everything, matching every other +// listing endpoint in this package. In production this branch is +// defense-in-depth rather than the live path today: buildAzureExchangeClient +// resolves subscriptionID via an exact external_id match, so an empty +// subscriptionID never matches a registered account and the graceful +// empty-state branch below returns before ListExchangeableReservations +// is ever called. It becomes the live path the moment a caller can reach +// ListExchangeableReservations without a subscription_id -- the +// azureExchangeFactory test seam already exercises exactly that -- which +// is why the filter stays rather than being deleted as unreachable. +// +// Rows that cannot be attributed to a registered CloudAccount (no +// BillingScopeID, or one that matches no CloudAccount) are dropped rather +// than kept, so an unresolvable scope never degrades to "show everything". +// That also keeps the two empty-result cases indistinguishable: a scoped +// caller whose own subscriptions hold zero exchangeable reservations sees +// the same empty list as one whose rows could not be attributed. +// +// When no Azure account is configured for the requested subscription (or no +// subscription is specified and none are registered), the handler returns an +// empty reservations list rather than a 500. func (h *Handler) listExchangeableAzureRIs(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { - if _, err := h.requirePermission(ctx, req, "view", "purchases"); err != nil { + session, err := h.requirePermission(ctx, req, "view", "purchases") + if err != nil { return nil, err } subscriptionID := req.QueryStringParameters["subscription_id"] + if subscriptionID != "" { + if scopeErr := h.requireAzureSubscriptionScope(ctx, session, subscriptionID); scopeErr != nil { + return nil, scopeErr + } + } + client, err := h.buildAzureExchangeClient(ctx, subscriptionID) if err != nil { return nil, fmt.Errorf("failed to build Azure exchange client: %w", err) @@ -263,9 +311,130 @@ func (h *Handler) listExchangeableAzureRIs(ctx context.Context, req *events.Lamb return nil, fmt.Errorf("failed to list exchangeable Azure reservations: %w", err) } + if subscriptionID != "" { + reservations = filterAzureReservationsBySubscription(reservations, subscriptionID) + } else { + reservations, err = h.filterAzureReservationsByScope(ctx, session, reservations) + if err != nil { + return nil, err + } + } + return &ExchangeableAzureRIsResponse{Reservations: reservations}, nil } +// filterAzureReservationsBySubscription narrows a tenant-wide reservation +// listing down to the rows billed to subscriptionID, using the same +// BillingScopeID discriminator requireAzureSourceOwnership keys on (issue +// #1527). Reservations with no BillingScopeID, or one billed to a different +// subscription, are dropped. +// +// Called only after requireAzureSubscriptionScope has cleared the caller for +// subscriptionID; a reservation billed elsewhere could never be named as a +// valid exchange source against this subscription anyway +// (requireAzureSourceOwnership refuses cross-subscription sources), so +// returning the wider tenant listing here would just be the enumeration leak +// issue #1656 describes. +func filterAzureReservationsBySubscription(reservations []azurecompute.ExchangeableReservation, subscriptionID string) []azurecompute.ExchangeableReservation { + scope := azureBillingScopeID(subscriptionID) + filtered := make([]azurecompute.ExchangeableReservation, 0, len(reservations)) + for i := range reservations { + if strings.EqualFold(reservations[i].BillingScopeID, scope) { + filtered = append(filtered, reservations[i]) + } + } + return filtered +} + +// filterAzureReservationsByScope narrows a tenant-wide Azure reservation +// listing down to the rows billed to a subscription the session's +// allowed_accounts scope covers. Mirrors filterDashboardRecommendations +// (handler_dashboard.go): unrestricted/admin sessions pass through +// unchanged; a restricted session keeps only the rows it can resolve back to +// one of its allowed CloudAccounts. +// +// Each reservation's BillingScopeID ("/subscriptions/{subscriptionID}") is +// the same ownership signal requireAzureSourceOwnership keys on (issue +// #1527) -- the subscription actually charged, which is the correct +// discriminator even for AppliedScopeType == Shared. +// +// Fails closed: a reservation with no BillingScopeID, or one that matches no +// registered Azure CloudAccount, is dropped rather than kept. Counts, not +// the dropped reservation/billing-scope identifiers, are safe to log -- +// logging the identifiers would just relocate the disclosure this filter +// exists to prevent. +func (h *Handler) filterAzureReservationsByScope(ctx context.Context, session *Session, reservations []azurecompute.ExchangeableReservation) ([]azurecompute.ExchangeableReservation, error) { + allowed, err := h.getAllowedAccounts(ctx, session) + if err != nil { + return nil, fmt.Errorf("failed to get allowed accounts: %w", err) + } + if auth.IsUnrestrictedAccess(allowed) { + // client.ListExchangeableReservations may return a nil slice for zero + // results; every other path here returns a non-nil empty slice, so + // normalize here too rather than letting an admin session alone see + // {"reservations": null}. + if reservations == nil { + reservations = []azurecompute.ExchangeableReservation{} + } + return reservations, nil + } + + provider := "azure" + accounts, err := h.config.ListCloudAccounts(ctx, config.CloudAccountFilter{Provider: &provider}) + if err != nil { + return nil, fmt.Errorf("failed to list cloud accounts: %w", err) + } + + filtered := filterReservationsByScopeIndex(reservations, azureScopeIndex(accounts), allowed) + if dropped := len(reservations) - len(filtered); dropped > 0 { + logging.Debugf("azure exchange listing: filtered %d of %d tenant-wide reservations outside session scope", dropped, len(reservations)) + } + return filtered, nil +} + +// azureScopeIndex builds a lookup from ARM billing scope (lower-cased) to the +// registered Azure CloudAccount that owns it, so filterAzureReservationsByScope +// can resolve each reservation's BillingScopeID straight to its CloudAccount +// without re-deriving azureBillingScopeID per row. Accounts with no +// ExternalID are skipped -- they cannot own any BillingScopeID. +// +// Split out of filterAzureReservationsByScope, together with +// filterReservationsByScopeIndex below, to stay under the project's gocyclo +// threshold (mirrors validateAzureOfferingsBody's precedent). +func azureScopeIndex(accounts []config.CloudAccount) map[string]config.CloudAccount { + index := make(map[string]config.CloudAccount, len(accounts)) + for i := range accounts { + if accounts[i].ExternalID == "" { + continue + } + index[strings.ToLower(azureBillingScopeID(accounts[i].ExternalID))] = accounts[i] + } + return index +} + +// filterReservationsByScopeIndex narrows reservations down to the rows whose +// BillingScopeID resolves, via scopeToAccount, to a CloudAccount the allowed +// list covers (auth.MatchesAccount). Fails closed: a reservation with no +// BillingScopeID, or one that resolves to no registered CloudAccount, is +// dropped rather than kept. +func filterReservationsByScopeIndex(reservations []azurecompute.ExchangeableReservation, scopeToAccount map[string]config.CloudAccount, allowed []string) []azurecompute.ExchangeableReservation { + filtered := make([]azurecompute.ExchangeableReservation, 0, len(reservations)) + for i := range reservations { + r := reservations[i] + if r.BillingScopeID == "" { + continue + } + account, ok := scopeToAccount[strings.ToLower(r.BillingScopeID)] + if !ok { + continue + } + if auth.MatchesAccount(allowed, account.ID, account.Name) { + filtered = append(filtered, r) + } + } + return filtered +} + // maxAzureExchangeItems caps the number of sources/targets accepted per // Azure exchange request, guarding against an oversized request fanning out // into an enormous CalculateExchange payload. diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 9c8c8b126..11fb15e81 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -822,6 +822,202 @@ func TestListExchangeableAzureRIs_SubscriptionIDPassedToFactory(t *testing.T) { assert.Equal(t, "sub-abc", capturedSubID) } +// --- allowed_accounts scoping on the tenant-wide listing (issue #1656) --- +// +// ListExchangeableReservations returns every reservation the deployment's +// Azure credential can read, tenant-wide. Before this fix, a caller with +// "view:purchases" saw every subscription's reservations regardless of their +// own allowed_accounts scope. These tests exercise the fix directly: a +// scoped session must see only the rows billed to a subscription it is +// scoped to, both with and without ?subscription_id=. + +// TestListExchangeableAzureRIs_ScopedFiltersToOwnSubscription is the core +// regression: a session scoped to account A's subscription, calling the +// listing with NO subscription_id filter, must receive only account A's +// reservation. Account B's reservation -- present in the same tenant-wide +// listing -- must not appear anywhere in the response. +func TestListExchangeableAzureRIs_ScopedFiltersToOwnSubscription(t *testing.T) { + ctx := context.Background() + mockAuth := scopedAzureAuth(t, "view", "purchases", []string{"acct-a"}) + store := &MockConfigStore{} + store.ListCloudAccountsFn = func(_ context.Context, filter config.CloudAccountFilter) ([]config.CloudAccount, error) { + require.NotNil(t, filter.Provider, "the scope filter must narrow to azure accounts") + require.Equal(t, "azure", *filter.Provider) + return []config.CloudAccount{ + {ID: "acct-a", Name: "A Team", Provider: "azure", ExternalID: "sub-a"}, + {ID: "acct-b", Name: "B Team", Provider: "azure", ExternalID: "sub-b"}, + }, nil + } + stub := &stubAzureExchangeClient{reservations: []azurecompute.ExchangeableReservation{ + {ReservationID: "res-a", BillingScopeID: "/subscriptions/sub-a", SKU: "Standard_D2s_v3", Quantity: 1, Region: "eastus"}, + {ReservationID: "res-b", BillingScopeID: "/subscriptions/sub-b", SKU: "Standard_D4s_v3", Quantity: 3, Region: "westeurope"}, + }} + + h := &Handler{ + auth: mockAuth, + config: store, + azureExchangeFactory: func(_ string) azureExchangeClient { return stub }, + } + + res, err := h.listExchangeableAzureRIs(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + }) + require.NoError(t, err) + resp, ok := res.(*ExchangeableAzureRIsResponse) + require.True(t, ok) + require.Len(t, resp.Reservations, 1, "only the scoped subscription's reservation must be returned") + assert.Equal(t, "res-a", resp.Reservations[0].ReservationID) + for _, r := range resp.Reservations { + assert.NotEqual(t, "res-b", r.ReservationID, "account B's reservation must never appear in a scoped response") + assert.NotContains(t, r.BillingScopeID, "sub-b", "account B's billing scope must not leak into a scoped response") + } +} + +// TestListExchangeableAzureRIs_ScopedEmptyCasesIndistinguishable asserts +// that a scoped caller cannot tell "my subscriptions genuinely have zero +// exchangeable reservations" apart from "the tenant-wide listing had rows, +// but none could be attributed to a registered account I'm scoped to" -- +// both must render as the same empty list, per the doc comment on +// listExchangeableAzureRIs: an unresolvable scope must never degrade to +// "show everything", and must not leak its distinctness either. +func TestListExchangeableAzureRIs_ScopedEmptyCasesIndistinguishable(t *testing.T) { + ctx := context.Background() + + // Case 1: the scoped account (A) is registered, but the only tenant-wide + // reservation belongs to a different, unscoped subscription (B). After + // filtering, account A itself simply has zero reservations. + zeroOwnAuth := scopedAzureAuth(t, "view", "purchases", []string{"acct-a"}) + zeroOwnStore := &MockConfigStore{} + zeroOwnStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{ + {ID: "acct-a", Name: "A Team", Provider: "azure", ExternalID: "sub-a"}, + {ID: "acct-b", Name: "B Team", Provider: "azure", ExternalID: "sub-b"}, + }, nil + } + zeroOwnStub := &stubAzureExchangeClient{reservations: []azurecompute.ExchangeableReservation{ + {ReservationID: "res-b", BillingScopeID: "/subscriptions/sub-b", SKU: "Standard_D4s_v3", Quantity: 1, Region: "eastus"}, + }} + hZeroOwn := &Handler{ + auth: zeroOwnAuth, + config: zeroOwnStore, + azureExchangeFactory: func(_ string) azureExchangeClient { return zeroOwnStub }, + } + zeroOwnRes, zeroOwnErr := hZeroOwn.listExchangeableAzureRIs(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + }) + + // Case 2: the scoped account (A) is registered, and the tenant-wide + // listing has a row, but that row cannot be attributed to ANY registered + // CloudAccount (no matching BillingScopeID) -- e.g. a reservation billed + // to a subscription that was never onboarded. + unattributedAuth := scopedAzureAuth(t, "view", "purchases", []string{"acct-a"}) + unattributedStore := &MockConfigStore{} + unattributedStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{ + {ID: "acct-a", Name: "A Team", Provider: "azure", ExternalID: "sub-a"}, + }, nil + } + unattributedStub := &stubAzureExchangeClient{reservations: []azurecompute.ExchangeableReservation{ + {ReservationID: "res-orphan", BillingScopeID: "/subscriptions/sub-unregistered", SKU: "Standard_D4s_v3", Quantity: 1, Region: "eastus"}, + }} + hUnattributed := &Handler{ + auth: unattributedAuth, + config: unattributedStore, + azureExchangeFactory: func(_ string) azureExchangeClient { return unattributedStub }, + } + unattributedRes, unattributedErr := hUnattributed.listExchangeableAzureRIs(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + }) + + require.NoError(t, zeroOwnErr) + require.NoError(t, unattributedErr) + zeroOwnResp, ok := zeroOwnRes.(*ExchangeableAzureRIsResponse) + require.True(t, ok) + unattributedResp, ok := unattributedRes.(*ExchangeableAzureRIsResponse) + require.True(t, ok) + assert.Empty(t, zeroOwnResp.Reservations) + assert.Empty(t, unattributedResp.Reservations) + assert.Equal(t, zeroOwnResp.Reservations, unattributedResp.Reservations, + "a scoped caller with genuinely zero reservations and one whose rows could not be attributed must render identically") +} + +// TestListExchangeableAzureRIs_SubscriptionIDOutOfScope asserts the +// ?subscription_id= path returns the same not-found shape (errNotFound) the +// POST siblings (getAzureCompatibleOfferings, executeAzureExchange) use for +// an out-of-scope subscription, so a scoped caller cannot use the listing +// endpoint to probe which subscriptions exist outside their scope. +func TestListExchangeableAzureRIs_SubscriptionIDOutOfScope(t *testing.T) { + ctx := context.Background() + opsClient := new(mockAzureExchangeOpsClient) + ownsAzureSource(opsClient) // Maybe(): must never actually be called + t.Cleanup(func() { opsClient.AssertExpectations(t) }) + + h := &Handler{ + auth: scopedAzureAuth(t, "view", "purchases", []string{"acct-mine"}), + config: scopedAzureStore(), // only registered account is acct-other + azureExchangeFactory: func(_ string) azureExchangeClient { return opsClient }, + } + + _, err := h.listExchangeableAzureRIs(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + QueryStringParameters: map[string]string{"subscription_id": "sub-1"}, + }) + require.Error(t, err, "a scoped session must not list an out-of-scope subscription's reservations") + assert.ErrorIs(t, err, errNotFound, "must be the generic scope-check 404, matching the POST siblings") + // ownsAzureSource registers ListExchangeableReservations with .Maybe(), so + // AssertExpectations above passes whether it was called or not; assert it + // explicitly to prove the scope gate refuses the request before the + // tenant-wide listing is ever fetched. + opsClient.AssertNotCalled(t, "ListExchangeableReservations") +} + +// TestListExchangeableAzureRIs_SubscriptionIDFiltersToOwnRows exercises +// filterAzureReservationsBySubscription directly through the handler -- the +// row-level filter on the ?subscription_id= path, which is the only filter +// this listing's production callers can actually reach (buildAzureExchangeClient +// resolves subscriptionID via an exact external_id match, so the +// no-subscription_id branch that reaches filterAzureReservationsByScope never +// gets a live Azure client in production; see the doc comment on +// listExchangeableAzureRIs). +// +// The scoped session is authorized for subscription sub-1 (requireAzureSubscriptionScope +// passes), and the tenant-wide listing the injected client returns mixes rows +// billed to sub-1 and sub-2 -- the same tenant-wide breadth +// ListExchangeableReservations always returns regardless of which +// subscription's credentials made the call. Only the sub-1 row must survive. +func TestListExchangeableAzureRIs_SubscriptionIDFiltersToOwnRows(t *testing.T) { + ctx := context.Background() + store := &MockConfigStore{} + store.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + require.Equal(t, "azure", provider) + require.Equal(t, "sub-1", externalID) + return &config.CloudAccount{ID: "acct-mine", Name: "Mine Team", Provider: "azure", ExternalID: "sub-1"}, nil + } + stub := &stubAzureExchangeClient{reservations: []azurecompute.ExchangeableReservation{ + {ReservationID: "res-own", BillingScopeID: "/subscriptions/sub-1", SKU: "Standard_D2s_v3", Quantity: 1, Region: "eastus"}, + {ReservationID: "res-other", BillingScopeID: "/subscriptions/sub-2", SKU: "Standard_D4s_v3", Quantity: 5, Region: "westeurope"}, + }} + + h := &Handler{ + auth: scopedAzureAuth(t, "view", "purchases", []string{"acct-mine"}), + config: store, + azureExchangeFactory: func(_ string) azureExchangeClient { return stub }, + } + + res, err := h.listExchangeableAzureRIs(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer tok"}, + QueryStringParameters: map[string]string{"subscription_id": "sub-1"}, + }) + require.NoError(t, err) + resp, ok := res.(*ExchangeableAzureRIsResponse) + require.True(t, ok) + require.Len(t, resp.Reservations, 1, "only the requested subscription's own reservation must be returned") + assert.Equal(t, "res-own", resp.Reservations[0].ReservationID) + for _, r := range resp.Reservations { + assert.NotEqual(t, "res-other", r.ReservationID, "a reservation billed to a different subscription must never appear") + } +} + // --- Azure credential-resolution path tests (issue #871) --- // // These tests exercise the production path of buildAzureExchangeClient, which