From c4faec4b1e2df58f3beabaaef7e824c7619b9728 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 08:49:20 +0200 Subject: [PATCH 1/2] fix(credentials): require host identity for role_arn accounts without an ARN An AWS account with aws_auth_mode=role_arn and an empty aws_role_arn is treated as the CUDly host account and served with the host's own ambient credentials. Nothing checked that the account really was the host, so an operator who picked role_arn on a tenant account and left the ARN out got collection (and purchase execution) against the host account's resources, filed under the tenant account's UUID. Add credentials.VerifyHostAccount, which compares the account's external_id with the host's sts:GetCallerIdentity account and returns the typed ErrNotHostAccount on a mismatch, an STS failure, a missing STS client or an empty identity. The scheduler's ambient branch and resolveRoleARNProvider (used by purchase execution and the commitment-options probe) call it before handing out ambient credentials. Refs #402 --- internal/credentials/host_identity.go | 38 +++++++++++++ internal/credentials/resolver.go | 34 ++++++------ internal/credentials/resolver_test.go | 53 +++++++++++++++---- internal/purchase/coverage_extra_test.go | 26 +++++++++ internal/purchase/execution.go | 2 +- ...mendation_completeness_integration_test.go | 2 +- internal/scheduler/scheduler.go | 6 ++- internal/scheduler/scheduler_test.go | 30 +++++++++++ internal/server/app.go | 2 +- 9 files changed, 164 insertions(+), 29 deletions(-) create mode 100644 internal/credentials/host_identity.go diff --git a/internal/credentials/host_identity.go b/internal/credentials/host_identity.go new file mode 100644 index 00000000..532106ee --- /dev/null +++ b/internal/credentials/host_identity.go @@ -0,0 +1,38 @@ +package credentials + +import ( + "context" + "errors" + "fmt" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-sdk-go-v2/service/sts" +) + +// ErrNotHostAccount means a role_arn account with no role ARN could not be +// confirmed as the AWS account CUDly's own credentials belong to. +var ErrNotHostAccount = errors.New("credentials: aws_role_arn is empty but the account is not confirmed as the CUDly host account") + +// CallerIdentityClient is satisfied by *sts.Client. +type CallerIdentityClient interface { + GetCallerIdentity(ctx context.Context, params *sts.GetCallerIdentityInput, optFns ...func(*sts.Options)) (*sts.GetCallerIdentityOutput, error) +} + +// VerifyHostAccount confirms account.ExternalID is the AWS account that hostSTS, +// an STS client signed with the host's ambient credentials, runs as. +func VerifyHostAccount(ctx context.Context, account *config.CloudAccount, hostSTS CallerIdentityClient) error { + if hostSTS == nil { + return fmt.Errorf("%w: no STS client to resolve the host identity (account %s)", ErrNotHostAccount, account.ID) + } + out, err := hostSTS.GetCallerIdentity(ctx, &sts.GetCallerIdentityInput{}) + if err != nil { + return fmt.Errorf("%w: resolve host identity for account %s: %w", ErrNotHostAccount, account.ID, err) + } + if out == nil || out.Account == nil || *out.Account == "" { + return fmt.Errorf("%w: STS returned no host account ID (account %s)", ErrNotHostAccount, account.ID) + } + if account.ExternalID != *out.Account { + return fmt.Errorf("%w: account %s has external_id %q, host is %s; set aws_role_arn", ErrNotHostAccount, account.ID, account.ExternalID, *out.Account) + } + return nil +} diff --git a/internal/credentials/resolver.go b/internal/credentials/resolver.go index 8482d593..ad7832d1 100644 --- a/internal/credentials/resolver.go +++ b/internal/credentials/resolver.go @@ -79,14 +79,17 @@ type STSClientFactory func(provider aws.CredentialsProvider) STSClient // // AmbientProvider, when set, is returned for role_arn accounts whose // AWSRoleARN is empty (the "Self" account shape: auth_mode=role_arn with no -// role ARN). This mirrors the scheduler's collectAWSForAccount logic, which -// treats the same shape as an ambient-credentials case. +// role ARN), once AmbientSTS confirms the account is the host. This mirrors +// the scheduler's collectAWSForAccount logic. type AWSResolveOptions struct { AccountLookup AccountLookupFunc STSClientFactory STSClientFactory // AmbientProvider is the host Lambda / EC2 instance credentials provider. // Required when resolving a Self account (role_arn with empty AWSRoleARN). AmbientProvider aws.CredentialsProvider + // AmbientSTS is an STS client signed with AmbientProvider, used to check + // a Self account's external_id against the host identity. + AmbientSTS CallerIdentityClient } // ResolveAWSCredentialProvider is a back-compat wrapper that calls @@ -121,7 +124,7 @@ func ResolveAWSCredentialProviderWithOpts( case "access_keys": return resolveAccessKeyProvider(ctx, account, store) case "role_arn": - return resolveRoleARNProvider(ctx, account, stsClient, opts.AmbientProvider) + return resolveRoleARNProvider(ctx, account, stsClient, opts.AmbientProvider, opts.AmbientSTS) case "bastion": return resolveBastionProvider(ctx, account, store, stsClient, opts) case "workload_identity_federation": @@ -163,23 +166,24 @@ func resolveAccessKeyProvider(ctx context.Context, account *config.CloudAccount, // 1-hour STS token expiry problem of static credentials. // // When AWSRoleARN is empty (the "Self" account shape: auth_mode=role_arn with -// no role ARN), the ambient provider is returned directly so collection and -// execution agree on what this shape means. If ambient is nil in that case, -// a descriptive error is returned. +// no role ARN), the ambient provider is returned so collection and execution +// agree on what this shape means, but only once ambientSTS confirms the +// account is the host (ErrNotHostAccount otherwise). func resolveRoleARNProvider( - _ context.Context, + ctx context.Context, account *config.CloudAccount, stsClient STSClient, ambient aws.CredentialsProvider, + ambientSTS CallerIdentityClient, ) (aws.CredentialsProvider, error) { if account.AWSRoleARN == "" { - // Self-account: auth_mode=role_arn with no role ARN means "use the - // CUDly Lambda's own credentials to access this account." The - // scheduler's collectAWSForAccount handles the same shape identically. - if ambient != nil { - return ambient, nil + if ambient == nil { + return nil, fmt.Errorf("credentials: aws_role_arn is empty and no ambient credentials available (account %s)", account.ID) + } + if err := VerifyHostAccount(ctx, account, ambientSTS); err != nil { + return nil, err } - return nil, fmt.Errorf("credentials: aws_role_arn is empty and no ambient credentials available (account %s)", account.ID) + return ambient, nil } sessionSuffix := account.ID @@ -221,7 +225,7 @@ func resolveBastionProvider( } if opts.AccountLookup == nil || opts.STSClientFactory == nil { // Legacy fallback: trust caller's stsClient. Tracked in known_issues/03. - return resolveRoleARNProvider(ctx, account, stsClient, nil) + return resolveRoleARNProvider(ctx, account, stsClient, nil, nil) } bastion, err := opts.AccountLookup(ctx, account.AWSBastionID) if err != nil { @@ -244,7 +248,7 @@ func resolveBastionProvider( return nil, fmt.Errorf("credentials: resolve bastion %s creds: %w", bastion.ID, err) } bastionSTS := opts.STSClientFactory(bastionCreds) - return resolveRoleARNProvider(ctx, account, bastionSTS, nil) + return resolveRoleARNProvider(ctx, account, bastionSTS, nil, nil) } // resolveWebIdentityProvider returns a credential provider that exchanges an OIDC token diff --git a/internal/credentials/resolver_test.go b/internal/credentials/resolver_test.go index ab78bc98..a9c98c87 100644 --- a/internal/credentials/resolver_test.go +++ b/internal/credentials/resolver_test.go @@ -165,28 +165,22 @@ func TestResolveAWSCredentialProvider_RoleARN_NoARN_NilAmbient(t *testing.T) { // TestResolveAWSCredentialProvider_SelfAccount_WithAmbient verifies that a // Self-account (auth_mode=role_arn, empty AWSRoleARN) returns the ambient -// credentials provider when one is supplied via AWSResolveOptions. +// credentials provider when one is supplied via AWSResolveOptions and the +// host identity matches the account's external_id. func TestResolveAWSCredentialProvider_SelfAccount_WithAmbient(t *testing.T) { account := &config.CloudAccount{ ID: "self-acct", + ExternalID: "111111111111", AWSAuthMode: "role_arn", AWSRoleARN: "", // Self-account shape } - ambientCreds := aws.CredentialsProviderFunc(func(_ context.Context) (aws.Credentials, error) { - return aws.Credentials{ - AccessKeyID: "AMBIENTKEY", - SecretAccessKey: "ambientsecret", - Source: "test-ambient", - }, nil - }) - provider, err := ResolveAWSCredentialProviderWithOpts( context.Background(), account, newMockStore(), &mockSTSClient{}, - AWSResolveOptions{AmbientProvider: ambientCreds}, + AWSResolveOptions{AmbientProvider: testAmbientCreds, AmbientSTS: &callerIdentitySTS{account: "111111111111"}}, ) require.NoError(t, err) assert.NotNil(t, provider) @@ -197,6 +191,45 @@ func TestResolveAWSCredentialProvider_SelfAccount_WithAmbient(t *testing.T) { assert.Equal(t, "test-ambient", creds.Source) } +var testAmbientCreds = aws.CredentialsProviderFunc(func(_ context.Context) (aws.Credentials, error) { + return aws.Credentials{AccessKeyID: "AMBIENTKEY", SecretAccessKey: "ambientsecret", Source: "test-ambient"}, nil +}) + +type callerIdentitySTS struct { + account string + err error +} + +func (c *callerIdentitySTS) GetCallerIdentity(_ context.Context, _ *sts.GetCallerIdentityInput, _ ...func(*sts.Options)) (*sts.GetCallerIdentityOutput, error) { + if c.err != nil { + return nil, c.err + } + return &sts.GetCallerIdentityOutput{Account: aws.String(c.account)}, nil +} + +// Issue #402: a role_arn account with no role ARN gets the host's ambient +// credentials only when it is the host account. +func TestResolveAWSCredentialProvider_SelfAccount_NonHostFailsClosed(t *testing.T) { + for name, tc := range map[string]struct { + externalID string + hostSTS CallerIdentityClient + }{ + "different host account": {"222222222222", &callerIdentitySTS{account: "111111111111"}}, + "empty external_id": {"", &callerIdentitySTS{account: "111111111111"}}, + "empty host account": {"", &callerIdentitySTS{account: ""}}, + "host identity unknown": {"222222222222", &callerIdentitySTS{err: errors.New("sts down")}}, + "no host STS client": {"222222222222", nil}, + } { + t.Run(name, func(t *testing.T) { + account := &config.CloudAccount{ID: "tenant-acct", ExternalID: tc.externalID, AWSAuthMode: "role_arn"} + provider, err := ResolveAWSCredentialProviderWithOpts(context.Background(), account, newMockStore(), &mockSTSClient{}, + AWSResolveOptions{AmbientProvider: testAmbientCreds, AmbientSTS: tc.hostSTS}) + require.ErrorIs(t, err, ErrNotHostAccount) + assert.Nil(t, provider) + }) + } +} + // TestResolveAWSCredentialProvider_SelfAccount_NilAmbient verifies that a // Self-account with a nil AmbientProvider in AWSResolveOptions returns the // descriptive error instead of a panic or a misleading "aws_role_arn required" message. diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index 518de37c..4dcb88cd 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -8,6 +8,9 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/sts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -132,6 +135,29 @@ func TestResolveAWSProvider_NoSTS(t *testing.T) { assert.Nil(t, result) } +// Issue #402: purchasing for a role_arn account with no role ARN must not use +// the host's ambient credentials unless the account is the host. +func TestResolveAWSProvider_RoleARNWithoutARNOnNonHostFailsClosed(t *testing.T) { + ctx := context.Background() + hostSTS := new(MockSTSClient) + hostSTS.On("GetCallerIdentity", ctx, mock.Anything).Return(&sts.GetCallerIdentityOutput{Account: aws.String("111111111111")}, nil) + m := &Manager{ + assumeRoleSTS: new(MockAssumeRoleSTS), + stsClient: hostSTS, + ambientAWSCreds: aws.AnonymousCredentials{}, + } + account := config.CloudAccount{ID: "tenant", Provider: "aws", ExternalID: "222222222222", AWSAuthMode: "role_arn"} + + result, err := m.resolveAWSProvider(ctx, account) + require.ErrorIs(t, err, credentials.ErrNotHostAccount) + assert.Nil(t, result) + + account.ExternalID = "111111111111" + result, err = m.resolveAWSProvider(ctx, account) + require.NoError(t, err) + assert.Equal(t, aws.AnonymousCredentials{}, result.AWSCredentialsProvider) +} + // Tests for resolveAzureProvider without credStore and not managed_identity — returns error. func TestResolveAzureProvider_NoCredStoreNoManagedIdentity(t *testing.T) { m := &Manager{ diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index cca8433d..5ecbe816 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -458,7 +458,7 @@ func (m *Manager) resolveAWSProvider(ctx context.Context, account config.CloudAc return nil, fmt.Errorf("credentials: STS client not configured for non-access_keys mode (account %s)", account.ID) } awsCreds, err := credentials.ResolveAWSCredentialProviderWithOpts(ctx, &account, m.credStore, m.assumeRoleSTS, - credentials.AWSResolveOptions{AmbientProvider: m.ambientAWSCreds}) + credentials.AWSResolveOptions{AmbientProvider: m.ambientAWSCreds, AmbientSTS: m.stsClient}) if err != nil { logging.Errorf("purchase[resolveAWSProvider]: failed for account=%s after %s: %v", account.ID, time.Since(t0), err) diff --git a/internal/scheduler/recommendation_completeness_integration_test.go b/internal/scheduler/recommendation_completeness_integration_test.go index fa8814fe..126429d2 100644 --- a/internal/scheduler/recommendation_completeness_integration_test.go +++ b/internal/scheduler/recommendation_completeness_integration_test.go @@ -261,7 +261,7 @@ func TestAWSRecommendationCompletenessPersistence(t *testing.T) { Service: "savings-plans-compute", Savings: 40, Count: 1, Term: 1, Payment: "no-upfront"}) } require.NoError(t, store.UpsertRecommendations(ctx, time.Now().Add(-time.Hour), seed, nil)) - s := &Scheduler{config: store, providerFactory: factory} + s := &Scheduler{config: store, providerFactory: factory, stsClient: &fakeSTSClient{accountID: "111111111111"}} var globalCfg *config.GlobalConfig if tc.fallback != nil { globalCfg = &config.GlobalConfig{DefaultTerm: 1, DefaultPayment: "no-upfront", RecommendationsLookbackDays: 30} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index e8db1f2b..fcae8b57 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -765,8 +765,12 @@ func (s *Scheduler) collectAWSForAccount(ctx context.Context, globalCfg *config. // through to ResolveAWSCredentialProvider below so its switch fails // loud ("unsupported aws_auth_mode") instead of silently collecting the // HOST account's data and tagging it with this account's UUID - // (issue #107). + // (issue #107). A role_arn account that is not the host fails the + // host-identity check instead (issue #402). if acct.AWSAuthMode == "role_arn" && acct.AWSRoleARN == "" { + if err := credentials.VerifyHostAccount(ctx, &acct, s.stsClient); err != nil { + return nil, false, err + } prov, err := s.providerFactory.CreateAndValidateProvider(ctx, "aws", nil) if err != nil { return nil, false, fmt.Errorf("create ambient provider: %w", err) diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index a0e29933..e14d2168 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -14,6 +14,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" "github.com/LeanerCloud/cloud-commitments-platform/internal/email" "github.com/LeanerCloud/cloud-commitments-platform/internal/mocks" "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" @@ -1978,6 +1979,7 @@ func TestScheduler_CollectAWSForAccount_SelfAccountRoleARNModeUsesAmbient(t *tes scheduler := &Scheduler{ config: new(MockConfigStore), providerFactory: mockFactory, + stsClient: &fakeSTSClient{accountID: "111111111111"}, } recs, complete, err := scheduler.collectAWSForAccount(ctx, globalCfg, config.CloudAccount{ @@ -1995,6 +1997,34 @@ func TestScheduler_CollectAWSForAccount_SelfAccountRoleARNModeUsesAmbient(t *tes assert.Equal(t, "aws-self-account", *recs[0].CloudAccountID) } +// Issue #402: a role_arn account with no role ARN that is NOT the host must +// never reach the ambient (host) credentials provider. +func TestScheduler_CollectAWSForAccount_RoleARNWithoutARNOnNonHostFailsClosed(t *testing.T) { + for name, stsClient := range map[string]STSClient{ + "different host account": &fakeSTSClient{accountID: "999999999999"}, + "host identity unknown": &fakeSTSClient{err: errors.New("sts down")}, + "no STS client": nil, + } { + t.Run(name, func(t *testing.T) { + mockFactory := new(MockProviderFactory) + scheduler := &Scheduler{config: new(MockConfigStore), providerFactory: mockFactory, stsClient: stsClient} + + recs, complete, err := scheduler.collectAWSForAccount(context.Background(), &config.GlobalConfig{}, config.CloudAccount{ + ID: "tenant-account", + Provider: "aws", + AWSAuthMode: "role_arn", + ExternalID: "222222222222", + Enabled: true, + }) + + require.ErrorIs(t, err, credentials.ErrNotHostAccount) + assert.False(t, complete) + assert.Nil(t, recs) + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) + }) + } +} + // Test GCP recommendations with no accounts — should skip gracefully. func TestScheduler_CollectGCPRecommendations_NoAccounts(t *testing.T) { ctx := context.Background() diff --git a/internal/server/app.go b/internal/server/app.go index 0da40d4c..a784d046 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -845,7 +845,7 @@ func (app *Application) reinitializeAfterConnect(ctx context.Context, dbConn *da func(ctx context.Context, acct *config.CloudAccount) (aws.Config, error) { stsClient := sts.NewFromConfig(awsCfg) prov, err := credentials.ResolveAWSCredentialProviderWithOpts(ctx, acct, credStore, stsClient, - credentials.AWSResolveOptions{AmbientProvider: awsCfg.Credentials}) + credentials.AWSResolveOptions{AmbientProvider: awsCfg.Credentials, AmbientSTS: stsClient}) if err != nil { return aws.Config{}, err } From b07241c4b45c5eea363a633bf8267cf044cef6b7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 08:49:32 +0200 Subject: [PATCH 2/2] docs(api): document base64 encoding of setup-admin and reset passwords The setup-admin and password-reset-confirm handlers base64-decode the password like login does, but the OpenAPI schemas did not say so. Add the same "Base64-encoded" description login carries, plus a spec test that checks every such field. Refs #402 --- internal/api/openapi.yaml | 2 ++ .../api/openapi_password_encoding_test.go | 36 +++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 internal/api/openapi_password_encoding_test.go diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 9d97c87c..ebc9b156 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2084,6 +2084,7 @@ components: format: email password: type: string + description: Base64-encoded password PasswordResetRequest: type: object @@ -2101,6 +2102,7 @@ components: type: string new_password: type: string + description: Base64-encoded new password ProfileUpdateRequest: type: object diff --git a/internal/api/openapi_password_encoding_test.go b/internal/api/openapi_password_encoding_test.go new file mode 100644 index 00000000..99372516 --- /dev/null +++ b/internal/api/openapi_password_encoding_test.go @@ -0,0 +1,36 @@ +package api + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +// Issue #402: every request password the handlers run through +// decodeBase64Password must be documented as base64-encoded in the spec. +func TestOpenAPIPasswordFieldsDocumentBase64(t *testing.T) { + var spec struct { + Components struct { + Schemas map[string]struct { + Properties map[string]struct { + Description string `yaml:"description"` + } `yaml:"properties"` + } `yaml:"schemas"` + } `yaml:"components"` + } + require.NoError(t, yaml.Unmarshal(openapiSpec, &spec)) + + for schema, field := range map[string]string{ + "LoginRequest": "password", + "SetupAdminRequest": "password", + "PasswordResetConfirm": "new_password", + } { + props, ok := spec.Components.Schemas[schema] + require.True(t, ok, "schema %s missing", schema) + prop, ok := props.Properties[field] + require.True(t, ok, "%s.%s missing", schema, field) + assert.Contains(t, prop.Description, "Base64-encoded", "%s.%s", schema, field) + } +}