fix(connector): refresh cross-account STS credentials instead of caching them forever

[agustin-conductor]

Fixes CXP-801. Go-live blocker for The Trade Desk (Pylon #12224).

Problem

AWSClientFactory built per-child-account IAM clients from a single sts:AssumeRole call, wrapped the result in credentials.NewStaticCredentialsProvider (never refreshes), and cached the client in iamClientMap for the lifetime of the process — no TTL, no expiry check, no invalidation.

Assumed-role sessions last at most 1 hour: role chaining caps there, and an AssumeRole call with no DurationSeconds defaults there anyway, so the 1-hour ceiling applied in every deployment mode. Clients are created eagerly at account-listing time (account_iam.go) but used later during the child IAM crawl, so:

• any sync where >1h elapsed between creation and use died mid-sync with 403 ExpiredToken, and
• the cached client then failed every subsequent sync until the process restarted.

Fix

getConfig now builds aws.NewCredentialsCache(stscreds.NewAssumeRoleProvider(...)), matching the pattern already used for the connector's own credentials in (*AWS).getCallingConfig. Caching the client is safe once its provider re-assumes on expiry.

Three details that aren't obvious from the diff:

The eager Retrieve is load-bearing, not a sanity check. stscreds providers are lazy — constructing one issues no API call. accountIAMResourceType.parseAssumeRole relies on an assume-role failure to skip inaccessible accounts, so without the probe those accounts would be silently registered and fail later mid-crawl. This isn't hypothetical: the verification run below hit an account that exercises it.

LoadDefaultConfig is retained deliberately. I initially hand-built an aws.Config and measured what that drops: RetryMode, RetryMaxAttempts, AppID, RequestMinCompressSizeBytes, DisableRequestCompression, AccountIDEndpointMode, and ConfigSources. That last one is where service clients read FIPS/dualstack and endpoint settings at construction, so an empty one would silently disable AWS_USE_FIPS_ENDPOINT for cross-account clients only. GetAwsConfigOptionsForAssumeRole is replaced by GetAwsConfigOptionsForCredentials, which takes a CredentialsProvider instead of an *sts.AssumeRoleOutput. config.wrapWithCredentialsCache returns an existing *aws.CredentialsCache untouched rather than re-wrapping, so refresh behavior survives the round trip — pinned by a test.

Refreshing credentials introduce a new silent-data-loss path, closed here. Once credentials re-assume lazily, an STS AccessDenied on re-assume surfaces at the IAM call site through the %w-wrapped chain:

iam.<Op> err → smithy.OperationError{ServiceID:"IAM"} → "get identity: %w"
  → "get credentials: %w" → CredentialsCache → AssumeRoleProvider
  → smithy.OperationError{ServiceID:"STS"} → GenericAPIError{Code:"AccessDenied"}

errors.As traverses all of it, so the existing fail-soft guards would have mistaken a credentials outage for a resource-level denial and skipped — a sync reporting success with grants silently missing, which reads downstream as revoked access. Strictly worse than the loud ExpiredToken we're fixing. isCredentialsRetrievalError excludes those. Note it can't use a plain errors.As: that matches the outermost OperationError (IAM) and never sees the STS one beneath, so it steps past each non-STS match and resumes underneath.

Secondary fix (same error family, per the ticket)

isAccessDeniedError only matched "AccessDenied". IAM models no AccessDenied* error type and returns an unmodeled GenericAPIError with that code, but SSO Admin returns a typed AccessDeniedException whose ErrorCode() is "AccessDeniedException" — so the fail-soft skips at permission_set.go:136 and inline_policy.go:162 were dead code and failed the sync instead of degrading. Both spellings now match. (sts_actions.go:188 already had this shape.)

Verification

Live tenant, 2-account org, --global-aws-cross-account-iam-enabled.

Credential refresh — with a temporary always-expired ExpiryWindow plus per-call AssumeRole logging, one child account logged 42 successive AssumeRole calls (numbered 1→42) through the single cached IAM client. On the old code that number is exactly 1, forever.

Eager probe — account 852922979753 returned 403 AccessDenied on AssumeRole; the probe caught it and the account was skipped via Skipping account in Organizations, preserving pre-fix semantics.

A/B on the AccessDenied half — reverting only the code-matching and re-running the same sync:

	result
pre-fix matching	exit=1 — cancelling context due to error in action on ssoadmin.GetInlinePolicyForPermissionSet
this PR	exit=0 — 8 fail-soft skips logged as warnings, Sync complete.

The tenant's test-user lacks sso:ListManagedPoliciesInPermissionSet and sso:GetInlinePolicyForPermissionSet, so it reproduces the dead-guard bug directly.

Final clean run on the exact committed code (no instrumentation): exit=0, Sync complete., zero ExpiredToken.

go build, go vet, full go test, and -race all pass. golangci-lint: 12 issues vs. 14 on main — zero new, two pre-existing goconst findings cleared.

Tests

11 tests in pkg/connector/aws_client_factory_test.go, using an injectable stsClientFn seam. Each guard was mutation-tested to confirm it actually bites:

• drop the credentials guard → TestCredentialsFailureIsNotTreatedAsResourceDenial fails
• revert to AccessDenied-only → the SSO Admin subtest fails
• hand-build the aws.Config → TestGetConfigResolvesAmbientAWSSettings fails on AWS_RETRY_MODE

Acceptance criteria

• Cross-account IAM clients use auto-refreshing assumed-role credentials
• A sync against child accounts completes without ExpiredToken — refresh demonstrated via the 42-call sequence rather than a >1h wait
• [~] Subsequent syncs in a long-lived process succeed without restart — not directly observable via go run (one sync per process); same CredentialsCache mechanism as above
• isAccessDeniedError matches SSO Admin AccessDeniedException; skip paths covered by unit tests

Reviewer notes

• GetAwsConfigOptionsForCredentials replaces an exported symbol (GetAwsConfigOptionsForAssumeRole). Only caller was aws_client_factory.go, but worth confirming nothing outside this repo referenced it.
• isCredentialsRetrievalError is covered by unit tests only. Hitting it live needs a role that assumes successfully at probe time and then loses permission mid-sync — hard to stage deliberately.
• Deliberately out of scope, all pre-existing: no negative caching for unassumable accounts (13 call sites re-attempt AssumeRole per denied account); f.mutex is a single global lock held across the STS call; orgClientMap is declared and never read — note the ticket's criterion 1 says "IAM/org clients" but no cross-account org path exists. Plus the ticket's own README least-privilege follow-up.

🤖 Generated with Claude Code

[linear-code]

CXP-801

[github-actions]

Connector PR Review: fix(connector): refresh cross-account STS credentials instead of caching them forever

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

Review Summary

Full PR diff scanned for security and correctness. The change replaces the one-shot AssumeRole + StaticCredentialsProvider with aws.NewCredentialsCache(stscreds.NewAssumeRoleProvider(...)) so cross-account clients re-assume on expiry, retains LoadDefaultConfig to preserve ambient AWS settings, and adds isCredentialsRetrievalError so a mid-sync STS AccessDenied on re-assume is no longer swallowed by the fail-soft skips. The refresh/probe/credentials-guard logic is correct and thoroughly covered by the new tests; all new imports resolve to already-required modules (no go.mod/go.sum change). No security or correctness issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• pkg/connector/connector.go:625 — the exported GetAwsConfigOptionsForAssumeRole is removed and replaced by GetAwsConfigOptionsForCredentials (different signature). Verified the only in-repo caller is aws_client_factory.go; this is an exported-symbol break for any external importer. Already acknowledged in the PR's reviewer notes, so no action needed unless an out-of-repo consumer references it.

Prompt for AI agents

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

## Suggestions

In `pkg/connector/connector.go`:
- Around line 625: The exported helper `GetAwsConfigOptionsForAssumeRole` was removed and
  replaced by `GetAwsConfigOptionsForCredentials`, which takes an `awsSdk.CredentialsProvider`
  instead of an `*sts.AssumeRoleOutput`. The only in-repo caller is
  pkg/connector/aws_client_factory.go. If any code outside this repository imported the old
  exported symbol, it will fail to compile. Confirm there are no external consumers; if there
  are, keep a thin deprecated wrapper or coordinate the rename with them.

[github-actions]

No blocking issues found.

[github-actions]

No blocking issues found.