[CXP-524] feat: sync AWS resource tags behind --sync-resource-tags by agustin-conductor · Pull Request #158 · ConductorOne/baton-aws

agustin-conductor · 2026-09-16T16:31:51Z

What

Publishes AWS resource tags on accounts, IAM users, and IAM roles as a nested aws_tags profile map, behind a new opt-in --sync-resource-tags flag (default false).

The customer goal is dynamic policy routing: read the Owner tag on an AWS resource and route the access request to that team, instead of hard-coding approvers per entitlement.

Why it needs new API calls

None of the List* APIs return tags:

organizations.Account has no Tags field at all
iam:ListUsers / iam:ListRoles always return an empty Tags slice

aws_tags already existed on the IAM user and role profiles and was fed from those responses — so it has been silently reporting "no tags" for every user and role. This PR makes that field real.

Tags are only reachable via a separate per-resource call, which is why this is flag-gated. organizations:ListTagsForResource is throttled at 10 req/s (burst 15) per account, so a 1,000-account org spends real time on tag reads alone.

Design notes for review

Pagination is required, and is not a hand-rolled loop. The documented 50-tag quota counts only user-created tags — AWS-reserved aws: system tags are additional and excluded from it (see the CloudFormation tagging reference: "Tags with this prefix don't count toward the number of tags per resource"). Meanwhile iam:ListUserTags / iam:ListRoleTags cap their response array at 50 items. Neither API can be asked for a bigger page: organizations:ListTagsForResource takes no page-size parameter at all, and IAM's MaxItems cannot lift the response cap. Reading only the first response would silently drop tags, and IAM returns them sorted by tag key, so the dropped ones are not a random sample. The fetchers use the AWS SDK's own paginators with StopOnDuplicateToken. There is deliberately no page or tag cap: organizations:ListTagsForResource documents no page size at all, so any such bound would be a guess that could fail a legitimately-tagged resource, and a runaway would be caught by the sync deadline anyway.

A tag cursor can't be hoisted into the caller's page token: these are per-resource sub-fetches inside a List that already owns one pagination.Bag for its own page, and a resource's profile must be complete before the resource is emitted.

Every failure is fatal, including a missing tag permission. The flag is opt-in, so enabling it is an explicit request for tags that feed access-routing decisions. Degrading to untagged resources would leave policy rules evaluating against tags that are silently absent, on a sync that reported success — and nobody reads warnings on a green sync. Errors surface as PermissionDenied naming the action to grant. The page cap is an error for the same reason: truncated tags are as unusable for routing as missing ones.

Profile shape. Tags land as a nested map, verified to be CEL-addressable as resource.profile.aws_tags["Owner"]. Rule authors must guard lookups ("Owner" in resource.profile.aws_tags) — a missing key is an eval error, not null.

New permissions

organizations:ListTagsForResource, iam:ListUserTags, iam:ListRoleTags — added to the affected resource types' capabilityPermissions and to every IAM policy block in README.md and docs/connector.mdx, each marked as required only when the flag is enabled.

Also in this PR

Six goconst lint fixes (repeated string literals → named constants) in partition.go, iam_policy.go, inline_policy.go, resource_types.go, sts_actions.go. No behaviour change; they were pre-existing and blocking the commit hook.

The one remaining lint issue — the deprecated identitystore ListUsers Filters in sso_user.go — is left in place deliberately rather than suppressed with //nolint, so it stays visible. Tracked in CXP-1107: the fix is a GetUserId + DescribeUser migration that changes a provisioning code path and needs two new customer-facing IAM permissions, so it doesn't belong here.

Testing

pkg/connector/tags_test.go covers: profile shape end-to-end through account.List; no API call at all when the flag is off; access-denied is fatal with the right gRPC code and a message naming the action; non-permission errors propagate; multi-page accumulation for all three fetchers; single-page not costing a second call; the page cap; and the duplicate-token stop.

go build, go vet, and the full suite pass.

Not covered here

The other half of CXP-524 is a c1-side gap. EntitlementRoutingRuleEnv can already reach entitlement.scope.profile, and for Identity Center permission-set bindings the scope is the AWS account — so account tags become usable in routing rules immediately. But PolicyStepEnv, which resolves approvers, passes a nil resource, so approver expressions cannot read any resource profile yet. That needs a separate platform change; this PR is a prerequisite for it, not a substitute.

🤖 Generated with Claude Code

linear-code · 2026-09-16T16:32:05Z

CXP-524

github-actions · 2026-09-16T16:36:25Z


 		profile := accountProfile(ctx, account)
+
+		if o.syncResourceTags {


🟡 Suggestion: aws_tags is absent from the account profile when the flag is off, but IAM users/roles always publish it (roleProfile/iamUserProfile set it to an empty map unconditionally). That inconsistency matters for the CEL contract documented in tags.go: "Owner" in resource.profile.aws_tags is itself an eval error when the aws_tags key does not exist, so a rule written against accounts breaks differently depending on the flag. docs/connector.mdx:120 also states the field "is empty" without the flag, which is only true for users and roles. Consider setting profile[tagsProfileField] = map[string]interface{}{} in accountProfile so the key is always present.

Won't fix. We avoid changing existing profile fields, so aws_tags stays unconditional on users/roles. CEL guards absence fine: has(resource.profile.aws_tags) returns false without erroring.

github-actions · 2026-09-16T16:36:31Z

+			if err != nil {
+				return nil, nil, err
+			}
+			profile[tagsProfileField] = tags


🟡 Suggestion: this writes through the new tagsProfileField const, but the placeholder writes still hardcode the literal — role.go:299 (profile["aws_tags"] = roleTagsToMap(role)) and iam_user.go:246. That is exactly the drift the const was introduced to prevent, and it is the same cleanup this PR applied to aws_policy_name and expiration. Switch both to tagsProfileField.

Addressed in 06a9318.

github-actions · 2026-09-16T16:36:38Z

+// maxTagPages bounds every tag paginator below. The documented user-tag quota is 50 per
+// resource and system tags are a small fixed set per resource, so five pages is already
+// far past anything real — the bound exists so a misbehaving endpoint cannot stall a sync.
+const maxTagPages = 5


🟡 Suggestion: the 5-page bound is derived from a page size that is assumed rather than known. As the comment above says, organizations:ListTagsForResource takes no page-size parameter and AWS does not document its page size, so "5 pages" is not a tag count — if that API returns tags in small pages, an account with a legitimate 50 user tags plus aws: system tags exceeds the cap and errTagPageCap fails the entire account sync with a message telling the operator to turn the feature off. Bounding total accumulated tags (e.g. len(rv) > 500) instead of page count would make the limit mean what the comment says it means, and keep it independent of the API's page size.

Addressed in 06a9318 — removed the bound entirely rather than retuning it. ListTagsForResource documents no page size, so any cap is a guess.

github-actions · 2026-09-16T16:36:43Z

+	}}}, nil
+}
+
+func accountTagsFromProfile(t *testing.T, acct *accountResourceType, orgs *fakeOrgs) (map[string]interface{}, bool) {


🟡 Suggestion: the orgs *fakeOrgs parameter is never read in this helper — both callers already hold the orgs value they assert listTagsCalls on. Drop the parameter.

Addressed in 06a9318.

github-actions · 2026-09-16T16:36:50Z

 - **Sync secrets** — without it no access keys are synced, so none of the per-key detail above appears.
 - **Sync IAM User Console Access** (`BATON_SYNC_IAM_USER_CONSOLE_ACCESS`) — without it `console_access_enabled`, `password_reset_required`, and `login_profile_created_at` are not populated. It is off by default because it costs one `iam:GetLoginProfile` call per IAM user and requires `iam:GetLoginProfile` on the connector role. This setting detects an IAM console password; it does not detect access through Identity Center or an assumed role.

+- **Sync Resource Tags** (`BATON_SYNC_RESOURCE_TAGS`) — without it the `aws_tags` profile field on accounts, IAM users, and IAM roles is empty. It is off by default because tags are not returned by any `List*` call, so it costs at least one extra call per resource (`organizations:ListTagsForResource`, `iam:ListUserTags`, `iam:ListRoleTags`). The 50-tag quota covers only user-created tags — AWS-reserved `aws:` system tags are additional — so tags are read across pages. `organizations:ListTagsForResource` is throttled at 10 requests/second per account, which is the practical cost in a large organization. A missing tag permission fails the sync with a `PermissionDenied` naming the action to grant, rather than quietly syncing untagged resources: enabling this setting is an explicit request for tags, so losing them silently would leave policy rules evaluating against tags that are not there.


🟡 Suggestion: three places that document every other optional sync flag were not given the tags equivalent — the self-hosted env-var example (~line 789, which lists BATON_SYNC_SECRETS, BATON_SYNC_IAM_USER_CONSOLE_ACCESS, BATON_SYNC_SSO_USER_LAST_LOGIN but not BATON_SYNC_RESOURCE_TAGS), the cloud-hosted setup Steps (~line 698, which has an "Optional. Enable Sync IAM User Console Access" step), and the "Section 4: Other permissions" bullet list (~line 647) that explains each optional permission in the policy JSON this PR edited at lines 499-501. Following the iam:GetLoginProfile precedent in each spot would keep the flag discoverable from both install paths.

Addressed in 06a9318.

github-actions · 2026-09-16T16:45:09Z

Connector PR Review: [CXP-524] feat: sync AWS resource tags behind --sync-resource-tags

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base c85e94de3b56.
Review mode: full
View review run

Review Summary

Scanned the full PR diff for security and correctness: the new pkg/connector/tags.go fetchers, the four call sites (account.go, account_iam.go, iam_user.go, role.go), the config and capabilities wiring, and the six goconst extractions. The constant extractions in partition.go, iam_policy.go, inline_policy.go, resource_types.go, and sts_actions.go are behaviour-preserving — each new const matches the literal it replaces, and resourceTypeIDOrganizationalUnit correctly avoids the self-referential init cycle. Pagination uses the AWS SDK paginators with StopOnDuplicateToken, errors go through wrapAWSError (which matches both the IAM AccessDenied and the typed AccessDeniedException spellings, so the documented PermissionDenied behaviour holds for all three APIs), the o.aws != nil guards keep defaultCapabilitiesBuilder from dereferencing nil, and permissionSetAssignmentBuilder shares the account builder without re-invoking List, so tags are not fetched twice. No blocking issues; two non-blocking observations below, both distinct from the findings already recorded on this PR.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

pkg/connector/resource_types.go:174 (also :39, :90, :128) — the flag-gated tag permissions are added to the unconditional capabilityPermissions read set, so baton_capabilities.json now advertises them as required for every install; the equally flag-gated iam:GetLoginProfile and cloudtrail:LookupEvents are deliberately absent from both files, and README.md / docs/connector.mdx mark the tag actions as optional.
pkg/connector/tags.go:59 — the tag pagination loops have no page or tag bound; StopOnDuplicateToken only catches a byte-identical repeated token, so a token-rotating endpoint spins and grows the accumulator per resource until the sync deadline.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/resource_types.go`:
- Around lines 39, 90, 128, and 174: `iam:ListRoleTags`, `organizations:ListTagsForResource`
  (twice), and `iam:ListUserTags` were added to the unconditional `capabilityPermissions`
  read sets for the role, account, account_iam, and iam_user resource types. These three
  actions are only called when `--sync-resource-tags` is enabled, but `capabilityPermissions`
  has no notion of conditionality, so the regenerated `baton_capabilities.json` presents them
  as required for every install. The existing precedent in this repo is the opposite: the
  equally flag-gated `iam:GetLoginProfile` (`--sync-iam-user-console-access`) and
  `cloudtrail:LookupEvents` (`--sync-sso-user-last-login`) appear in neither
  `resource_types.go` nor `baton_capabilities.json`, and `README.md` / `docs/connector.mdx`
  both label the tag actions "Optional: only used with --sync-resource-tags". Either remove
  the four added entries (and regenerate `baton_capabilities.json` so the three permissions
  drop out of it), matching how the other opt-in permissions are handled, or state in the PR
  description why resource tags should diverge from that precedent and how C1 consumers of
  `baton_capabilities.json` should interpret a permission that is only needed under a flag.

In `pkg/connector/tags.go`:
- Around lines 59-67 (and the equivalent loops in `fetchIAMUserTags` at 85-93 and
  `fetchIAMRoleTags` at 111-119): the `for paginator.HasMorePages()` loops have no page or tag
  ceiling. `StopOnDuplicateToken` only terminates when the returned token is byte-identical
  to the immediately preceding one, so an endpoint (or an intermediary) that rotates tokens
  keeps `NextPage` looping and keeps appending into the result map for a single resource,
  repeated for every resource in the sync, bounded only by the sync deadline. Add a generous
  sanity ceiling — for example a `maxTagPages` const in the low hundreds — and return a
  wrapped error naming the resource when it is exceeded, so the failure mode is a clear error
  rather than a spinning sync. This matches the position taken in this PR that truncated tags
  are unusable for routing and must surface as an error; keep the bound high enough that no
  legitimately tagged resource can reach it.

github-actions · 2026-09-16T16:45:25Z

No description provided.

github-actions

No blocking issues found.

github-actions · 2026-09-16T18:14:04Z

 		profile := accountProfile(ctx, account)
+
+		if o.aws != nil && o.aws.syncResourceTags {
+			tags, err := fetchAccountTags(ctx, o.orgClient, awsSdk.ToString(account.Id))


🟡 Suggestion: Unlike account.go:194, this loop never filters on account.Status, so fetchAccountTags now fires for SUSPENDED / PENDING_CLOSURE accounts too — and every error here is fatal, so one bad account aborts the whole account_iam sync. It also burns organizations:ListTagsForResource calls (10 req/s per account) on accounts the org account syncer deliberately skips. Consider skipping non-ACTIVE accounts before the tag fetch, mirroring account.go.

github-actions · 2026-09-16T18:14:10Z

+// A missing IAM tag permission surfaces as PermissionDenied naming the action to grant,
+// rather than silently producing untagged users and roles.
+func TestFetchIAMTags_AccessDeniedIsFatal(t *testing.T) {
+	fake := &fakeIAMTags{err: &awsOrgsTypes.AccessDeniedException{Message: awsSdk.String("no perms")}}


🟡 Suggestion: This simulates an IAM denial with the Organizations-modeled AccessDeniedException, but per the comment on isAccessDeniedError (helpers.go:525-527) IAM models no such type and returns an unmodeled GenericAPIError with code AccessDenied. The test therefore only exercises the errCodeAccessDeniedException branch, not the spelling IAM actually produces. Both codes do map to PermissionDenied, so behavior is correct — but adding a &smithy.GenericAPIError{Code: "AccessDenied"} case would cover the real IAM shape.

github-actions

No blocking issues found.

mateoHernandez123

Approving. The tag sync is opt-in, fully paginated, and fails closed when the extra IAM/Organizations permissions are missing.

luisina-santos · 2026-09-16T17:43:12Z

+// far past anything real — the bound exists so a misbehaving endpoint cannot stall a sync.
+const maxTagPages = 5
+
+// None of the List* calls this connector uses return tags: organizations.Account has no


probably this comment can be reduced keeping the decisions and PR description data out of the code

github-actions · 2026-09-17T17:38:57Z

 				"iam:ListAttachedUserPolicies",
 				"iam:ListGroupsForUser",
+				// Only called when sync-resource-tags is enabled; ListUsers returns no tags.
+				"iam:ListUserTags",


🟡 Suggestion: capabilityPermissions in this repo deliberately omits flag-gated optional reads — iam:GetLoginProfile (gated on --sync-iam-user-console-access) and cloudtrail:LookupEvents (gated on --sync-sso-user-last-login) appear nowhere in resource_types.go or baton_capabilities.json. Adding iam:ListUserTags (plus iam:ListRoleTags at L39 and organizations:ListTagsForResource at L90/L128) under the unconditional // Read set means the regenerated baton_capabilities.json now advertises three permissions as required for every install, including the default --sync-resource-tags=false one, which contradicts the "Optional: only used with --sync-resource-tags" framing in README.md and docs/connector.mdx. Consider following the existing precedent and leaving them out, or note in the PR why tags should diverge from it.

github-actions · 2026-09-17T17:39:05Z

+	for paginator.HasMorePages() {
+		resp, err := paginator.NextPage(ctx)
+		if err != nil {
+			return nil, wrapAWSError(fmt.Errorf(
+				"baton-aws: organizations.ListTagsForResource failed for account %q "+
+					"(sync-resource-tags requires organizations:ListTagsForResource): %w", accountID, err))
+		}
+		putOrgTags(rv, resp.Tags)
+	}


🟡 Suggestion: this loop has no page or tag bound, and StopOnDuplicateToken only halts when a token is byte-identical to the immediately preceding one. An endpoint that rotates tokens (or a paginating proxy in front of it) keeps NextPage running and rv growing for one resource, per resource, with nothing but the sync deadline as a stop — the PR description accepts this consciously, but the same reasoning that makes truncated tags fatal also argues for a generous sanity bound (e.g. a few hundred pages) that errors out rather than spinning. The same applies to fetchIAMUserTags and fetchIAMRoleTags.

github-actions

No blocking issues found.

github-actions Bot reviewed Sep 16, 2026

View reviewed changes

agustin-conductor changed the title ~~feat: sync AWS resource tags behind --sync-resource-tags (CXP-524)~~ [CXP-524] feat: sync AWS resource tags behind --sync-resource-tags Sep 16, 2026

agustin-conductor requested review from FeliLucero1, JavierCarnelli-ConductorOne, johnallers, luisina-santos, mateoHernandez123, mateovespConductor and sergiocorral-conductorone September 16, 2026 17:25

agustin-conductor assigned johnallers Sep 16, 2026

github-actions Bot reviewed Sep 16, 2026

View reviewed changes

mateoHernandez123 approved these changes Sep 16, 2026

View reviewed changes

luisina-santos approved these changes Sep 16, 2026

View reviewed changes

agustin-conductor added 3 commits September 17, 2026 14:29

add sync resource tags feature

d453885

address pr comments and remove tags page limit logic

75beaa8

slim code comments

953892e

agustin-conductor force-pushed the feature/sync-resource-tags branch from 06a9318 to 953892e Compare September 17, 2026 17:34

github-actions Bot reviewed Sep 17, 2026

View reviewed changes


		profile := accountProfile(ctx, account)

		if o.syncResourceTags {

Conversation

agustin-conductor commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why it needs new API calls

Design notes for review

New permissions

Also in this PR

Testing

Not covered here

Uh oh!

linear-code Bot commented Sep 16, 2026

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

agustin-conductor Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

agustin-conductor Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

agustin-conductor Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

agustin-conductor Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

agustin-conductor Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Connector PR Review: [CXP-524] feat: sync AWS resource tags behind --sync-resource-tags

Review Summary

Security Issues

Correctness Issues

Suggestions

Uh oh!

github-actions Bot commented Sep 16, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mateoHernandez123 left a comment

Choose a reason for hiding this comment

Uh oh!

luisina-santos Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

agustin-conductor commented Sep 16, 2026 •

edited

Loading

github-actions Bot commented Sep 16, 2026 •

edited

Loading