From 9cf2035a595eef0c6bfad9a143c39ddf0ee81913 Mon Sep 17 00:00:00 2001 From: Charles Bryan Date: Wed, 12 Aug 2026 10:37:31 -0700 Subject: [PATCH] Resolve organization account role ARNs in the org's partition --- lib/srv/server/ec2_watcher.go | 24 +-- lib/srv/server/ec2_watcher_test.go | 75 +++++++-- lib/utils/aws/organizations/arn.go | 29 ++-- lib/utils/aws/organizations/arn_test.go | 46 ++++-- .../aws/organizations/matchingaccounts.go | 152 +++++++++--------- .../organizations/matchingaccounts_test.go | 90 +++++++++-- 6 files changed, 261 insertions(+), 155 deletions(-) diff --git a/lib/srv/server/ec2_watcher.go b/lib/srv/server/ec2_watcher.go index b9070765e1b35..d26336b3742e5 100644 --- a/lib/srv/server/ec2_watcher.go +++ b/lib/srv/server/ec2_watcher.go @@ -25,7 +25,6 @@ import ( "sync" "github.com/aws/aws-sdk-go-v2/aws" - "github.com/aws/aws-sdk-go-v2/aws/arn" "github.com/aws/aws-sdk-go-v2/service/ec2" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" "github.com/aws/aws-sdk-go-v2/service/sts" @@ -38,6 +37,7 @@ import ( awsregions "github.com/gravitational/teleport/lib/cloud/aws/regions" "github.com/gravitational/teleport/lib/cloud/awsconfig" "github.com/gravitational/teleport/lib/labels" + awsutils "github.com/gravitational/teleport/lib/utils/aws" "github.com/gravitational/teleport/lib/utils/aws/organizations" ) @@ -601,7 +601,7 @@ func (f *ec2InstanceFetcher) matcherRegions(ctx context.Context, params matcherR return regions, nil } -func (f *ec2InstanceFetcher) fetchAccountIDsUnderOrganization(ctx context.Context) ([]string, error) { +func (f *ec2InstanceFetcher) fetchAccountsUnderOrganization(ctx context.Context) (*organizations.Accounts, error) { awsOpts := []awsconfig.OptionsFn{ awsconfig.WithCredentialsMaybeIntegration(awsconfig.IntegrationMetadata{Name: f.Matcher.Integration}), } @@ -625,7 +625,7 @@ func (f *ec2InstanceFetcher) fetchAccountIDsUnderOrganization(ctx context.Contex }) } - accountIDs, err := organizations.MatchingAccounts(ctx, f.Logger, orgsClient, organizations.MatchingAccountsFilter{ + accounts, err := organizations.MatchingAccounts(ctx, f.Logger, orgsClient, organizations.MatchingAccountsFilter{ IncludeOUs: includeOUs, ExcludeOUs: excludeOUs, OrganizationID: organizationID, @@ -638,7 +638,7 @@ func (f *ec2InstanceFetcher) fetchAccountIDsUnderOrganization(ctx context.Contex }) } - return accountIDs, nil + return accounts, nil } type assumeRoleWithExternalID struct { @@ -668,23 +668,15 @@ func (f *ec2InstanceFetcher) allAssumeRoles(ctx context.Context) ([]assumeRoleWi return nil, trace.BadParameter("assume role name is required when using AWS organization discovery") } - accountIDs, err := f.fetchAccountIDsUnderOrganization(ctx) + accounts, err := f.fetchAccountsUnderOrganization(ctx) if err != nil { return nil, trace.Wrap(err) } - var allAssumeRoles []assumeRoleWithExternalID - for _, accountID := range accountIDs { - assumeRoleARN := arn.ARN{ - Partition: "aws", - Service: "iam", - Region: "", - AccountID: accountID, - Resource: "role/" + f.Matcher.AssumeRole.RoleName, - } - + allAssumeRoles := make([]assumeRoleWithExternalID, 0, len(accounts.IDs)) + for _, accountID := range accounts.IDs { allAssumeRoles = append(allAssumeRoles, assumeRoleWithExternalID{ - RoleARN: assumeRoleARN.String(), + RoleARN: awsutils.RoleARN(accounts.Partition, accountID, f.Matcher.AssumeRole.RoleName), ExternalID: f.Matcher.AssumeRole.ExternalID, }) } diff --git a/lib/srv/server/ec2_watcher_test.go b/lib/srv/server/ec2_watcher_test.go index d995c4db5f56b..c37f702c3eff5 100644 --- a/lib/srv/server/ec2_watcher_test.go +++ b/lib/srv/server/ec2_watcher_test.go @@ -19,6 +19,7 @@ package server import ( + "cmp" "context" "errors" "fmt" @@ -34,7 +35,7 @@ import ( organizationtypes "github.com/aws/aws-sdk-go-v2/service/organizations/types" "github.com/aws/aws-sdk-go-v2/service/sts" ststypes "github.com/aws/aws-sdk-go-v2/service/sts/types" - "github.com/google/go-cmp/cmp" + gocmp "github.com/google/go-cmp/cmp" "github.com/gravitational/trace" "github.com/stretchr/testify/require" @@ -101,14 +102,15 @@ func (m *mockAWSSTSClient) GetCallerIdentity(ctx context.Context, params *sts.Ge type mockOrganizationsClient struct { organizationID string rootOUID string + rootARN string ouItems map[string]ouItem responseError error } type ouItem struct { - innerOUs []string - innerAccounts []string - innerNotActiveAccounts []string + innerOUs []string + innerAccounts []string + innerInactiveAccounts []string } func TestEC2WatcherResolveCallerIdentity(t *testing.T) { @@ -208,7 +210,7 @@ func (m *mockOrganizationsClient) ListRoots(ctx context.Context, input *organiza if m.responseError != nil { return nil, m.responseError } - rootARN := fmt.Sprintf("arn:aws:organizations::0000000000:root/%s/%s", m.organizationID, m.rootOUID) + rootARN := cmp.Or(m.rootARN, fmt.Sprintf("arn:aws:organizations::0000000000:root/%s/%s", m.organizationID, m.rootOUID)) return &organizations.ListRootsOutput{ Roots: []organizationtypes.Root{ { @@ -230,19 +232,15 @@ func (m *mockOrganizationsClient) ListAccountsForParent(ctx context.Context, inp var accounts []organizationtypes.Account for _, accountID := range ouItem.innerAccounts { - accountARN := fmt.Sprintf("arn:aws:organizations::0000000000:account/%s/%s", m.organizationID, accountID) accounts = append(accounts, organizationtypes.Account{ Id: aws.String(accountID), State: organizationtypes.AccountStateActive, - Arn: aws.String(accountARN), }) } - for _, accountID := range ouItem.innerNotActiveAccounts { - accountARN := fmt.Sprintf("arn:aws:organizations::0000000000:account/%s/%s", m.organizationID, accountID) + for _, accountID := range ouItem.innerInactiveAccounts { accounts = append(accounts, organizationtypes.Account{ Id: aws.String(accountID), State: organizationtypes.AccountStateSuspended, - Arn: aws.String(accountARN), }) } return &organizations.ListAccountsForParentOutput{ @@ -842,9 +840,9 @@ func TestEC2WatcherCallerIdentityFailureDoesNotBlockOrganizationDiscovery(t *tes Logger: logtest.NewLogger(), }) - accountIDs, err := fetcher.fetchAccountIDsUnderOrganization(t.Context()) + accounts, err := fetcher.fetchAccountsUnderOrganization(t.Context()) require.Error(t, err) - require.Empty(t, accountIDs) + require.Empty(t, accounts) permissionErrors := EC2IAMPermissionErrors(err) require.Len(t, permissionErrors, 1) @@ -853,6 +851,57 @@ func TestEC2WatcherCallerIdentityFailureDoesNotBlockOrganizationDiscovery(t *tes require.True(t, trace.IsAccessDenied(permissionErrors[0].Err)) } +func TestEC2WatcherAssumeRolesUseOrganizationPartition(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + rootARN string + expectedRoleARN string + }{ + { + name: "aws-us-gov", + rootARN: "arn:aws-us-gov:organizations::000000000000:root/o-1/r-1", + expectedRoleARN: "arn:aws-us-gov:iam::000000000001:role/MyRole", + }, + { + name: "aws-cn", + rootARN: "arn:aws-cn:organizations::000000000000:root/o-1/r-1", + expectedRoleARN: "arn:aws-cn:iam::000000000001:role/MyRole", + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fetcher := newEC2InstanceFetcher(ec2FetcherConfig{ + Matcher: types.AWSMatcher{ + AssumeRole: &types.AssumeRole{RoleName: "MyRole", ExternalID: "ext-1"}, + Organization: &types.AWSOrganizationMatcher{ + OrganizationID: "o-1", + OrganizationalUnits: &types.AWSOrganizationUnitsMatcher{Include: []string{types.Wildcard}}, + }, + }, + AWSOrganizationsGetter: func(ctx context.Context, opts ...awsconfig.OptionsFn) (liborganizations.OrganizationsClient, error) { + return &mockOrganizationsClient{ + organizationID: "o-1", + rootOUID: "r-1", + rootARN: tt.rootARN, + ouItems: map[string]ouItem{"r-1": {innerAccounts: []string{"000000000001"}}}, + }, nil + }, + Logger: logtest.NewLogger(), + }) + + assumeRoles, err := fetcher.allAssumeRoles(t.Context()) + require.NoError(t, err) + require.Equal(t, []assumeRoleWithExternalID{{ + RoleARN: tt.expectedRoleARN, + ExternalID: "ext-1", + }}, assumeRoles) + }) + } +} + func TestEC2WatcherOrganizationClientCreationPermissionErrorReturnsPermissionError(t *testing.T) { t.Parallel() @@ -1600,7 +1649,7 @@ func TestConvertEC2InstancesToServerInfos(t *testing.T) { require.NoError(t, err) require.Len(t, serverInfos, 1) - require.Empty(t, cmp.Diff(expected, serverInfos[0])) + require.Empty(t, gocmp.Diff(expected, serverInfos[0])) } func TestMakeEvents(t *testing.T) { diff --git a/lib/utils/aws/organizations/arn.go b/lib/utils/aws/organizations/arn.go index 2d4314080ce0d..e42e85c975e06 100644 --- a/lib/utils/aws/organizations/arn.go +++ b/lib/utils/aws/organizations/arn.go @@ -28,28 +28,23 @@ import ( // OrganizationIDFromAccountARN extracts the organization ID from an account ARN. // Example ARN: arn:aws:organizations:::account// func OrganizationIDFromAccountARN(accountARN string) (string, error) { - return organizationIDFromARN(accountARN, "account") -} - -// organizationIDFromRootOUARN extracts the organization ID from an root Organizational Unit ARN. -// Example ARN: arn:aws:organizations:::root// -func organizationIDFromRootOUARN(rootOUARN string) (string, error) { - return organizationIDFromARN(rootOUARN, "root") -} - -func organizationIDFromARN(orgARN string, resourceType string) (string, error) { - arnParsed, err := arn.Parse(orgARN) + parsed, err := arn.Parse(accountARN) if err != nil { return "", trace.Wrap(err) } - resourceSplitted := strings.Split(arnParsed.Resource, "/") - if len(resourceSplitted) != 3 { + return organizationIDFromARN(parsed, "account") +} + +// organizationIDFromARN extracts the organization ID from an AWS Organizations ARN +// whose resource is resourceType//. +func organizationIDFromARN(orgARN arn.ARN, resourceType string) (string, error) { + parts := strings.Split(orgARN.Resource, "/") + if len(parts) != 3 { return "", trace.BadParameter("unexpected resource received in ARN from organizations API call: %s", orgARN) } - if resourceSplitted[0] != resourceType { - return "", trace.BadParameter("expected resource type %s but received unexpected resource type %s in ARN from organizations API call: %s", resourceType, resourceSplitted[0], orgARN) + if parts[0] != resourceType { + return "", trace.BadParameter("expected resource type %s but received unexpected resource type %s in ARN from organizations API call: %s", resourceType, parts[0], orgARN) } - organizationID := resourceSplitted[1] - return organizationID, nil + return parts[1], nil } diff --git a/lib/utils/aws/organizations/arn_test.go b/lib/utils/aws/organizations/arn_test.go index ec6e98bdaef98..0103b51216dfd 100644 --- a/lib/utils/aws/organizations/arn_test.go +++ b/lib/utils/aws/organizations/arn_test.go @@ -21,6 +21,7 @@ package organizations import ( "testing" + "github.com/aws/aws-sdk-go-v2/aws/arn" "github.com/stretchr/testify/require" ) @@ -56,32 +57,45 @@ func TestOrganizationIDFromAccountARN(t *testing.T) { } } -func TestOrganizationIDFromRootOUARN(t *testing.T) { +func errContains(substr string) require.ErrorAssertionFunc { + return func(t require.TestingT, err error, msgAndArgs ...any) { + require.ErrorContains(t, err, substr, msgAndArgs...) + } +} + +func TestOrganizationIDFromARN(t *testing.T) { for _, tt := range []struct { - name string - accountARN string - expectedOrg string - errCheck require.ErrorAssertionFunc + name string + orgARN string + resourceType string + expectedOrg string + errCheck require.ErrorAssertionFunc }{ { - name: "valid account ARN", - accountARN: "arn:aws:organizations::123456789012:root/o-exampleorgid/111111111111", - expectedOrg: "o-exampleorgid", - errCheck: require.NoError, + name: "root ARN", + orgARN: "arn:aws:organizations::123456789012:root/o-exampleorgid/r-exampleroot", + resourceType: "root", + expectedOrg: "o-exampleorgid", + errCheck: require.NoError, }, { - name: "invalid ARN format", - accountARN: "invalid-arn-format", - errCheck: require.Error, + name: "too few resource parts", + orgARN: "arn:aws:organizations::123456789012:root/o-exampleorgid", + resourceType: "root", + errCheck: errContains("unexpected resource received"), }, { - name: "wrong resource type", - accountARN: "arn:aws:organizations::123456789012:account/o-exampleorgid/111111111111", - errCheck: require.Error, + name: "wrong resource type", + orgARN: "arn:aws:organizations::123456789012:account/o-exampleorgid/111111111111", + resourceType: "root", + errCheck: errContains("expected resource type root"), }, } { t.Run(tt.name, func(t *testing.T) { - gotOrg, err := organizationIDFromRootOUARN(tt.accountARN) + parsed, err := arn.Parse(tt.orgARN) + require.NoError(t, err) + + gotOrg, err := organizationIDFromARN(parsed, tt.resourceType) tt.errCheck(t, err) require.Equal(t, tt.expectedOrg, gotOrg) }) diff --git a/lib/utils/aws/organizations/matchingaccounts.go b/lib/utils/aws/organizations/matchingaccounts.go index f8c9ee4cb925e..e966d2b792fe7 100644 --- a/lib/utils/aws/organizations/matchingaccounts.go +++ b/lib/utils/aws/organizations/matchingaccounts.go @@ -19,15 +19,19 @@ package organizations import ( + "cmp" "context" "log/slog" "slices" "strings" "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/aws/arn" "github.com/aws/aws-sdk-go-v2/service/organizations" organizationstypes "github.com/aws/aws-sdk-go-v2/service/organizations/types" "github.com/gravitational/trace" + + awsutils "github.com/gravitational/teleport/api/utils/aws" ) const ( @@ -51,13 +55,6 @@ type OrganizationsClient interface { organizations.ListAccountsForParentAPIClient } -type awsOrgItem struct { - id string - organizationalUnits []*awsOrgItem - accounts []string - notActiveAccounts []string -} - // MatchingAccountsFilter defines the filter to apply when retrieving matching accounts from an AWS Organization. type MatchingAccountsFilter struct { // OrganizationID is the ID of the AWS Organization to query. @@ -93,73 +90,67 @@ func (m *MatchingAccountsFilter) checkAndSetDefaults() error { return nil } -// MatchingAccounts returns the list of account IDs that are part of the organization and match the filter. +// Accounts holds the accounts of an AWS Organization that match a filter. +type Accounts struct { + // Partition is the AWS partition of the organization, such as aws, aws-us-gov + // or aws-cn. + Partition string + + // IDs are the IDs of the active accounts that match the filter. + IDs []string +} + +// MatchingAccounts returns the accounts that are part of the organization and match the filter. // Every OU in ExcludeOUs is excluded from the results, including its children OUs. -func MatchingAccounts(ctx context.Context, log *slog.Logger, orgsClient OrganizationsClient, filter MatchingAccountsFilter) ([]string, error) { +func MatchingAccounts(ctx context.Context, log *slog.Logger, orgsClient OrganizationsClient, filter MatchingAccountsFilter) (*Accounts, error) { if err := filter.checkAndSetDefaults(); err != nil { return nil, trace.Wrap(err) } - orgTree, err := buildOrgTree(ctx, orgsClient, filter) + rootOUID, partition, err := organizationRoot(ctx, orgsClient, filter.OrganizationID) if err != nil { return nil, trace.Wrap(err) } - includedActiveAccounts, includedNotActiveAccounts := collectIncludedAccounts(orgTree, filter, nil) - logNotActiveAccountsAreIgnored(ctx, log, filter.OrganizationID, includedNotActiveAccounts) - - return includedActiveAccounts, nil -} - -func collectIncludedAccounts(orgItem *awsOrgItem, filter MatchingAccountsFilter, accountOUAncestors []string) (activeAccounts []string, notActiveAccounts []string) { - accountOrganizationalUnits := append(slices.Clone(accountOUAncestors), orgItem.id) - - if OrganizationalUnitsMatch(filter, accountOrganizationalUnits) { - activeAccounts = append(activeAccounts, orgItem.accounts...) - notActiveAccounts = append(notActiveAccounts, orgItem.notActiveAccounts...) - } - - for _, orgUnit := range orgItem.organizationalUnits { - childAccountIDs, childnotActiveAccounts := collectIncludedAccounts(orgUnit, filter, accountOrganizationalUnits) - - activeAccounts = append(activeAccounts, childAccountIDs...) - notActiveAccounts = append(notActiveAccounts, childnotActiveAccounts...) + activeAccounts, inactiveAccounts, err := includedAccounts(ctx, orgsClient, rootOUID, nil, filter) + if err != nil { + return nil, trace.Wrap(err) } + logInactiveAccountsAreIgnored(ctx, log, filter.OrganizationID, inactiveAccounts) - return activeAccounts, notActiveAccounts + return &Accounts{ + Partition: partition, + IDs: activeAccounts, + }, nil } -func logNotActiveAccountsAreIgnored(ctx context.Context, log *slog.Logger, organizationID string, notActiveAccountIDs []string) { - if len(notActiveAccountIDs) == 0 { +func logInactiveAccountsAreIgnored(ctx context.Context, log *slog.Logger, organizationID string, inactiveAccountIDs []string) { + if len(inactiveAccountIDs) == 0 { return } - // Log only the first 10 non-active accounts to avoid log flooding. - if len(notActiveAccountIDs) > 10 { - notActiveAccountIDs = notActiveAccountIDs[:10] + // Log only the first 10 inactive accounts to avoid log flooding. + loggedAccountIDs := inactiveAccountIDs + if len(loggedAccountIDs) > 10 { + loggedAccountIDs = loggedAccountIDs[:10] } - notActiveAccounts := strings.Join(notActiveAccountIDs, ", ") - log.DebugContext(ctx, "non-active accounts under organization were ignored", + log.DebugContext(ctx, "inactive accounts under organization were ignored", "organization_id", organizationID, - "total_not_active", len(notActiveAccountIDs), - "not_active_accounts", notActiveAccounts, + "total_inactive", len(inactiveAccountIDs), + "inactive_accounts", strings.Join(loggedAccountIDs, ", "), ) } -// Limits of AWS Organizations: -// https://docs.aws.amazon.com/organizations/latest/userguide/orgs_reference_limits.html -// Most relevant limits: -// Max OU depth is 5 levels. -// At most there will be 2000 OUs in an AWS Organization. -// At most there will be 10 accounts, but that's configurable. -func buildOrgTree(ctx context.Context, orgsClient OrganizationsClient, filter MatchingAccountsFilter) (*awsOrgItem, error) { +// organizationRoot returns the ID of the root organizational unit of +// organizationID and the AWS partition the organization lives in. +func organizationRoot(ctx context.Context, orgsClient OrganizationsClient, organizationID string) (rootOUID string, partition string, err error) { paginator := organizations.NewListRootsPaginator(orgsClient, &organizations.ListRootsInput{}) var roots []organizationstypes.Root for paginator.HasMorePages() { page, err := paginator.NextPage(ctx) if err != nil { - return nil, trace.Wrap(err) + return "", "", trace.Wrap(err) } roots = append(roots, page.Roots...) } @@ -167,58 +158,67 @@ func buildOrgTree(ctx context.Context, orgsClient OrganizationsClient, filter Ma // AWS Docs state that: // > You can have only one root. AWS Organizations automatically creates the root for you when you create an organization. if len(roots) != 1 { - return nil, trace.BadParameter("expected exactly one root organizational unit, got %d", len(roots)) + return "", "", trace.BadParameter("expected exactly one root organizational unit, got %d", len(roots)) } root := roots[0] - rootsOrganization, err := organizationIDFromRootOUARN(aws.ToString(root.Arn)) + rootARN, err := arn.Parse(aws.ToString(root.Arn)) if err != nil { - return nil, trace.Wrap(err) - } - if rootsOrganization != filter.OrganizationID { - return nil, trace.BadParameter("the AWS Organizations client is not part of the expected Organization %s", filter.OrganizationID) + return "", "", trace.Wrap(err) } - rootOU, err := organizationalUnitDetails(ctx, orgsClient, aws.ToString(root.Id), filter.ExcludeOUs) + rootsOrganization, err := organizationIDFromARN(rootARN, "root") if err != nil { - return nil, trace.Wrap(err) + return "", "", trace.Wrap(err) + } + if rootsOrganization != organizationID { + return "", "", trace.BadParameter("the AWS Organizations client is not part of the expected Organization %s", organizationID) } - return rootOU, nil + return aws.ToString(root.Id), cmp.Or(rootARN.Partition, awsutils.StandardPartition), nil } -func organizationalUnitDetails(ctx context.Context, orgsClient OrganizationsClient, ouID string, excludedOUs []string) (*awsOrgItem, error) { - ret := &awsOrgItem{ - id: ouID, - } - +// includedAccounts returns the accounts of the OU ouID and of every OU below it +// that the filter includes, split by account state. +// +// Limits of AWS Organizations: +// https://docs.aws.amazon.com/organizations/latest/userguide/orgs_reference_limits.html +// Most relevant limits: +// Max OU depth is 5 levels. +// At most there will be 2000 OUs in an AWS Organization. +// At most there will be 10 accounts, but that's configurable. +func includedAccounts(ctx context.Context, orgsClient OrganizationsClient, ouID string, ancestorOUs []string, filter MatchingAccountsFilter) (activeAccounts []string, inactiveAccounts []string, err error) { // Everything under an excluded OU is not considered. - if slices.Contains(excludedOUs, ouID) { - return ret, nil + if slices.Contains(filter.ExcludeOUs, ouID) { + return nil, nil, nil } - activeAccountIDs, notActiveAccountIDs, err := accountsInOrganizationalUnit(ctx, orgsClient, ouID) - if err != nil { - return nil, trace.Wrap(err) + // Cloned so that sibling OUs do not share the backing array. + ouChain := append(slices.Clone(ancestorOUs), ouID) + + if OrganizationalUnitsMatch(filter, ouChain) { + activeAccounts, inactiveAccounts, err = accountsInOrganizationalUnit(ctx, orgsClient, ouID) + if err != nil { + return nil, nil, trace.Wrap(err) + } } - ret.accounts = activeAccountIDs - ret.notActiveAccounts = notActiveAccountIDs childrenOUIDs, err := childrenOUs(ctx, orgsClient, ouID) if err != nil { - return nil, trace.Wrap(err) + return nil, nil, trace.Wrap(err) } for _, childOUID := range childrenOUIDs { - childOU, err := organizationalUnitDetails(ctx, orgsClient, childOUID, excludedOUs) + childActive, childInactive, err := includedAccounts(ctx, orgsClient, childOUID, ouChain, filter) if err != nil { - return nil, trace.Wrap(err) + return nil, nil, trace.Wrap(err) } - ret.organizationalUnits = append(ret.organizationalUnits, childOU) + activeAccounts = append(activeAccounts, childActive...) + inactiveAccounts = append(inactiveAccounts, childInactive...) } - return ret, nil + return activeAccounts, inactiveAccounts, nil } func childrenOUs(ctx context.Context, orgChildrenLister organizations.ListChildrenAPIClient, ouID string) ([]string, error) { @@ -243,7 +243,7 @@ func childrenOUs(ctx context.Context, orgChildrenLister organizations.ListChildr return childOUs, nil } -func accountsInOrganizationalUnit(ctx context.Context, orgChildrenLister organizations.ListAccountsForParentAPIClient, ouID string) (activeAccountIDs []string, notActiveAccountIDs []string, err error) { +func accountsInOrganizationalUnit(ctx context.Context, orgChildrenLister organizations.ListAccountsForParentAPIClient, ouID string) (activeAccountIDs []string, inactiveAccountIDs []string, err error) { paginator := organizations.NewListAccountsForParentPaginator(orgChildrenLister, &organizations.ListAccountsForParentInput{ ParentId: aws.String(ouID), }) @@ -258,10 +258,10 @@ func accountsInOrganizationalUnit(ctx context.Context, orgChildrenLister organiz if account.State == organizationstypes.AccountStateActive { activeAccountIDs = append(activeAccountIDs, aws.ToString(account.Id)) } else { - notActiveAccountIDs = append(notActiveAccountIDs, aws.ToString(account.Id)) + inactiveAccountIDs = append(inactiveAccountIDs, aws.ToString(account.Id)) } } } - return activeAccountIDs, notActiveAccountIDs, nil + return activeAccountIDs, inactiveAccountIDs, nil } diff --git a/lib/utils/aws/organizations/matchingaccounts_test.go b/lib/utils/aws/organizations/matchingaccounts_test.go index 5dca2a8499987..30421576e980f 100644 --- a/lib/utils/aws/organizations/matchingaccounts_test.go +++ b/lib/utils/aws/organizations/matchingaccounts_test.go @@ -19,6 +19,7 @@ package organizations import ( + "cmp" "context" "fmt" "testing" @@ -35,13 +36,14 @@ import ( type mockOrganizationsClient struct { organizationID string rootOUID string + rootARN string ouItems map[string]ouItem } type ouItem struct { - innerOUs []string - innerAccounts []string - innerNotActiveAccounts []string + innerOUs []string + innerAccounts []string + innerInactiveAccounts []string } func (m *mockOrganizationsClient) ListChildren(ctx context.Context, input *organizations.ListChildrenInput, opts ...func(*organizations.Options)) (*organizations.ListChildrenOutput, error) { @@ -67,7 +69,7 @@ func (m *mockOrganizationsClient) ListChildren(ctx context.Context, input *organ } func (m *mockOrganizationsClient) ListRoots(ctx context.Context, input *organizations.ListRootsInput, opts ...func(*organizations.Options)) (*organizations.ListRootsOutput, error) { - rootARN := fmt.Sprintf("arn:aws:organizations::0000000000:root/%s/%s", m.organizationID, m.rootOUID) + rootARN := cmp.Or(m.rootARN, fmt.Sprintf("arn:aws:organizations::0000000000:root/%s/%s", m.organizationID, m.rootOUID)) return &organizations.ListRootsOutput{ Roots: []organizationstypes.Root{ { @@ -86,19 +88,15 @@ func (m *mockOrganizationsClient) ListAccountsForParent(ctx context.Context, inp var accounts []organizationstypes.Account for _, accountID := range ouItem.innerAccounts { - accountARN := fmt.Sprintf("arn:aws:organizations::0000000000:account/%s/%s", m.organizationID, accountID) accounts = append(accounts, organizationstypes.Account{ Id: aws.String(accountID), State: organizationstypes.AccountStateActive, - Arn: aws.String(accountARN), }) } - for _, accountID := range ouItem.innerNotActiveAccounts { - accountARN := fmt.Sprintf("arn:aws:organizations::0000000000:account/%s/%s", m.organizationID, accountID) + for _, accountID := range ouItem.innerInactiveAccounts { accounts = append(accounts, organizationstypes.Account{ Id: aws.String(accountID), State: organizationstypes.AccountStateSuspended, - Arn: aws.String(accountARN), }) } return &organizations.ListAccountsForParentOutput{ @@ -148,8 +146,7 @@ func TestMatchingAccounts(t *testing.T) { }, }, }, - errCheck: require.Error, - expectedAccounts: []string{}, + errCheck: require.Error, }, { name: "missing organization id returns an error", @@ -166,11 +163,10 @@ func TestMatchingAccounts(t *testing.T) { }, }, }, - errCheck: require.Error, - expectedAccounts: []string{}, + errCheck: require.Error, }, { - name: "non-active accounts are discarded", + name: "inactive accounts are discarded", filter: MatchingAccountsFilter{ OrganizationID: "o-1", IncludeOUs: []string{"*"}, @@ -181,7 +177,7 @@ func TestMatchingAccounts(t *testing.T) { ouItems: map[string]ouItem{ "r-1": { innerAccounts: []string{"o1-r1-01", "o1-r1-02"}, - innerNotActiveAccounts: []string{ + innerInactiveAccounts: []string{ "o1-r1-01-suspended", "o1-r1-02-suspended", "o1-r1-03-suspended", @@ -218,6 +214,24 @@ func TestMatchingAccounts(t *testing.T) { }, errCheck: require.Error, }, + { + name: "unparseable root ARN: returns error", + filter: MatchingAccountsFilter{ + OrganizationID: "o-1", + IncludeOUs: []string{"*"}, + }, + orgsClient: &mockOrganizationsClient{ + organizationID: "o-1", + rootOUID: "r-1", + rootARN: "not-an-arn", + ouItems: map[string]ouItem{ + "r-1": { + innerAccounts: []string{"o1-r1-01"}, + }, + }, + }, + errCheck: errContains("arn: invalid prefix"), + }, { name: "one excluded, but wrong organization id: returns error", filter: MatchingAccountsFilter{ @@ -456,9 +470,51 @@ func TestMatchingAccounts(t *testing.T) { tt.filter, ) tt.errCheck(t, err) - if tt.expectedAccounts != nil { - require.ElementsMatch(t, tt.expectedAccounts, matchingAccounts) + if err != nil { + require.Empty(t, matchingAccounts) + return + } + require.ElementsMatch(t, tt.expectedAccounts, matchingAccounts.IDs) + }) + } +} + +func TestMatchingAccountsPartition(t *testing.T) { + for _, tt := range []struct { + name string + rootARN string + expectedPartition string + }{ + { + name: "partition from the root ARN", + rootARN: "arn:aws-us-gov:organizations::000000000000:root/o-1/r-1", + expectedPartition: "aws-us-gov", + }, + { + name: "root ARN without a partition falls back to aws", + rootARN: "arn::organizations::000000000000:root/o-1/r-1", + expectedPartition: "aws", + }, + } { + t.Run(tt.name, func(t *testing.T) { + orgsClient := &mockOrganizationsClient{ + organizationID: "o-1", + rootOUID: "r-1", + rootARN: tt.rootARN, + ouItems: map[string]ouItem{ + "r-1": {innerAccounts: []string{"111111111111"}}, + }, } + + matchingAccounts, err := MatchingAccounts( + t.Context(), + logtest.NewLogger(), + orgsClient, + MatchingAccountsFilter{OrganizationID: "o-1", IncludeOUs: []string{"*"}}, + ) + require.NoError(t, err) + require.Equal(t, tt.expectedPartition, matchingAccounts.Partition) + require.Equal(t, []string{"111111111111"}, matchingAccounts.IDs) }) } }