From 1e7c394aee889823dca043638e4c84e102b00209 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 22:34:25 +0100 Subject: [PATCH] fix(api): sign no-cloud-account marketplace rows as the host only for the host account Marketplace list and cancel already resolve a row's cloud account through credentials.ResolveAWSCredentialProviderWithOpts (#554), but a row with no cloud_account_id still fell back to the host's ambient credentials whatever AWS account the RI belongs to. Such rows come from the ambient single-account purchase path, from before migration 000011 and from deleted accounts (ON DELETE SET NULL). Use the host credentials for those rows only when the row's AWS account id passes credentials.VerifyHostAccount, the same check the resolver applies to a role_arn account with no role ARN. An empty account id, a different account or a failed host identity lookup is refused with a 400 before any EC2 client is built. Authorization is unchanged. Closes #261 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_marketplace.go | 32 +++- .../handler_marketplace_credentials_test.go | 154 ++++++++++++++++-- ...onstraints_marketplace_integration_test.go | 4 +- 3 files changed, 172 insertions(+), 18 deletions(-) diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index ed2b3f644..c7e1d64fd 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -61,14 +61,16 @@ func (h *Handler) buildMarketplaceEC2Client(cfg aws.Config) marketplaceEC2Client // credential resolver the purchase path uses. A role_arn account without a role // ARN is accepted only when it is the host account (credentials.ErrNotHostAccount // otherwise). Rows without a cloud account (reachable only by sell-any callers -// with unrestricted scope) keep the host's own credentials. +// with unrestricted scope) get the host's own credentials only when the row's +// AWS account id is confirmed as the host account; see +// loadMarketplaceHostAWSConfig. func (h *Handler) loadMarketplaceAWSConfig(ctx context.Context, row *config.PurchaseHistoryRecord) (aws.Config, error) { base, err := h.loadAWSConfigWithRegion(ctx, row.Region) if err != nil { return aws.Config{}, fmt.Errorf("failed to load AWS config: %w", err) } if row.CloudAccountID == nil || strings.TrimSpace(*row.CloudAccountID) == "" { - return base, nil + return loadMarketplaceHostAWSConfig(ctx, row, base) } account, err := h.config.GetCloudAccount(ctx, *row.CloudAccountID) if err != nil { @@ -94,6 +96,32 @@ func (h *Handler) loadMarketplaceAWSConfig(ctx context.Context, row *config.Purc return cfg, nil } +// marketplaceNoCloudAccountMessage is the tenant-visible refusal for a row with +// no cloud account whose AWS account is not the host account. Like +// credentials.ErrNotHostAccount it never carries the host account id. +const marketplaceNoCloudAccountMessage = "this purchase is not linked to a cloud account and its AWS account is not confirmed as the CUDly host account; it cannot be listed or canceled from here" + +// loadMarketplaceHostAWSConfig handles a row with no cloud account: the +// ambient single-account deployment (purchase.SingleCloudAccountIDFromRecs +// returns nil) and legacy or account-deleted rows (cloud_account_id has no +// backfill and is ON DELETE SET NULL). The host's credentials can only act on +// the host account, so they are used only when the row's AWS account id +// passes the same host identity check the resolver applies to a role_arn +// account with no role ARN. Anything else, including an empty account id or a +// failed STS lookup, is refused before any EC2 client is built. +func loadMarketplaceHostAWSConfig(ctx context.Context, row *config.PurchaseHistoryRecord, base aws.Config) (aws.Config, error) { + accountID := strings.TrimSpace(row.AccountID) + if accountID == "" { + logging.Warnf("marketplace: purchase %s has neither a cloud account nor an AWS account id; refusing to sign with host credentials", row.PurchaseID) + return aws.Config{}, NewClientError(400, marketplaceNoCloudAccountMessage) + } + hostCheck := &config.CloudAccount{ID: "purchase " + row.PurchaseID, Provider: "aws", ExternalID: accountID} + if err := credentials.VerifyHostAccount(ctx, hostCheck, sts.NewFromConfig(base)); err != nil { + return aws.Config{}, NewClientError(400, marketplaceNoCloudAccountMessage) + } + return base, nil +} + // MarketplacePriceTier is the JSON-decodable shape accepted in the request body. type MarketplacePriceTier struct { // TermMonths is the remaining-months count this tier covers. diff --git a/internal/api/handler_marketplace_credentials_test.go b/internal/api/handler_marketplace_credentials_test.go index a5a70db43..05f429297 100644 --- a/internal/api/handler_marketplace_credentials_test.go +++ b/internal/api/handler_marketplace_credentials_test.go @@ -10,6 +10,7 @@ import ( awsprovider "github.com/LeanerCloud/cloud-commitments-go/providers/aws" ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" "github.com/aws/aws-sdk-go-v2/aws" @@ -313,26 +314,149 @@ func TestLoadMarketplaceAWSConfig_FailsClosed(t *testing.T) { }) } -// Rows without a cloud account keep the host's ambient credentials (reachable -// only by sell-any callers); this documents the current behavior. -func TestLoadMarketplaceAWSConfig_NoCloudAccountKeepsHostCredentials(t *testing.T) { - empty := "" - for name, id := range map[string]*string{"nil": nil, "empty": &empty} { +// countingSTS records how many STS calls were made and answers them with the +// wrapped transport. +type countingSTS struct { + calls *int + next aws.HTTPClient +} + +func (c countingSTS) Do(r *http.Request) (*http.Response, error) { + *c.calls++ + return c.next.Do(r) +} + +// failingSTS fails every STS call at the transport. +type failingSTS struct{} + +func (failingSTS) Do(*http.Request) (*http.Response, error) { + return nil, errors.New("sts unreachable") +} + +// noCloudAccountRow is a purchase row with no cloud account (ambient +// single-account deployment, legacy or account-deleted row) owned by the given +// AWS account. +func noCloudAccountRow(cloudAccountID *string, awsAccountID string) *config.PurchaseHistoryRecord { + row := standardRow() + row.CloudAccountID = cloudAccountID + row.AccountID = awsAccountID + return row +} + +func hostOnlyHandler(transport aws.HTTPClient) *Handler { + // No GetCloudAccount expectation: a row without a cloud account must not + // look one up (the mock panics on an unexpected call). + h := &Handler{config: &MockConfigStore{}} + h.awsCfgOnce.Do(func() { + h.awsCfg = aws.Config{ + Region: "us-east-1", + Credentials: awscreds.NewStaticCredentialsProvider(marketplaceHostKeyID, "host-secret", ""), + HTTPClient: transport, + } + }) + return h +} + +// A row with no cloud account gets the host's credentials only when its AWS +// account id is the host account (issue #261). +func TestLoadMarketplaceAWSConfig_NoCloudAccountHostRowUsesHostCredentials(t *testing.T) { + empty, blank := "", " " + for name, id := range map[string]*string{"nil": nil, "empty": &empty, "blank": &blank} { t.Run(name, func(t *testing.T) { - h := &Handler{config: &MockConfigStore{}} - h.awsCfgOnce.Do(func() { - h.awsCfg = aws.Config{ - Region: "us-east-1", - Credentials: awscreds.NewStaticCredentialsProvider(marketplaceHostKeyID, "host-secret", ""), - } - }) - row := standardRow() - row.CloudAccountID = id + calls := 0 + h := hostOnlyHandler(countingSTS{&calls, hostSTSTransport{}}) - cfg, err := h.loadMarketplaceAWSConfig(context.Background(), row) + cfg, err := h.loadMarketplaceAWSConfig(context.Background(), noCloudAccountRow(id, marketplaceHostAccount)) require.NoError(t, err) assert.Equal(t, marketplaceHostKeyID, signingKeyID(t, cfg)) + assert.Equal(t, 1, calls, "the host identity must be checked before the host's credentials are used") + }) + } +} + +func TestLoadMarketplaceAWSConfig_NoCloudAccountRefusedUnlessHost(t *testing.T) { + cases := map[string]struct { + owner string + transport aws.HTTPClient + wantSTS bool + }{ + "member account": {owner: "222222222222", transport: hostSTSTransport{}, wantSTS: true}, + "empty account id": {owner: "", transport: hostSTSTransport{}, wantSTS: false}, + "blank account id": {owner: " ", transport: hostSTSTransport{}, wantSTS: false}, + "host identity fail": {owner: marketplaceHostAccount, transport: failingSTS{}, wantSTS: true}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + calls := 0 + h := hostOnlyHandler(countingSTS{&calls, tc.transport}) + h.awsCfg.Retryer = noRetry + + cfg, err := h.loadMarketplaceAWSConfig(context.Background(), noCloudAccountRow(nil, tc.owner)) + + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error, got %v", err) + assert.Equal(t, 400, ce.code) + assert.Equal(t, marketplaceNoCloudAccountMessage, ce.message) + assert.NotContains(t, ce.message, marketplaceHostAccount) + assert.Nil(t, cfg.Credentials, "a refused row must not get the host's credentials") + assert.Equal(t, tc.wantSTS, calls > 0) + }) + } +} + +// End to end through both handlers: a no-cloud-account row owned by a member +// account never reaches EC2, and one owned by the host account is signed with +// the host's credentials. +func TestMarketplaceHandlers_NoCloudAccountRow(t *testing.T) { + run := map[string]func(*Handler) (any, error){ + "list": func(h *Handler) (any, error) { + return h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + }, + "cancel": func(h *Handler) (any, error) { + return h.marketplaceCancel(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + }, + } + // A row with no cloud account is reachable only by an unrestricted + // sell-any caller; authorization itself is unchanged. + unrestricted := func(h *Handler) { + h.auth.(*MockAuthService).On("HasPermissionForConstraintsAPI", mock.Anything, "admin", auth.ActionSellAny, auth.ResourcePurchases, + []auth.PermissionConstraints{{StrictScope: true, Regions: []string{"us-east-1"}}}).Return(true, nil) + } + for name, call := range run { + row := func(accountID string) *config.PurchaseHistoryRecord { + r := noCloudAccountRow(nil, accountID) + if name == "cancel" { + r.ListingState = config.ListingStateActive + r.ListingID = "ril-cancel" + } + return r + } + t.Run(name+"/member account refused", func(t *testing.T) { + h, built := marketplaceAccountHandler(t, nil, row("222222222222")) + unrestricted(h) + built.Region = "unset" + + _, err := call(h) + + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error, got %v", err) + assert.Equal(t, 400, ce.code) + assert.Equal(t, marketplaceNoCloudAccountMessage, ce.message) + assert.Equal(t, "unset", built.Region, "no EC2 client may be built for a refused row") + store := h.config.(*MockConfigStore) + store.AssertNotCalled(t, "GetCloudAccount", mock.Anything, mock.Anything) + }) + t.Run(name+"/host account uses host credentials", func(t *testing.T) { + h, built := marketplaceAccountHandler(t, nil, row(marketplaceHostAccount)) + unrestricted(h) + + _, err := call(h) + + require.NoError(t, err) + assert.Equal(t, marketplaceHostKeyID, signingKeyID(t, *built)) + store := h.config.(*MockConfigStore) + store.AssertNotCalled(t, "GetCloudAccount", mock.Anything, mock.Anything) }) } } diff --git a/internal/api/permission_constraints_marketplace_integration_test.go b/internal/api/permission_constraints_marketplace_integration_test.go index c8e3945e4..fd071468a 100644 --- a/internal/api/permission_constraints_marketplace_integration_test.go +++ b/internal/api/permission_constraints_marketplace_integration_test.go @@ -193,7 +193,9 @@ func TestMarketplacePermissionConstraintsHTTP(t *testing.T) { row.PurchaseID, row.CloudAccountID, row.Provider, row.Service = uuid.NewString(), &account, "aws", "ec2" switch tc.missing { case "account": - row.CloudAccountID = nil + // A row with no cloud account is signed with the host's + // credentials only when it belongs to the host account (#261). + row.CloudAccountID, row.AccountID = nil, marketplaceHostAccount case "provider": row.Provider = " " case "service":