Skip to content

ENTERPRISE-8875 assume role list - #466

Open
gnyahay wants to merge 10 commits into
anchore:mainfrom
gnyahay:assume-role-list
Open

ENTERPRISE-8875 assume role list#466
gnyahay wants to merge 10 commits into
anchore:mainfrom
gnyahay:assume-role-list

Conversation

@gnyahay

@gnyahay gnyahay commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

ENTERPRISE-8875

Summary

Adds an assume-role list so a single agent can inventory multiple AWS
account-regions, each via its own IAM role (optionally with an external ID) and
its own region. Zero entries preserves the previous behavior (inventory the
agent's own account using the top-level region). Includes a 50-entry limit and a
startup pre-flight that fails fast if any configured role cannot be assumed;
once the poll loop is running, per-cycle failures are logged and retried rather
than exiting.

Release notes

  • --region / ANCHORE_ECS_INVENTORY_REGION is now ignored (with a warning)
    whenever assume-role entries are configured.
    Each entry supplies its own
    region; the top-level region applies only to the zero-entry (ambient) case.

Testing

make unit and make integration both pass. The integration suite runs against
a local moto server (STS + ECS) started by
test/integration/run.sh.

Coverage boundary: moto emulates STS and the ECS control plane but does
not populate a task's runtime Containers, so image extraction — the product's
actual output — is not exercised end to end over the wire. The integration tests
cover assume-role auth, multi-pass account/region isolation (each pass sees
only its own account's clusters), and report delivery to a stand-in Anchore API;
image-level extraction stays covered by the unit tests with a synthetic ECS API.

@gnyahay
gnyahay requested a review from a team as a code owner August 3, 2026 22:10
@gnyahay gnyahay changed the title Assume role list ENTERPRISE-8875 assume role list Aug 4, 2026
@gnyahay
gnyahay force-pushed the assume-role-list branch 2 times, most recently from 33d6d93 to 0d6731a Compare August 5, 2026 14:09
@gnyahay

gnyahay commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Please note that I have implemented an assumed role list limit of 50 and pre-flight checks that will fail startup if any configured roles cannot be assumed. If assumed roles stop working after the fact the agent continues to run.

@gnyahay
gnyahay force-pushed the assume-role-list branch 2 times, most recently from 0116efa to e348f2e Compare August 14, 2026 16:50

@bradleyjones bradleyjones left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posting a few findings from a review pass as inline comments (verified against a local build of this branch).

Comment thread internal/config/config.go
Comment thread cmd/root.go Outdated
Comment thread pkg/inventory/report.go
Comment thread pkg/lib.go
Comment thread pkg/lib.go Outdated
Comment thread pkg/lib.go Outdated
…gions

Add an assume-role list to the config, each entry with its own role ARN,
region, and optional external ID, so a single agent can inventory multiple
AWS accounts/regions via STS AssumeRole (same or cross-account). Zero
entries preserves the previous single-account behavior.

On startup the agent runs a pre-flight that resolves credentials for every
configured entry and exits non-zero if any cannot be assumed. Once running,
per-cycle failures are logged and retried rather than exiting, so a role
that breaks later never stops the other passes. Per-region log lines now
include the assumed role ARN.

Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
@bradleyjones

Copy link
Copy Markdown
Member

Reviewed at 344c35d. Ran both suites locally: make unit passes (70.5%, above the gate) and make integration passes (moto starts, all 3 integration tests green). I also probed moto directly to work out what it can actually prove.

The feature code looks good — building the credentials cache once and reusing it, the asymmetric startup validation, and the exit-on-startup-failure / tolerate-runtime-failure split are all correct and unusually well documented. Most of what follows is about the tests.

Verdict on the integration tests

Keep the harness, rewrite the tests. The moto setup is sound and cheap, but the three tests as written barely exercise the feature. Two of them can't fail for the reason they claim, and the headline capability — multiple passes across multiple account-regions — is never tested.

The key thing I verified: moto enforces real account isolation for assumed sessions. Seeding a cluster with base credentials (account 123456789012) and then assuming arn:aws:iam::999999999999:role/other gives an empty ListClusters, while a same-account role sees all the seeded clusters. You can also seed into separate accounts through assumed roles:

A(111111111111/us-east-1): [arn:aws:ecs:us-east-1:111111111111:cluster/acct-a-cluster]
B(222222222222/eu-west-1): [arn:aws:ecs:eu-west-1:222222222222:cluster/acct-b-cluster]

So the exact scenario this PR exists for is fully expressible in moto. The tests just don't use it.

Test by test

TestIntegration_AssumeRoleThenInventory — tautological as written. The role ARN (123456789012) is in the same account as the base credentials that seeded the cluster. If configureAssumeRole were a complete no-op and the client silently used ambient credentials, this test would still pass. The comment's claim that it "proves the assumed role discovered the seeded service" isn't supported. Fix: seed via role A into 111111111111, then assert role B in 222222222222 sees only its own cluster and not A's. That version fails loudly if the wiring breaks.

TestIntegration_ExternalIDIsAccepted — suggest deleting. Its own comment concedes moto doesn't enforce external IDs. The assertions are a strict subset of the previous test, and the wiring is already covered by TestAssumeRoleOptionsCarryARNSessionAndExternalID in report_test.go. It's a passing test that can never fail.

TestIntegration_ReportDeliveredToAnchore — mostly duplicates pkg/reporter. TestPost already covers v2 path selection, headers, and gock. The unique signal here is "a report assembled from real ECS discovery survives the JSON round trip" — worth keeping, but the moto seeding is incidental to it, and the gock.EnableNetworking() dance against a globally installed transport is fragile.

Biggest gap: multi-pass fan-out is untested end to end. preparePasses is unit-tested with a fake builder and buildInventoryPasses is table-tested, but nothing runs two real passes and asserts each report contains only its account's clusters. That's the one regression that would actually hurt in production (all passes silently hitting the same account, or cross-account leakage), it's the whole point of the PR, and moto makes it nearly free. It needs to live in package pkg to reach preparePasses/readyPass.

Worth stating the coverage boundary in the PR description. moto doesn't populate task Containers, so image extraction — the actual product output — never goes over the wire. The suite covers auth plus control-plane listing only. The file comment says this; the PR description should too, so nobody reads "integration tests" as "end to end."

Harness

  • run.sh runs go test -tags integration ./pkg/..., which re-runs the entire unit suite inside the integration job (reporter/connection/inventory unit tests all execute). Add -run 'TestIntegration' or move these into their own package.
  • ${AWS_ACCESS_KEY_ID:-test} lets a developer's real credentials and AWS_PROFILE leak into the run. Force fixed dummies, unset AWS_PROFILE AWS_SESSION_TOKEN AWS_SHARED_CREDENTIALS_FILE, and set AWS_EC2_METADATA_DISABLED=true. Harmless against moto, but avoids SSO-resolution hangs on a dev box.
  • The GO_BIN / asdf-shim fallback block reads as personal-environment scaffolding in a shared script — CI and other contributors just want go. (.tool-versions itself is fine as repo tooling.)
  • Fixed container name means two parallel runs collide. Minor.

Non-test findings

pkg/lib.go:91 — logs report the wrong region for the ambient pass. readyPass{region: pass.region} stores the configured region, not the resolved one. When the region comes from AWS_REGION or IMDS (explicitly supported, and validated at pkg/lib.go:75 via cfg.Region != ""), every log line and tracker message from pkg/lib.go:174 onward reads region="". Store cfg.Region instead.

GetInventoryReportsForRegion(cfg, region, ...) takes a region used only for log text, which can disagree with cfg.Region. Dropping the parameter and reading cfg.Region fixes the above by construction.

Duplicate helpers: passLogAttrs (pkg/lib.go) and appendAssumedRole (pkg/inventory/report.go) are the same function.

Serial fan-out vs. polling interval. Up to 20 roles x full ECS listing, all sequential, against a 300s default. A slow account can push a cycle past the interval; the ticker then just fires immediately and reports go stale with no signal. Worth a warning when a cycle exceeds the interval, or bounded parallelism.

checkAWSCredentials removal is a UX regression for the ambient path. The friendly "check ~/.aws/credentials" message is gone, so the most common misconfiguration now surfaces as a recurring ECS API error. Deliberate and documented, but consider a startup credential check for the ambient pass that warns rather than fails — that keeps the diagnostic without reintroducing the exit-on-blip behavior you were avoiding.

Release-note worthy: --region / ANCHORE_ECS_INVENTORY_REGION is now ignored (with a warning) whenever assume-role entries exist.

gnyahay and others added 3 commits August 27, 2026 11:06
Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The moto-based integration tests (make integration) had no CI workflow
running them; unit-test.yaml only runs make unit, which excludes the
integration build tag. Add a workflow that runs make integration, which
starts/stops the pinned moto server itself (Docker is preinstalled on the
GitHub-hosted ubuntu runner).

Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gnyahay and others added 6 commits August 27, 2026 13:22
The moto harness was sound but the tests barely exercised the feature:

- Rewrite the assume-role test into a real cross-account/region isolation
  check. The previous version assumed a role in the SAME account as the base
  credentials, so it would still pass if configureAssumeRole were a no-op. Now
  it seeds a cluster into account A (us-east-1) and another into account B
  (eu-west-1) via their own assumed roles and asserts each role sees ONLY its
  own cluster; moto enforces account isolation, so leakage fails loudly.
- Add a package-pkg fan-out test that drives the real preparePasses/readyPass
  orchestration across two account-regions and asserts per-pass isolation --
  the feature's headline capability, previously untested end to end.
- Delete TestIntegration_ExternalIDIsAccepted: moto does not enforce external
  IDs, its assertions were a subset of the isolation test, and the wiring is
  already covered by a unit test.

Harness (run.sh):
- Only run the integration tests (-run 'TestIntegration') instead of re-running
  the whole unit suite inside the integration job.
- Force fixed dummy credentials and clear AWS_PROFILE / AWS_SESSION_TOKEN /
  AWS_SHARED_CREDENTIALS_FILE and disable IMDS so a developer's real creds
  cannot leak into the run.
- Drop the asdf/GO_BIN shim scaffolding; just use go.
- Unique moto container name so parallel runs do not collide.

Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
…w-cycle warnings

Correctness and diagnostics fixes from the PR review, each guarded by a unit test
that fails against the old behavior:

- Log the region the AWS SDK actually resolved. preparePasses stored the
  configured region on each ready pass, so an ambient pass whose region came from
  AWS_REGION or instance metadata logged region="" on every line. Store cfg.Region,
  and have GetInventoryReportsForRegion read cfg.Region instead of taking a
  log-only region parameter that could disagree with it.
- Deduplicate the structured-log helper: passLogAttrs now delegates to the (newly
  exported) inventory.AppendAssumedRole so both packages format the assumedRole
  attribute identically.
- Warn (do not fail) when the ambient pass cannot resolve credentials at startup,
  restoring the friendly diagnostic the removed checkAWSCredentials gave without
  reintroducing exit-on-blip for the ambient path.
- Warn when an inventory cycle runs longer than the polling interval, so a serial
  fan-out silently falling behind is surfaced.

Tests: preparePasses asserts the resolved region is stored and that an ambient
credential failure warns-but-stays-ready; a cycleOverran table test; HandleReport
and Post tests that pin "Reporting results to Anchore" to the real report path so
the startup credential check can never mislabel itself again.

Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
…imit value

Add assertions for behaviors that were untested or only tested against a constant
(so they could drift silently):

- Offline mode: omitting the Anchore API details (IsValid() == false) makes the
  agent gather inventory but skip reporting -- printing to stdout unless quiet and
  never posting. Mirrors k8s-inventory's implicit offline mode; now covered for
  both quiet and non-quiet, asserting the "not reporting" warning fires and that
  it never claims to report.
- Region resolves from ANCHORE_ECS_INVENTORY_REGION and overrides the config file
  (env wins; the file value is only a fallback).
- The assume-role cap is 20: assert the error says "maximum is 20" and that
  MaxAssumeRoleEntries == 20, so the limit can't drift. The existing exceed/at-limit
  tests keyed off the constant and would have passed at any value.

Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Config validation only checked that role-arn and region were non-empty, so an
entry with embedded control characters (CR, LF, tab, ...) -- e.g. from a
copy/paste or templating slip -- passed validation and only surfaced later as a
murky STS/endpoint error during startup pre-flight. Reject them in Build() with a
clear per-entry message (role-arn / region / external-id), tested against LF, CR,
CRLF, and tab plus a clean-entry happy path.

Scoped to the assume-role fields this PR introduces. The pre-existing anchore.*
and top-level region fields are left as-is: Go's net/url and net/http already
neutralize CR/LF there (url.JoinPath errors on a bad URL; header values have CR/LF
folded to spaces; basic-auth is base64-encoded), so there is no injection -- only
differing failure modes -- and broadening validation to them is out of scope here.

Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Lock the current (unchanged) behavior for inaccessible files:
- An unwritable log file makes InitZapLogger panic (via zap.Must). Captured with a
  characterization test -- documented, not endorsed; graceful handling would be a
  separate change.
- An unreadable explicitly-configured config file returns "unable to read config",
  which cmd.InitAppConfig turns into a message + os.Exit(1).

Both skip when running as root, since root bypasses file permissions. No
production code changed.

Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
…-bomb inputs

The config surface has grown (assume-role list, quiet, dry-run, per-entry region
and external-id), so cover more of the malformed inputs an operator could hand the
loader. All fail cleanly today; these lock that:

- a binary blob supplied as the config file -> "unable to read config" (YAML parse
  error), not a crash
- an assume-role list larger than the cap, parsed from a file -> rejected via the
  full read -> unmarshal -> Build path (not just in-memory struct validation)
- a YAML "billion laughs" alias bomb -> rejected by go-yaml's alias-expansion cap,
  so a tiny file cannot blow up memory/CPU; the load returns promptly, not hangs

Tests only; no production code changed.

Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Greg Nyahay <greg.nyahay@anchore.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants