Add AWS access review

[lukkor]

No description provided.

[cubic-dev-ai]

7 issues found and verified against the latest diff

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="pkg/accessreview/drivers/aws_iam.go">

<violation number="1" location="pkg/accessreview/drivers/aws_iam.go:109">
P2: When the credential report fails to generate (transient error or an IAM permission gap), the error is swallowed and the root account is never listed. Root is not an IAM user and only appears via the report, so a report failure makes the review silently omit the account's most privileged identity plus all MFA/auth-method signals for every user, with no indication that anything is missing. Consider degrading credential signals as documented but still surfacing the report failure or the root identity so the omission is visible.</violation>

<violation number="2" location="pkg/accessreview/drivers/aws_iam.go:362">
P2: For GovCloud or China sessions, this constructs the root identity with the commercial ARN partition, so its `ExternalID` does not match the account’s canonical root ARN. Build the root ARN with the partition of the connected session.</violation>
</file>

<file name="pkg/cloud/aws/pagination.go">

<violation number="1" location="pkg/cloud/aws/pagination.go:34">
P3: This file redefines `maxPaginationPages = 500` and `ErrPaginationLimitReached` (same name, value, and error text) that already exist in `pkg/accessreview/drivers/driver.go`. The driver package imports this cloud/aws package (as `cloudaws`) in `aws_iam.go`, so the duplicate definitions can drift apart. Consolidate to a single definition, ideally reusing this exported error from the driver instead of keeping the local copy.</violation>
</file>

<file name="pkg/accessreview/drivers/aws.go">

<violation number="1" location="pkg/accessreview/drivers/aws.go:80">
P2: When a group or non-AWS policy is named `AdministratorAccess`, this mapping falsely sets `IsAdmin` to true because it loses the distinction between group names and policy identities. Preserve the policy type/ARN and match only the AWS-managed `AdministratorAccess` policy.</violation>
</file>

<file name="pkg/cloud/aws/iam.go">

<violation number="1" location="pkg/cloud/aws/iam.go:115">
P2: `ListAccountIdentities` is not used by the AWS access-review driver, so this new implementation cannot affect access-review results and now duplicates the complete IAM flow in `pkg/accessreview/drivers/aws_iam.go`. Wire the driver to this method and remove the duplicate implementation, or remove this unused API.</violation>

<violation number="2" location="pkg/cloud/aws/iam.go:126">
P2: When the credential report is unavailable, the root identity disappears even though the method intentionally returns other users with unknown credential signals. Append a root skeleton with nil credential fields independently, then overlay report values when the report succeeds.</violation>

<violation number="3" location="pkg/cloud/aws/iam.go:383">
P2: For GovCloud and China sessions, this constructs the root external ID with the commercial `aws` partition. Build the root ARN using the session's actual partition or preserve the root ARN from the credential report.</violation>
</file>

<sub>Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: For GovCloud or China sessions, this constructs the root identity with the commercial ARN partition, so its ExternalID does not match the account’s canonical root ARN. Build the root ARN with the partition of the connected session.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/aws_iam.go, line 362:

<comment>For GovCloud or China sessions, this constructs the root identity with the commercial ARN partition, so its `ExternalID` does not match the account’s canonical root ARN. Build the root ARN with the partition of the connected session.</comment>

<file context>
@@ -0,0 +1,401 @@
+	}
+
+	return iamUser{
+		ARN:             arn.IAM(accountID, "root"),
+		Name:            rootUser,
+		MFAEnabled:      aws.Bool(row.mfaActive),
</file context>

[cubic-dev-ai]

P2: When a group or non-AWS policy is named AdministratorAccess, this mapping falsely sets IsAdmin to true because it loses the distinction between group names and policy identities. Preserve the policy type/ARN and match only the AWS-managed AdministratorAccess policy.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/aws.go, line 80:

<comment>When a group or non-AWS policy is named `AdministratorAccess`, this mapping falsely sets `IsAdmin` to true because it loses the distinction between group names and policy identities. Preserve the policy type/ARN and match only the AWS-managed `AdministratorAccess` policy.</comment>

<file context>
@@ -0,0 +1,143 @@
+	return AccountRecord{
+		FullName:    user.Name,
+		Roles:       grants,
+		IsAdmin:     awsIsAdmin(grants),
+		MFAStatus:   awsMFAStatus(user.MFAEnabled),
+		AuthMethod:  iamAuthMethod(user),
</file context>

[cubic-dev-ai]

P2: ListAccountIdentities is not used by the AWS access-review driver, so this new implementation cannot affect access-review results and now duplicates the complete IAM flow in pkg/accessreview/drivers/aws_iam.go. Wire the driver to this method and remove the duplicate implementation, or remove this unused API.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/cloud/aws/iam.go, line 115:

<comment>`ListAccountIdentities` is not used by the AWS access-review driver, so this new implementation cannot affect access-review results and now duplicates the complete IAM flow in `pkg/accessreview/drivers/aws_iam.go`. Wire the driver to this method and remove the duplicate implementation, or remove this unused API.</comment>

<file context>
@@ -0,0 +1,437 @@
+// iam:GetCredentialReport returns MFA and credential activity for all of them
+// in one CSV. Asking per user instead would multiply the call count by the user
+// count and outgrow the access-review fetch budget on any real organization.
+func (s *Session) ListAccountIdentities(ctx context.Context) (AccountIdentities, error) {
+	client := s.iam()
+
</file context>

[cubic-dev-ai]

P2: For GovCloud and China sessions, this constructs the root external ID with the commercial aws partition. Build the root ARN using the session's actual partition or preserve the root ARN from the credential report.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/cloud/aws/iam.go, line 383:

<comment>For GovCloud and China sessions, this constructs the root external ID with the commercial `aws` partition. Build the root ARN using the session's actual partition or preserve the root ARN from the credential report.</comment>

<file context>
@@ -0,0 +1,437 @@
+	}
+
+	return IAMUser{
+		ARN:             arn.IAM(accountID, "root"),
+		Name:            rootUser,
+		MFAEnabled:      aws.Bool(row.mfaActive),
</file context>

[cubic-dev-ai]

P2: When the credential report is unavailable, the root identity disappears even though the method intentionally returns other users with unknown credential signals. Append a root skeleton with nil credential fields independently, then overlay report values when the report succeeds.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/cloud/aws/iam.go, line 126:

<comment>When the credential report is unavailable, the root identity disappears even though the method intentionally returns other users with unknown credential signals. Append a root skeleton with nil credential fields independently, then overlay report values when the report succeeds.</comment>

<file context>
@@ -0,0 +1,437 @@
+	// The credential report only decorates identities that were already
+	// listed. A refusal or a report still building leaves the three-valued
+	// credential fields nil, which the driver reports as "no signal".
+	if report, err := credentialReport(ctx, client); err == nil {
+		applyCredentialReport(users, report)
+
</file context>

[cubic-dev-ai]

P2: When the credential report fails to generate (transient error or an IAM permission gap), the error is swallowed and the root account is never listed. Root is not an IAM user and only appears via the report, so a report failure makes the review silently omit the account's most privileged identity plus all MFA/auth-method signals for every user, with no indication that anything is missing. Consider degrading credential signals as documented but still surfacing the report failure or the root identity so the omission is visible.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/aws_iam.go, line 109:

<comment>When the credential report fails to generate (transient error or an IAM permission gap), the error is swallowed and the root account is never listed. Root is not an IAM user and only appears via the report, so a report failure makes the review silently omit the account's most privileged identity plus all MFA/auth-method signals for every user, with no indication that anything is missing. Consider degrading credential signals as documented but still surfacing the report failure or the root identity so the omission is visible.</comment>

<file context>
@@ -0,0 +1,401 @@
+	// The credential report only decorates identities that were already
+	// listed. A refusal or a report still building leaves the three-valued
+	// credential fields nil, which the driver reports as "no signal".
+	if report, err := credentialReport(ctx, client); err == nil {
+		applyCredentialReport(users, report)
+
</file context>

[cubic-dev-ai]

P3: This file redefines maxPaginationPages = 500 and ErrPaginationLimitReached (same name, value, and error text) that already exist in pkg/accessreview/drivers/driver.go. The driver package imports this cloud/aws package (as cloudaws) in aws_iam.go, so the duplicate definitions can drift apart. Consolidate to a single definition, ideally reusing this exported error from the driver instead of keeping the local copy.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/cloud/aws/pagination.go, line 34:

<comment>This file redefines `maxPaginationPages = 500` and `ErrPaginationLimitReached` (same name, value, and error text) that already exist in `pkg/accessreview/drivers/driver.go`. The driver package imports this cloud/aws package (as `cloudaws`) in `aws_iam.go`, so the duplicate definitions can drift apart. Consolidate to a single definition, ideally reusing this exported error from the driver instead of keeping the local copy.</comment>

<file context>
@@ -0,0 +1,34 @@
+// without reaching the end of the result set. It is an error rather than a
+// truncated success: a partial account list reads as "nobody else has access",
+// which is the wrong answer to hand a reviewer.
+var ErrPaginationLimitReached = fmt.Errorf("pagination limit of %d pages reached", maxPaginationPages)
</file context>

[cubic-dev-ai]

5 existing issues remain and 5 new issues found across 16 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="pkg/accessreview/drivers/aws.go">

<violation number="1" location="pkg/accessreview/drivers/aws.go:77">
P2: Every AWS record leaves `Active` unknown, but the access-review upsert converts nil to `true`, misleading reviewers about IAM account status. Preserve NULL for unknown `Active` values through persistence.</violation>
</file>

<file name="pkg/cloud/aws/iam.go">

<violation number="1" location="pkg/cloud/aws/iam.go:191">
P2: When authorization details require exactly `maxPaginationPages` pages, this returns `ErrPaginationLimitReached` despite having fetched the complete result. Check `HasMorePages` after the final fetch before returning the limit error.</violation>
</file>

<file name="pkg/cloud/aws/session.go">

<violation number="1" location="pkg/cloud/aws/session.go:186">
P3: NewSessionFromConfig is exported from the security-sensitive cloud/aws package but only used by tests (aws_test.go, session_test.go); production always builds sessions via NewSession. As a public API it constructs a Session from arbitrary caller-supplied credentials, bypassing the package's web-identity model. Since it exists purely to let the driver's tests inject VCR/static credentials, scope it to the driver (per the review note that such plumbing should live in the driver) rather than exporting it from cloud/aws.</violation>
</file>

<file name="pkg/accessreview/drivers/testdata/aws.yaml">

<violation number="1" location="pkg/accessreview/drivers/testdata/aws.yaml:25">
P2: Every response in this hand-authored cassette stores a plaintext body without `uncompressed: true`. go-vcr v4's cassette loader gunzips any response whose `Uncompressed` is false, so loading fails and newAWSRecorder falls through to `t.Skipf("cassette not found")` — TestAWSDriver then passes without running. Mark the bodies as uncompressed, matching the repo's other hand-authored cassettes (e.g. apollo.yaml sets `uncompressed: true` in each response).</violation>
</file>

<file name="pkg/accessreview/drivers/vcr_test.go">

<violation number="1" location="pkg/accessreview/drivers/vcr_test.go:178">
P2: awsIAMQueryMatcher matches only on Action and host, so any two requests to the same IAM Action are interchangeable regardless of their other body parameters. GetAccountAuthorizationDetails is driven by a paginator: once the cassette's response is truncated (IsTruncated=true), each follow-up page shares the Action but differs only in the Marker field, so every page matches the same first interaction and the paginator re-reads page one or loops. Because the matcher also ignores Filter/MaxItems, a driver regression that changes those parameters would still replay the stale cassette silently. Compare the parsed form fields beyond Action (e.g., require an equal parameter set), and add a page with a distinct Marker if pagination is expected.</violation>
</file>

<sub>Requires human review: Auto-approval blocked because this review re-detected 5 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: Every AWS record leaves Active unknown, but the access-review upsert converts nil to true, misleading reviewers about IAM account status. Preserve NULL for unknown Active values through persistence.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/aws.go, line 77:

<comment>Every AWS record leaves `Active` unknown, but the access-review upsert converts nil to `true`, misleading reviewers about IAM account status. Preserve NULL for unknown `Active` values through persistence.</comment>

<file context>
@@ -0,0 +1,143 @@
+func iamUserRecord(user iamUser) AccountRecord {
+	grants := slices.Concat(user.Groups, user.AttachedPolicies, user.InlinePolicies)
+
+	return AccountRecord{
+		FullName:    user.Name,
+		Roles:       grants,
</file context>

[cubic-dev-ai]

P2: When authorization details require exactly maxPaginationPages pages, this returns ErrPaginationLimitReached despite having fetched the complete result. Check HasMorePages after the final fetch before returning the limit error.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/cloud/aws/iam.go, line 191:

<comment>When authorization details require exactly `maxPaginationPages` pages, this returns `ErrPaginationLimitReached` despite having fetched the complete result. Check `HasMorePages` after the final fetch before returning the limit error.</comment>

<file context>
@@ -0,0 +1,437 @@
+		}
+	}
+
+	return nil, fmt.Errorf("cannot read all the authorization details of an aws account: %w", ErrPaginationLimitReached)
+}
+
</file context>

[cubic-dev-ai]

P2: Every response in this hand-authored cassette stores a plaintext body without uncompressed: true. go-vcr v4's cassette loader gunzips any response whose Uncompressed is false, so loading fails and newAWSRecorder falls through to t.Skipf("cassette not found") — TestAWSDriver then passes without running. Mark the bodies as uncompressed, matching the repo's other hand-authored cassettes (e.g. apollo.yaml sets uncompressed: true in each response).

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/testdata/aws.yaml, line 25:

<comment>Every response in this hand-authored cassette stores a plaintext body without `uncompressed: true`. go-vcr v4's cassette loader gunzips any response whose `Uncompressed` is false, so loading fails and newAWSRecorder falls through to `t.Skipf("cassette not found")` — TestAWSDriver then passes without running. Mark the bodies as uncompressed, matching the repo's other hand-authored cassettes (e.g. apollo.yaml sets `uncompressed: true` in each response).</comment>

<file context>
@@ -0,0 +1,77 @@
+        proto_major: 1
+        proto_minor: 1
+        content_length: -1
+        body: '<GetAccountAuthorizationDetailsResponse xmlns="https://iam.amazonaws.com/doc/2010-05-08/"><GetAccountAuthorizationDetailsResult><UserDetailList><member><Path>/</Path><UserName>alice</UserName><UserId>AIDAEXAMPLEALICE</UserId><Arn>arn:aws:iam::123456789012:user/alice</Arn><CreateDate>2026-01-02T03:04:05Z</CreateDate><GroupList><member>Admins</member></GroupList><AttachedManagedPolicies><member><PolicyName>AdministratorAccess</PolicyName><PolicyArn>arn:aws:iam::aws:policy/AdministratorAccess</PolicyArn></member></AttachedManagedPolicies></member><member><Path>/</Path><UserName>ci-deploy</UserName><UserId>AIDAEXAMPLECICD</UserId><Arn>arn:aws:iam::123456789012:user/ci-deploy</Arn><CreateDate>2026-02-01T00:00:00Z</CreateDate><UserPolicyList><member><PolicyName>deploy</PolicyName></member></UserPolicyList></member></UserDetailList><GroupDetailList><member><Path>/</Path><GroupName>Admins</GroupName><GroupId>AGPAEXAMPLEADMIN</GroupId><Arn>arn:aws:iam::123456789012:group/Admins</Arn><CreateDate>2026-01-01T00:00:00Z</CreateDate><AttachedManagedPolicies><member><PolicyName>AdministratorAccess</PolicyName><PolicyArn>arn:aws:iam::aws:policy/AdministratorAccess</PolicyArn></member></AttachedManagedPolicies></member></GroupDetailList><IsTruncated>false</IsTruncated></GetAccountAuthorizationDetailsResult></GetAccountAuthorizationDetailsResponse>'
+        headers:
+            Content-Type:
</file context>

[cubic-dev-ai]

P2: awsIAMQueryMatcher matches only on Action and host, so any two requests to the same IAM Action are interchangeable regardless of their other body parameters. GetAccountAuthorizationDetails is driven by a paginator: once the cassette's response is truncated (IsTruncated=true), each follow-up page shares the Action but differs only in the Marker field, so every page matches the same first interaction and the paginator re-reads page one or loops. Because the matcher also ignores Filter/MaxItems, a driver regression that changes those parameters would still replay the stale cassette silently. Compare the parsed form fields beyond Action (e.g., require an equal parameter set), and add a page with a distinct Marker if pagination is expected.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/vcr_test.go, line 178:

<comment>awsIAMQueryMatcher matches only on Action and host, so any two requests to the same IAM Action are interchangeable regardless of their other body parameters. GetAccountAuthorizationDetails is driven by a paginator: once the cassette's response is truncated (IsTruncated=true), each follow-up page shares the Action but differs only in the Marker field, so every page matches the same first interaction and the paginator re-reads page one or loops. Because the matcher also ignores Filter/MaxItems, a driver regression that changes those parameters would still replay the stale cassette silently. Compare the parsed form fields beyond Action (e.g., require an equal parameter set), and add a page with a distinct Marker if pagination is expected.</comment>

<file context>
@@ -90,6 +88,96 @@ func newRecorder(t *testing.T, cassettePath string, envVar string) *recorder.Rec
+
+	action := values.Get("Action")
+
+	return action != "" && action == i.Form.Get("Action")
+}
+
</file context>

[cubic-dev-ai]

P3: NewSessionFromConfig is exported from the security-sensitive cloud/aws package but only used by tests (aws_test.go, session_test.go); production always builds sessions via NewSession. As a public API it constructs a Session from arbitrary caller-supplied credentials, bypassing the package's web-identity model. Since it exists purely to let the driver's tests inject VCR/static credentials, scope it to the driver (per the review note that such plumbing should live in the driver) rather than exporting it from cloud/aws.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At pkg/cloud/aws/session.go, line 186:

<comment>NewSessionFromConfig is exported from the security-sensitive cloud/aws package but only used by tests (aws_test.go, session_test.go); production always builds sessions via NewSession. As a public API it constructs a Session from arbitrary caller-supplied credentials, bypassing the package's web-identity model. Since it exists purely to let the driver's tests inject VCR/static credentials, scope it to the driver (per the review note that such plumbing should live in the driver) rather than exporting it from cloud/aws.</comment>

<file context>
@@ -181,6 +181,12 @@ func (s *Session) Config() awssdk.Config {
 
+// NewSessionFromConfig builds a session from an already-resolved SDK config.
+// Production uses NewSession, which obtains credentials through web identity.
+func NewSessionFromConfig(accountID string, cfg awssdk.Config) *Session {
+	return &Session{cfg: cfg, accountID: accountID}
+}
</file context>