diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index ed2b3f64..c7e1d64f 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 a5a70db4..05f42929 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 c8e3945e..fd071468 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":