[IAmJSD]

Summary

This finishes the last open item of #263: turning TestResolveBastionProvider_LegacyFallback into an error assertion once the fallback is gone (audit finding A03-011). #1152 handled the other items. This PR removes the fallback, wires the bastion options into every AWS resolver caller, and adds the tests.

Behaviour change (owner sign-off needed)

A bastion-mode account whose target role trusts the CUDly host instead of the bastion stops working. Until now CUDly assumed every bastion account's target role directly with its own host credentials (see Root cause). So a target role whose trust policy names the CUDly host's role or account worked by accident. After this PR the target role is assumed by the bastion account. That role's AssumeRole call now fails with AccessDenied. The account test endpoint reports it as assume role denied for <role ARN>, and collection and purchases for that account fail with the same STS error.

What an affected customer has to change:

1. Target role trust policy: allow the bastion account as Principal, either arn:aws:iam::<bastion account ID>:root or the bastion's specific IAM user or role, with the same sts:ExternalId condition as before. Settings > Accounts already renders exactly this snippet for bastion mode, so they can copy it from there.
2. Bastion identity policy: the bastion's IAM user or role needs sts:AssumeRole on the target role ARN.
3. CUDly deployment grant: when the bastion itself is reached via a role (role_arn mode), the host's cross-account grant must list the bastion's account ID (cross_account_target_account_ids / CrossAccountTargetAccountIds). docs/DEPLOYMENT.md already says this. The target account no longer needs to be listed.

Setups that followed the UI snippet and docs/DEPLOYMENT.md already match this and need no change. In fact, before this PR such a setup could fail, because the host tried to assume a target account its own IAM grant does not list. Unchanged: a bastion that is the CUDly host account itself (role_arn with no role ARN), which still resolves to the host credentials after the host-identity check.

Root cause

resolveBastionProvider loads the bastion account, resolves its credentials, builds an STS client from them and assumes the target role with it, but only when AWSResolveOptions.AccountLookup and STSClientFactory are both set. When either was nil it fell back to resolveRoleARNProvider with the caller's STS client, which carries the CUDly host's credentials. No production caller set either option. Two callers used the no-options ResolveAWSCredentialProvider, and four passed only the ambient options. So every bastion account in production went through the fallback, and the bastion was never used. known_issues/03 marked this entry RESOLVED, but only the plumbing had landed.

Fix

• internal/credentials/resolver.go: resolveBastionProvider now returns an error when either option is nil, so it never falls back to the host's STS client. The bastion's own credentials are now resolved with the caller's ambient options (the bastion lookup and factory are still dropped, so a bastion can't chain to another). Without this, a bastion that is the host account would have stopped working when the fallback went.
• internal/credentials/bastion_options.go (new): AccountLookupFromStore (config store GetCloudAccount; a nil store gives a nil lookup, which fails closed) and NewSTSClientFactory(base). The factory builds an STS client from the host's AWS config (region, endpoint, HTTP client, retryer) signed with the bastion's credentials. Those credentials are wrapped in an aws.CredentialsCache (once), so a role-based bastion is not re-assumed on every call.
• Callers, all now passing both options:
  • internal/scheduler/scheduler.go: collectAWSForAccount (was the no-options call). New SchedulerConfig.BastionSTSFactory and AmbientAWSCreds, so a bastion that is the host account still works.
  • internal/purchase/execution.go / manager.go: resolveAWSProvider, with the new ManagerConfig.BastionSTSFactory.
  • internal/server/app.go: wires the factory and ambient credentials into the re-initialized manager and scheduler. newCommitmentProbeConfigBuilder now takes the account store and the host aws.Config.
  • internal/api/handler_marketplace.go, handler_accounts_awsprobe.go, handler_accounts.go (buildOrgRootAWSConfig, was the no-options call): all use the new Handler.bastionResolveOptions(base).
  • A re-grep found no other caller and no direct stscreds.NewAssumeRoleProvider outside internal/credentials.
• known_issues/03_credentials_resolver.md: the entry now records that the first fix left the fallback live, what this PR changes, the behaviour change, and the tests that lock it down.

No DB change, no migration, no concurrency surface. Each resolution builds its own STS client, and the factory only reads the base config.

Closes #263

Verification

All of this is local unit, mock and fixture evidence. No real STS calls. Callers in credentials, scheduler and purchase get in-process STS fakes. The api and server tests use the real SDK STS client against a fake HTTPClient that answers AssumeRole and records which access key signed each request (SigV4 Credential=). So they prove the target AssumeRole was signed with the bastion's keys and not the host's. All runs used AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.

New and changed tests:

Caller / unit	Test(s)
resolver	TestResolveBastionProvider_LegacyFallback (now: no options, lookup only, factory only, ambient only all error; the host STS client is never used), _TargetAssumedWithBastionSTS, _HostAccountBastion (host: ambient keys; non-host: ErrNotHostAccount, no client built), _WithBastionAndRoleARN / _NoRoleARN (moved to the options path), TestAccountLookupFromStore, TestNewSTSClientFactory_SignsWithGivenProvider
scheduler	TestScheduler_CollectAWSForAccount_BastionUsesBastionCredentials, _BastionWithoutFactoryFailsClosed, _HostAccountBastionUsesAmbient
purchase	TestResolveAWSProvider_BastionUsesBastionCredentials, _BastionWithoutFactoryFailsClosed
server probe	TestCommitmentProbeConfigBuilder_BastionAssumesTargetAsBastion, _BastionWithoutAccountStoreFailsClosed (existing _HostIdentityCheck updated for the new signature)
API marketplace + org discovery	TestAPIAWSCallers_BastionAssumesTargetAsBastion, _BastionWithoutConfigStoreFailsClosed
API account test	bastion case of TestTestAccountCredentials_AWSValidConfigsProbeCallerIdentity now asserts the AssumeRole signer is the bastion's key; the fixture store returns a real bastion row

Fail-before. I put the old behaviour back in place (legacy fallback restored, each caller back to its main options) and kept the new types and fields so the tests compiled:

go test ./internal/<pkg>/ -count=1 -run '<the tests above>' -v
--- FAIL: TestResolveBastionProvider_LegacyFallback (no_options, lookup_only, factory_only, ambient_only)
--- FAIL: TestScheduler_CollectAWSForAccount_BastionUsesBastionCredentials
--- FAIL: TestScheduler_CollectAWSForAccount_BastionWithoutFactoryFailsClosed
--- FAIL: TestResolveAWSProvider_BastionUsesBastionCredentials    (bastion STS client must be built from the bastion's own keys)
--- FAIL: TestResolveAWSProvider_BastionWithoutFactoryFailsClosed (An error is expected but got nil)
--- FAIL: TestCommitmentProbeConfigBuilder_BastionAssumesTargetAsBastion
--- FAIL: TestCommitmentProbeConfigBuilder_BastionWithoutAccountStoreFailsClosed
--- FAIL: TestAPIAWSCallers_BastionAssumesTargetAsBastion (marketplace, org_discovery_root)
--- FAIL: TestAPIAWSCallers_BastionWithoutConfigStoreFailsClosed (marketplace, org_discovery_root)
--- FAIL: TestTestAccountCredentials_AWSValidConfigsProbeCallerIdentity/bastion_with_bastion_id_and_role_ARN

A mutation probe removed AmbientProvider from the scheduler's options, and TestScheduler_CollectAWSForAccount_HostAccountBastionUsesAmbient failed (aws_role_arn is empty and no ambient credentials available).

Pass-after (b4799568):

go build ./...                                                    # ok
go vet ./... && go vet -tags integration ./...                     # ok
go test -count=1 ./internal/credentials/                           # ok
go test -count=1 ./internal/scheduler/                             # ok
go test -count=1 ./internal/purchase/                              # ok
go test -count=1 ./internal/server/                                # ok
go test -count=1 ./internal/api/                                   # ok
go test -count=1 ./internal/mocks/                                 # ok (vacuous-assertion guard included)
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/{credentials,scheduler,purchase,server,api}/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" <changed non-test files>     # silent
bash scripts/gosec-hook.sh <changed non-test files>               # exit 0

No integration tests were added: the change has no DB-level behaviour.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 400d256b = origin/main 7b86e386 plus these heads, merged in this order:

Branch	Tested head
fix/issue-243-rest	007c163
fix/issue-1206	312072f
fix/issue-111-email	6d422e2
fix/issue-111-fail-closed	3817e29
fix/issue-712-age-cap	a91120b
fix/issue-712-fargate	aeaf0d4
fix/issue-613-legacy-ids	350e837
fix/issue-120-guards	6f38ebc
fix/issue-120-shared-predicate	cb25735
fix/issue-151-account-update-keep	d20cf01
fix/issue-713-upcoming-filter	e73fdfb
fix/issue-263-bastion-opts	7861f32
fix/issue-43-fixture-ids	8cee203
fix/issue-177-makefile-walk	365a9d6
fix/issue-27-seller-not-registered	d845f66
fix/issue-313-user-columns	6d759dd
fix/issue-353-mock-panics	aac2edf
fix/issue-8-config-stamps	b548ae5
fix/issue-30-monthly-totals	f637921

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on f7c48020, which differs from this tree only by test-only commits on fix/issue-30, fix/issue-713 and fix/issue-263 that replace placeholder account-number literals with named constants:

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/analytics, internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/scheduler and internal/server all pass. Migrations 000149, 000150 and 000151 apply in order on this tree.
• Frontend: the changed jest files (approval-details, settings-accounts, api) pass (221 tests).
• Secrets: the first run flagged 9 new fixture lines (placeholder account numbers next to an account key); fixed with named constants. Re-run on this tree: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run locally (CI runs them). Nothing was applied to any cloud.

Out of scope

• The rest of test(auth): carve-out, recovery-code, deactivation and reset-token failures have no test #263 was handled by test(auth): cover remaining auth failure paths; fix invite re-send and sticky KMS signer errors #1152 and earlier PRs.
• buildOrgRootAWSConfig (org discovery) still passes no ambient options, as on main. So an org root that is a role_arn account with no role ARN still fails discovery, and so does a bastion-mode org root whose bastion is the host account. Adding ambient options there would make a host-account org root usable, which is a separate behaviour change. Left for a follow-up if wanted.
• For a bastion account with an empty target role ARN, the "aws_role_arn is required" error now comes after the bastion has loaded, so a broken bastion is reported first. The messages themselves are unchanged.
• known_issues/03_credentials_resolver.md stays in known_issues/: every entry in it is already resolved, but the file is not tracked by test(auth): carve-out, recovery-code, deactivation and reset-token failures have no test #263 alone. Moving it to known_issues/resolved/ can happen in the next sweep.

Frontend follow-up for the UI owner

None needed. The bastion trust-policy snippet in Settings > Accounts already uses the bastion account as Principal, which is what the backend now requires. Optional: release notes could point bastion users at that snippet (see the behaviour change above).

Maintainer: please mirror #263's triage labels on this PR: priority/p2, severity/medium, impact/internal, type/chore, urgency/this-quarter, effort/m, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE ac69d7a48acbe7fdf9d368e68f333dfe9f6ff6c3

Blocking:

• none

Non-blocking:

• Owner decision: the behaviour change the body flags. Bastion accounts whose target role trusts the CUDly host, rather than the bastion, start failing AssumeRole with AccessDenied on deploy. That covers collection, purchases, the account test, marketplace and org discovery. The new behaviour is the correct one, and it matches the UI snippet and docs/DEPLOYMENT.md. Two things to do with it: put it in the release notes, and after deploy run the account test on every aws_auth_mode = 'bastion' account so any that worked only by accident show up at once instead of as a silent collection gap.
• internal/credentials/bastion_options.go AccountLookupFromStore: a typed-nil store pointer wrapped in the interface passes the store == nil check and would panic on first use instead of failing closed. No production caller passes one. Noting it only for completeness.
• NewSTSClientFactory wraps the bastion provider in a fresh CredentialsCache on every factory call. So a role-based bastion is re-assumed once per resolution (per account, per run), not once per process. That's fine, but it's weaker than the comment suggests.

Attacked:

• Callers: re-grepped every AWS caller of the resolver. The scheduler, purchase, marketplace, account test, org discovery and commitment-probe callers all pass both bastion options now. There's no direct stscreds.NewAssumeRoleProvider outside internal/credentials. The pre-DB NewManager/NewScheduler in app.go have no credential store, so they never resolve accounts. The recursive bastion resolution keeps only the ambient options, so a bastion can't chain, and the bastion's own role_arn is still assumed with the host STS client, as it should be. The scheduler's self-account branch (role_arn with no ARN) runs before the resolver, so the new AmbientProvider only affects bastion recursion.
• Fail-before: restored the legacy fallback in resolveBastionProvider and the main-branch options in the scheduler, purchase and api callers. These failed: TestResolveBastionProvider_LegacyFallback, both scheduler CollectAWSForAccount_Bastion* tests, both purchase ResolveAWSProvider_Bastion* tests, TestAPIAWSCallers_Bastion* (both), TestTestAccountCredentials_AWSValidConfigsProbeCallerIdentity and TestCommitmentProbeConfigBuilder_BastionWithoutAccountStoreFailsClosed. Restored afterwards.
• Commands: go build ./...; go vet ./...; go vet -tags integration on credentials, scheduler, purchase, server and api; e2e vet. go test -race -short -count=1 on credentials, scheduler, purchase, server, api and mocks (all ok). go test -race -tags integration -count=1 on the whole of internal/server and internal/purchase, and on -run 'Bastion|Account|Credential|Marketplace|Collect' for api and scheduler (all ok).
• Closes #263 is right. test(auth): cover remaining auth failure paths; fix invite re-send and sticky KMS signer errors #1152 (merged) covered every other item and explicitly left this one.
• Head moved: the brief named 7861f325. The new head ac69d7a4 only adds the golang.org/x/net v0.62.0 bump, byte-identical to fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) #1564's. Everything above ran on ac69d7a4.

Body-stale: "Pass-after (b4799568)" is two commits behind the head. The body doesn't mention the x/net bump or the placeholder-constants test commit. Both are non-blocking.

_{Reviewer: rG4. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE ac69d7a48acbe7fdf9d368e68f333dfe9f6ff6c3

Blocking:

• none

Non-blocking:

• Follow-up: aws_bastion_id has no scope check when an account is written. createAccount and updateAccount (internal/api/handler_accounts.go) save req.AWSBastionID as sent: nothing checks that the bastion is in the caller's allowed_accounts, is an AWS account, or exists. AccountLookupFromStore is a plain GetCloudAccount. Before this PR that field did nothing at resolve time, so it didn't matter. From now on, a user scoped to account A who holds update:accounts can point A at an out-of-scope bastion B, and the scheduler, purchase path and probes will sign A's target AssumeRole with B's credentials. They need B's UUID, which is unguessable and hidden from scoped listings. The target role must also trust B with an external ID they know, or with none, as same-account roles that trust B:root often do. So I don't count it as blocking. I'd still file a follow-up: on create and update, require a bastion to be requireAccountAccess-visible to the caller, an AWS account, and not itself in bastion mode.
• Owner sign-off: the body correctly flags this as a behaviour change. Bastion targets whose trust policy names the CUDly host instead of the bastion will start failing with AccessDenied. It needs a release note pointing at the Settings > Accounts trust snippet.
• The branch still has its own copy of the x/net v0.62.0 bump (ac69d7a). Main already has it via fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) #1564, so it will drop out of the diff on the next main merge.

Attacked:

• Read every non-test change. The resolver now fails closed when AccountLookup or STSClientFactory is nil. The bastion's own creds are resolved with the host STS and ambient options only, so it can't chain, and the bastion must be found, enabled and not itself a bastion. The target is assumed with STSClientFactory(bastionCreds). Re-grepped for resolver callers and constructors. All six production ResolveAWSCredentialProvider* callers (scheduler, purchase, server probe, API marketplace, API account test, org-root discovery) now pass both options. The no-options wrapper has no production caller left, and there's no direct stscreds use outside internal/credentials. The pre-DB NewManager/NewScheduler have no cred store and are replaced in reinitializeAfterConnect, which wires the factory.
• Ran go build ./..., go vet ./... and go vet -tags integration ./...: clean. Ran go test -race -short -count=1 on credentials, scheduler, purchase, server and api: ok. Ran go test -race -tags integration -count=1 on the same five packages one at a time: all ok.
• Fail-before:
  • With internal/credentials/resolver.go put back to main's version, TestResolveBastionProvider_LegacyFallback (all 4 subtests) and _HostAccountBastion fail.
  • With the scheduler call put back to the no-options ResolveAWSCredentialProvider, TestScheduler_CollectAWSForAccount_BastionUsesBastionCredentials and _HostAccountBastionUsesAmbient fail.
  • With internal/api/handler_marketplace.go put back to main's version, TestAPIAWSCallers_BastionAssumesTargetAsBastion/marketplace fails.
  • Restored everything afterwards.
• "Closes test(auth): carve-out, recovery-code, deactivation and reset-token failures have no test #263": the other checklist items landed in merged test(auth): cover remaining auth failure paths; fix invite re-send and sticky KMS signer errors #1152: wildcard carve-out tests, the recovery-code persist failure, the DeleteUserSessions expectation, the MFA-enabled refusal, the forgot-password and reset cases, and the signer recovery. CI on this head: 39 pass, 2 skipping.

Body-stale:

• none

_{Reviewer: rH3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at ac69d7a48acbe7fdf9d368e68f333dfe9f6ff6c3 merged onto main (3ec21a0c), together with #1556 in that order (combined tree 840c9932, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on ac69d7a4 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly ac69d7a48acbe7fdf9d368e68f333dfe9f6ff6c3.