From de7f1072d2d0b62aa9f2cdad4151dbbd15c52667 Mon Sep 17 00:00:00 2001 From: Nicolas Borges Date: Mon, 17 Aug 2026 17:33:47 -0400 Subject: [PATCH] fix: validate existing IAM role trust policies --- src/cli/aws/__tests__/iam.test.ts | 50 ++++++ src/cli/aws/iam.ts | 41 +++++ .../jobs/ab-test/__tests__/resolve.test.ts | 162 +++++++++++++++++- src/cli/operations/jobs/ab-test/resolve.ts | 12 +- 4 files changed, 261 insertions(+), 4 deletions(-) create mode 100644 src/cli/aws/__tests__/iam.test.ts create mode 100644 src/cli/aws/iam.ts diff --git a/src/cli/aws/__tests__/iam.test.ts b/src/cli/aws/__tests__/iam.test.ts new file mode 100644 index 000000000..7f2cb95a9 --- /dev/null +++ b/src/cli/aws/__tests__/iam.test.ts @@ -0,0 +1,50 @@ +import { ValidationError } from '../../../lib/errors/types.js'; +import { validateIamRoleTrustPolicy } from '../iam'; +import { describe, expect, it } from 'vitest'; + +const expectedPolicy = { + Version: '2012-10-17', + Statement: [ + { + Effect: 'Allow', + Principal: { Service: 'bedrock-agentcore.amazonaws.com' }, + Action: 'sts:AssumeRole', + }, + ], +}; + +describe('validateIamRoleTrustPolicy', () => { + it('accepts structurally equal policies regardless of object key order', () => { + const reorderedPolicy = { + Statement: [ + { + Action: 'sts:AssumeRole', + Principal: { Service: 'bedrock-agentcore.amazonaws.com' }, + Effect: 'Allow', + }, + ], + Version: '2012-10-17', + }; + + expect(() => + validateIamRoleTrustPolicy(reorderedPolicy, expectedPolicy, 'TestRole', 'Delete the role.') + ).not.toThrow(); + }); + + it('accepts a URL-encoded matching policy', () => { + const encodedPolicy = encodeURIComponent(JSON.stringify(expectedPolicy)); + + expect(() => + validateIamRoleTrustPolicy(encodedPolicy, expectedPolicy, 'TestRole', 'Delete the role.') + ).not.toThrow(); + }); + + it.each([undefined, 'not-json', { ...expectedPolicy, Statement: [] }])( + 'rejects a missing, malformed, or mismatched policy', + actualPolicy => { + expect(() => validateIamRoleTrustPolicy(actualPolicy, expectedPolicy, 'TestRole', 'Delete the role.')).toThrow( + ValidationError + ); + } + ); +}); diff --git a/src/cli/aws/iam.ts b/src/cli/aws/iam.ts new file mode 100644 index 000000000..2145ad1d9 --- /dev/null +++ b/src/cli/aws/iam.ts @@ -0,0 +1,41 @@ +import { ValidationError } from '../../lib/errors/types.js'; +import stableStringify from 'fast-json-stable-stringify'; + +export type IamPolicyDocument = Record; + +function parseIamPolicyDocument(policy: unknown): unknown { + if (typeof policy !== 'string') return policy; + + try { + return JSON.parse(decodeURIComponent(policy)); + } catch { + return undefined; + } +} + +/** + * Reject an IAM role whose trust policy does not structurally match the expected policy. + * + * IAM may return AssumeRolePolicyDocument as an object or a URL-encoded JSON string. + */ +export function validateIamRoleTrustPolicy( + actualPolicy: unknown, + expectedPolicy: IamPolicyDocument, + roleName: string, + remediation: string +): void { + const parsedPolicy = parseIamPolicyDocument(actualPolicy); + if ( + parsedPolicy !== null && + typeof parsedPolicy === 'object' && + !Array.isArray(parsedPolicy) && + stableStringify(parsedPolicy) === stableStringify(expectedPolicy) + ) { + return; + } + + throw new ValidationError( + `Refusing to reuse existing IAM role "${roleName}" because its trust policy does not match ` + + `the policy required by AgentCore. ${remediation}` + ); +} diff --git a/src/cli/operations/jobs/ab-test/__tests__/resolve.test.ts b/src/cli/operations/jobs/ab-test/__tests__/resolve.test.ts index 9cfa82e73..df678500b 100644 --- a/src/cli/operations/jobs/ab-test/__tests__/resolve.test.ts +++ b/src/cli/operations/jobs/ab-test/__tests__/resolve.test.ts @@ -1,6 +1,164 @@ import type { AgentCoreProjectSpec } from '../../../../../schema'; -import { resolveRuntimeTargetNames } from '../resolve'; -import { describe, expect, it } from 'vitest'; +import { getOrCreateABTestRole, resolveRuntimeTargetNames } from '../resolve'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { mockIamSend } = vi.hoisted(() => ({ + mockIamSend: vi.fn(), +})); + +vi.mock('@aws-sdk/client-iam', () => ({ + IAMClient: class { + send = mockIamSend; + }, + CreateRoleCommand: class { + constructor(public input: unknown) {} + }, + GetRoleCommand: class { + constructor(public input: unknown) {} + }, + PutRolePolicyCommand: class { + constructor(public input: unknown) {} + }, + DeleteRolePolicyCommand: class { + constructor(public input: unknown) {} + }, + DeleteRoleCommand: class { + constructor(public input: unknown) {} + }, +})); + +vi.mock('../../../../aws/account', () => ({ + getCredentialProvider: vi.fn().mockReturnValue({}), +})); + +const accountId = '123456789012'; +const roleArn = `arn:aws:iam::${accountId}:role/AgentCore-Test-ABTestExperiment`; + +interface TestTrustPolicy { + Version: string; + Statement: { + Effect: string; + Principal: Record; + Action: string; + Condition?: { + StringEquals: Record; + ArnLike: Record; + }; + }[]; +} + +function expectedTrustPolicy(): TestTrustPolicy { + return { + Version: '2012-10-17', + Statement: [ + { + Effect: 'Allow', + Principal: { Service: 'bedrock-agentcore.amazonaws.com' }, + Action: 'sts:AssumeRole', + Condition: { + StringEquals: { 'aws:SourceAccount': accountId }, + ArnLike: { 'aws:SourceArn': `arn:aws:bedrock-agentcore:*:${accountId}:ab-test/*` }, + }, + }, + ], + }; +} + +function roleOptions() { + return { + region: 'us-east-1', + projectName: 'Test', + testName: 'Experiment', + gatewayArn: `arn:aws:bedrock-agentcore:us-east-1:${accountId}:gateway/test`, + propagationDelayMs: 0, + }; +} + +function entityAlreadyExistsError(): Error { + return Object.assign(new Error('Role already exists'), { name: 'EntityAlreadyExistsException' }); +} + +describe('getOrCreateABTestRole', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('creates a new role and applies its inline permissions policy', async () => { + mockIamSend.mockResolvedValueOnce({ Role: { Arn: roleArn } }).mockResolvedValueOnce({}); + + await expect(getOrCreateABTestRole(roleOptions())).resolves.toBe(roleArn); + expect(mockIamSend).toHaveBeenCalledTimes(2); + }); + + it('reuses an existing role when its trust policy matches', async () => { + mockIamSend + .mockRejectedValueOnce(entityAlreadyExistsError()) + .mockResolvedValueOnce({ + Role: { Arn: roleArn, AssumeRolePolicyDocument: expectedTrustPolicy() }, + }) + .mockResolvedValueOnce({}); + + await expect(getOrCreateABTestRole(roleOptions())).resolves.toBe(roleArn); + expect(mockIamSend).toHaveBeenCalledTimes(3); + }); + + it('reuses an existing role with a URL-encoded matching trust policy', async () => { + const encodedPolicy = encodeURIComponent(JSON.stringify(expectedTrustPolicy())); + mockIamSend + .mockRejectedValueOnce(entityAlreadyExistsError()) + .mockResolvedValueOnce({ + Role: { Arn: roleArn, AssumeRolePolicyDocument: encodedPolicy }, + }) + .mockResolvedValueOnce({}); + + await expect(getOrCreateABTestRole(roleOptions())).resolves.toBe(roleArn); + expect(mockIamSend).toHaveBeenCalledTimes(3); + }); + + it('rejects an existing role with an additional trusted principal', async () => { + const trustPolicy = expectedTrustPolicy(); + trustPolicy.Statement.push({ + Effect: 'Allow', + Principal: { AWS: `arn:aws:iam::${accountId}:user/attacker` }, + Action: 'sts:AssumeRole', + }); + mockIamSend.mockRejectedValueOnce(entityAlreadyExistsError()).mockResolvedValueOnce({ + Role: { Arn: roleArn, AssumeRolePolicyDocument: trustPolicy }, + }); + + await expect(getOrCreateABTestRole(roleOptions())).rejects.toThrow(/trust policy does not match/); + expect(mockIamSend).toHaveBeenCalledTimes(2); + }); + + const weakenedConditions: [string, (policy: TestTrustPolicy) => void][] = [ + [ + 'SourceAccount', + policy => { + policy.Statement[0]!.Condition!.StringEquals['aws:SourceAccount'] = '*'; + }, + ], + [ + 'SourceArn', + policy => { + policy.Statement[0]!.Condition!.ArnLike['aws:SourceArn'] = '*'; + }, + ], + ]; + + it.each(weakenedConditions)( + 'rejects an existing role with a weakened %s condition', + async (_condition, weakenPolicy) => { + const trustPolicy = expectedTrustPolicy(); + weakenPolicy(trustPolicy); + mockIamSend.mockRejectedValueOnce(entityAlreadyExistsError()).mockResolvedValueOnce({ + Role: { Arn: roleArn, AssumeRolePolicyDocument: trustPolicy }, + }); + + await expect(getOrCreateABTestRole(roleOptions())).rejects.toThrow(/trust policy does not match/); + expect(mockIamSend).toHaveBeenCalledTimes(2); + } + ); +}); type GatewaysOnly = Pick; diff --git a/src/cli/operations/jobs/ab-test/resolve.ts b/src/cli/operations/jobs/ab-test/resolve.ts index 9c338b710..8a1f0bd25 100644 --- a/src/cli/operations/jobs/ab-test/resolve.ts +++ b/src/cli/operations/jobs/ab-test/resolve.ts @@ -8,6 +8,7 @@ import type { AgentCoreProjectSpec, DeployedResourceState } from '../../../../schema'; import { getCredentialProvider } from '../../../aws/account'; import type { ABTestEvaluationConfig, ABTestVariant } from '../../../aws/agentcore-ab-tests'; +import { validateIamRoleTrustPolicy } from '../../../aws/iam'; import { arnPrefix } from '../../../aws/partition'; import { CreateRoleCommand, @@ -61,7 +62,7 @@ export async function getOrCreateABTestRole(options: CreateABTestRoleOptions): P const accountId = gatewayArn.split(':')[4] ?? '*'; const roleName = generateRoleName(projectName, testName); - const trustPolicy = JSON.stringify({ + const trustPolicyDocument = { Version: '2012-10-17', Statement: [ { @@ -74,7 +75,8 @@ export async function getOrCreateABTestRole(options: CreateABTestRoleOptions): P }, }, ], - }); + }; + const trustPolicy = JSON.stringify(trustPolicyDocument); let roleArn: string; try { @@ -102,6 +104,12 @@ export async function getOrCreateABTestRole(options: CreateABTestRoleOptions): P if (!roleArn) { throw new Error(`Role "${roleName}" already exists but ARN could not be retrieved`); } + validateIamRoleTrustPolicy( + existing.Role?.AssumeRolePolicyDocument, + trustPolicyDocument, + roleName, + 'Delete the conflicting role or provide a customer-managed role with --role-arn.' + ); } else { throw err; }