diff --git a/pkg/connector/account.go b/pkg/connector/account.go index 2fc6ff5a..dfc5b32c 100644 --- a/pkg/connector/account.go +++ b/pkg/connector/account.go @@ -121,6 +121,16 @@ type entitlementsPageState struct { PermissionSetIndex int `json:"psi"` } +// HierarchySyncFlags reports whether a sync run will actually emit the (OptInRequired) +// organization/organizational_unit resource types. Grouped into a named struct rather than +// two positional bools so a call site can't silently swap Organization and +// OrganizationalUnit - the compiler catches a missing/misnamed field, and a transposition +// requires actually swapping the field names, not just their order. +type HierarchySyncFlags struct { + Organization bool + OrganizationalUnit bool +} + type accountResourceType struct { resourceType *v2.ResourceType orgClient orgsAPI @@ -129,6 +139,12 @@ type accountResourceType struct { identityInstance *awsSsoAdminTypes.InstanceMetadata identityClient client.IdentityStoreClient region string + + // hierarchySync reports whether this sync run will actually sync the corresponding + // (OptInRequired) hierarchy resource type. Account re-parenting (see List) is gated on + // this so accounts never point at a Root/OU resource that this run never syncs, which + // would otherwise leave a dangling "MISSING RESOURCE" parent. + hierarchySync HierarchySyncFlags } func (o *accountResourceType) ResourceType(_ context.Context) *v2.ResourceType { @@ -187,18 +203,22 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r }), } - // Sparse ACLs hierarchy (Phase 2): re-parent the account under its Root/OU so c1's - // by-inheritance review can walk Account → OU → Root with the role pinned. Fail-soft: - // without organizations:ListParents the account stays flat (parentless) and we WARN once. - parentID, accessDenied, err := accountParentResourceID(ctx, o.orgClient, accountId) - if err != nil { - return nil, nil, err - } - if accessDenied { - orgReadDenied = true - } - if parentID != nil { - resourceOpts = append(resourceOpts, resourceSdk.WithParentResourceID(parentID)) + // Gated on hierarchySync.Organization: organization and organizational_unit are OptInRequired, + if o.hierarchySync.Organization { + parentID, accessDenied, err := accountParentResourceID(ctx, o.orgClient, accountId) + if err != nil { + return nil, nil, err + } + if accessDenied { + orgReadDenied = true + } + if parentID != nil { + willSyncParentType := parentID.ResourceType == resourceTypeOrganization.Id || + (parentID.ResourceType == resourceTypeOrganizationalUnit.Id && o.hierarchySync.OrganizationalUnit) + if willSyncParentType { + resourceOpts = append(resourceOpts, resourceSdk.WithParentResourceID(parentID)) + } + } } resourceOpts = append(resourceOpts, resourceSdk.WithResourceProfile(profile)) @@ -215,7 +235,7 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r rv = append(rv, userResource) } if orgReadDenied { - l.Warn("baton-aws: missing organizations:ListParents permission; accounts synced flat (no Root/OU hierarchy). " + + l.Debug("baton-aws: missing organizations:ListParents permission; accounts synced flat (no Root/OU hierarchy). " + "Add organizations:ListParents to enable by-inheritance review across the org tree.") } @@ -952,6 +972,7 @@ func accountBuilder( identityInstance *awsSsoAdminTypes.InstanceMetadata, region string, identityClient client.IdentityStoreClient, + hierarchySync HierarchySyncFlags, ) *accountResourceType { return &accountResourceType{ resourceType: resourceTypeAccount, @@ -961,6 +982,7 @@ func accountBuilder( identityClient: identityClient, identityInstance: identityInstance, region: region, + hierarchySync: hierarchySync, } } diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 8599d415..3a758d82 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -96,6 +96,12 @@ type AWS struct { cloudTrailClient *cloudtrail.Client assumeRoleWithWebIdentity func(context.Context, *sts.AssumeRoleWithWebIdentityInput) (*sts.AssumeRoleWithWebIdentityOutput, error) + // willSyncOrganization/willSyncOrganizationalUnit report whether this sync run's + // resource-type filter (if any) includes the OptInRequired org/OU hierarchy types. + // See accountResourceType.List. + willSyncOrganization bool + willSyncOrganizationalUnit bool + syncSecrets bool syncSSOUserLastLogin bool syncOnlyAttachedPolicies bool @@ -242,15 +248,24 @@ func validateConfig(awsc *cfg.Aws) error { return nil } -func New(ctx context.Context, awsc *cfg.Aws, opts *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) { +func New(ctx context.Context, awsc *cfg.Aws, connectorOpts *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) { l := ctxzap.Extract(ctx) + // Default to "will sync" when the caller gave no explicit resource-type filter (or no + // ConnectorOpts at all), matching ConnectorOpts.WillSyncResourceType's own default. + willSyncOrganization := true + willSyncOrganizationalUnit := true + if connectorOpts != nil { + willSyncOrganization = connectorOpts.WillSyncResourceType(resourceTypeOrganization.Id) + willSyncOrganizationalUnit = connectorOpts.WillSyncResourceType(resourceTypeOrganizationalUnit.Id) + } + // syncIAMPolicyGrants gates the cross-type iam_policy grants that iamUser, role, // iamGroup, and permissionSet emit as a sync optimization. When the caller has // explicitly filtered resource types and iam_policy isn't among them, emitting // those grants is invalid/wasteful. A nil opts (e.g. capabilities generation or // callers that don't pass one) is treated the same as "no filter" -> sync everything. - syncIAMPolicyGrants := opts == nil || opts.WillSyncResourceType("iam_policy") + syncIAMPolicyGrants := connectorOpts == nil || connectorOpts.WillSyncResourceType("iam_policy") err := field.Validate(cfg.Config, awsc) if err != nil { @@ -320,6 +335,9 @@ func New(ctx context.Context, awsc *cfg.Aws, opts *cli.ConnectorOpts) (connector syncIAMPolicyGrants: syncIAMPolicyGrants, accountProvisioningTarget: config.AccountProvisioningTarget, + + willSyncOrganization: willSyncOrganization, + willSyncOrganizationalUnit: willSyncOrganizationalUnit, } rv.awsClientFactory = NewAWSClientFactory(config, rv, httpClient) @@ -482,7 +500,8 @@ func (c *AWS) ResourceSyncers(ctx context.Context) []connectorbuilder.ResourceSy if c.orgsEnabled && c.ssoEnabled { l.Debug("orgsEnabled. creating accountBuilder") - acct := accountBuilder(c.orgClient, c.roleARN, c.ssoAdminClient, c.identityInstance, c.ssoRegion, c.identityStoreClient) + acct := accountBuilder(c.orgClient, c.roleARN, c.ssoAdminClient, c.identityInstance, c.ssoRegion, c.identityStoreClient, + HierarchySyncFlags{Organization: c.willSyncOrganization, OrganizationalUnit: c.willSyncOrganizationalUnit}) rs = append(rs, acct, // Sparse ACLs (Cloud Infrastructure Access): permission set as role, and the @@ -528,9 +547,9 @@ func (d *defaultCapabilitiesBuilder) ResourceSyncers(_ context.Context) []connec inlinePolicyBuilder(nil, nil, nil, nil), ssoUserBuilder("", nil, nil, nil, nil), ssoGroupBuilder("", nil, nil, nil), - accountBuilder(nil, "", nil, nil, "", nil), + accountBuilder(nil, "", nil, nil, "", nil, HierarchySyncFlags{Organization: true, OrganizationalUnit: true}), permissionSetBuilder(nil, nil, true), - permissionSetAssignmentBuilder(accountBuilder(nil, "", nil, nil, "", nil)), + permissionSetAssignmentBuilder(accountBuilder(nil, "", nil, nil, "", nil, HierarchySyncFlags{Organization: true, OrganizationalUnit: true})), organizationBuilder(nil), organizationalUnitBuilder(nil), accountIAMBuilder(nil, nil, nil), diff --git a/pkg/connector/organization_test.go b/pkg/connector/organization_test.go index 59e721be..af3332be 100644 --- a/pkg/connector/organization_test.go +++ b/pkg/connector/organization_test.go @@ -46,7 +46,7 @@ func TestOrganizationList_EmitsRoots(t *testing.T) { // organizationResource must attach a ChildResourceType annotation to the emitted resource // instance — this is what the SDK's syncer actually reads to schedule the organizational_unit // child crawl (a ResourceType-level annotation alone does not drive dispatch). Regression test -// for CXP-756, where this was missing and the OU tier silently never synced. +// for the case where this was missing and the OU tier silently never synced. func TestOrganizationResource_EmitsChildResourceTypeAnnotation(t *testing.T) { r, err := organizationResource(awsOrgsTypes.Root{Id: awsSdk.String(testRootID)}) require.NoError(t, err) @@ -62,7 +62,7 @@ func TestOrganizationResource_EmitsChildResourceTypeAnnotation(t *testing.T) { } // organizationalUnitResource must attach a ChildResourceType annotation pointing at itself so -// the SDK recurses into nested OUs. Regression test for CXP-756. +// the SDK recurses into nested OUs. Regression test for the same issue as above. func TestOrganizationalUnitResource_EmitsChildResourceTypeAnnotation(t *testing.T) { parent := &v2.ResourceId{ResourceType: resourceTypeOrganization.Id, Resource: testRootID} r, err := organizationalUnitResource(awsOrgsTypes.OrganizationalUnit{Id: awsSdk.String(testOUID)}, parent) @@ -264,12 +264,110 @@ func TestAccountList_FlatWhenOrgReadDenied(t *testing.T) { assert.Nil(t, resources[0].ParentResourceId, "account stays flat when re-parenting is denied") } +// account.List must not call ListParents at all, and must leave the account flat, when neither +// hierarchy type is opted into this sync run — otherwise the account gets a parent pointing at +// a resource type that will never be synced ("MISSING RESOURCE"). +func TestAccountList_SkipsReparentWhenHierarchyNotSynced(t *testing.T) { + ctx := context.Background() + orgs := &fakeOrgs{ + listAccountsFn: func(_ *awsOrgs.ListAccountsInput) (*awsOrgs.ListAccountsOutput, error) { + return &awsOrgs.ListAccountsOutput{Accounts: []awsOrgsTypes.Account{{ + Id: awsSdk.String(testAccountID), + Name: awsSdk.String("prod"), + Status: awsOrgsTypes.AccountStatusActive, + }}}, nil + }, + listParentsFn: func(_ *awsOrgs.ListParentsInput) (*awsOrgs.ListParentsOutput, error) { + return &awsOrgs.ListParentsOutput{Parents: []awsOrgsTypes.Parent{{ + Id: awsSdk.String(testOUID), + Type: awsOrgsTypes.ParentTypeOrganizationalUnit, + }}}, nil + }, + } + acct := newOrgAccountWithSyncFilter(orgs, false, false) + + resources, _, err := acct.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, resources, 1) + assert.Nil(t, resources[0].ParentResourceId, "account must stay flat when hierarchy isn't synced") + assert.Equal(t, 0, orgs.listParentsCalls, "must not call ListParents when neither hierarchy type is synced") +} + +// account.List must not call ListParents when organization is opted out even if +// organizational_unit is opted in — OU sync is itself seeded from Root (see organization.go), +// so OU can never actually be synced when organization is not, and resolving the parent would +// be wasted work. This also means a ListParents access-denied error can never surface the +// "missing organizations:ListParents permission" WARN in this specific combo; the account still +// ends up flat either way, so this is a lost diagnostic, not a correctness bug (see the comment +// on the ListParents gate in account.go). +func TestAccountList_SkipsReparentWhenOnlyOrganizationalUnitSynced(t *testing.T) { + ctx := context.Background() + orgs := &fakeOrgs{ + listAccountsFn: func(_ *awsOrgs.ListAccountsInput) (*awsOrgs.ListAccountsOutput, error) { + return &awsOrgs.ListAccountsOutput{Accounts: []awsOrgsTypes.Account{{ + Id: awsSdk.String(testAccountID), + Name: awsSdk.String("prod"), + Status: awsOrgsTypes.AccountStatusActive, + }}}, nil + }, + listParentsFn: func(_ *awsOrgs.ListParentsInput) (*awsOrgs.ListParentsOutput, error) { + return nil, &awsOrgsTypes.AccessDeniedException{Message: awsSdk.String("no perms")} + }, + } + acct := newOrgAccountWithSyncFilter(orgs, false, true) + + resources, _, err := acct.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, resources, 1) + assert.Nil(t, resources[0].ParentResourceId, "account must stay flat when organization isn't synced") + assert.Equal(t, 0, orgs.listParentsCalls, "must not call ListParents when organization is opted out, regardless of organizational_unit") +} + +// account.List resolves the parent but only attaches it when the resolved parent's specific +// type (organization vs organizational_unit) is one this run will actually sync — partial +// opt-in must not produce a dangling parent either. +func TestAccountList_SkipsReparentWhenResolvedParentTypeNotSynced(t *testing.T) { + ctx := context.Background() + orgs := &fakeOrgs{ + listAccountsFn: func(_ *awsOrgs.ListAccountsInput) (*awsOrgs.ListAccountsOutput, error) { + return &awsOrgs.ListAccountsOutput{Accounts: []awsOrgsTypes.Account{{ + Id: awsSdk.String(testAccountID), + Name: awsSdk.String("prod"), + Status: awsOrgsTypes.AccountStatusActive, + }}}, nil + }, + listParentsFn: func(_ *awsOrgs.ListParentsInput) (*awsOrgs.ListParentsOutput, error) { + return &awsOrgs.ListParentsOutput{Parents: []awsOrgsTypes.Parent{{ + Id: awsSdk.String(testOUID), + Type: awsOrgsTypes.ParentTypeOrganizationalUnit, + }}}, nil + }, + } + // organization is opted in but organizational_unit is not; the account's actual parent + // resolves to an OU, so it must still be left flat. + acct := newOrgAccountWithSyncFilter(orgs, true, false) + + resources, _, err := acct.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, resources, 1) + assert.Nil(t, resources[0].ParentResourceId, "account must stay flat when its resolved parent type isn't synced") + assert.Equal(t, 1, orgs.listParentsCalls, "must still resolve the parent when at least one hierarchy type is synced") +} + // newOrgAccount builds an accountResourceType backed by the given fakeOrgs (SSO client unused -// by the List/re-parent path under test). +// by the List/re-parent path under test), with the org/OU hierarchy types opted into this sync. func newOrgAccount(orgs *fakeOrgs) *accountResourceType { + return newOrgAccountWithSyncFilter(orgs, true, true) +} + +// newOrgAccountWithSyncFilter is like newOrgAccount but lets the test control whether the +// organization/organizational_unit hierarchy types are opted into this sync run, to exercise +// the re-parenting gate. +func newOrgAccountWithSyncFilter(orgs *fakeOrgs, willSyncOrganization, willSyncOrganizationalUnit bool) *accountResourceType { identityInstance := &awsSsoAdminTypes.InstanceMetadata{ InstanceArn: awsSdk.String(behaviorInstanceArn), IdentityStoreId: awsSdk.String(behaviorIdentityStoreID), } - return accountBuilder(orgs, "", &fakeSSOAdmin{}, identityInstance, behaviorRegion, nil) + return accountBuilder(orgs, "", &fakeSSOAdmin{}, identityInstance, behaviorRegion, nil, + HierarchySyncFlags{Organization: willSyncOrganization, OrganizationalUnit: willSyncOrganizationalUnit}) } diff --git a/pkg/connector/permission_set_assignment_behavior_test.go b/pkg/connector/permission_set_assignment_behavior_test.go index dc860e62..19915b17 100644 --- a/pkg/connector/permission_set_assignment_behavior_test.go +++ b/pkg/connector/permission_set_assignment_behavior_test.go @@ -162,6 +162,8 @@ type fakeOrgs struct { listOUsFn func(*awsOrgs.ListOrganizationalUnitsForParentInput) (*awsOrgs.ListOrganizationalUnitsForParentOutput, error) listParentsFn func(*awsOrgs.ListParentsInput) (*awsOrgs.ListParentsOutput, error) describeAccountFn func(*awsOrgs.DescribeAccountInput) (*awsOrgs.DescribeAccountOutput, error) + + listParentsCalls int } func (f *fakeOrgs) DescribeAccount(_ context.Context, in *awsOrgs.DescribeAccountInput, _ ...func(*awsOrgs.Options)) (*awsOrgs.DescribeAccountOutput, error) { @@ -197,6 +199,7 @@ func (f *fakeOrgs) ListOrganizationalUnitsForParent( } func (f *fakeOrgs) ListParents(_ context.Context, in *awsOrgs.ListParentsInput, _ ...func(*awsOrgs.Options)) (*awsOrgs.ListParentsOutput, error) { + f.listParentsCalls++ if f.listParentsFn != nil { return f.listParentsFn(in) } @@ -215,7 +218,8 @@ func newBehaviorAccount(sso *fakeSSOAdmin) *accountResourceType { InstanceArn: awsSdk.String(behaviorInstanceArn), IdentityStoreId: awsSdk.String(behaviorIdentityStoreID), } - return accountBuilder(&fakeOrgs{}, "", sso, identityInstance, behaviorRegion, &test.MockedIdentityStoreClient{}) + return accountBuilder(&fakeOrgs{}, "", sso, identityInstance, behaviorRegion, &test.MockedIdentityStoreClient{}, + HierarchySyncFlags{Organization: true, OrganizationalUnit: true}) } func behaviorBinding(t *testing.T) (*v2.Resource, *v2.Entitlement) {