From 83dc95149f831259610a6f01c66aaa9ea6134a37 Mon Sep 17 00:00:00 2001 From: agustin-conductor Date: Thu, 30 Jul 2026 12:51:56 -0300 Subject: [PATCH 1/2] fix(connector): refresh cross-account STS credentials instead of caching them forever MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AWSClientFactory built per-child-account IAM clients from a single sts:AssumeRole call, wrapped the result in credentials.NewStaticCredentialsProvider, and cached the client in iamClientMap for the lifetime of the process with no TTL or expiry check. Assumed-role sessions last at most 1 hour (role chaining caps there, and an AssumeRole call with no DurationSeconds defaults there anyway), while clients are created eagerly at account-listing time and used later during the child IAM crawl. Any sync where more than an hour elapsed between creation and use died with 403 ExpiredToken, and the cached client then failed every subsequent sync until the process restarted. 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 worth noting: - stscreds providers are lazy, so constructing one issues no API call. getConfig retrieves once up front because accountIAMResourceType.parseAssumeRole relies on an assume-role failure to skip inaccessible accounts; without the probe those accounts would be registered and fail later mid-crawl instead. - LoadDefaultConfig is retained (via the new GetAwsConfigOptionsForCredentials, replacing GetAwsConfigOptionsForAssumeRole) so child-account clients keep resolving ambient AWS settings. Hand-building an aws.Config drops RetryMode, RetryMaxAttempts, AppID, AccountIDEndpointMode and ConfigSources — the last of which is where service clients read FIPS/dualstack and endpoint settings, so losing it would silently disable AWS_USE_FIPS_ENDPOINT for cross-account clients only. - Once credentials refresh lazily, an STS AccessDenied on re-assume surfaces at the IAM call site through the %w-wrapped credentials chain, where the fail-soft guards would mistake it for a resource-level denial and skip. That turns a credentials outage into a sync reporting success with grants silently missing, which reads downstream as revoked access. isCredentialsRetrievalError excludes those errors so credential problems stay loud. Also fixes isAccessDeniedError, which only matched "AccessDenied". SSO Admin returns a typed AccessDeniedException whose ErrorCode() is "AccessDeniedException", so the fail-soft skips at permission_set.go and inline_policy.go were dead code and failed the sync instead of degrading. Verified against a live tenant: before the change the sync exits 1 on ssoadmin.GetInlinePolicyForPermissionSet; after, it completes with 8 skips logged as warnings. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/connector/aws_client_factory.go | 47 +++- pkg/connector/aws_client_factory_test.go | 293 +++++++++++++++++++++++ pkg/connector/connector.go | 21 +- pkg/connector/helpers.go | 60 ++++- 4 files changed, 404 insertions(+), 17 deletions(-) create mode 100644 pkg/connector/aws_client_factory_test.go diff --git a/pkg/connector/aws_client_factory.go b/pkg/connector/aws_client_factory.go index fd74ea7b..fcf9cf57 100644 --- a/pkg/connector/aws_client_factory.go +++ b/pkg/connector/aws_client_factory.go @@ -8,13 +8,17 @@ import ( awsSdk "github.com/aws/aws-sdk-go-v2/aws" awsConfig "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/credentials/stscreds" "github.com/aws/aws-sdk-go-v2/service/iam" awsOrgs "github.com/aws/aws-sdk-go-v2/service/organizations" - "github.com/aws/aws-sdk-go-v2/service/sts" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" ) +// crossAccountRoleSessionName identifies baton's cross-account sessions in the +// target account's CloudTrail. +const crossAccountRoleSessionName = "BatonCrossAccountSession" + type AWSClientFactory struct { mutex sync.Mutex @@ -22,6 +26,10 @@ type AWSClientFactory struct { baseClient *http.Client aws *AWS + // stsClientFn resolves the STS client used to assume into child accounts. It is a + // field so tests can inject a fake; production always uses (*AWS).getSTSClient. + stsClientFn func(ctx context.Context) (stscreds.AssumeRoleAPIClient, error) + // Map for accountId iamClientMap map[string]*iam.Client orgClientMap map[string]*awsOrgs.Client @@ -35,30 +43,53 @@ func NewAWSClientFactory(config Config, aws *AWS, baseClient *http.Client) *AWSC iamClientMap: make(map[string]*iam.Client), orgClientMap: make(map[string]*awsOrgs.Client), aws: aws, + stsClientFn: func(ctx context.Context) (stscreds.AssumeRoleAPIClient, error) { + return aws.getSTSClient(ctx) + }, } } +// getConfig builds an aws config for a child account backed by auto-refreshing +// assumed-role credentials. +// +// The credentials MUST be refreshing rather than a one-shot AssumeRole wrapped in a +// static provider: assumed-role sessions are capped at 1 hour (role chaining caps there, +// and an AssumeRole call with no DurationSeconds defaults there anyway), while clients +// built here are cached for the lifetime of the process and created eagerly at +// account-listing time. Any sync where more than an hour elapses between creation and use +// died mid-sync with `ExpiredToken`, and the cached client then failed every subsequent +// sync until the process restarted. aws.NewCredentialsCache re-assumes on expiry, which +// makes caching the client safe. This mirrors the connector's own credentials +// (see (*AWS).getCallingConfig). +// +// LoadDefaultConfig is retained deliberately: it is what lets child-account clients pick +// up ambient AWS settings (retry mode/attempts, FIPS and dualstack via ConfigSources, +// endpoint mode, app id). Hand-building an awsSdk.Config here would silently drop them. func (f *AWSClientFactory) getConfig(ctx context.Context, accountId string) (awsSdk.Config, error) { l := ctxzap.Extract(ctx) roleArn := fmt.Sprintf("arn:aws:iam::%s:role/%s", accountId, f.config.IamAssumeRoleName) - stsClient, err := f.aws.getSTSClient(ctx) + stsClient, err := f.stsClientFn(ctx) if err != nil { return awsSdk.Config{}, fmt.Errorf("baton-aws: getSTSClient failed: %w", err) } - output, err := stsClient.AssumeRole(ctx, &sts.AssumeRoleInput{ - RoleArn: awsSdk.String(roleArn), - RoleSessionName: awsSdk.String("BatonCrossAccountSession"), - }) + creds := awsSdk.NewCredentialsCache( + stscreds.NewAssumeRoleProvider(stsClient, roleArn, func(aro *stscreds.AssumeRoleOptions) { + aro.RoleSessionName = crossAccountRoleSessionName + }), + ) - if err != nil { + // stscreds providers are lazy — constructing one issues no API call. Retrieve once so + // this still reports whether the role is assumable at all, which callers rely on to + // skip inaccessible accounts (see accountIAMResourceType.parseAssumeRole). + if _, err := creds.Retrieve(ctx); err != nil { l.Warn("Failed to assume role", zap.Error(err), zap.String("roleArn", roleArn)) return awsSdk.Config{}, err } - opts := GetAwsConfigOptionsForAssumeRole(output, f.baseClient, f.config) + opts := GetAwsConfigOptionsForCredentials(creds, f.baseClient, f.config) baseConfig, err := awsConfig.LoadDefaultConfig(ctx, opts...) if err != nil { diff --git a/pkg/connector/aws_client_factory_test.go b/pkg/connector/aws_client_factory_test.go new file mode 100644 index 00000000..11f09ba4 --- /dev/null +++ b/pkg/connector/aws_client_factory_test.go @@ -0,0 +1,293 @@ +package connector + +import ( + "context" + "errors" + "fmt" + "net/http" + "sync" + "testing" + "time" + + awsSdk "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/credentials/stscreds" + awsIam "github.com/aws/aws-sdk-go-v2/service/iam" + awsOrgs "github.com/aws/aws-sdk-go-v2/service/organizations" + awsSsoAdminTypes "github.com/aws/aws-sdk-go-v2/service/ssoadmin/types" + "github.com/aws/aws-sdk-go-v2/service/sts" + stsTypes "github.com/aws/aws-sdk-go-v2/service/sts/types" + smithy "github.com/aws/smithy-go" + "github.com/stretchr/testify/require" +) + +// fakeSTSClient records AssumeRole calls and returns queued responses. Credentials +// expire after expiresIn so a CredentialsCache is forced to re-assume. +type fakeSTSClient struct { + mu sync.Mutex + calls int + expiresIn time.Duration + // errAfter, when > 0, makes every call at or beyond that ordinal fail with err. + errAfter int + err error +} + +func (f *fakeSTSClient) AssumeRole(ctx context.Context, in *sts.AssumeRoleInput, _ ...func(*sts.Options)) (*sts.AssumeRoleOutput, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.calls++ + + if f.errAfter > 0 && f.calls >= f.errAfter { + return nil, f.err + } + + return &sts.AssumeRoleOutput{Credentials: fakeCredentials(f.expiresIn)}, nil +} + +func (f *fakeSTSClient) callCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return f.calls +} + +func fakeCredentials(expiresIn time.Duration) *stsTypes.Credentials { + return &stsTypes.Credentials{ + AccessKeyId: awsSdk.String("AKIAFAKE"), + SecretAccessKey: awsSdk.String("secret"), + SessionToken: awsSdk.String("token"), + Expiration: awsSdk.Time(time.Now().Add(expiresIn)), + } +} + +func newTestFactory(stsClient stscreds.AssumeRoleAPIClient) *AWSClientFactory { + return &AWSClientFactory{ + mutex: sync.Mutex{}, + config: Config{GlobalRegion: "us-east-1", IamAssumeRoleName: "BatonRole"}, + baseClient: http.DefaultClient, + iamClientMap: make(map[string]*awsIam.Client), + orgClientMap: make(map[string]*awsOrgs.Client), + stsClientFn: func(ctx context.Context) (stscreds.AssumeRoleAPIClient, error) { + return stsClient, nil + }, + } +} + +// TestGetConfigCredentialsRefreshOnExpiry is the regression test for CXP-801: the +// credentials a cross-account client is built with must re-assume once the session +// expires, instead of being frozen at creation time by a static provider. +func TestGetConfigCredentialsRefreshOnExpiry(t *testing.T) { + ctx := context.Background() + // Already expired on arrival, so the second Retrieve must trigger a fresh AssumeRole. + fake := &fakeSTSClient{expiresIn: -time.Minute} + f := newTestFactory(fake) + + cfg, err := f.getConfig(ctx, "123456789012") + require.NoError(t, err) + require.Equal(t, 1, fake.callCount(), "getConfig should probe assumability exactly once") + + _, err = cfg.Credentials.Retrieve(ctx) + require.NoError(t, err) + require.Equal(t, 2, fake.callCount(), "expired credentials must trigger a re-assume, not be served stale") +} + +// TestGetConfigCredentialsCachedWhileValid guards the other direction: an unexpired +// session must not re-assume on every request. +func TestGetConfigCredentialsCachedWhileValid(t *testing.T) { + ctx := context.Background() + fake := &fakeSTSClient{expiresIn: time.Hour} + f := newTestFactory(fake) + + cfg, err := f.getConfig(ctx, "123456789012") + require.NoError(t, err) + + for range 5 { + _, err = cfg.Credentials.Retrieve(ctx) + require.NoError(t, err) + } + require.Equal(t, 1, fake.callCount(), "valid credentials should be served from cache") +} + +// TestGetConfigProbesAssumability covers what accountIAMResourceType.parseAssumeRole +// relies on: an unassumable role must surface as an error from getConfig. stscreds +// providers are lazy, so without the eager Retrieve this would silently succeed and the +// account would fail later mid-crawl instead of being skipped. +func TestGetConfigProbesAssumability(t *testing.T) { + ctx := context.Background() + fake := &fakeSTSClient{ + expiresIn: time.Hour, + errAfter: 1, + err: stsAccessDenied(), + } + f := newTestFactory(fake) + + _, err := f.getConfig(ctx, "123456789012") + require.Error(t, err, "getConfig must report a role it cannot assume") + require.Equal(t, 1, fake.callCount()) +} + +// TestGetConfigUsesStableSessionName keeps the CloudTrail session name intact. +func TestGetConfigUsesStableSessionName(t *testing.T) { + ctx := context.Background() + rec := &sessionNameRecorder{expiresIn: time.Hour} + f := newTestFactory(rec) + + _, err := f.getConfig(ctx, "123456789012") + require.NoError(t, err) + require.Equal(t, crossAccountRoleSessionName, rec.sessionName) +} + +// TestGetConfigResolvesAmbientAWSSettings pins the behavior that makes getConfig go +// through awsConfig.LoadDefaultConfig rather than hand-building an awsSdk.Config: +// child-account clients must keep resolving ambient AWS settings. ConfigSources is the +// load-bearing one — service clients read FIPS/dualstack and endpoint settings out of it +// at construction, so an empty ConfigSources silently disables AWS_USE_FIPS_ENDPOINT for +// cross-account clients. +func TestGetConfigResolvesAmbientAWSSettings(t *testing.T) { + t.Setenv("AWS_RETRY_MODE", "adaptive") + t.Setenv("AWS_MAX_ATTEMPTS", "7") + t.Setenv("AWS_SDK_UA_APP_ID", "baton-aws-probe") + + ctx := context.Background() + f := newTestFactory(&fakeSTSClient{expiresIn: time.Hour}) + + cfg, err := f.getConfig(ctx, "123456789012") + require.NoError(t, err) + + require.Equal(t, awsSdk.RetryModeAdaptive, cfg.RetryMode, "AWS_RETRY_MODE must reach child-account clients") + require.Equal(t, 7, cfg.RetryMaxAttempts, "AWS_MAX_ATTEMPTS must reach child-account clients") + require.Equal(t, "baton-aws-probe", cfg.AppID) + require.NotEmpty(t, cfg.ConfigSources, "ConfigSources must be populated so FIPS/dualstack/endpoint settings resolve") + + // The pinned fields must still win over anything ambient. + require.Equal(t, "us-east-1", cfg.Region) + require.Equal(t, http.DefaultClient, cfg.HTTPClient) +} + +// TestGetConfigPreservesRefreshingCredentialsThroughLoadDefaultConfig guards the +// interaction between the two halves of the fix: LoadDefaultConfig must hand back the +// *awsSdk.CredentialsCache it was given rather than re-wrapping or replacing it, or the +// refresh behavior would be silently lost on the way out. +func TestGetConfigPreservesRefreshingCredentialsThroughLoadDefaultConfig(t *testing.T) { + ctx := context.Background() + fake := &fakeSTSClient{expiresIn: -time.Minute} + f := newTestFactory(fake) + + cfg, err := f.getConfig(ctx, "123456789012") + require.NoError(t, err) + require.IsType(t, &awsSdk.CredentialsCache{}, cfg.Credentials, + "LoadDefaultConfig must pass the CredentialsCache through untouched") + + before := fake.callCount() + _, err = cfg.Credentials.Retrieve(ctx) + require.NoError(t, err) + require.Greater(t, fake.callCount(), before, + "credentials returned by getConfig must still re-assume on expiry") +} + +// TestGetConfigRoleARN verifies the assumed role ARN is composed from the account id +// and the configured role name. +func TestGetConfigRoleARN(t *testing.T) { + ctx := context.Background() + rec := &sessionNameRecorder{expiresIn: time.Hour} + f := newTestFactory(rec) + + _, err := f.getConfig(ctx, "123456789012") + require.NoError(t, err) + require.Equal(t, "arn:aws:iam::123456789012:role/BatonRole", rec.roleARN) +} + +type sessionNameRecorder struct { + expiresIn time.Duration + sessionName string + roleARN string +} + +func (r *sessionNameRecorder) AssumeRole(ctx context.Context, in *sts.AssumeRoleInput, _ ...func(*sts.Options)) (*sts.AssumeRoleOutput, error) { + r.sessionName = awsSdk.ToString(in.RoleSessionName) + r.roleARN = awsSdk.ToString(in.RoleArn) + return &sts.AssumeRoleOutput{Credentials: fakeCredentials(r.expiresIn)}, nil +} + +// TestIsAccessDeniedErrorSSOAdmin covers the second half of CXP-801: SSO Admin returns a +// typed AccessDeniedException whose code is "AccessDeniedException", which the old +// "AccessDenied"-only comparison missed, leaving the fail-soft skips in permission_set.go +// and inline_policy.go dead. +func TestIsAccessDeniedErrorSSOAdmin(t *testing.T) { + t.Run("iam unmodeled AccessDenied", func(t *testing.T) { + require.True(t, isAccessDeniedError(iamAccessDenied())) + }) + + t.Run("sso admin typed AccessDeniedException", func(t *testing.T) { + err := &awsSsoAdminTypes.AccessDeniedException{Message: awsSdk.String("nope")} + require.True(t, isAccessDeniedError(wrapAsOperationError("SSO Admin", "ListManagedPoliciesInPermissionSet", err))) + }) + + t.Run("unrelated error", func(t *testing.T) { + require.False(t, isAccessDeniedError(errors.New("boom"))) + }) + + t.Run("different api error code", func(t *testing.T) { + err := &smithy.GenericAPIError{Code: "ValidationException", Message: "bad input"} + require.False(t, isAccessDeniedError(wrapAsOperationError("IAM", "ListGroups", err))) + }) + + t.Run("nil", func(t *testing.T) { + require.False(t, isAccessDeniedError(nil)) + }) +} + +// TestCredentialsFailureIsNotTreatedAsResourceDenial is the guard against the regression +// the refreshing-credentials fix would otherwise introduce. Once credentials re-assume +// lazily, an STS AccessDenied surfaces at the IAM call site through the credentials +// chain. If isAccessDeniedError matched it, every fail-soft skip would swallow a +// credentials outage and the sync would report success with grants silently missing — +// which reads downstream as revoked access. +func TestCredentialsFailureIsNotTreatedAsResourceDenial(t *testing.T) { + // Mirrors the real wrap chain: + // IAM OperationError → "get identity: %w" → "get credentials: %w" → STS OperationError + stsErr := wrapAsOperationError(sts.ServiceID, "AssumeRole", &smithy.GenericAPIError{ + Code: errCodeAccessDenied, + Message: "User is not authorized to perform: sts:AssumeRole", + }) + credsErr := fmt.Errorf("get identity: %w", fmt.Errorf("get credentials: %w", stsErr)) + iamErr := wrapAsOperationError(awsIam.ServiceID, "ListAttachedUserPolicies", credsErr) + + require.True(t, isCredentialsRetrievalError(iamErr), + "an STS error nested under an IAM operation must be recognized as a credentials failure") + require.False(t, isAccessDeniedError(iamErr), + "a credentials failure must NOT be skippable — it has to fail the sync loudly") +} + +// TestIsCredentialsRetrievalErrorIgnoresResourceDenials ensures a genuine IAM denial is +// still skippable, i.e. the new guard is not over-broad. +func TestIsCredentialsRetrievalErrorIgnoresResourceDenials(t *testing.T) { + err := iamAccessDenied() + require.False(t, isCredentialsRetrievalError(err)) + require.True(t, isAccessDeniedError(err)) +} + +// TestIsCredentialsRetrievalErrorDirectSTSCall documents that a denial raised by a direct +// STS call (not through the credentials chain) is also classified as a credentials +// failure. baton-aws only calls STS to obtain credentials, so this is the correct read. +func TestIsCredentialsRetrievalErrorDirectSTSCall(t *testing.T) { + err := stsAccessDenied() + require.True(t, isCredentialsRetrievalError(err)) + require.False(t, isAccessDeniedError(err)) +} + +func iamAccessDenied() error { + return wrapAsOperationError(awsIam.ServiceID, "ListAttachedUserPolicies", &smithy.GenericAPIError{ + Code: errCodeAccessDenied, + Message: "not authorized", + }) +} + +func stsAccessDenied() error { + return wrapAsOperationError(sts.ServiceID, "AssumeRole", &smithy.GenericAPIError{ + Code: errCodeAccessDenied, + Message: "not authorized to assume role", + }) +} + +func wrapAsOperationError(serviceID, op string, err error) error { + return &smithy.OperationError{ServiceID: serviceID, OperationName: op, Err: err} +} diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index af79a8e7..8599d415 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -611,18 +611,23 @@ func GetAwsConfigOptions(httpClient *http.Client, config Config) []func(*awsConf return opts } -func GetAwsConfigOptionsForAssumeRole(output *sts.AssumeRoleOutput, httpClient *http.Client, config Config) []func(*awsConfig.LoadOptions) error { +// GetAwsConfigOptionsForCredentials builds load options for a config backed by an +// explicit credentials provider. +// +// These are passed to awsConfig.LoadDefaultConfig rather than used to hand-build an +// awsSdk.Config so that cross-account clients keep resolving ambient AWS settings — +// AWS_RETRY_MODE, AWS_MAX_ATTEMPTS, AWS_SDK_UA_APP_ID, request-compression settings, +// AccountIDEndpointMode, and (via ConfigSources) FIPS / dualstack / endpoint resolution. +// Only credentials, region, HTTP client, and defaults mode are pinned here. +// +// The provider is expected to be an *awsSdk.CredentialsCache; LoadDefaultConfig passes +// one through untouched instead of re-wrapping it, preserving its refresh behavior. +func GetAwsConfigOptionsForCredentials(creds awsSdk.CredentialsProvider, httpClient *http.Client, config Config) []func(*awsConfig.LoadOptions) error { opts := []func(*awsConfig.LoadOptions) error{ awsConfig.WithHTTPClient(httpClient), awsConfig.WithRegion(config.GlobalRegion), awsConfig.WithDefaultsMode(awsSdk.DefaultsModeInRegion), - awsConfig.WithCredentialsProvider( - credentials.NewStaticCredentialsProvider( - *output.Credentials.AccessKeyId, - *output.Credentials.SecretAccessKey, - *output.Credentials.SessionToken, - ), - ), + awsConfig.WithCredentialsProvider(creds), } return opts diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index f5101189..528dca0e 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -14,6 +14,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws/arn" aws_middleware "github.com/aws/aws-sdk-go-v2/aws/middleware" awsIdentityStoreTypes "github.com/aws/aws-sdk-go-v2/service/identitystore/types" + "github.com/aws/aws-sdk-go-v2/service/sts" smithy "github.com/aws/smithy-go" "github.com/aws/smithy-go/middleware" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -23,6 +24,14 @@ import ( "google.golang.org/protobuf/proto" ) +const ( + // errCodeAccessDenied is the unmodeled code IAM (and STS) return for an authorization + // failure; errCodeAccessDeniedException is the code carried by service-modeled + // AccessDeniedException types, e.g. SSO Admin's. + errCodeAccessDenied = "AccessDenied" + errCodeAccessDeniedException = "AccessDeniedException" +) + const ( MembershipEntitlementIDTemplate = "%s:%s:member" V1MembershipEntitlementIDTemplate = "membership:%s" @@ -428,13 +437,62 @@ func wrapAWSError(err error) error { return err } +// isCredentialsRetrievalError reports whether err originated in an STS credential +// retrieval rather than in the API call the caller actually made. +// +// Cross-account clients hold auto-refreshing assumed-role credentials, so when a +// re-assume fails mid-sync (role deleted, trust policy edited, SCP change) STS returns +// AccessDenied and the SDK surfaces it at the call site — the whole chain is %w-wrapped +// and reachable by errors.As: +// +// iam. err → smithy.OperationError{ServiceID:"IAM"} → "get identity: %w" +// → "get credentials: %w" → CredentialsCache → AssumeRoleProvider +// → smithy.OperationError{ServiceID:"STS"} → GenericAPIError{Code:"AccessDenied"} +// +// Without this check isAccessDeniedError matches that error and the fail-soft skips +// treat a credentials outage as "this resource denied us", producing a sync that reports +// success with grants silently missing. Absent grants read as revoked access downstream, +// so a partial sync is worse than a hard failure: credential problems must be loud. +// +// A plain errors.As is not enough: it matches the OUTERMOST *smithy.OperationError, which +// is the caller's own service (IAM), and would never see the STS one nested beneath it. +// So each match that is not STS is stepped past, and the search resumes underneath it. +func isCredentialsRetrievalError(err error) bool { + for e := err; e != nil; { + var opErr *smithy.OperationError + if !errors.As(e, &opErr) { + return false + } + if opErr.ServiceID == sts.ServiceID { + return true + } + e = errors.Unwrap(opErr) + } + return false +} + +// isAccessDeniedError reports whether err is a resource-level authorization denial that +// the caller may safely skip. +// +// Both spellings are matched: IAM models no AccessDenied* error type and returns an +// unmodeled GenericAPIError with code "AccessDenied", while SSO Admin returns a typed +// *ssoadmin.AccessDeniedException whose ErrorCode() is "AccessDeniedException". Matching +// only the former left the fail-soft skips in permission_set.go and inline_policy.go dead +// for SSO Admin, failing the sync instead of degrading. +// +// Credential-retrieval failures are explicitly excluded — see isCredentialsRetrievalError. func isAccessDeniedError(err error) bool { + if isCredentialsRetrievalError(err) { + return false + } + var apiErr smithy.APIError if !errors.As(err, &apiErr) { return false } + switch apiErr.ErrorCode() { - case "AccessDenied", "AccessDeniedException": + case errCodeAccessDenied, errCodeAccessDeniedException: return true default: return false From 3a8c5917c9036923ba6215a853d055e47a672769 Mon Sep 17 00:00:00 2001 From: agustin-conductor Date: Fri, 31 Jul 2026 14:55:57 -0300 Subject: [PATCH 2/2] fix(connector): pin cross-account session duration and refresh window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 7c7c621d, addressing three findings from review of that change. **Session duration.** stscreds does not inherit the STS API's 1-hour default. AssumeRoleProvider.Retrieve back-fills an unset Duration with stscreds.DefaultDuration (15 minutes) and then always sends DurationSeconds, so switching to a refreshing provider silently quartered cross-account session length and quadrupled AssumeRole volume against every child account — on exactly the large-Organization deployment CXP-801 was filed from. The replaced code sent no DurationSeconds and so received 3600s. aro.Duration now says 3600s explicitly, restoring parity. One hour is also the ceiling: role chaining (which both assume-role modes use) rejects only DurationSeconds above 3600, and where chaining does not apply the target role's MaxSessionDuration governs, whose minimum permitted value is itself 3600 — so it is accepted in every auth mode. **Refresh window.** The cache was built with the SDK default ExpiryWindow of 0, which serves credentials until the exact expiry instant. A request issued in the final round-trip is signed with credentials that expire in flight and comes back 403 ExpiredToken, which is absent from the SDK's retryable error codes (only RequestTimeout*, the throttle codes, and three clock-skew codes are retried), so it fails the sync task rather than being retried. A 5-minute window removes the race; at 1-hour sessions the cost is a refresh every ~55 minutes instead of 60. This is not a regression from 7c7c621d — the cross-account path only acquires expiry boundaries by refreshing at all, where before it failed permanently — but it is cheap to close while the path is being fixed. **Test hermeticity.** getConfig goes through awsConfig.LoadDefaultConfig, which reads the ambient environment and shared config files, so the tests added in 7c7c621d inherited whatever AWS configuration the host had. Six of seven failed with "failed to get shared config profile" on any machine with AWS_PROFILE set. isolateAWSEnv neutralizes profile and credential-file resolution and is wired into newTestFactory, so every getConfig test is isolated without per-test setup. Both new guards are mutation-tested: dropping aro.Duration fails TestGetConfigRequestsOneHourSessions with 900 != 3600, and dropping the ExpiryWindow option fails TestGetConfigRefreshesBeforeExpiry. The duration assertion deliberately checks the value on the wire rather than the constant, since asserting the constant would still pass if aro.Duration were removed. Verified against a live 2-account Organization: sync completes with exit 0, no ExpiredToken, no DurationSeconds or MaxSessionDuration rejection, and the same one denied account and eight SSO Admin fail-soft skips as before. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/connector/aws_client_factory.go | 28 +++++--- pkg/connector/aws_client_factory_test.go | 83 ++++++++++++++++++++---- 2 files changed, 92 insertions(+), 19 deletions(-) diff --git a/pkg/connector/aws_client_factory.go b/pkg/connector/aws_client_factory.go index fcf9cf57..e60525cc 100644 --- a/pkg/connector/aws_client_factory.go +++ b/pkg/connector/aws_client_factory.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "sync" + "time" awsSdk "github.com/aws/aws-sdk-go-v2/aws" awsConfig "github.com/aws/aws-sdk-go-v2/config" @@ -19,6 +20,14 @@ import ( // target account's CloudTrail. const crossAccountRoleSessionName = "BatonCrossAccountSession" +// crossAccountSessionDuration is explicit because stscreds defaults to 15 minutes rather +// than inheriting the STS API's 1 hour; 1h is also the maximum role chaining permits. +const crossAccountSessionDuration = time.Hour + +// crossAccountCredentialRefreshWindow re-assumes this far before real expiry, so no request +// is signed with credentials that expire in flight (ExpiredToken is not retried). +const crossAccountCredentialRefreshWindow = 5 * time.Minute + type AWSClientFactory struct { mutex sync.Mutex @@ -53,14 +62,13 @@ func NewAWSClientFactory(config Config, aws *AWS, baseClient *http.Client) *AWSC // assumed-role credentials. // // The credentials MUST be refreshing rather than a one-shot AssumeRole wrapped in a -// static provider: assumed-role sessions are capped at 1 hour (role chaining caps there, -// and an AssumeRole call with no DurationSeconds defaults there anyway), while clients -// built here are cached for the lifetime of the process and created eagerly at -// account-listing time. Any sync where more than an hour elapses between creation and use -// died mid-sync with `ExpiredToken`, and the cached client then failed every subsequent -// sync until the process restarted. aws.NewCredentialsCache re-assumes on expiry, which -// makes caching the client safe. This mirrors the connector's own credentials -// (see (*AWS).getCallingConfig). +// static provider: assumed-role sessions here last at most 1 hour (see +// crossAccountSessionDuration), while clients built here are cached for the lifetime of +// the process and created eagerly at account-listing time. Any sync where more than an +// hour elapsed between creation and use died mid-sync with `ExpiredToken`, and the cached +// client then failed every subsequent sync until the process restarted. +// aws.NewCredentialsCache re-assumes on expiry, which makes caching the client safe. This +// mirrors the connector's own credentials (see (*AWS).getCallingConfig). // // LoadDefaultConfig is retained deliberately: it is what lets child-account clients pick // up ambient AWS settings (retry mode/attempts, FIPS and dualstack via ConfigSources, @@ -78,7 +86,11 @@ func (f *AWSClientFactory) getConfig(ctx context.Context, accountId string) (aws creds := awsSdk.NewCredentialsCache( stscreds.NewAssumeRoleProvider(stsClient, roleArn, func(aro *stscreds.AssumeRoleOptions) { aro.RoleSessionName = crossAccountRoleSessionName + aro.Duration = crossAccountSessionDuration }), + func(o *awsSdk.CredentialsCacheOptions) { + o.ExpiryWindow = crossAccountCredentialRefreshWindow + }, ) // stscreds providers are lazy — constructing one issues no API call. Retrieve once so diff --git a/pkg/connector/aws_client_factory_test.go b/pkg/connector/aws_client_factory_test.go index 11f09ba4..00e87eef 100644 --- a/pkg/connector/aws_client_factory_test.go +++ b/pkg/connector/aws_client_factory_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "os" "sync" "testing" "time" @@ -58,7 +59,27 @@ func fakeCredentials(expiresIn time.Duration) *stsTypes.Credentials { } } -func newTestFactory(stsClient stscreds.AssumeRoleAPIClient) *AWSClientFactory { +// isolateAWSEnv detaches the process's ambient AWS configuration for the duration of a +// test. getConfig goes through awsConfig.LoadDefaultConfig, which reads the environment +// and the shared config files, so without this a developer machine with AWS_PROFILE set +// fails these tests with "failed to get shared config profile" instead of exercising the +// code under test. Only profile/credential-file resolution is neutralized — tests that +// assert on ambient settings set their own AWS_* vars on top of this. +func isolateAWSEnv(t *testing.T) { + t.Helper() + t.Setenv("AWS_PROFILE", "") + t.Setenv("AWS_DEFAULT_PROFILE", "") + t.Setenv("AWS_CONFIG_FILE", os.DevNull) + t.Setenv("AWS_SHARED_CREDENTIALS_FILE", os.DevNull) + // Credentials are always supplied explicitly here, so a stray IMDS probe would only + // add latency and a dependency on the host. + t.Setenv("AWS_EC2_METADATA_DISABLED", "true") +} + +func newTestFactory(t *testing.T, stsClient stscreds.AssumeRoleAPIClient) *AWSClientFactory { + t.Helper() + isolateAWSEnv(t) + return &AWSClientFactory{ mutex: sync.Mutex{}, config: Config{GlobalRegion: "us-east-1", IamAssumeRoleName: "BatonRole"}, @@ -78,7 +99,7 @@ func TestGetConfigCredentialsRefreshOnExpiry(t *testing.T) { ctx := context.Background() // Already expired on arrival, so the second Retrieve must trigger a fresh AssumeRole. fake := &fakeSTSClient{expiresIn: -time.Minute} - f := newTestFactory(fake) + f := newTestFactory(t, fake) cfg, err := f.getConfig(ctx, "123456789012") require.NoError(t, err) @@ -94,7 +115,7 @@ func TestGetConfigCredentialsRefreshOnExpiry(t *testing.T) { func TestGetConfigCredentialsCachedWhileValid(t *testing.T) { ctx := context.Background() fake := &fakeSTSClient{expiresIn: time.Hour} - f := newTestFactory(fake) + f := newTestFactory(t, fake) cfg, err := f.getConfig(ctx, "123456789012") require.NoError(t, err) @@ -106,6 +127,26 @@ func TestGetConfigCredentialsCachedWhileValid(t *testing.T) { require.Equal(t, 1, fake.callCount(), "valid credentials should be served from cache") } +// TestGetConfigRefreshesBeforeExpiry pins the refresh window. Credentials whose remaining +// life falls inside crossAccountCredentialRefreshWindow must be re-assumed rather than +// served, because a request signed with them can reach AWS after they expire and come back +// 403 ExpiredToken — which is absent from the SDK's retryable error codes. +func TestGetConfigRefreshesBeforeExpiry(t *testing.T) { + ctx := context.Background() + // Still valid, but well inside the refresh window. + fake := &fakeSTSClient{expiresIn: crossAccountCredentialRefreshWindow / 2} + f := newTestFactory(t, fake) + + cfg, err := f.getConfig(ctx, "123456789012") + require.NoError(t, err) + + before := fake.callCount() + _, err = cfg.Credentials.Retrieve(ctx) + require.NoError(t, err) + require.Greater(t, fake.callCount(), before, + "credentials expiring inside the refresh window must be re-assumed, not signed with") +} + // TestGetConfigProbesAssumability covers what accountIAMResourceType.parseAssumeRole // relies on: an unassumable role must surface as an error from getConfig. stscreds // providers are lazy, so without the eager Retrieve this would silently succeed and the @@ -117,7 +158,7 @@ func TestGetConfigProbesAssumability(t *testing.T) { errAfter: 1, err: stsAccessDenied(), } - f := newTestFactory(fake) + f := newTestFactory(t, fake) _, err := f.getConfig(ctx, "123456789012") require.Error(t, err, "getConfig must report a role it cannot assume") @@ -128,7 +169,7 @@ func TestGetConfigProbesAssumability(t *testing.T) { func TestGetConfigUsesStableSessionName(t *testing.T) { ctx := context.Background() rec := &sessionNameRecorder{expiresIn: time.Hour} - f := newTestFactory(rec) + f := newTestFactory(t, rec) _, err := f.getConfig(ctx, "123456789012") require.NoError(t, err) @@ -147,7 +188,7 @@ func TestGetConfigResolvesAmbientAWSSettings(t *testing.T) { t.Setenv("AWS_SDK_UA_APP_ID", "baton-aws-probe") ctx := context.Background() - f := newTestFactory(&fakeSTSClient{expiresIn: time.Hour}) + f := newTestFactory(t, &fakeSTSClient{expiresIn: time.Hour}) cfg, err := f.getConfig(ctx, "123456789012") require.NoError(t, err) @@ -169,7 +210,7 @@ func TestGetConfigResolvesAmbientAWSSettings(t *testing.T) { func TestGetConfigPreservesRefreshingCredentialsThroughLoadDefaultConfig(t *testing.T) { ctx := context.Background() fake := &fakeSTSClient{expiresIn: -time.Minute} - f := newTestFactory(fake) + f := newTestFactory(t, fake) cfg, err := f.getConfig(ctx, "123456789012") require.NoError(t, err) @@ -188,22 +229,42 @@ func TestGetConfigPreservesRefreshingCredentialsThroughLoadDefaultConfig(t *test func TestGetConfigRoleARN(t *testing.T) { ctx := context.Background() rec := &sessionNameRecorder{expiresIn: time.Hour} - f := newTestFactory(rec) + f := newTestFactory(t, rec) _, err := f.getConfig(ctx, "123456789012") require.NoError(t, err) require.Equal(t, "arn:aws:iam::123456789012:role/BatonRole", rec.roleARN) } +// TestGetConfigRequestsOneHourSessions pins the session length actually sent to STS. +// stscreds does not inherit the STS API's 1-hour default: AssumeRoleProvider.Retrieve +// back-fills an unset Duration with stscreds.DefaultDuration (15 minutes) and always sends +// DurationSeconds. Dropping aro.Duration would therefore quarter the session length and +// quadruple AssumeRole volume across every child account, silently and with no test +// failure — hence this assertion on the wire value rather than on the constant. +func TestGetConfigRequestsOneHourSessions(t *testing.T) { + ctx := context.Background() + rec := &sessionNameRecorder{expiresIn: time.Hour} + f := newTestFactory(t, rec) + + _, err := f.getConfig(ctx, "123456789012") + require.NoError(t, err) + require.Equal(t, int32(3600), awsSdk.ToInt32(rec.durationSeconds), + "cross-account sessions must request 3600s: the maximum role chaining permits, "+ + "and what the pre-refresh code received from the STS API default") +} + type sessionNameRecorder struct { - expiresIn time.Duration - sessionName string - roleARN string + expiresIn time.Duration + sessionName string + roleARN string + durationSeconds *int32 } func (r *sessionNameRecorder) AssumeRole(ctx context.Context, in *sts.AssumeRoleInput, _ ...func(*sts.Options)) (*sts.AssumeRoleOutput, error) { r.sessionName = awsSdk.ToString(in.RoleSessionName) r.roleARN = awsSdk.ToString(in.RoleArn) + r.durationSeconds = in.DurationSeconds return &sts.AssumeRoleOutput{Credentials: fakeCredentials(r.expiresIn)}, nil }