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) + } +} 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 }