Skip to content

Commit de7f107

Browse files
nborges-awsnborges-aws
committed
fix: validate existing IAM role trust policies
1 parent 85b953e commit de7f107

4 files changed

Lines changed: 261 additions & 4 deletions

File tree

‎src/cli/aws/__tests__/iam.test.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
import { ValidationError } from '../../../lib/errors/types.js';
2+
import { validateIamRoleTrustPolicy } from '../iam';
3+
import { describe, expect, it } from 'vitest';
4+
5+
const expectedPolicy = {
6+
Version: '2012-10-17',
7+
Statement: [
8+
{
9+
Effect: 'Allow',
10+
Principal: { Service: 'bedrock-agentcore.amazonaws.com' },
11+
Action: 'sts:AssumeRole',
12+
},
13+
],
14+
};
15+
16+
describe('validateIamRoleTrustPolicy', () => {
17+
it('accepts structurally equal policies regardless of object key order', () => {
18+
const reorderedPolicy = {
19+
Statement: [
20+
{
21+
Action: 'sts:AssumeRole',
22+
Principal: { Service: 'bedrock-agentcore.amazonaws.com' },
23+
Effect: 'Allow',
24+
},
25+
],
26+
Version: '2012-10-17',
27+
};
28+
29+
expect(() =>
30+
validateIamRoleTrustPolicy(reorderedPolicy, expectedPolicy, 'TestRole', 'Delete the role.')
31+
).not.toThrow();
32+
});
33+
34+
it('accepts a URL-encoded matching policy', () => {
35+
const encodedPolicy = encodeURIComponent(JSON.stringify(expectedPolicy));
36+
37+
expect(() =>
38+
validateIamRoleTrustPolicy(encodedPolicy, expectedPolicy, 'TestRole', 'Delete the role.')
39+
).not.toThrow();
40+
});
41+
42+
it.each([undefined, 'not-json', { ...expectedPolicy, Statement: [] }])(
43+
'rejects a missing, malformed, or mismatched policy',
44+
actualPolicy => {
45+
expect(() => validateIamRoleTrustPolicy(actualPolicy, expectedPolicy, 'TestRole', 'Delete the role.')).toThrow(
46+
ValidationError
47+
);
48+
}
49+
);
50+
});

‎src/cli/aws/iam.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import { ValidationError } from '../../lib/errors/types.js';
2+
import stableStringify from 'fast-json-stable-stringify';
3+
4+
export type IamPolicyDocument = Record<string, unknown>;
5+
6+
function parseIamPolicyDocument(policy: unknown): unknown {
7+
if (typeof policy !== 'string') return policy;
8+
9+
try {
10+
return JSON.parse(decodeURIComponent(policy));
11+
} catch {
12+
return undefined;
13+
}
14+
}
15+
16+
/**
17+
* Reject an IAM role whose trust policy does not structurally match the expected policy.
18+
*
19+
* IAM may return AssumeRolePolicyDocument as an object or a URL-encoded JSON string.
20+
*/
21+
export function validateIamRoleTrustPolicy(
22+
actualPolicy: unknown,
23+
expectedPolicy: IamPolicyDocument,
24+
roleName: string,
25+
remediation: string
26+
): void {
27+
const parsedPolicy = parseIamPolicyDocument(actualPolicy);
28+
if (
29+
parsedPolicy !== null &&
30+
typeof parsedPolicy === 'object' &&
31+
!Array.isArray(parsedPolicy) &&
32+
stableStringify(parsedPolicy) === stableStringify(expectedPolicy)
33+
) {
34+
return;
35+
}
36+
37+
throw new ValidationError(
38+
`Refusing to reuse existing IAM role "${roleName}" because its trust policy does not match ` +
39+
`the policy required by AgentCore. ${remediation}`
40+
);
41+
}

‎src/cli/operations/jobs/ab-test/__tests__/resolve.test.ts‎

Lines changed: 160 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,164 @@
11
import type { AgentCoreProjectSpec } from '../../../../../schema';
2-
import { resolveRuntimeTargetNames } from '../resolve';
3-
import { describe, expect, it } from 'vitest';
2+
import { getOrCreateABTestRole, resolveRuntimeTargetNames } from '../resolve';
3+
import { beforeEach, describe, expect, it, vi } from 'vitest';
4+
5+
const { mockIamSend } = vi.hoisted(() => ({
6+
mockIamSend: vi.fn(),
7+
}));
8+
9+
vi.mock('@aws-sdk/client-iam', () => ({
10+
IAMClient: class {
11+
send = mockIamSend;
12+
},
13+
CreateRoleCommand: class {
14+
constructor(public input: unknown) {}
15+
},
16+
GetRoleCommand: class {
17+
constructor(public input: unknown) {}
18+
},
19+
PutRolePolicyCommand: class {
20+
constructor(public input: unknown) {}
21+
},
22+
DeleteRolePolicyCommand: class {
23+
constructor(public input: unknown) {}
24+
},
25+
DeleteRoleCommand: class {
26+
constructor(public input: unknown) {}
27+
},
28+
}));
29+
30+
vi.mock('../../../../aws/account', () => ({
31+
getCredentialProvider: vi.fn().mockReturnValue({}),
32+
}));
33+
34+
const accountId = '123456789012';
35+
const roleArn = `arn:aws:iam::${accountId}:role/AgentCore-Test-ABTestExperiment`;
36+
37+
interface TestTrustPolicy {
38+
Version: string;
39+
Statement: {
40+
Effect: string;
41+
Principal: Record<string, string>;
42+
Action: string;
43+
Condition?: {
44+
StringEquals: Record<string, string>;
45+
ArnLike: Record<string, string>;
46+
};
47+
}[];
48+
}
49+
50+
function expectedTrustPolicy(): TestTrustPolicy {
51+
return {
52+
Version: '2012-10-17',
53+
Statement: [
54+
{
55+
Effect: 'Allow',
56+
Principal: { Service: 'bedrock-agentcore.amazonaws.com' },
57+
Action: 'sts:AssumeRole',
58+
Condition: {
59+
StringEquals: { 'aws:SourceAccount': accountId },
60+
ArnLike: { 'aws:SourceArn': `arn:aws:bedrock-agentcore:*:${accountId}:ab-test/*` },
61+
},
62+
},
63+
],
64+
};
65+
}
66+
67+
function roleOptions() {
68+
return {
69+
region: 'us-east-1',
70+
projectName: 'Test',
71+
testName: 'Experiment',
72+
gatewayArn: `arn:aws:bedrock-agentcore:us-east-1:${accountId}:gateway/test`,
73+
propagationDelayMs: 0,
74+
};
75+
}
76+
77+
function entityAlreadyExistsError(): Error {
78+
return Object.assign(new Error('Role already exists'), { name: 'EntityAlreadyExistsException' });
79+
}
80+
81+
describe('getOrCreateABTestRole', () => {
82+
beforeEach(() => {
83+
vi.clearAllMocks();
84+
});
85+
86+
it('creates a new role and applies its inline permissions policy', async () => {
87+
mockIamSend.mockResolvedValueOnce({ Role: { Arn: roleArn } }).mockResolvedValueOnce({});
88+
89+
await expect(getOrCreateABTestRole(roleOptions())).resolves.toBe(roleArn);
90+
expect(mockIamSend).toHaveBeenCalledTimes(2);
91+
});
92+
93+
it('reuses an existing role when its trust policy matches', async () => {
94+
mockIamSend
95+
.mockRejectedValueOnce(entityAlreadyExistsError())
96+
.mockResolvedValueOnce({
97+
Role: { Arn: roleArn, AssumeRolePolicyDocument: expectedTrustPolicy() },
98+
})
99+
.mockResolvedValueOnce({});
100+
101+
await expect(getOrCreateABTestRole(roleOptions())).resolves.toBe(roleArn);
102+
expect(mockIamSend).toHaveBeenCalledTimes(3);
103+
});
104+
105+
it('reuses an existing role with a URL-encoded matching trust policy', async () => {
106+
const encodedPolicy = encodeURIComponent(JSON.stringify(expectedTrustPolicy()));
107+
mockIamSend
108+
.mockRejectedValueOnce(entityAlreadyExistsError())
109+
.mockResolvedValueOnce({
110+
Role: { Arn: roleArn, AssumeRolePolicyDocument: encodedPolicy },
111+
})
112+
.mockResolvedValueOnce({});
113+
114+
await expect(getOrCreateABTestRole(roleOptions())).resolves.toBe(roleArn);
115+
expect(mockIamSend).toHaveBeenCalledTimes(3);
116+
});
117+
118+
it('rejects an existing role with an additional trusted principal', async () => {
119+
const trustPolicy = expectedTrustPolicy();
120+
trustPolicy.Statement.push({
121+
Effect: 'Allow',
122+
Principal: { AWS: `arn:aws:iam::${accountId}:user/attacker` },
123+
Action: 'sts:AssumeRole',
124+
});
125+
mockIamSend.mockRejectedValueOnce(entityAlreadyExistsError()).mockResolvedValueOnce({
126+
Role: { Arn: roleArn, AssumeRolePolicyDocument: trustPolicy },
127+
});
128+
129+
await expect(getOrCreateABTestRole(roleOptions())).rejects.toThrow(/trust policy does not match/);
130+
expect(mockIamSend).toHaveBeenCalledTimes(2);
131+
});
132+
133+
const weakenedConditions: [string, (policy: TestTrustPolicy) => void][] = [
134+
[
135+
'SourceAccount',
136+
policy => {
137+
policy.Statement[0]!.Condition!.StringEquals['aws:SourceAccount'] = '*';
138+
},
139+
],
140+
[
141+
'SourceArn',
142+
policy => {
143+
policy.Statement[0]!.Condition!.ArnLike['aws:SourceArn'] = '*';
144+
},
145+
],
146+
];
147+
148+
it.each(weakenedConditions)(
149+
'rejects an existing role with a weakened %s condition',
150+
async (_condition, weakenPolicy) => {
151+
const trustPolicy = expectedTrustPolicy();
152+
weakenPolicy(trustPolicy);
153+
mockIamSend.mockRejectedValueOnce(entityAlreadyExistsError()).mockResolvedValueOnce({
154+
Role: { Arn: roleArn, AssumeRolePolicyDocument: trustPolicy },
155+
});
156+
157+
await expect(getOrCreateABTestRole(roleOptions())).rejects.toThrow(/trust policy does not match/);
158+
expect(mockIamSend).toHaveBeenCalledTimes(2);
159+
}
160+
);
161+
});
4162

5163
type GatewaysOnly = Pick<AgentCoreProjectSpec, 'agentCoreGateways'>;
6164

‎src/cli/operations/jobs/ab-test/resolve.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import type { AgentCoreProjectSpec, DeployedResourceState } from '../../../../schema';
99
import { getCredentialProvider } from '../../../aws/account';
1010
import type { ABTestEvaluationConfig, ABTestVariant } from '../../../aws/agentcore-ab-tests';
11+
import { validateIamRoleTrustPolicy } from '../../../aws/iam';
1112
import { arnPrefix } from '../../../aws/partition';
1213
import {
1314
CreateRoleCommand,
@@ -61,7 +62,7 @@ export async function getOrCreateABTestRole(options: CreateABTestRoleOptions): P
6162
const accountId = gatewayArn.split(':')[4] ?? '*';
6263
const roleName = generateRoleName(projectName, testName);
6364

64-
const trustPolicy = JSON.stringify({
65+
const trustPolicyDocument = {
6566
Version: '2012-10-17',
6667
Statement: [
6768
{
@@ -74,7 +75,8 @@ export async function getOrCreateABTestRole(options: CreateABTestRoleOptions): P
7475
},
7576
},
7677
],
77-
});
78+
};
79+
const trustPolicy = JSON.stringify(trustPolicyDocument);
7880

7981
let roleArn: string;
8082
try {
@@ -102,6 +104,12 @@ export async function getOrCreateABTestRole(options: CreateABTestRoleOptions): P
102104
if (!roleArn) {
103105
throw new Error(`Role "${roleName}" already exists but ARN could not be retrieved`);
104106
}
107+
validateIamRoleTrustPolicy(
108+
existing.Role?.AssumeRolePolicyDocument,
109+
trustPolicyDocument,
110+
roleName,
111+
'Delete the conflicting role or provide a customer-managed role with --role-arn.'
112+
);
105113
} else {
106114
throw err;
107115
}

0 commit comments

Comments
 (0)